cleanup and streamline status computation #1032

ffromani · 2024-10-03T13:51:45Z

The way we computed the NUMAResourcesOperator status was messy, relying on inner functions and helpers to reported somehow if something changed in the object, abusing the name of the conditions (leading to awkward function signatures), doing and undoing checks and so forth.

Besides messy and unecessarily hard to read code, the outcome was that we both sometimes missed to update the status, leading to not major bugs (yet) but to less than ideal experience.

To untangle this mess, the new approach is to just mutate the status freely during the reconciliation loop in the reconciliation sub-step. Every step is free to mutate the status just reporting error or not, and the top-level reconciliation code will detect semantic differences (e.g. if only timestamps changed but say nothing else, this is not a semantically relevant difference so no actual update should be sent).

Detecting changes this way needs nested comparations of objects but it's a very major simplifications; if we implement carefully the comparison code (coming soon), benchmarks show good numbers (= still some theoretical slowdowns, but the overall state is not terrible and better than expected) wrt the current implementation, so win-win.

openshift-ci · 2024-10-03T13:51:57Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ffromani

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~~ [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-03T13:52:02Z

/hold

need to wait for all the HCP work to go in anyway

ffromani · 2024-10-28T15:25:47Z

/cc @shajmakh

ffromani · 2024-10-29T07:34:27Z

/hold cancel

ffromani · 2024-10-29T11:18:55Z

@shajmakh hey! let's discuss the cleanups in the last 2 commits to see if they can help you

shajmakh

Thanks for this!
reviews are related to the last 2 commits

shajmakh · 2024-10-29T11:29:49Z

controllers/controllers.go

+	return conditionInfo{
+		Type:    status.ConditionDegraded,
+		Message: messageFromError(err),
+	}


should we add reason here?
Reason: reasonFromError(err)

we probably should, yes. It should be helpful and harmless.

shajmakh · 2024-10-29T11:35:23Z

controllers/numaresourcesoperator_controller.go

 	if ok {
 		instance.Status.Conditions = conditions
 	}
 }

 func (r *NUMAResourcesOperatorReconciler) degradeStatus(ctx context.Context, instance *nropv1.NUMAResourcesOperator, reason string, stErr error) (ctrl.Result, error) {
-	message := messageFromError(stErr)
+	info := degradedConditionInfoFromError(stErr)
+	info.Reason = reason


if reason is empty it will override info.Reason (keep it as "InternalError"?) if we set it above Reason: reasonFromError(err)

indeed, good catch. I think I fixed in the later commits by rearranging the flow.

shajmakh · 2024-10-29T11:37:13Z

controllers/numaresourcesoperator_controller.go

@@ -207,25 +208,27 @@ func (r *NUMAResourcesOperatorReconciler) degradeStatus(ctx context.Context, ins
 	return ctrl.Result{}, nil
 }

-func (r *NUMAResourcesOperatorReconciler) reconcileResourceAPI(ctx context.Context, instance *nropv1.NUMAResourcesOperator, trees []nodegroupv1.Tree) (bool, ctrl.Result, string, error) {
+func (r *NUMAResourcesOperatorReconciler) reconcileResourceAPI(ctx context.Context, instance *nropv1.NUMAResourcesOperator, trees []nodegroupv1.Tree) (bool, ctrl.Result, conditionInfo, error) {


I don't see a reason to keep the returned error, it is already part of the returned conditionInfo

same below in the other reconciliation substeps

that's a good point. What it should go is not the top level error though, which is used to report to the upper layers, but the inner error in ConditionInfo. Let's see what I can do.

s4cratch that, fixed in the following commit

controllers/numaresourcesoperator_controller.go

controllers/controllers.go

Inline status update in the happy path, if the reconciliation loop completed all the expected steps. This is a intermediate step towards the final cleanup, and should cause no changes in behavior. Signed-off-by: Francesco Romani <fromani@redhat.com>

Signed-off-by: Francesco Romani <fromani@redhat.com>

Instead of relying in the chain of helpers to report if something changed in status, thus warrants a update, run a full diff of our status explictely. If something worthy (whose definition depends on the helper implemented in `pkg/status`) changed, then we will push a status update. This is probably slower than relying on the reports from subfunctions, but simplifies and streamlines the code significantly. Signed-off-by: Francesco Romani <fromani@redhat.com>

The only reason why we update the status conditions outside reconcileResources, while we update everything else related to status inside, is historical. We are now enabled to close this gap and streamline the code further. Signed-off-by: Francesco Romani <fromani@redhat.com>

we never use the return value, good riddance. Signed-off-by: Francesco Romani <fromani@redhat.com>

instead of passing condition types, possibly message, maybe error, then derive the full condition data in many place, factor all the data in a condition info struct, to be used as basis for creating the real metav1.Condition. This clean up things and unlocks further cleanups. Signed-off-by: Francesco Romani <fromani@redhat.com>

ffromani · 2024-10-30T19:05:50Z

/hold
need to test

the reconciliation steps are returning a common (and growing) set of values, let's pack them in a struct, since we always want to return the same tuple anyway for consistency. Signed-off-by: Francesco Romani <fromani@redhat.com>

ffromani · 2024-10-31T08:03:13Z

know controller test failure. It's legit. I'll have a look and fix ASAP

openshift-ci · 2024-10-31T08:20:50Z

@ffromani: The following test 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/ci-unit	`b4f8b44`	link	true	`/test ci-unit`

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/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 3, 2024

openshift-ci bot requested review from swatisehgal and Tal-or October 3, 2024 13:51

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 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 3, 2024

ffromani mentioned this pull request Oct 3, 2024

WIP: adjust the operator log level programmatically #1022

Open

ffromani force-pushed the cleanup-status-computation branch 5 times, most recently from 4cb9131 to 75c5334 Compare October 9, 2024 07:39

ffromani force-pushed the cleanup-status-computation branch from 75c5334 to c425e08 Compare October 11, 2024 08:19

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

ffromani force-pushed the cleanup-status-computation branch from c425e08 to 1ed3294 Compare October 23, 2024 07:43

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

ffromani force-pushed the cleanup-status-computation branch from 1ed3294 to 153f5c7 Compare October 24, 2024 07:21

ffromani mentioned this pull request Oct 24, 2024

Prepare for cleaning up status computation #1057

Merged

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

ffromani force-pushed the cleanup-status-computation branch from 153f5c7 to e223f7c Compare October 28, 2024 15:20

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

ffromani force-pushed the cleanup-status-computation branch from e223f7c to e25b479 Compare October 28, 2024 15:24

openshift-ci bot requested a review from shajmakh October 28, 2024 15:25

ffromani changed the title ~~WIP: cleanup status computation~~ cleanup and streamline status computation Oct 29, 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 29, 2024

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

ffromani mentioned this pull request Oct 29, 2024

cleanup PR: reflect SELinux policy in RTE DS and report Progressing condition message #1058

Open

shajmakh reviewed Oct 29, 2024

View reviewed changes

controllers/controllers.go Outdated Show resolved Hide resolved

shajmakh reviewed Oct 29, 2024

View reviewed changes

controllers/controllers.go Show resolved Hide resolved

ffromani added 6 commits October 30, 2024 20:03

ctrl: nrop: updateStatus -> degradeStatus

0f549ae

Signed-off-by: Francesco Romani <fromani@redhat.com>

controller: nrop: cleanup: fix updateStatusIfNeeded

22242e1

we never use the return value, good riddance. Signed-off-by: Francesco Romani <fromani@redhat.com>

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

ffromani force-pushed the cleanup-status-computation branch from edbe1bc to e2ecbe6 Compare October 30, 2024 19:11

ctrl: nrop: pack the common return args in a struct

b4f8b44

the reconciliation steps are returning a common (and growing) set of values, let's pack them in a struct, since we always want to return the same tuple anyway for consistency. Signed-off-by: Francesco Romani <fromani@redhat.com>

ffromani force-pushed the cleanup-status-computation branch from e2ecbe6 to b4f8b44 Compare October 31, 2024 08:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cleanup and streamline status computation #1032

cleanup and streamline status computation #1032

ffromani commented Oct 3, 2024 •

edited

Loading

openshift-ci bot commented Oct 3, 2024

ffromani commented Oct 3, 2024

ffromani commented Oct 28, 2024

ffromani commented Oct 29, 2024

ffromani commented Oct 29, 2024

shajmakh left a comment

shajmakh Oct 29, 2024

ffromani Oct 30, 2024

shajmakh Oct 29, 2024

ffromani Oct 30, 2024

shajmakh Oct 29, 2024

shajmakh Oct 29, 2024

ffromani Oct 30, 2024

ffromani Oct 30, 2024

ffromani commented Oct 30, 2024

ffromani commented Oct 31, 2024

openshift-ci bot commented Oct 31, 2024 •

edited

Loading

cleanup and streamline status computation #1032

Are you sure you want to change the base?

cleanup and streamline status computation #1032

Conversation

ffromani commented Oct 3, 2024 • edited Loading

openshift-ci bot commented Oct 3, 2024

ffromani commented Oct 3, 2024

ffromani commented Oct 28, 2024

ffromani commented Oct 29, 2024

ffromani commented Oct 29, 2024

shajmakh 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffromani commented Oct 30, 2024

ffromani commented Oct 31, 2024

openshift-ci bot commented Oct 31, 2024 • edited Loading

ffromani commented Oct 3, 2024 •

edited

Loading

openshift-ci bot commented Oct 31, 2024 •

edited

Loading