controllers: Do not update client operator in provider mode #393
Conversation
openshift-ci
bot
added
the
approved
| @@ -259,6 +263,7 @@ func (r *StorageSystemReconciler) SetupWithManager(mgr ctrl.Manager) error { | |||
| return ctrl.NewControllerManagedBy(mgr). | |||
| For(&odfv1alpha1.StorageSystem{}, builder.WithPredicates(generationChangedPredicate)). | |||
| Owns(&operatorv1alpha1.Subscription{}, builder.WithPredicates(generationChangedPredicate, ignoreCreatePredicate)). | |||
| Owns(&ocsv1.StorageCluster{}, builder.WithPredicates(generationChangedPredicate)). | |||
There was a problem hiding this comment.
in existing code, is storagesystem being set as a controller owner for storagecluster?
| @@ -51,6 +52,14 @@ func CheckExistingSubscriptions(cli client.Client, desiredSubscription *operator | |||
| odfSub.Spec.Config = &operatorv1alpha1.SubscriptionConfig{} | |||
| } | |||
|
|
|||
| var isProvider bool | |||
| if desiredSubscription.Spec.Package == OcsClientSubscriptionPackage { | |||
There was a problem hiding this comment.
is OcsClientSubscriptionPackage already defined? lint might've already failed but just confirming.
There was a problem hiding this comment.
Yes it is defined in the defaults.go
| if !isProvider { | ||
| actualSub.Spec.Channel = desiredSubscription.Spec.Channel | ||
| } |
There was a problem hiding this comment.
I see only channel update is being skipped, no qualms w/ that, small question, does odf-operator overwrites any metadata in the subscription?
There was a problem hiding this comment.
no, it does not overwrite metadata.
controllers/subscriptions.go
Outdated
| @@ -102,6 +114,23 @@ func CheckExistingSubscriptions(cli client.Client, desiredSubscription *operator | |||
| return desiredSubscription, nil | |||
| } | |||
|
|
|||
| func isProviderServer(cli client.Client) (bool, error) { | |||
There was a problem hiding this comment.
not sure isProviderServer sounds correct, to be abstract can use isProviderMode or to be exact can use isAllowRemoteStorageConsumersSet
There was a problem hiding this comment.
I will change it to isProviderMode as we always refer it as provider.
| storageclusters := &ocsv1.StorageClusterList{} | ||
| err := cli.List(context.TODO(), storageclusters) | ||
| if err != nil { | ||
| return false, err | ||
| } | ||
|
|
||
| for _, storagecluster := range storageclusters.Items { | ||
| if storagecluster.Spec.AllowRemoteStorageConsumers { | ||
| return true, nil | ||
| } | ||
| } | ||
|
|
||
| return false, nil |
There was a problem hiding this comment.
sry, this seems a lot of work to find the fields values, not mandatory and here's what I'm thinking, cli.List will try to list storageclusters in all namespaces and storagecluster in for loop is making a copy of every storagecluster in the Items.
It might be good to limit the result to operator ns and taken an index into Items? again, only a suggestion.
There was a problem hiding this comment.
I would say let it be. We may extend the func later on.
Client operator manages its own upgrade life cycle in the provider mode, Do not update its channel in the provider mode. Signed-off-by: Nitin Goyal <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: iamniting, leelavg 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 096506a
into
red-hat-storage:main
|
/cherry-pick release-4.16 |
|
@iamniting: new pull request created: #395 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 kubernetes/test-infra repository. |
Client operator manages its own upgrade life cycle in the provider mode, Do not update its channel in the provider mode.
Signed-off-by: Nitin Goyal [email protected]