CNF-14084: controller: update MachineConfig reconciliation #1025

Tal-or · 2024-09-24T17:41:20Z

Starting from 4.18 RTE pods can use a built-in SELinux policy instead
of custom one.

This means that MachineConfig deployment is not mandatory anymore.
However, it is still an option to deploy a custom policy using MachineConfig.

This commit updates the MahcineConfig reconciliation logic.

By default when upgrade is done from 4.1X -> 4.18 the controller
tries to remove the redundant MahcineConfig, unless user state
explicitly that custom policy is needed.

Signed-off-by: Talor Itzhak [email protected]

Tal-or · 2024-09-24T17:41:27Z

/hold

openshift-ci-robot · 2024-09-24T17:42:18Z

@Tal-or: This pull request references CNF-14084 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the story to target the "4.18.0" version, but no target version was set.

In response to this:

Starting from 4.18 RTE pods can use a built-in SELinux policy instead
of custom one.

This means that MachineConfig deployment is not mandatory anymore.
However, it is still an option to deploy a custom policy using MachineConfig.

This commit updates the MahcineConfig reconciliation logic.

By default when upgrade is done from 4.1X -> 4.18 the controller
tries to remove the redundant MahcineConfig, unless user state
explicitly that custom policy is needed.

Signed-off-by: Talor Itzhak [email protected]

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

ffromani · 2024-09-24T17:47:21Z

Starting from 4.18 RTE pods can use a built-in SELinux policy instead of custom one.

This means that MachineConfig deployment is not mandatory anymore. However, it is still an option to deploy a custom policy using MachineConfig.

This commit updates the MahcineConfig reconciliation logic.

By default when upgrade is done from 4.1X -> 4.18 the controller tries to remove the redundant MahcineConfig, unless user state explicitly that custom policy is needed.

this could be the bit that pushes us to v2.
New deployments should use the new builtin policy
Old deployments should keep using the old one till they upgrade explicitely

But then we have conflicting defaults :\

Tal-or · 2024-09-24T17:56:25Z

Starting from 4.18 RTE pods can use a built-in SELinux policy instead of custom one.
This means that MachineConfig deployment is not mandatory anymore. However, it is still an option to deploy a custom policy using MachineConfig.
This commit updates the MahcineConfig reconciliation logic.
By default when upgrade is done from 4.1X -> 4.18 the controller tries to remove the redundant MahcineConfig, unless user state explicitly that custom policy is needed.

this could be the bit that pushes us to v2. New deployments should use the new builtin policy Old deployments should keep using the old one till they upgrade explicitely

But then we have conflicting defaults :\

Let's discuss it more deeply offline

internal/api/features/types.go

Signed-off-by: Talor Itzhak <[email protected]>

Tal-or · 2024-09-30T20:38:29Z

/hold cancel

Tal-or · 2024-10-01T08:47:22Z

/jira-refresh

Tal-or · 2024-10-01T09:15:18Z

/jira help

Tal-or · 2024-10-01T13:29:55Z

/cc @shajmakh

ffromani

most comments are about code organization. The overall direction is reasonnable, but we would need some work still

internal/api/features/types.go

api/numaresourcesoperator/v1/numaresourcesoperator_types.go

ffromani · 2024-10-02T14:11:02Z

controllers/numaresourcesoperator_controller.go

+	// unless an emergency annotation is provided which forces the operator to use custom policy
+	if !instance.IsCustomPolicyEnabled() {
+		for _, objState := range objStates {
+			if !objState.IsNotFoundError() {


this looks fragile. We want to target only the MachineConfig. So it's probably better to add new functionalities to pkg/objectstate/rte

I'm also thinking about deleteUnusedMachineConfigs. We can perhaps reuse that function - or even better get rid of it.

...or not? #1029

What you did in #1029 is true, but we still need to remove the machine config explicitly when moving to built-in policy, because the CR remain in the cluster.
And as long as it persist on the cluster the machine-config won't go anywhere

OK, let's try to remove them in the reconciliation loop rather than explicitely in the deleteXXX functions.

this looks fragile. We want to target only the MachineConfig. So it's probably better to add new functionalities to pkg/objectstate/rte

We do. you have the objStates := existing.MachineConfigsState(r.RTEManifests) that filters the MachineConfigs out of all the objects for you. (line 331 in the code)

OK, let's try to remove them in the reconciliation loop rather than explicitely in the deleteXXX functions.

This code is part of the reconciliation loop
it's called in syncMachineConfigs

yes, I commented before to starting the effort in #1029 . It seems this of yours is the best approach on the table, and the removal of the deleteXXX functions should be tried (and preferably done!) on the side

ffromani · 2024-10-02T14:11:46Z

controllers/numaresourcesoperator_controller.go

-		}
-		if !updated {
-			continue
+			_, _, err2 := apply.ApplyObject(ctx, r.Client, objState)


let's see if we can add apply.DeleteObject. I'll think about it

controllers/numaresourcesoperator_controller.go

pkg/objectstate/rte/rte.go

ffromani · 2024-10-02T14:18:02Z

test/e2e/install/install_test.go

@@ -288,7 +288,7 @@ var _ = Describe("[Install] durability", func() {
 			By("checking there are no leftovers")
 			// by taking the ns from the ds we're avoiding the need to figure out in advanced
 			// at which ns we should look for the resources
-			mf, err := rte.GetManifests(configuration.Plat, configuration.PlatVersion, ds.Namespace, true)
+			mf, err := rte.GetManifests(configuration.Plat, configuration.PlatVersion, ds.Namespace, true, true)


time to cleanup this signature if time allows

Will do, but I wouldn't block this PR for that.

of course no, this must to be a separate independent effort

test/utils/deploy/deploy.go

controllers/numaresourcesoperator_controller.go

When cluster gets upgrade from 4.1X -> 4.18 the operator removes the MachineConfig (that contains the custom SELinux policy) and using the built-in policy instead. But we still want to have a way to enable the custom SELinux in case that something breaks during upgrade. For that we introduce a special annotation that would be present in the NUMAResourcesOperator CR and would force it to use the custom (legacy) SELinux. Once the annotation specified it would apply on all RTE pods regardless of their association with the different NodeGroups. Signed-off-by: Talor Itzhak <[email protected]>

Starting from 4.18 RTE pods can use a built-in SELinux policy instead of custom one. This means that MachineConfig deployment is not mandatory anymore. However, it is still an option to deploy a custom policy using MachineConfig, in case of a failure. This commit updates the MahcineConfig reconciliation logic. By default when upgrade is done from 4.1X -> 4.18 the controller tries to remove the redundant MahcineConfig, unless user state explicitly that custom policy is needed using special annotation. Signed-off-by: Talor Itzhak <[email protected]>

Use the legacy RTE pod context, if the emergency annotation is provided. Signed-off-by: Talor Itzhak <[email protected]>

Emulating an integration test that emulates an upgrade from 4.1X -> 4.18 and verifies that the MachineConfigs are deleted. Signed-off-by: Talor Itzhak <[email protected]>

Signed-off-by: Talor Itzhak <[email protected]>

This function waits for MCP defined condition, and can wait for multiple MCPs in parallel. Make the function public so it could be used in later commit. Signed-off-by: Talor Itzhak <[email protected]>

Now that we're using a built-in SELinux policy, we do not wait for the MachineConfig creation. We'll wait for MCP update only when the legacy policy is used or when we create KubeletConfig for the tests. In addition, we'll wait for MCP to transition from updated <-> updating. The old behavior was to wait for specific MachineConfig name, but in case of KubeletConfig creation for example it is not good enough. Signed-off-by: Talor Itzhak <[email protected]>

set the SSC with the correct SELinux option, given we use custom policy or not. Signed-off-by: Talor Itzhak <[email protected]>

After RTE pod deployment, the test validates it's running with the correct SELinux context. Signed-off-by: Talor Itzhak <[email protected]>

ffromani

/approve
/lgtm

I'm not completely happy with how the code would look like, but I prefer to merge the functionality and improve in followup PRs.

@Tal-or please unhold if you're happy with this work and if you think the test coverage is sufficient

openshift-ci · 2024-10-08T11:50:13Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani, Tal-or

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [Tal-or,ffromani]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

ffromani · 2024-10-08T11:50:15Z

/hold

Tal-or · 2024-10-08T11:55:33Z

/approve /lgtm

I'm not completely happy with how the code would look like, but I prefer to merge the functionality and improve in followup PRs.

@Tal-or please unhold if you're happy with this work and if you think the test coverage is sufficient

We can wait and have another iteration if you're not happy with code flow. I don't mind at all

ffromani · 2024-10-08T12:25:59Z

/approve /lgtm
I'm not completely happy with how the code would look like, but I prefer to merge the functionality and improve in followup PRs.
@Tal-or please unhold if you're happy with this work and if you think the test coverage is sufficient

We can wait and have another iteration if you're not happy with code flow. I don't mind at all

for starters, I'd love to push the Delete logic in the apply package, for consistency. Something like #1038

Tal-or · 2024-10-09T08:24:21Z

/retest

the new API ApplyState should initially used only in the new code which deals with HCP and machineconfig removal (PR #1025) Signed-off-by: Francesco Romani <[email protected]>

Tal-or · 2024-10-09T11:54:36Z

/unhold

openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 24, 2024

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 24, 2024

openshift-ci bot requested review from ffromani and shajmakh September 24, 2024 17:41

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 24, 2024

Tal-or changed the title ~~WIP: controller: update MachineConfig reconciliation Beta~~ WIP: CNF-14084: controller: update MachineConfig reconciliation Beta Sep 24, 2024

Tal-or changed the title ~~WIP: CNF-14084: controller: update MachineConfig reconciliation Beta~~ WIP: CNF-14084: controller: update MachineConfig reconciliation Sep 24, 2024

Tal-or force-pushed the remove_machineconfig branch 2 times, most recently from 8dab674 to 63cc507 Compare September 26, 2024 08:20

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2024

Tal-or commented Sep 26, 2024

View reviewed changes

internal/api/features/types.go Outdated Show resolved Hide resolved

Tal-or force-pushed the remove_machineconfig branch from 63cc507 to 402ba05 Compare September 26, 2024 08:23

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2024

Tal-or changed the title ~~WIP: CNF-14084: controller: update MachineConfig reconciliation~~ CNF-14084: controller: update MachineConfig reconciliation Sep 26, 2024

openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 26, 2024

Tal-or force-pushed the remove_machineconfig branch from 402ba05 to 502014a Compare September 26, 2024 09:49

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2024

vendor: bump deployer to v0.21.0

95f3e60

Signed-off-by: Talor Itzhak <[email protected]>

Tal-or force-pushed the remove_machineconfig branch from 502014a to c480dca Compare September 26, 2024 09:50

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 26, 2024

Tal-or force-pushed the remove_machineconfig branch from b196aae to 0d6dc0e Compare September 30, 2024 20:38

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 30, 2024

Tal-or force-pushed the remove_machineconfig branch 2 times, most recently from 70d035e to 26b08af Compare October 1, 2024 07:20

ffromani reviewed Oct 2, 2024

View reviewed changes

Tal-or added 2 commits October 7, 2024 14:29

Tal-or force-pushed the remove_machineconfig branch from 26b08af to a2e9708 Compare October 7, 2024 11:42

Tal-or added 6 commits October 7, 2024 15:06

selinux: use correct SELinux context

3e1611a

Use the legacy RTE pod context, if the emergency annotation is provided. Signed-off-by: Talor Itzhak <[email protected]>

test-unit: emulating upgrade to 4.18

21a1b61

Emulating an integration test that emulates an upgrade from 4.1X -> 4.18 and verifies that the MachineConfigs are deleted. Signed-off-by: Talor Itzhak <[email protected]>

tests: fix GetManifests signature

1f59b17

Signed-off-by: Talor Itzhak <[email protected]>

e2e: waitForMcpsCondition -> WaitForMCPsCondition

2c17b34

This function waits for MCP defined condition, and can wait for multiple MCPs in parallel. Make the function public so it could be used in later commit. Signed-off-by: Talor Itzhak <[email protected]>

ssc: update correct SELinux option

1327010

set the SSC with the correct SELinux option, given we use custom policy or not. Signed-off-by: Talor Itzhak <[email protected]>

Tal-or force-pushed the remove_machineconfig branch from a2e9708 to 1327010 Compare October 7, 2024 12:25

e2e: validate selinux context

2823ed0

After RTE pod deployment, the test validates it's running with the correct SELinux context. Signed-off-by: Talor Itzhak <[email protected]>

Tal-or force-pushed the remove_machineconfig branch from e0d00ee to 2823ed0 Compare October 8, 2024 10:58

ffromani reviewed Oct 8, 2024

View reviewed changes

openshift-ci bot assigned ffromani Oct 8, 2024

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Oct 8, 2024

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 8, 2024

ffromani added a commit that referenced this pull request Oct 9, 2024

WIP: demo: ApplyState usage

2f145d9

the new API ApplyState should initially used only in the new code which deals with HCP and machineconfig removal (PR #1025) Signed-off-by: Francesco Romani <[email protected]>

Tal-or mentioned this pull request Oct 9, 2024

rte: controller: push down machineconfig deletion support into library packages #1038

Merged

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 9, 2024

openshift-merge-bot bot merged commit bec60a1 into openshift-kni:main Oct 9, 2024
14 checks passed

Tal-or deleted the remove_machineconfig branch October 9, 2024 13:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CNF-14084: controller: update MachineConfig reconciliation #1025

CNF-14084: controller: update MachineConfig reconciliation #1025

Tal-or commented Sep 24, 2024

Tal-or commented Sep 24, 2024

openshift-ci-robot commented Sep 24, 2024

ffromani commented Sep 24, 2024

Tal-or commented Sep 24, 2024

Tal-or commented Sep 30, 2024

Tal-or commented Oct 1, 2024

Tal-or commented Oct 1, 2024

Tal-or commented Oct 1, 2024

ffromani left a comment

ffromani Oct 2, 2024

ffromani Oct 2, 2024

ffromani Oct 2, 2024

Tal-or Oct 7, 2024

ffromani Oct 7, 2024

Tal-or Oct 7, 2024 •

edited

Loading

ffromani Oct 7, 2024

ffromani Oct 2, 2024

ffromani Oct 2, 2024

Tal-or Oct 7, 2024

ffromani Oct 7, 2024

ffromani left a comment

openshift-ci bot commented Oct 8, 2024

ffromani commented Oct 8, 2024

Tal-or commented Oct 8, 2024

ffromani commented Oct 8, 2024

Tal-or commented Oct 9, 2024

Tal-or commented Oct 9, 2024

CNF-14084: controller: update MachineConfig reconciliation #1025

CNF-14084: controller: update MachineConfig reconciliation #1025

Conversation

Tal-or commented Sep 24, 2024

Tal-or commented Sep 24, 2024

openshift-ci-robot commented Sep 24, 2024

ffromani commented Sep 24, 2024

Tal-or commented Sep 24, 2024

Tal-or commented Sep 30, 2024

Tal-or commented Oct 1, 2024

Tal-or commented Oct 1, 2024

Tal-or commented Oct 1, 2024

ffromani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Tal-or Oct 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffromani left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Oct 8, 2024

ffromani commented Oct 8, 2024

Tal-or commented Oct 8, 2024

ffromani commented Oct 8, 2024

Tal-or commented Oct 9, 2024

Tal-or commented Oct 9, 2024

Tal-or Oct 7, 2024 •

edited

Loading