objstate: rte: isolate platform-specific code #1114

ffromani · 2024-12-12T16:42:23Z

isolate per-platform specific code in their own types and source code, to reduce the clutter without intentional changes of behavior.
The idea is to gradually move towards full isolation and containment of the platform-specific code, and this change is a step in that direction.
Actually, we dopn't really care about the platform but rather about how we find the nodes, but the method (poolName vs machineconfigpool) is still tied to the platform, hence the ambiguity.
In the long run we want to support only the newer pool approach introduced with HCP.

ffromani · 2024-12-16T11:20:59Z

/cc @Tal-or

ffromani · 2024-12-17T11:13:09Z

/hold

need to merge more urgent changes before

Tal-or · 2024-12-17T12:33:23Z

pkg/objectstate/rte/rte.go

@@ -45,6 +45,13 @@ const (
 	HyperShiftNodePoolLabel = "hypershift.openshift.io/nodePool"
 )

+// TODO: ugly name. At least it's only internal
+type rteHelper interface {


StateFinder?

thought about it but it reads poorly with the method names, which in turn are called this way to suggest where the helper is supposed to be used (in FromClient and State functions)

Tal-or · 2024-12-17T12:45:56Z

controllers/numaresourcesoperator_controller.go

@@ -558,8 +558,8 @@ func (r *NUMAResourcesOperatorReconciler) syncNUMAResourcesOperatorResources(ctx
 		return nil
 	}

-	existing := rtestate.FromClient(ctx, r.Client, r.Platform, r.RTEManifests, instance, trees, r.Namespace)
-	for _, objState := range existing.State(r.RTEManifests, processor, annotations.IsCustomPolicyEnabled(instance.Annotations)) {
+	existing := rtestate.FromClient(ctx, r.Client, r.Platform, r.RTEManifests, instance, trees, r.Namespace).WithManifestsUpdater(processor)


If we're going with a builder pattern (AKA method-changing) then let's do it for the additional args as well

still not sure about going this route, but will take this into consideration

the last commit should clarify why I'm keeping the current approach

extract helper to update the state from either NodeGroups or MachineConfigPools into helpers. The goal here is to isolate this code to make the main calling functions more targeted and to unblock future refactoring. Signed-off-by: Francesco Romani <[email protected]>

extract helper to fetch the state from either NodeGroups or MachineConfigPools into helpers. The goal here is to isolate this code to make the main calling functions more targeted and to unblock future refactoring. Add helpers to inject parameters. This helps making the objectstate flavours all consistent with each other; previously `rte` was the outlier and the only variant with extra parameters. This require the constructor function to return a pointer to a struct (not a struct). Update all the variants for consistency. Signed-off-by: Francesco Romani <[email protected]>

Tal-or · 2025-01-07T09:02:30Z

related to: #1142

ffromani · 2025-01-07T09:31:04Z

/hold cancel

work here is done, pending only review and validation

pkg/objectstate/rte/machineconfigpool.go

pkg/objectstate/rte/nodegroup.go

move helpers in per-mode files. This is meant to keep the filesize at bay and to enable future code extraction. No intended change in behavior. Signed-off-by: Francesco Romani <[email protected]>

the helpers we extracted in previous commits still share some state; pack them in a struct, which makes also nicer to consume them from the caller site(s), acting almost like a type. In the (far?) future, when we will move completely to nodegroups (if ever), this code should be dissolved back in the callers. Signed-off-by: Francesco Romani <[email protected]>

We used to get the current cluster state both when attempting to sync the machine config state in the OpenShift flow, and when attemptiny to sync the RTE daemonsets in both flows. Due to the way `rtestate.FromClient` works, we end up reading the full cluster state twice in the openshift flow, which is unnecessary and wasteful. The fix is to get the cluster state once in the higher level function and pass the state in the specific reconcile step functions. The machine config and daemonset functions touch not overlapping objects (bar bugs) so sharing the state between them is expected to be fine. In other words, we happen to fetch the state twice most likely because of oversight, not because a deliberate decision about data sharing/unsharing (which, should that be the case, would need some extra refactoring and more explicit code). Signed-off-by: Francesco Romani <[email protected]>

openshift-ci · 2025-01-07T11:11:12Z

[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 · 2025-01-07T11:45:08Z

/cherry-pick release-4.18

openshift-cherrypick-robot · 2025-01-07T11:45:10Z

@ffromani: once the present PR merges, I will cherry-pick it on top of release-4.18 in a new PR and assign it to you.

In response to this:

/cherry-pick release-4.18

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 kubernetes-sigs/prow repository.

openshift-cherrypick-robot · 2025-01-07T12:37:32Z

@ffromani: #1114 failed to apply on top of branch "release-4.18":

Applying: objectstate: rte: extract platform-specific update
Using index info to reconstruct a base tree...
M	pkg/objectstate/rte/rte.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/objectstate/rte/rte.go
CONFLICT (content): Merge conflict in pkg/objectstate/rte/rte.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 objectstate: rte: extract platform-specific update

In response to this:

/cherry-pick release-4.18

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 kubernetes-sigs/prow repository.

ffromani · 2025-01-07T12:40:27Z

@ffromani: #1114 failed to apply on top of branch "release-4.18":

Applying: objectstate: rte: extract platform-specific update
Using index info to reconstruct a base tree...
M	pkg/objectstate/rte/rte.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/objectstate/rte/rte.go
CONFLICT (content): Merge conflict in pkg/objectstate/rte/rte.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
hint: When you have resolved this problem, run "git am --continue".
hint: If you prefer to skip this patch, run "git am --skip" instead.
hint: To restore the original branch and stop patching, run "git am --abort".
hint: Disable this message with "git config advice.mergeConflict false"
Patch failed at 0001 objectstate: rte: extract platform-specific update

fair enough. Let's make a more targeted fix for 4.18

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 Dec 12, 2024

openshift-ci bot requested review from shajmakh and swatisehgal December 12, 2024 16:42

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

ffromani force-pushed the plat-cleanup-objstate branch 2 times, most recently from ccb669d to a78f266 Compare December 16, 2024 08:35

ffromani changed the title ~~WIP DNM~~ objstate: rte: isolate platform-specific code Dec 16, 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 Dec 16, 2024

openshift-ci bot requested a review from Tal-or December 16, 2024 11:21

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

Tal-or reviewed Dec 17, 2024

View reviewed changes

ffromani added 2 commits January 7, 2025 08:26

ffromani force-pushed the plat-cleanup-objstate branch from a78f266 to d9488d2 Compare January 7, 2025 07:57

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

Tal-or reviewed Jan 7, 2025

View reviewed changes

pkg/objectstate/rte/machineconfigpool.go Outdated Show resolved Hide resolved

pkg/objectstate/rte/nodegroup.go Outdated Show resolved Hide resolved

pkg/objectstate/rte/nodegroup.go Outdated Show resolved Hide resolved

ffromani added 3 commits January 7, 2025 11:16

objectstate: rte: extract helpers

6668e12

move helpers in per-mode files. This is meant to keep the filesize at bay and to enable future code extraction. No intended change in behavior. Signed-off-by: Francesco Romani <[email protected]>

ffromani force-pushed the plat-cleanup-objstate branch from e341b37 to 835a9a8 Compare January 7, 2025 10:17

Tal-or approved these changes Jan 7, 2025

View reviewed changes

openshift-ci bot assigned Tal-or Jan 7, 2025

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 7, 2025

ffromani added the cherry-pick-candidate Possible cherry-pick in the future label Jan 7, 2025

openshift-merge-bot bot merged commit e70e002 into main Jan 7, 2025
16 checks passed

ffromani deleted the plat-cleanup-objstate branch January 7, 2025 12:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

objstate: rte: isolate platform-specific code #1114

objstate: rte: isolate platform-specific code #1114

ffromani commented Dec 12, 2024 •

edited

Loading

ffromani commented Dec 16, 2024

ffromani commented Dec 17, 2024

Tal-or Dec 17, 2024

ffromani Dec 17, 2024

Tal-or Dec 17, 2024

ffromani Dec 17, 2024

ffromani Jan 7, 2025

Tal-or commented Jan 7, 2025

ffromani commented Jan 7, 2025

openshift-ci bot commented Jan 7, 2025

ffromani commented Jan 7, 2025

openshift-cherrypick-robot commented Jan 7, 2025

openshift-cherrypick-robot commented Jan 7, 2025

ffromani commented Jan 7, 2025

objstate: rte: isolate platform-specific code #1114

objstate: rte: isolate platform-specific code #1114

Conversation

ffromani commented Dec 12, 2024 • edited Loading

ffromani commented Dec 16, 2024

ffromani commented Dec 17, 2024

Tal-or Dec 17, 2024

Choose a reason for hiding this comment

ffromani Dec 17, 2024

Choose a reason for hiding this comment

Tal-or Dec 17, 2024

Choose a reason for hiding this comment

ffromani Dec 17, 2024

Choose a reason for hiding this comment

ffromani Jan 7, 2025

Choose a reason for hiding this comment

Tal-or commented Jan 7, 2025

ffromani commented Jan 7, 2025

openshift-ci bot commented Jan 7, 2025

ffromani commented Jan 7, 2025

openshift-cherrypick-robot commented Jan 7, 2025

openshift-cherrypick-robot commented Jan 7, 2025

ffromani commented Jan 7, 2025

ffromani commented Dec 12, 2024 •

edited

Loading