Add support for approving InstallPlans with multiple CSVs #260
Conversation
The operatorpolicy.policy.open-cluster-management.io/managed label is set to an empty value on the Subscription. The operatorpolicy.policy.open-cluster-management.io/managed annotation is set to <policy name>.<policy namespace>. The label doesn't set a value because the value could be too long. This will allow querying for subscriptions managed by operator policies. Signed-off-by: mprahl <mprahl@users.noreply.github.com>
mprahl
force-pushed
the
multiple-csvs-ip
branch
2 times, most recently
from
1462df0
to
522a4f6
Compare
| if containsCurrentCSV { | ||
| phase, _, _ := unstructured.NestedString(installPlans[i].Object, "status", "phase") | ||
| ipInstalling := string(operatorv1alpha1.InstallPlanPhaseInstalling) | ||
| ipComplete := string(operatorv1alpha1.InstallPlanPhaseComplete) |
There was a problem hiding this comment.
I understand prioritizing the Installing plan, but why also Complete plans? It feels like there can be multiple old InstallPlans that have that one, if the operator is done installing?
There was a problem hiding this comment.
@JustinKuli if an InstallPlan is complete for currentCSV (not installedCSV) then that means this InstallPlan successfully installed the latest CSV in the channel for this subscription. Otherwise, due to OLM race conditions and also just how it works, it's possible that there are multiple InstallPlans for currentCSV where one is approved and completed and one is awaiting approval.
|
@JustinKuli not sure what is going on in the tests in GitHub since it was saying that the test was expecting the I can't reproduce it locally, so I am rerunning it now. |
|
|
||
| // If there is more than one CSV in the InstallPlan, make sure this CSV failed before marking it as | ||
| // noncompliant. | ||
| for _, step := range latestInstallPlan.Status.Plan { |
There was a problem hiding this comment.
@JustinKuli I read the OLM code and it seems that the only successful final states are created and present (already there or updated). Do you think it's best to do this filtering or should we just mark the InstallPlan as failed even if this operator was successful?
I don't really know how I can get test coverage on this because clusterServiceVersionNames order in the InstallPlan are not deterministic so even if I have an InstallPlan with a failing operator, that one may have been processed first and not move on to the successful operator. I suppose I could modify the InstallPlan status to order the expected successful one but that seems quite hacky.
There was a problem hiding this comment.
I didn't fully understand this part, so if you think it can work without it, that would be fine? I don't love the idea of modifying the InstallPlan status
There was a problem hiding this comment.
@JustinKuli to clarify, modifying the InstallPlan status would be for test coverage.
So the question is, should an InstallPlan be marked as failed in the OperatorPolicy if the overall InstallPlan failed but the part for the operator being managed by OperatorPolicy succeeded?
mprahl
force-pushed
the
multiple-csvs-ip
branch
2 times, most recently
from
cb9e08a
to
acfa895
Compare
| allowedCSVs := make([]policyv1.NonEmptyString, 0, len(policy.Spec.Versions)+1) | ||
| allowedCSVs = append(allowedCSVs, policy.Spec.Versions...) | ||
|
|
||
| if sub.Spec.StartingCSV != "" { | ||
| allowedCSVs = append(allowedCSVs, policyv1.NonEmptyString(sub.Spec.StartingCSV)) | ||
| } |
There was a problem hiding this comment.
As a weird note, previously allowedVersions was the variable name.
In this review, it is difficult to tell what is new-new code and what is small changes to existing pieces. At least in GH, it's not very good at aligning the old and new... This is obviously not a major issue, but if there were some additional "anchor points" where there weren't changes (these six lines, for example), it might help git blame in the future.
There was a problem hiding this comment.
The side by side diff is easier for this.
| if len(policy.Spec.Versions) == 0 { | ||
| // currentCSV is the CSV related to the latest InstallPlan | ||
| if sub.Status.CurrentCSV != "" { | ||
| return sub.Status.CurrentCSV | ||
| } |
There was a problem hiding this comment.
Is there a guarantee that sub.Status.CurrentCSV is in csvNames?
There was a problem hiding this comment.
It feels like there could be items in csvNames that "should" be approved by a policy with no versions specified - I thought we were considering some sort of name matching in this case. Is that somewhere else? Or is it not necessary?
There was a problem hiding this comment.
@JustinKuli right. The OLM logic is that if the latest CSV for the channel in the catalog doesn't match sub.Status.CurrentCSV or sub.Status.CurrentCSV does not point to an existing CSV object in the namespace, sub.Status.CurrentCSV gets set to the latest and that CSV is put in the InstallPlan.
There was a problem hiding this comment.
Here are the breadcrumbs:
https://github.com/operator-framework/operator-lifecycle-manager/blob/889ae50a66fe36a46a3c756a0ad7b481b73672f1/pkg/controller/operators/catalog/operator.go#L1291
https://github.com/operator-framework/operator-lifecycle-manager/blob/889ae50a66fe36a46a3c756a0ad7b481b73672f1/pkg/controller/registry/resolver/step_resolver.go#L123-L127
https://github.com/operator-framework/operator-lifecycle-manager/blob/889ae50a66fe36a46a3c756a0ad7b481b73672f1/pkg/controller/registry/resolver/step_resolver.go#L132-L136
https://github.com/operator-framework/operator-lifecycle-manager/blob/889ae50a66fe36a46a3c756a0ad7b481b73672f1/pkg/controller/registry/resolver/step_resolver.go#L197-L204
https://github.com/operator-framework/operator-lifecycle-manager/blob/889ae50a66fe36a46a3c756a0ad7b481b73672f1/pkg/controller/operators/catalog/operator.go#L1428-L1433
There was a problem hiding this comment.
@JustinKuli I'll revise my statement of "latest in the channel" but rather the latest that replaces the current version.
When an InstallPlan contains multiple CSVs, it will be approved if all CSVs can be approved by the current operator policies. Rather than query the operator policies directly, the subscriptions in the namespace that are managed by an operator policy are queried and then their associated operator policies are queried to avoid having to resolve templates to see if they manage a subscription in the namespace. Relates: https://issues.redhat.com/browse/ACM-11981 Signed-off-by: mprahl <mprahl@users.noreply.github.com>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: JustinKuli, mprahl 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 4f7cda6
into
open-cluster-management-io:main
First Commit
The operatorpolicy.policy.open-cluster-management.io/managed label is set to an empty value on the Subscription. The
operatorpolicy.policy.open-cluster-management.io/managed annotation is set to ..
The label doesn't set a value because the value could be too long.
This will allow querying for subscriptions managed by operator policies.
Second Commit
When an InstallPlan contains multiple CSVs, it will be approved if all CSVs can be approved by the current operator policies.
Rather than query the operator policies directly, the subscriptions in the namespace that are managed by an operator policy are queried and then their associated operator policies are queried to avoid having to resolve templates.
Relates:
https://issues.redhat.com/browse/ACM-11981