scheduler controller: resource status cleanup #1715
Conversation
syncNUMASchedulerResources() is constructing the sched status and later in the reconcile loop this status is overriding the main instance status. Concerning status conditions, the only situation where the conditions are not set is on first creation of the scheduler CR, the rest of the cases the conditions are there but they are continuously overridden with nil in syncNUMASchedulerResources(). This in turn leads to updating the conditions in r.updateStatus() whenever it is invoked regardless of the actual existing conditions. In this commit, ensure that the original current status is the base of the new one. Signed-off-by: Shereen Haj <[email protected]>
The goal of `IsUpdatedNUMAResourceOperator()` is to verify if an update is needed for the resource status or not. Clarify that by changing the name of the function to `NUMAResourceOperatorNeedsUpdate()` so it'll be invoked as `status.NUMAResourceOperatorNeedsUpdate()`. Signed-off-by: Shereen Haj <[email protected]>
So far updatinf the status was done in every reconcilation iteration even if the spec fields reflected the same values. This commits saves a copy of the initial resource status and uses it as a reference to identify if the resource requires a status update or not and act accordingly. Signed-off-by: Shereen Haj <[email protected]>
openshift-ci
bot
added
the
approved
|
@shajmakh: The following test failed, say
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. |
| func NUMAResourcesSchedulerNeedsUpdate(oldStatus, newStatus nropv1.NUMAResourcesSchedulerStatus) bool { | ||
| os := oldStatus.DeepCopy() | ||
| ns := newStatus.DeepCopy() | ||
|
|
||
| resetIncomparableConditionFields(os.Conditions) | ||
| resetIncomparableConditionFields(ns.Conditions) | ||
|
|
||
| return !reflect.DeepEqual(os, ns) | ||
| } | ||
|
|
There was a problem hiding this comment.
duplicates the function right above
There was a problem hiding this comment.
no, different object: scheduler vs operator
| @@ -191,6 +192,58 @@ func TestNUMAResourceOperatorNeedsUpdate(t *testing.T) { | |||
| } | |||
| } | |||
|
|
|||
| func TestNUMAResourcesSchedulerNeedsUpdate(t *testing.T) { | |||
There was a problem hiding this comment.
does this add coverage? do we want in the previous commit?
There was a problem hiding this comment.
no, different object: scheduler vs operator
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ffromani, 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:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 64a17ad
into
openshift-kni:main
This PR fixes status reporting issues and is a preliminary for adding scheduler informer reporting under status conditions.
please see commits for details.