NE-1907: Manage OSSM operator subscription manually to ensure a compatible version is installed #1112
Conversation
openshift-ci
bot
added
the
do-not-merge/work-in-progress
|
/assign |
rfredette
changed the title
|
@rfredette: This pull request references NE-1817 which is a valid jira issue. In response to this:
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
openshift-ci
bot
removed
the
do-not-merge/work-in-progress
|
/retest |
rfredette
force-pushed
the
gwapi-subscription
branch
from
1e5e244 to
01e4600
Compare
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
test failed due to some test infra issue: /retest |
…sion is installed Istio and the Gateway API CRDs need to be in sync to work. The CRDs are baked into a particular openshift release, so this change updates the ingress operator to install a compatible version with the CRDs it already installs.
rfredette
force-pushed
the
gwapi-subscription
branch
from
01e4600 to
894eac5
Compare
rfredette
changed the title
|
@rfredette: This pull request references NE-1907 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:
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. |
|
/jira refresh |
|
@rfredette: This pull request references NE-1907 which is a valid jira issue. In response to this:
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. |
Miciah
added
the
priority/backlog
|
single node test failure is unrelated. |
|
@rfredette: all tests passed! 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. |
| @@ -61,6 +62,22 @@ func NewUnmanaged(mgr manager.Manager, config Config) (controller.Controller, er | |||
| if err := c.Watch(source.Kind[client.Object](operatorCache, &gatewayapiv1.GatewayClass{}, &handler.EnqueueRequestForObject{}, isOurGatewayClass, notIstioGatewayClass)); err != nil { | |||
| return nil, err | |||
| } | |||
| isOurInstallPlan := predicate.NewPredicateFuncs(func(o client.Object) bool { | |||
| installPlan := o.(*operatorsv1alpha1.InstallPlan) | |||
| for _, csv := range installPlan.Spec.ClusterServiceVersionNames { | |||
There was a problem hiding this comment.
Should you check to make sure len(ClusterServiceVersionNames) > 0 first?
| } | ||
| } | ||
| } | ||
| return nil, fmt.Errorf("No InstallPlan with cluster service version %s found", serviceMeshOperatorDesiredVersion) |
There was a problem hiding this comment.
| return nil, fmt.Errorf("No InstallPlan with cluster service version %s found", serviceMeshOperatorDesiredVersion) | |
| return nil, fmt.Errorf("no InstallPlan with cluster service version %s found in namespace %s", serviceMeshOperatorDesiredVersion, serviceMeshOperatorNamespace) |
| return false, currentInstallPlan, nil | ||
| } | ||
|
|
||
| func (r *reconciler) getCurrentInstallPlan(ctx context.Context) (*operatorsv1alpha1.InstallPlan, error) { |
There was a problem hiding this comment.
nit: use the function name we normally use.
| func (r *reconciler) getCurrentInstallPlan(ctx context.Context) (*operatorsv1alpha1.InstallPlan, error) { | |
| func (r *reconciler) currentInstallPlan(ctx context.Context) (*operatorsv1alpha1.InstallPlan, error) { |
| } else if currentInstallPlan == nil { | ||
| return false, nil, nil | ||
| } | ||
| if !currentInstallPlan.Spec.Approved { |
There was a problem hiding this comment.
The idiomatic way to write an "ensure" function is to get the current object, get the desired object, compare them, and use the "have" and "want" values to determine whether to create or update. Can you update this to use the idiomatic form?
|
|
||
| func (r *reconciler) getCurrentInstallPlan(ctx context.Context) (*operatorsv1alpha1.InstallPlan, error) { | ||
| InstallPlans := &operatorsv1alpha1.InstallPlanList{} | ||
| if err := r.client.List(ctx, InstallPlans, client.InNamespace(serviceMeshOperatorNamespace)); err != nil { |
There was a problem hiding this comment.
So the installPlan doesn't have a unique name?
| @@ -16,6 +17,11 @@ import ( | |||
| "k8s.io/apimachinery/pkg/types" | |||
| ) | |||
|
|
|||
| var ( | |||
| serviceMeshOperatorDesiredVersion = "servicemeshoperator.v2.6.2" | |||
There was a problem hiding this comment.
I'm not sure this is going to work. If we change the version in a future release and need to backport a different change in the same file, won't this get overwritten? I think normally version information is recorded in a manifest yaml. Can you find a way to do this without hardcoding the version here?
I'll check with @Miciah because I don't know the answer.
|
@rfredette can you add a unit test for the ensure function? |
Istio and the Gateway API CRDs need to be in sync to work. The CRDs are baked into a particular openshift release, so this change updates the ingress operator to install a compatible version with the CRDs it already installs.