Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

controllers: Do not update client operator in provider mode #393

Merged
merged 1 commit into from
Apr 4, 2024
Merged

controllers: Do not update client operator in provider mode #393

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion controllers/storagesystem_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/reconcile"

operatorv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
ocsv1 "github.com/red-hat-storage/ocs-operator/api/v4/v1"
odfv1alpha1 "github.com/red-hat-storage/odf-operator/api/v1alpha1"
"github.com/red-hat-storage/odf-operator/metrics"
"github.com/red-hat-storage/odf-operator/pkg/util"
Expand Down Expand Up @@ -219,7 +220,10 @@ func (r *StorageSystemReconciler) ensureSubscriptions(instance *odfv1alpha1.Stor

func (r *StorageSystemReconciler) isVendorCsvReady(instance *odfv1alpha1.StorageSystem, logger logr.Logger) error {

csvNames := GetVendorCsvNames(instance.Spec.Kind)
csvNames, err := GetVendorCsvNames(r.Client, instance.Spec.Kind)
if err != nil {
return err
}

var returnErr error
for _, csvName := range csvNames {
Expand Down Expand Up @@ -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)).
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

in existing code, is storagesystem being set as a controller owner for storagecluster?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes

WithOptions(controller.Options{MaxConcurrentReconciles: 1}).
Complete(r)
}
19 changes: 17 additions & 2 deletions controllers/subscription_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,12 @@ func (r *SubscriptionReconciler) ensureSubscriptions(logger logr.Logger, namespa
}

for kind := range subsList {
for _, csvName := range GetVendorCsvNames(kind) {
csvNames, csvErr := GetVendorCsvNames(r.Client, kind)
if csvErr != nil {
return csvErr
}

for _, csvName := range csvNames {
_, csvErr := EnsureVendorCsv(r.Client, csvName)
if csvErr != nil {
multierr.AppendInto(&err, csvErr)
Expand All @@ -140,7 +145,17 @@ func (r *SubscriptionReconciler) setOperatorCondition(logger logr.Logger, namesp
return err
}

condNames := append(GetVendorCsvNames(StorageClusterKind), GetVendorCsvNames(FlashSystemKind)...)
condNames, err := GetVendorCsvNames(r.Client, StorageClusterKind)
if err != nil {
return err
}

condNamesFlashSystem, err := GetVendorCsvNames(r.Client, FlashSystemKind)
if err != nil {
return err
}

condNames = append(condNames, condNamesFlashSystem...)

condMap := make(map[string]struct{}, len(condNames))
for i := range condNames {
Expand Down
43 changes: 39 additions & 4 deletions controllers/subscriptions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"

operatorv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
ocsv1 "github.com/red-hat-storage/ocs-operator/api/v4/v1"
odfv1alpha1 "github.com/red-hat-storage/odf-operator/api/v1alpha1"
"github.com/red-hat-storage/odf-operator/pkg/util"
)
Expand All @@ -51,6 +52,14 @@ func CheckExistingSubscriptions(cli client.Client, desiredSubscription *operator
odfSub.Spec.Config = &operatorv1alpha1.SubscriptionConfig{}
}

var isProvider bool
if desiredSubscription.Spec.Package == OcsClientSubscriptionPackage {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is OcsClientSubscriptionPackage already defined? lint might've already failed but just confirming.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes it is defined in the defaults.go

isProvider, err = isProviderMode(cli)
if err != nil {
return nil, err
}
}

subsList := &operatorv1alpha1.SubscriptionList{}
err = cli.List(context.TODO(), subsList, &client.ListOptions{Namespace: desiredSubscription.Namespace})
if err != nil {
Expand All @@ -68,7 +77,10 @@ func CheckExistingSubscriptions(cli client.Client, desiredSubscription *operator
return nil, fmt.Errorf("multiple Subscriptions found for package '%s': %v", pkgName, foundSubs)
}
actualSub = &subsList.Items[i]
actualSub.Spec.Channel = desiredSubscription.Spec.Channel

if !isProvider {
actualSub.Spec.Channel = desiredSubscription.Spec.Channel
}
Comment on lines +81 to +83
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see only channel update is being skipped, no qualms w/ that, small question, does odf-operator overwrites any metadata in the subscription?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, it does not overwrite metadata.


if actualSub.Spec.Config == nil && desiredSubscription.Spec.Config == nil {
actualSub.Spec.Config = &operatorv1alpha1.SubscriptionConfig{}
Expand Down Expand Up @@ -102,6 +114,23 @@ func CheckExistingSubscriptions(cli client.Client, desiredSubscription *operator
return desiredSubscription, nil
}

func isProviderMode(cli client.Client) (bool, error) {

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
Comment on lines +119 to +131
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say let it be. We may extend the func later on.

}

func getMergedTolerations(tol1, tol2 []corev1.Toleration) []corev1.Toleration {

if len(tol1) == 0 {
Expand Down Expand Up @@ -216,18 +245,24 @@ func GetOdfSubscription(cli client.Client) (*operatorv1alpha1.Subscription, erro
return nil, fmt.Errorf("odf-operator subscription not found")
}

func GetVendorCsvNames(kind odfv1alpha1.StorageKind) []string {
func GetVendorCsvNames(cli client.Client, kind odfv1alpha1.StorageKind) ([]string, error) {

var csvNames []string
var err error
var isProvider bool

if kind == VendorFlashSystemCluster() {
csvNames = []string{IbmSubscriptionStartingCSV}
} else if kind == VendorStorageCluster() {
csvNames = []string{OcsSubscriptionStartingCSV, RookSubscriptionStartingCSV, NoobaaSubscriptionStartingCSV,
CSIAddonsSubscriptionStartingCSV, OcsClientSubscriptionStartingCSV, PrometheusSubscriptionStartingCSV}
CSIAddonsSubscriptionStartingCSV, PrometheusSubscriptionStartingCSV}

if isProvider, err = isProviderMode(cli); !isProvider {
csvNames = append(csvNames, OcsClientSubscriptionStartingCSV)
}
}

return csvNames
return csvNames, err
}

func EnsureVendorCsv(cli client.Client, csvName string) (*operatorv1alpha1.ClusterServiceVersion, error) {
Expand Down
Loading