HOSTEDCP-1984: Refactor capi logic out from NodePool controller #4795

enxebre · 2024-09-25T09:21:14Z

This moves the CAPI related logic into their own file and add test coverage particularly for the reconcile function. Additional refactor of the business logic itself is left out intentionally for now to contain the scope of the refactor and avoid backward compatibility issues.

➜  hypershift git:(nodepool-refactor-capi) ✗ go tool cover -func=coverage.out | grep capi
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:45:			newCAPI						100.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:60:			Reconcile					65.2%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:175:			cleanupMachineTemplates				78.1%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:235:			deleteMachineDeployment				30.8%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:256:			pauseMachineDeployment				70.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:274:			deleteMachineSet				30.8%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:295:			Pause						60.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:309:			pauseMachineSet					70.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:329:			deleteMachineHealthCheck			30.8%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:350:			reconcileMachineDeployment			72.2%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:571:			taintsToJSON					75.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:580:			reconcileMachineHealthCheck			96.4%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:662:			setMachineDeploymentReplicas			100.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:695:			machineTemplateBuilders				26.3%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:852:			generateMachineTemplateName			100.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:859:			reconcileMachineSet				0.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1112:			machineSetInPlaceRolloutIsComplete		0.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1118:			setMachineSetReplicas				78.6%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1148:			getInPlaceMaxUnavailable			90.9%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1166:			machineDeployment				100.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1175:			machineSet					100.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1184:			machineHealthCheck				100.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1195:			generateName					100.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1204:			getName						66.7%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1230:			max						0.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1238:			min						0.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1245:			listMachineTemplates				59.3%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1306:			ensureMachineDeletion				66.7%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1321:			getMachinesForNodePool				90.0%

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes #

Checklist

Subject and description added to both, commit and PR.
Relevant issues have been referenced.
This change includes docs.
This change includes unit tests.

openshift-ci · 2024-09-25T09:21:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: enxebre

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

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

muraee · 2024-09-25T10:05:21Z

hypershift-operator/controllers/nodepool/capi.go

+	if err != nil {
+		return err
+	}
+	if result, err := c.CreateOrUpdate(ctx, c.Client, template, func() error {


I think we are introducing a tight coupling with the Token struct by using its client, createOrUpdate, nodePool, etc. fields.
should we explicitly pass those options to the Reconcile() function or add them as fields to CAPI?

I would imagine the Token object we pass to CAPI to just be an interface exposing the functions CAPI needs.

I agree, I'd expect that's exactly the end state (fields in capi + interface) and even consider separate packages so unexported fields are not accessible. I chose coupling for simplicity for now, the fields are accessible within the same package anyways and otherwise you would endup with "duplicated" fields which is also confusing. Any follow up refinement in that direction can be done now with confidence since we have decent coverage.
Added a TODO

muraee · 2024-09-25T10:34:46Z

hypershift-operator/controllers/nodepool/capi.go

+		}
+		return fmt.Errorf("error getting MachineHealthCheck: %w", err)
+	}
+	if mhc.DeletionTimestamp != nil {


why do we need to fetch the object and check the DeletionTimestamp first? isn't Delete a no-op if the object is already deleting?

This is just the business logic of the original function. I'd prefer to keep the scope of this PR small to reduce risk of regression and let any follow up iteration be test driven now we have decent coverage.
Added a TODO

muraee · 2024-09-25T10:36:13Z

hypershift-operator/controllers/nodepool/capi.go

+	// TODO(Alberto): drop this an rely on core in-place propagation once CAPI 1.4.0 https://github.com/kubernetes-sigs/cluster-api/releases comes through the payload.
+	// https://issues.redhat.com/browse/HOSTEDCP-971
+	machineList := &capiv1.MachineList{}
+	if err := c.List(context.TODO(), machineList, client.InNamespace(machineDeployment.Namespace)); err != nil {


I think we should pass a proper context here instead of context.TODO()

muraee · 2024-09-25T10:36:34Z

hypershift-operator/controllers/nodepool/capi.go

+			machine.Labels = make(map[string]string)
+		}
+
+		if result, err := controllerutil.CreateOrPatch(context.TODO(), c.Client, &machine, func() error {


I think we should pass a proper context here instead of context.TODO()

netlify · 2024-09-25T10:56:26Z

✅ Deploy Preview for hypershift-docs ready!

Name	Link
🔨 Latest commit	`ab740d1`
🔍 Latest deploy log	https://app.netlify.com/sites/hypershift-docs/deploys/66f3ec0db7f77a0008736074
😎 Deploy Preview	https://deploy-preview-4795--hypershift-docs.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

bryan-cox · 2024-09-25T10:56:37Z

hypershift-operator/controllers/nodepool/aws.go

+	if nodePool.Spec.Platform.AWS.AMI != "" {
+		ami = nodePool.Spec.Platform.AWS.AMI
+	} else {
+		// TODO: Should the region be included in the NodePool platform information?


Do we still need this TODO?

Yes, I don't want to couple dropping that with this PR. fwiw this is just moving existing code around, intentionally modifying as less as possible in this PR and increasing coverage to an acceptable bar to enable further changes

bryan-cox · 2024-09-25T11:00:47Z

hypershift-operator/controllers/nodepool/capi.go

+	// Check if platform machine template needs to be updated.
+	targetMachineTemplate := template.GetName()
+	if isUpdatingMachineTemplate(nodePool, targetMachineTemplate) {
+		// TODO (alberto): deocuple all conditions handling from this file into nodepool_controller.go dedicated function.


We should tag Jira tickets to TODOs in the code so we don't lose track of items like this.

bryan-cox · 2024-09-25T11:02:30Z

hypershift-operator/controllers/nodepool/capi.go

+	// Set defaults. These are normally set by the CAPI machinedeployment webhook.
+	// However, since we don't run the webhook, CAPI updates the machinedeployment
+	// after it has been created with defaults.
+	machineDeployment.Spec.MinReadySeconds = k8sutilspointer.Int32(0)


k8sutilspointer.Int32 is deprecated. These should be updated to use ptr.To[int32]

bryan-cox · 2024-09-25T11:03:20Z

hypershift-operator/controllers/nodepool/capi.go

+				Kind:       gvk.Kind,
+				APIVersion: gvk.GroupVersion().String(),
+				Namespace:  machineTemplateCR.GetNamespace(),
+				// keep current tempalte name for later check.


Suggested change

// keep current tempalte name for later check.

// keep current template name for later check.

bryan-cox · 2024-09-25T11:04:56Z

hypershift-operator/controllers/nodepool/capi.go

+	targetVersion := c.Version()
+	targetConfigHash := c.HashWithoutVersion()
+	targetConfigVersionHash := c.Hash()
+	if userDataSecret.Name != k8sutilspointer.StringDeref(machineDeployment.Spec.Template.Spec.Bootstrap.DataSecretName, "") {


StringDeref is also deprecated. Should use ptr.Deref

bryan-cox · 2024-09-25T11:09:56Z

hypershift-operator/controllers/nodepool/capi.go

+}
+
+func deleteMachineSet(ctx context.Context, c client.Client, ms *capiv1.MachineSet) error {
+	// TODO(alberto): why do we need to fetch the object and check the DeletionTimestamp first?


TODO should link to a Jira ticket

bryan-cox · 2024-09-25T11:10:07Z

hypershift-operator/controllers/nodepool/capi.go

+}
+
+func deleteMachineHealthCheck(ctx context.Context, c client.Client, mhc *capiv1.MachineHealthCheck) error {
+	// TODO(alberto): why do we need to fetch the object and check the DeletionTimestamp first?


TODO should link to a Jira ticket

bryan-cox · 2024-09-25T11:11:20Z

hypershift-operator/controllers/nodepool/capi.go

+	}
+}
+
+// TODO (alberto) drop this deterministic naming logic and get the name for child MachineDeployment from the status/annotation/label?


TODO should link to a Jira ticket

bryan-cox · 2024-09-25T11:11:39Z

hypershift-operator/controllers/nodepool/capi.go

+	return filtered, nil
+}
+
+// TODO (alberto): Let the all the deletion logic be a capi func.


TODO should link to a Jira ticket

bryan-cox · 2024-09-25T11:12:34Z

hypershift-operator/controllers/nodepool/capi_test.go

+}
+
+func TestMachineTemplateBuildersPreexisting(t *testing.T) {
+	//RunTestMachineTemplateBuilders(t, true)


Does this need uncommented?

This moves the CAPI related logic into their own file and add test coverage particularly for the reconcile function. Additional refactor of the business logic itself is left out intentionally for now to contain the scope of the refactor and avoid backward compatibility issues.

enxebre · 2024-09-25T11:30:39Z

TODO should link to a Jira ticket

Fleshing out all the follow ups for these series of refactors is a task on its own not to be done in this PR yet. The TODOs are the placeholders for that task.

enxebre · 2024-09-25T11:41:00Z

/retest

muraee · 2024-09-25T14:17:54Z

/lgtm

openshift-ci-robot · 2024-09-26T06:59:08Z

@enxebre: This pull request references HOSTEDCP-1984 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:

This moves the CAPI related logic into their own file and add test coverage particularly for the reconcile function. Additional refactor of the business logic itself is left out intentionally for now to contain the scope of the refactor and avoid backward compatibility issues.

➜  hypershift git:(nodepool-refactor-capi) ✗ go tool cover -func=coverage.out | grep capi
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:45:			newCAPI						100.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:60:			Reconcile					65.2%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:175:			cleanupMachineTemplates				78.1%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:235:			deleteMachineDeployment				30.8%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:256:			pauseMachineDeployment				70.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:274:			deleteMachineSet				30.8%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:295:			Pause						60.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:309:			pauseMachineSet					70.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:329:			deleteMachineHealthCheck			30.8%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:350:			reconcileMachineDeployment			72.2%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:571:			taintsToJSON					75.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:580:			reconcileMachineHealthCheck			96.4%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:662:			setMachineDeploymentReplicas			100.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:695:			machineTemplateBuilders				26.3%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:852:			generateMachineTemplateName			100.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:859:			reconcileMachineSet				0.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1112:			machineSetInPlaceRolloutIsComplete		0.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1118:			setMachineSetReplicas				78.6%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1148:			getInPlaceMaxUnavailable			90.9%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1166:			machineDeployment				100.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1175:			machineSet					100.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1184:			machineHealthCheck				100.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1195:			generateName					100.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1204:			getName						66.7%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1230:			max						0.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1238:			min						0.0%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1245:			listMachineTemplates				59.3%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1306:			ensureMachineDeletion				66.7%
github.com/openshift/hypershift/hypershift-operator/controllers/nodepool/capi.go:1321:			getMachinesForNodePool				90.0%

What this PR does / why we need it:

Which issue(s) this PR fixes (optional, use fixes #<issue_number>(, fixes #<issue_number>, ...) format, where issue_number might be a GitHub issue, or a Jira story:
Fixes #

Checklist

Subject and description added to both, commit and PR.
Relevant issues have been referenced.
This change includes docs.
This change includes unit tests.

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.

enxebre · 2024-09-26T06:59:11Z

/jira refresh

openshift-ci-robot · 2024-09-26T06:59:14Z

@enxebre: This pull request references HOSTEDCP-1984 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:

/jira refresh

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.

openshift-ci-robot · 2024-09-26T11:43:50Z

/retest-required

Remaining retests: 0 against base HEAD 5ad3d23 and 2 for PR HEAD d965bf9 in total

openshift-ci · 2024-09-26T14:28:31Z

@enxebre: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

openshift-ci bot added the do-not-merge/needs-area label Sep 25, 2024

openshift-ci bot requested review from nirarg and sjenning September 25, 2024 09:21

openshift-ci bot added the area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release label Sep 25, 2024

openshift-ci bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. and removed do-not-merge/needs-area labels Sep 25, 2024

enxebre force-pushed the nodepool-refactor-capi branch from 0a9c1c5 to bc26251 Compare September 25, 2024 09:35

muraee reviewed Sep 25, 2024

View reviewed changes

enxebre force-pushed the nodepool-refactor-capi branch from bc26251 to ab740d1 Compare September 25, 2024 10:55

enxebre force-pushed the nodepool-refactor-capi branch from ab740d1 to 0b4cd22 Compare September 25, 2024 11:09

bryan-cox reviewed Sep 25, 2024

View reviewed changes

enxebre force-pushed the nodepool-refactor-capi branch from 0b4cd22 to c056c49 Compare September 25, 2024 11:24

enxebre force-pushed the nodepool-refactor-capi branch from c056c49 to d965bf9 Compare September 25, 2024 11:24

openshift-ci bot assigned muraee Sep 25, 2024

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

enxebre changed the title ~~Refactor capi logic out from NodePool controller~~ HOSTEDCP-1984: Refactor capi logic out from NodePool controller Sep 26, 2024

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Sep 26, 2024

openshift-merge-bot bot merged commit 238c6ac into openshift:main Sep 26, 2024
15 checks passed

davidvossel mentioned this pull request Oct 3, 2024

NO-JIRA: Fixes kubevirt image cacher #4847

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HOSTEDCP-1984: Refactor capi logic out from NodePool controller #4795

HOSTEDCP-1984: Refactor capi logic out from NodePool controller #4795

enxebre commented Sep 25, 2024

openshift-ci bot commented Sep 25, 2024

muraee Sep 25, 2024 •

edited

Loading

enxebre Sep 25, 2024

muraee Sep 25, 2024

enxebre Sep 25, 2024 •

edited

Loading

muraee Sep 25, 2024

muraee Sep 25, 2024 •

edited

Loading

netlify bot commented Sep 25, 2024

bryan-cox Sep 25, 2024

enxebre Sep 25, 2024

bryan-cox Sep 25, 2024

bryan-cox Sep 25, 2024

bryan-cox Sep 25, 2024

bryan-cox Sep 25, 2024

bryan-cox Sep 25, 2024

bryan-cox Sep 25, 2024

bryan-cox Sep 25, 2024

bryan-cox Sep 25, 2024

bryan-cox Sep 25, 2024

enxebre commented Sep 25, 2024

enxebre commented Sep 25, 2024

muraee commented Sep 25, 2024

openshift-ci-robot commented Sep 26, 2024 •

edited by openshift-ci bot

Loading

enxebre commented Sep 26, 2024

openshift-ci-robot commented Sep 26, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Sep 26, 2024

openshift-ci bot commented Sep 26, 2024

	// keep current tempalte name for later check.
	// keep current template name for later check.

HOSTEDCP-1984: Refactor capi logic out from NodePool controller #4795

HOSTEDCP-1984: Refactor capi logic out from NodePool controller #4795

Conversation

enxebre commented Sep 25, 2024

openshift-ci bot commented Sep 25, 2024

muraee Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

enxebre Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muraee Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

netlify bot commented Sep 25, 2024

✅ Deploy Preview for hypershift-docs ready!

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

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

enxebre commented Sep 25, 2024

enxebre commented Sep 25, 2024

muraee commented Sep 25, 2024

openshift-ci-robot commented Sep 26, 2024 • edited by openshift-ci bot Loading

enxebre commented Sep 26, 2024

openshift-ci-robot commented Sep 26, 2024 • edited by openshift-ci bot Loading

openshift-ci-robot commented Sep 26, 2024

openshift-ci bot commented Sep 26, 2024

muraee Sep 25, 2024 •

edited

Loading

enxebre Sep 25, 2024 •

edited

Loading

muraee Sep 25, 2024 •

edited

Loading

openshift-ci-robot commented Sep 26, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Sep 26, 2024 •

edited by openshift-ci bot

Loading