HOSTEDCP-2018: Reorder Nodepool conditions and move nto reconcile logic #4859

enxebre · 2024-10-08T09:38:20Z

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-robot · 2024-10-08T09:38:25Z

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

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-10-08T09:38:27Z

/hold a bit

openshift-ci · 2024-10-08T09:39:18Z

[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

netlify · 2024-10-08T09:39:36Z

✅ Deploy Preview for hypershift-docs ready!

Name	Link
🔨 Latest commit	`53880a3`
🔍 Latest deploy log	https://app.netlify.com/sites/hypershift-docs/deploys/6704fd8f844f91000895342c
😎 Deploy Preview	https://deploy-preview-4859--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.

openshift-ci-robot · 2024-10-08T09:42:16Z

@enxebre: This pull request references HOSTEDCP-1976 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 epic to target the "4.18.0" version, but no target version was set.

In response to this:

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.

Patryk-Stefanski · 2024-10-08T10:36:11Z

hypershift-operator/controllers/nodepool/nto.go

+			return reconcilePerformanceProfileConfigMap(performanceProfileConfigMap, nodePool, performanceProfileConfig)
+		})
+		if err != nil {
+			fmt.Errorf("failed to reconcile PerformanceProfile ConfigMap: %w", err)


missing return here

thanks! fixed.

muraee · 2024-10-08T11:05:02Z

hypershift-operator/controllers/nodepool/nodepool_controller.go

 	infraID := hcluster.Spec.InfraID

+	// Loop over all conditions.
+	// Order matter as conditions might choose to short circuit returning ctrl.Result or error.
+	signalConditions := map[string]func(ctx context.Context, nodePool *hyperv1.NodePool, hcluster *hyperv1.HostedCluster, conditionType string) (*ctrl.Result, error){


why do we need to pass conditionType to the functions, shouldn't each function know which condition it is updating?

yes, it's not programmatically needed. It seemed easier to grasp at a glance to me what conditions are being applied. They can also be used to unit test everything in the index end up set.
I'm happy to change the approach to something else if you prefer.

muraee · 2024-10-08T11:07:18Z

hypershift-operator/controllers/nodepool/nodepool_controller.go

+	_, err := r.getReleaseImage(ctx, hcluster, nodePool.Status.Version, nodePool.Spec.Release.Image)
+	if err != nil {
+		SetStatusCondition(&nodePool.Status.Conditions, hyperv1.NodePoolCondition{
+			Type:               conditionType,


related to the question above, imo it would be more clear for someone reading those functions later if the condition type is explicitly set here instead of the passed in conditionType parameter.

This centralizes all conditions logic in a sincle place to improve readability. It also enables further changes to add/collapse conditions consistently.

…ot found Since this runs early now, shortcicuiting would never give the token secret the chance to be reconciled

muraee · 2024-10-08T13:14:37Z

/lgtm

enxebre · 2024-10-09T08:46:37Z

eventually.go:379: observed invalid **v1beta1.NodePool state after 10m0s
eventually.go:396: - observed **v1beta1.NodePool e2e-clusters-blx2m/example-cvjf7-test-ntoperformanceprofile invalid: condition performance.operator.openshift.io/Available is not present

/retest

openshift-ci · 2024-10-09T16:37:55Z

New changes are detected. LGTM label has been removed.

openshift-ci · 2024-10-10T12:30:36Z

@enxebre: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-4-17	`ee4e243`	link	true	`/test e2e-aws-4-17`
ci/prow/e2e-aws	`ee4e243`	link	true	`/test e2e-aws`

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-robot · 2024-10-14T14:08:33Z

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

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.

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

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

openshift-ci bot requested review from csrwng and isco-rodriguez October 8, 2024 09:39

openshift-ci bot added area/hypershift-operator Indicates the PR includes changes for the hypershift operator and API - outside an OCP release and removed do-not-merge/needs-area labels Oct 8, 2024

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

enxebre changed the title ~~HOSTEDCP-1989: Reorder Nodepool conditions and move nto reconcile logic~~ HOSTEDCP-1989 / HOSTEDCP-1976: Reorder Nodepool conditions and move nto reconcile logic Oct 8, 2024

Patryk-Stefanski reviewed Oct 8, 2024

View reviewed changes

enxebre force-pushed the nodepool-conditions branch from 53880a3 to 95a1526 Compare October 8, 2024 10:43

muraee reviewed Oct 8, 2024

View reviewed changes

enxebre added 2 commits October 8, 2024 14:56

Refactor NodePool conditions into a centralized loop

2b16e00

This centralizes all conditions logic in a sincle place to improve readability. It also enables further changes to add/collapse conditions consistently.

Let conditions getting the token secret to not shortcircuit when is n…

3826dd6

…ot found Since this runs early now, shortcicuiting would never give the token secret the chance to be reconciled

enxebre force-pushed the nodepool-conditions branch from cde7c1a to 67891b5 Compare October 8, 2024 12:56

openshift-ci bot assigned muraee Oct 8, 2024

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

enxebre force-pushed the nodepool-conditions branch from 67891b5 to b5d7b6a Compare October 9, 2024 16:37

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

enxebre force-pushed the nodepool-conditions branch from b5d7b6a to 74b0389 Compare October 9, 2024 18:58

Move NodePool conditions logic into their own file

7f3239d

Drop type from the signature

ee4e243

enxebre force-pushed the nodepool-conditions branch from 74b0389 to ee4e243 Compare October 10, 2024 10:01

enxebre changed the title ~~HOSTEDCP-1989 / HOSTEDCP-1976: Reorder Nodepool conditions and move nto reconcile logic~~ HOSTEDCP-2018: Reorder Nodepool conditions and move nto reconcile logic Oct 14, 2024

enxebre mentioned this pull request Oct 15, 2024

WIP: HCCO: copy KubeletConfigs to hosted-cluster #4833

Open

7 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HOSTEDCP-2018: Reorder Nodepool conditions and move nto reconcile logic #4859

HOSTEDCP-2018: Reorder Nodepool conditions and move nto reconcile logic #4859

enxebre commented Oct 8, 2024

openshift-ci-robot commented Oct 8, 2024 •

edited by openshift-ci bot

Loading

enxebre commented Oct 8, 2024

openshift-ci bot commented Oct 8, 2024

netlify bot commented Oct 8, 2024

openshift-ci-robot commented Oct 8, 2024 •

edited by openshift-ci bot

Loading

Patryk-Stefanski Oct 8, 2024

enxebre Oct 8, 2024

muraee Oct 8, 2024

enxebre Oct 8, 2024

muraee Oct 8, 2024 •

edited

Loading

muraee commented Oct 8, 2024

enxebre commented Oct 9, 2024

openshift-ci bot commented Oct 9, 2024

openshift-ci bot commented Oct 10, 2024

openshift-ci-robot commented Oct 14, 2024 •

edited by openshift-ci bot

Loading

HOSTEDCP-2018: Reorder Nodepool conditions and move nto reconcile logic #4859

Are you sure you want to change the base?

HOSTEDCP-2018: Reorder Nodepool conditions and move nto reconcile logic #4859

Conversation

enxebre commented Oct 8, 2024

openshift-ci-robot commented Oct 8, 2024 • edited by openshift-ci bot Loading

enxebre commented Oct 8, 2024

openshift-ci bot commented Oct 8, 2024

netlify bot commented Oct 8, 2024

✅ Deploy Preview for hypershift-docs ready!

openshift-ci-robot commented Oct 8, 2024 • edited by openshift-ci bot Loading

Patryk-Stefanski Oct 8, 2024

Choose a reason for hiding this comment

enxebre Oct 8, 2024

Choose a reason for hiding this comment

muraee Oct 8, 2024

Choose a reason for hiding this comment

enxebre Oct 8, 2024

Choose a reason for hiding this comment

muraee Oct 8, 2024 • edited Loading

Choose a reason for hiding this comment

muraee commented Oct 8, 2024

enxebre commented Oct 9, 2024

openshift-ci bot commented Oct 9, 2024

openshift-ci bot commented Oct 10, 2024

openshift-ci-robot commented Oct 14, 2024 • edited by openshift-ci bot Loading

openshift-ci-robot commented Oct 8, 2024 •

edited by openshift-ci bot

Loading

openshift-ci-robot commented Oct 8, 2024 •

edited by openshift-ci bot

Loading

muraee Oct 8, 2024 •

edited

Loading

openshift-ci-robot commented Oct 14, 2024 •

edited by openshift-ci bot

Loading