ctrl: HCP adaptations for NodeGroup validation and RTE #1027

shajmakh · 2024-09-27T10:56:09Z

Adapt the possibility of running the operator on Hypershift platform by
replacing MCP context with PoolName where relevant; In addition to extra
NodeGroup validation on hypershift.

openshift-ci · 2024-09-27T10:56:21Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shajmakh

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~~ [shajmakh]

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

shajmakh · 2024-09-27T10:57:09Z

#1018 is needed first

shajmakh · 2024-10-31T07:33:19Z

depends on #1015

Tal-or

I think we need to come with a better approach than all the branching in the flow per platform type.
I'll try to experiment with the code and will provide some feedback soon

pkg/objectstate/rte/rte.go

Tal-or · 2024-11-03T17:28:00Z

depends on #1015

Why this PR depends on #1015?

api/numaresourcesoperator/v1/helper/nodegroup/nodegroup.go

ffromani

let's add the missing FindTree alias per inline comment and we can merge.

It's more important to fix CI than polish, but we need to polish as soon as we have CI running.

ffromani · 2024-11-19T18:33:37Z

pkg/objectstate/rte/rte.go

@@ -257,11 +260,64 @@ func (em *ExistingManifests) State(mf rtemanifests.Manifests, updater GenerateDe
 	}

 	for _, tree := range em.trees {
-		for _, mcp := range tree.MachineConfigPools {
+		if em.plat == platform.OpenShift {


we can merge to have actually CI running, but this code is now untenable, we need to plan a cleanup/possibly a redesign ASAP

will add an issue/card to track this work

pkg/validation/validation_test.go

api/numaresourcesoperator/v1/helper/nodegroup/nodegroup.go

ffromani · 2024-11-19T18:37:16Z

controllers/numaresourcesoperator_controller.go

+	if err := validation.NodeGroups(instance.Spec.NodeGroups, r.Platform); err != nil {
 		return r.updateStatus(ctx, instance, status.ConditionDegraded, validation.NodeGroupsError, err)
 	}

-	trees, err := getTreesByNodeGroup(ctx, r.Client, instance.Spec.NodeGroups)
+	trees, err := getTreesByNodeGroup(ctx, r.Client, instance.Spec.NodeGroups, r.Platform)
 	if err != nil {
 		return r.updateStatus(ctx, instance, status.ConditionDegraded, validation.NodeGroupsError, err)
 	}

-	multiMCPsErr := validation.MultipleMCPsPerTree(instance.Annotations, trees)
+	var multiMCPsErr error
+	if r.Platform == platform.OpenShift {
+		multiMCPsErr = validation.MultipleMCPsPerTree(instance.Annotations, trees)

-	if err := validation.MachineConfigPoolDuplicates(trees); err != nil {
-		return r.updateStatus(ctx, instance, status.ConditionDegraded, validation.NodeGroupsError, err)
+		if err := validation.MachineConfigPoolDuplicates(trees); err != nil {
+			return r.updateStatus(ctx, instance, status.ConditionDegraded, validation.NodeGroupsError, err)
+		}


all the validation code should be abstracted (in the aforementioned future cleanup once CI runs) in a platform-specific function instead of having plenty of if platform == like we have today

will add an issue/card to track this work

Tal-or · 2024-11-19T20:38:46Z

let's add the missing FindTree alias per inline comment and we can merge.

It's more important to fix CI than polish, but we need to polish as soon as we have CI running.

It's possible to rebase this one on top of #1071 to make sure CI is happy

Adapt the possibility of running the operator on Hypershift platform by replacing MCP context with PoolName where relevant; In addition to extra NodeGroup validation on hypershift. Signed-off-by: Shereen Haj <[email protected]>

The controller reconciliation should reflect the node group statuses after verifying all components are up and healthy. To preserve backward compatibility, for OCP we copy the node group config from MachineConfigPools[].Config to NodeGroupStatus[].Config. For hypershift we don't have MCP so we copy that info from the NodeGroup[].Config. That is safe because we know the controller loop will exit if a node group is misconfigured. Signed-off-by: Shereen Haj <[email protected]>

ffromani

/lgtm

ffromani · 2024-11-20T14:21:26Z

/override ci/prow/ci-e2e-install-hypershift

this is still expected

openshift-ci · 2024-11-20T14:21:31Z

@ffromani: Overrode contexts on behalf of ffromani: ci/prow/ci-e2e-install-hypershift

In response to this:

/override ci/prow/ci-e2e-install-hypershift

this is still expected

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-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 27, 2024

openshift-ci bot requested review from ffromani and mrniranjan September 27, 2024 10:56

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

shajmakh mentioned this pull request Oct 15, 2024

api: add PoolName and NodeGroupStatus #1018

Merged

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

shajmakh force-pushed the HCP-adaptations branch from 35e2ad6 to 82e9115 Compare October 30, 2024 15:54

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

shajmakh force-pushed the HCP-adaptations branch from 82e9115 to a0c30fd Compare October 30, 2024 15:56

shajmakh changed the title ~~WIP: Hypershift RTE adaptations~~ ctrl: HCP adaptations for NodeGroup validation and RTE Oct 30, 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 Oct 30, 2024

Tal-or reviewed Nov 3, 2024

View reviewed changes

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

Tal-or reviewed Nov 11, 2024

View reviewed changes

api/numaresourcesoperator/v1/helper/nodegroup/nodegroup.go Show resolved Hide resolved

shajmakh mentioned this pull request Nov 13, 2024

e2e: mustgather: fix test to verify TAS enabled nodes only #1068

Merged

Tal-or mentioned this pull request Nov 19, 2024

e2e: refactor NRO deployment #1071

Merged

ffromani reviewed Nov 19, 2024

View reviewed changes

shajmakh mentioned this pull request Nov 20, 2024

Support NROP controller tests on hypershift #1083

Merged

shajmakh force-pushed the HCP-adaptations branch 4 times, most recently from f033f45 to 7f40dbf Compare November 20, 2024 12:44

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

shajmakh force-pushed the HCP-adaptations branch from 7f40dbf to 75c629e Compare November 20, 2024 12:45

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

ctrl: HCP adaptations for NodeGroup validation and RTE

5083043

Adapt the possibility of running the operator on Hypershift platform by replacing MCP context with PoolName where relevant; In addition to extra NodeGroup validation on hypershift. Signed-off-by: Shereen Haj <[email protected]>

shajmakh force-pushed the HCP-adaptations branch from 75c629e to bcac52d Compare November 20, 2024 12:52

ffromani reviewed Nov 20, 2024

View reviewed changes

openshift-ci bot assigned ffromani Nov 20, 2024

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

ffromani merged commit 576c0cc into openshift-kni:main Nov 20, 2024
13 of 15 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ctrl: HCP adaptations for NodeGroup validation and RTE #1027

ctrl: HCP adaptations for NodeGroup validation and RTE #1027

shajmakh commented Sep 27, 2024 •

edited

Loading

openshift-ci bot commented Sep 27, 2024

shajmakh commented Sep 27, 2024

shajmakh commented Oct 31, 2024

Tal-or left a comment

Tal-or commented Nov 3, 2024

ffromani left a comment

ffromani Nov 19, 2024

ffromani Nov 20, 2024

ffromani Nov 19, 2024

ffromani Nov 20, 2024

Tal-or commented Nov 19, 2024 •

edited

Loading

ffromani left a comment

ffromani commented Nov 20, 2024

openshift-ci bot commented Nov 20, 2024

ctrl: HCP adaptations for NodeGroup validation and RTE #1027

ctrl: HCP adaptations for NodeGroup validation and RTE #1027

Conversation

shajmakh commented Sep 27, 2024 • edited Loading

openshift-ci bot commented Sep 27, 2024

shajmakh commented Sep 27, 2024

shajmakh commented Oct 31, 2024

Tal-or left a comment

Choose a reason for hiding this comment

Tal-or commented Nov 3, 2024

ffromani left a comment

Choose a reason for hiding this comment

ffromani Nov 19, 2024

Choose a reason for hiding this comment

ffromani Nov 20, 2024

Choose a reason for hiding this comment

ffromani Nov 19, 2024

Choose a reason for hiding this comment

ffromani Nov 20, 2024

Choose a reason for hiding this comment

Tal-or commented Nov 19, 2024 • edited Loading

ffromani left a comment

Choose a reason for hiding this comment

ffromani commented Nov 20, 2024

openshift-ci bot commented Nov 20, 2024

shajmakh commented Sep 27, 2024 •

edited

Loading

Tal-or commented Nov 19, 2024 •

edited

Loading