Handle InstallPlan approval based on spec.versions #199
Conversation
JustinKuli
requested review from
JeffeyL
and removed request for
mprahl and
gparvin
JustinKuli
force-pushed
the
9286-installplans
branch
3 times, most recently
from
7f3fd49
to
a7a3fd8
Compare
JustinKuli
force-pushed
the
9286-installplans
branch
from
a7a3fd8
to
d00ac42
Compare
JustinKuli
force-pushed
the
9286-installplans
branch
6 times, most recently
from
e84d62d
to
54dc084
Compare
|
I've finally got consistently passing tests here! Sorry for the noise, I think this is ready for review now. |
JustinKuli
force-pushed
the
9286-installplans
branch
from
54dc084
to
a737bd6
Compare
| phase = "" | ||
| } | ||
|
|
||
| relObj := policyv1.RelatedObject{ |
There was a problem hiding this comment.
nit: It might be too messy due to the added phase logic but putting this in a function might match the other implementation patterns better, something like func existingInstallPlanObj(ip unstructured.Unstructured, phase string) (policyv1.RelatedObject) {} or func existingInstallPlanObj(ip unstructured.Unstructured, phase string) (policyv1.RelatedObject, bool, bool) {} if the phase logic needs to be included.
There was a problem hiding this comment.
That kind of function would probably be nice for consistency. Because of the other stuff using phase, it's not a huge benefit, but I don't think it would be any messier.
| // FUTURE: check policy.spec.statusConfig.upgradesAvailable to determine `compliant`. | ||
| // For now this condition assumes it is set to 'NonCompliant' | ||
| cond := installPlanUpgradeCond(allUpgradeVersions) | ||
| cond.Message += " but not allowed by the specified versions in the policy" |
There was a problem hiding this comment.
Could be better for clarity to just add this as a case within the installPlanUpgradeCond() function unless there may be more future message modifications under the same case
There was a problem hiding this comment.
That would probably be nicer.
| // FUTURE: check policy.spec.statusConfig.upgradesAvailable to determine `compliant`. | ||
| // For now this condition assumes it is set to 'NonCompliant' | ||
| cond := installPlanUpgradeCond(allUpgradeVersions) | ||
| cond.Message += " but multiple of those match the versions specified in the policy" |
There was a problem hiding this comment.
Same as the message case for len(approvableInstallPlans) == 0
| } | ||
|
|
||
| // handle some special phases | ||
| switch phase { |
There was a problem hiding this comment.
From my understanding handleCSV didn't need to consider the Failed phase since it took the condition directly from the CSV itself, but in this case should we not report a failed InstallPlan?
There was a problem hiding this comment.
I should add a comment for this, because it's a great question.
My thinking was: we don't always want to report NonCompliance if there's a Failed InstallPlan, because InstallPlans can be left over from previous installations or upgrades. So if the "current" InstallPlan is good, reporting NonCompliant for an old Failed one would be bad/confusing.
But right now, this implementation doesn't even report that a current Failed InstallPlan would cause NonCompliance... that's probably not perfect, although I'd expect the Subscription or CSV in this situation would have some status that still causes the policy to be NonCompliant overall. The subscription does point to a current InstallPlan, maybe it can check that one specifically.
| "InstallPlan.Name", installPlan.GetName()) | ||
|
|
||
| // The InstallPlan will be added as unknown | ||
| phase = "" |
There was a problem hiding this comment.
Might be worth mentioning in the relevant relatedObj if the phase field wasn't found. A message in the relObj.Reason field looks like a good place to put it, unless this is required for functionality in another place in the controller.
JustinKuli
force-pushed
the
9286-installplans
branch
2 times, most recently
from
9ab77b0
to
d05f5b0
Compare
JustinKuli
force-pushed
the
9286-installplans
branch
from
d05f5b0
to
09679c5
Compare
|
|
||
| OpLog := ctrl.LoggerFrom(ctx) | ||
| relatedInstallPlans := make([]policyv1.RelatedObject, len(ownedInstallPlans)) | ||
| ipsRequiringApproval := make([]unstructured.Unstructured, 0) |
There was a problem hiding this comment.
Why this is size 0?
There was a problem hiding this comment.
I just prefer this to an alternate syntax like ipsRequiringApproval := [] unstructured.Unstructured{}.
It needs to be length 0 to start so that we can append an unknown number of items to it.
|
|
||
| err := r.updateStatus(ctx, policy, cond, nonCompObj(foundSub, subResFailed.Reason)) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("error updating the status for a Subscription in ResolutionFailed: %w", err) |
There was a problem hiding this comment.
(Optional) I think this could be clearer to include what's being set. Maybe something like:
| return nil, fmt.Errorf("error updating the status for a Subscription in ResolutionFailed: %w", err) | |
| return nil, fmt.Errorf("error updating the ResolutionFailed status to false for the Subscription: %w", err) |
controllers/operatorpolicy_status.go
Outdated
| Message: "a relevant InstallPlan is actively installing", | ||
| } | ||
|
|
||
| // installPlansFineCond is a Compliant condition with Reason 'InstallPlansFine' |
There was a problem hiding this comment.
"Fine" is too abstract/non-specific and I feel like it wouldn't be widely understood when translated. Unless this is some alignment with OLM, is there a better word like "Succeeded", "Complete", or "Installed"?
There was a problem hiding this comment.
I agree, I had no better ideas when I made it up 😆
I think originally I thought this would need to cover more cases than it does - so I deliberately avoided something like "Succeeded" because I expected it would cover a case where nothing had been installed yet... but now I think this situation is really just "there are no InstallPlans considered for approval", so I can report that as the condition.
JustinKuli
force-pushed
the
9286-installplans
branch
from
09679c5
to
b51e655
Compare
When enforcing a policy that specifies which versions should be allowed, the controller will now set the Subscription.InstallPlanApproval to Manual, and will approve only the InstallPlans that match the versions specified in the policy. The controller now also reports the status of related InstallPlans. Refs: - https://issues.redhat.com/browse/ACM-9286 Signed-off-by: Justin Kulikauskas <jkulikau@redhat.com>
JustinKuli
force-pushed
the
9286-installplans
branch
from
b51e655
to
83cc523
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: dhaiducek, JustinKuli, mprahl, yiraeChristineKim 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 |
|
/unhold |
openshift-merge-bot
bot
merged commit 41aa493
into
open-cluster-management-io:main
When enforcing a policy that specifies which versions should be allowed, the controller will now set the Subscription.InstallPlanApproval to Manual, and will approve only the InstallPlans that match the versions specified in the policy.
The controller now also reports the status of related InstallPlans.
Refs: