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.

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

new controller for restricting upgrade of operator #60

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
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
5 changes: 4 additions & 1 deletion api/v1alpha1/storageclient_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,13 @@ type StorageClientSpec struct {

// StorageClientStatus defines the observed state of StorageClient
type StorageClientStatus struct {
// NOTE: Do not remove "omitempty" from the tags as that'll make mergepatch incompatible

Phase storageClientPhase `json:"phase,omitempty"`

// ConsumerID will hold the identity of this cluster inside the attached provider cluster
ConsumerID string `json:"id,omitempty"`
ConsumerID string `json:"id,omitempty"`
DesiredOperatorVersion string `json:"desiredOperatorVersion,omitempty"`
}

//+kubebuilder:object:root=true
Expand Down
23 changes: 23 additions & 0 deletions bundle/manifests/ocs-client-operator.clusterserviceversion.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -225,6 +225,23 @@ spec:
- get
- list
- watch
- apiGroups:
- operators.coreos.com
resources:
- operatorconditions
verbs:
- get
- list
- update
- watch
- apiGroups:
- operators.coreos.com
resources:
- subscriptions
verbs:
- get
- list
- watch
- apiGroups:
- security.openshift.io
resources:
Expand Down Expand Up @@ -599,6 +616,12 @@ spec:
verbs:
- get
- list
- apiGroups:
- ocs.openshift.io
resources:
- storageclients/status
verbs:
- patch
- apiGroups:
- ""
resources:
Expand Down
2 changes: 2 additions & 0 deletions bundle/manifests/ocs.openshift.io_storageclients.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,8 @@ spec:
status:
description: StorageClientStatus defines the observed state of StorageClient
properties:
desiredOperatorVersion:
type: string
id:
description: ConsumerID will hold the identity of this cluster inside
the attached provider cluster
Expand Down
2 changes: 2 additions & 0 deletions config/crd/bases/ocs.openshift.io_storageclients.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -58,6 +58,8 @@ spec:
status:
description: StorageClientStatus defines the observed state of StorageClient
properties:
desiredOperatorVersion:
type: string
id:
description: ConsumerID will hold the identity of this cluster inside
the attached provider cluster
Expand Down
17 changes: 17 additions & 0 deletions config/rbac/role.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,23 @@ rules:
- get
- list
- watch
- apiGroups:
- operators.coreos.com
resources:
- operatorconditions
verbs:
- get
- list
- update
- watch
- apiGroups:
- operators.coreos.com
resources:
- subscriptions
verbs:
- get
- list
- watch
- apiGroups:
- security.openshift.io
resources:
Expand Down
6 changes: 6 additions & 0 deletions config/rbac/status-reporter-clusterrole.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,12 @@ rules:
verbs:
- get
- list
- apiGroups:
- ocs.openshift.io
resources:
- storageclients/status
verbs:
- patch
- apiGroups:
- ""
resources:
Expand Down
22 changes: 4 additions & 18 deletions controllers/storageclient_controller.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,14 +23,12 @@ import (
"encoding/json"
"fmt"
"os"
"strings"
"time"

"github.com/red-hat-storage/ocs-client-operator/api/v1alpha1"
"github.com/red-hat-storage/ocs-client-operator/pkg/utils"

configv1 "github.com/openshift/api/config/v1"
opv1a1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
providerClient "github.com/red-hat-storage/ocs-operator/v4/services/provider/client"
"google.golang.org/grpc/codes"
"google.golang.org/grpc/status"
Expand Down Expand Up @@ -61,8 +59,6 @@ const (
storageClientNameLabel = "ocs.openshift.io/storageclient.name"
storageClientNamespaceLabel = "ocs.openshift.io/storageclient.namespace"
storageClientFinalizer = "storageclient.ocs.openshift.io"

csvPrefix = "ocs-client-operator"
)

// StorageClientReconciler reconciles a StorageClient object
Expand Down Expand Up @@ -271,16 +267,10 @@ func (s *StorageClientReconciler) onboardConsumer(instance *v1alpha1.StorageClie
return reconcile.Result{}, fmt.Errorf("failed to get the clusterVersion version of the OCP cluster: %v", err)
}

// TODO Have a version file corresponding to the release
csvList := opv1a1.ClusterServiceVersionList{}
if err = s.list(&csvList, client.InNamespace(s.OperatorNamespace)); err != nil {
return reconcile.Result{}, fmt.Errorf("failed to list csv resources in ns: %v, err: %v", s.OperatorNamespace, err)
}
csv := utils.Find(csvList.Items, func(csv *opv1a1.ClusterServiceVersion) bool {
return strings.HasPrefix(csv.Name, csvPrefix)
})
if csv == nil {
return reconcile.Result{}, fmt.Errorf("unable to find csv with prefix %q", csvPrefix)
csv, err := utils.GetCSVByPrefix(s.ctx, s.Client, "ocs-client-operator", s.OperatorNamespace)
if err != nil {
s.Log.Error(err, "failed to get clusterserviceverison of ocs client operator")
Copy link
Contributor

Choose a reason for hiding this comment

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

Please log in the original error

return reconcile.Result{}, fmt.Errorf("failed to get clusterserviceversion of ocs client operator: %v", err)
}
name := fmt.Sprintf("storageconsumer-%s", clusterVersion.Spec.ClusterID)
onboardRequest := providerClient.NewOnboardConsumerRequest().
Expand Down Expand Up @@ -506,7 +496,3 @@ func (s *StorageClientReconciler) reconcileClientStatusReporterJob(instance *v1a
}
return reconcile.Result{}, nil
}

func (s *StorageClientReconciler) list(obj client.ObjectList, listOptions ...client.ListOption) error {
return s.Client.List(s.ctx, obj, listOptions...)
}
232 changes: 232 additions & 0 deletions controllers/upgrade_controller.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,232 @@
/*
Copyright 2024 Red Hat, Inc.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/

package controllers

import (
"context"
"fmt"
"strings"

"github.com/red-hat-storage/ocs-client-operator/api/v1alpha1"
"github.com/red-hat-storage/ocs-client-operator/pkg/utils"

"github.com/blang/semver/v4"
"github.com/go-logr/logr"
opv1a1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
"github.com/operator-framework/operator-lib/conditions"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
ctrl "sigs.k8s.io/controller-runtime"
"sigs.k8s.io/controller-runtime/pkg/builder"
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/event"
"sigs.k8s.io/controller-runtime/pkg/handler"
"sigs.k8s.io/controller-runtime/pkg/log"
"sigs.k8s.io/controller-runtime/pkg/predicate"
)

const (
ocsClientOperatorSubscriptionPackageName = "ocs-client-operator"
// TODO(lgangava): there can be "eus" or "fast" or "candidate" supported channels
// and custom channels also might exist, either get this data as part of a configmap
// or cut the name by '-'
subscriptionChannelNamePrefix = "stable-"
controllerName = "upgrade"
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
controllerName = "upgrade"
controllerName = "upgrade-controller"

upgradeConditionReason = "ConstraintsSatisfied"
dummyPatchVersion = ".99"
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see a need for an arbitrary value that can be different in any call site without any impact

)

type UpgradeReconciler struct {
client.Client
ctx context.Context
log logr.Logger

Scheme *runtime.Scheme
OperatorCondition conditions.Condition
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't this specifically the upgrade condition? If so, I prefer that the context will be in the name.
Also, this entire code is inside the operator, so no need to specify the obvious

Suggested change
OperatorCondition conditions.Condition
UpgradeCondition conditions.Condition

OperatorNamespace string
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
OperatorNamespace string
Namespace string

}

func (r *UpgradeReconciler) SetupWithManager(mgr ctrl.Manager) error {
storageClientDesiredVersionPredicate := predicate.Funcs{
CreateFunc: func(_ event.CreateEvent) bool {
return false
},
UpdateFunc: func(e event.UpdateEvent) bool {
if e.ObjectOld == nil || e.ObjectNew == nil {
return false
}

oldObj, _ := e.ObjectOld.(*v1alpha1.StorageClient)
newObj, _ := e.ObjectNew.(*v1alpha1.StorageClient)
if oldObj == nil || newObj == nil {
return false
}

return oldObj.Status.DesiredOperatorVersion != newObj.Status.DesiredOperatorVersion
},
Comment on lines +65 to +80
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need this? StorageClient is a very static CR, and we do need to take into account new client CRs upon creation! I would not use a predicate at all for storage clients.

}
ocsClientOperatorSubscriptionPredicate := predicate.Funcs{
CreateFunc: func(_ event.CreateEvent) bool {
return false
},
UpdateFunc: func(e event.UpdateEvent) bool {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe this predicate needs to be applied to all events! not just Update events.

if e.ObjectNew == nil {
return false
}
Comment on lines +87 to +89
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not needed, it will be captured in the nill check after the cast.

Suggested change
if e.ObjectNew == nil {
return false
}


newObj, _ := e.ObjectNew.(*opv1a1.Subscription)
if newObj == nil {
return false
}

return newObj.Spec.Package == ocsClientOperatorSubscriptionPackageName
},
DeleteFunc: func(_ event.DeleteEvent) bool {
return false
},
}
return ctrl.NewControllerManagedBy(mgr).
Named(controllerName).
Watches(&v1alpha1.StorageClient{}, &handler.EnqueueRequestForObject{}, builder.WithPredicates(storageClientDesiredVersionPredicate)).
Watches(&opv1a1.Subscription{}, &handler.EnqueueRequestForObject{}, builder.WithPredicates(ocsClientOperatorSubscriptionPredicate)).
Comment on lines +104 to +105
Copy link
Contributor

Choose a reason for hiding this comment

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

You should not use EnqueueRequestForObject as it will create a reconciler that has an input CR in the request of different types for different reconciles!

Complete(r)
}

//+kubebuilder:rbac:groups=operators.coreos.com,resources=subscriptions,verbs=get;list;watch
//+kubebuilder:rbac:groups=operators.coreos.com,resources=operatorconditions,verbs=get;list;update;watch

func (r *UpgradeReconciler) Reconcile(ctx context.Context, _ ctrl.Request) (ctrl.Result, error) {
r.ctx = ctx
r.log = log.FromContext(ctx)

r.log.Info("starting reconcile")
result, err := r.reconcilePhases()
if err != nil {
r.log.Error(err, "an error occurred during reconcilePhases")
}
r.log.Info("reconciling completed")
Copy link
Contributor

Choose a reason for hiding this comment

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

Reconciliation is not completed in the case of an error


return result, err
}

func (r *UpgradeReconciler) reconcilePhases() (ctrl.Result, error) {
if err := r.reconcileOperatorCondition(); err != nil {
return ctrl.Result{}, err
}
return ctrl.Result{}, nil
}

func (r *UpgradeReconciler) reconcileOperatorCondition() error {

var messages []string
currentVersion, err := r.getCurrentOperatorVersion()
if err != nil {
r.log.Error(err, "failed to get current operator version")
messages = append(messages, "failed to get current operator version")
}
desiredVersion, err := r.getDesiredOperatorVersion()
if err != nil {
r.log.Error(err, "failed to get desired operator version")
messages = append(messages, "failed to get desired operator version")
}

if len(messages) > 0 {
return r.setOperatorCondition(metav1.ConditionFalse, strings.Join(messages, "; "))
}

if currentVersion == nil || desiredVersion == nil {
return r.setOperatorCondition(metav1.ConditionTrue, "No errors are reported.")
}

if currentVersion.Major > desiredVersion.Major || currentVersion.Minor > desiredVersion.Minor {
return r.setOperatorCondition(metav1.ConditionFalse, "Current operator version is ahead of desired operator version")
}

return r.setOperatorCondition(metav1.ConditionTrue, "No errors are reported.")
}

func (r *UpgradeReconciler) setOperatorCondition(isUpgradeable metav1.ConditionStatus, message string) error {
return r.OperatorCondition.
Set(r.ctx, isUpgradeable, conditions.WithReason(upgradeConditionReason), conditions.WithMessage(message))
}

// returns current version of the operator based on the subscription
func (r *UpgradeReconciler) getCurrentOperatorVersion() (*semver.Version, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This method returns the version from the subscription! it is not the operator version. The operator version is specified in the current CSV. Also, subscriptions do not specify versions they specify channels, we should not build logic based on channel naming conventions.

subscriptionList := &opv1a1.SubscriptionList{}
if err := r.list(subscriptionList, client.InNamespace(r.OperatorNamespace)); err != nil {
return nil, err
}

subscription := utils.Find(subscriptionList.Items, func(sub *opv1a1.Subscription) bool {
return sub.Spec.Package == ocsClientOperatorSubscriptionPackageName
})

if subscription == nil {
r.log.Info("subscription of ocs-client-operator doesn't exist")
return nil, nil
}

channel := subscription.Spec.Channel
version, ok := strings.CutPrefix(channel, subscriptionChannelNamePrefix)
if !ok {
return nil, fmt.Errorf("subscription doesn't refer to a stable channel")
}
// appending patchversion to pass semver checks
version += dummyPatchVersion

currentVersion, err := semver.Make(version)
if err != nil {
return nil, err
}

return &currentVersion, nil
}

// returns desired version of the operator based on storageclients
func (r *UpgradeReconciler) getDesiredOperatorVersion() (*semver.Version, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
func (r *UpgradeReconciler) getDesiredOperatorVersion() (*semver.Version, error) {
func (r *UpgradeReconciler) getLowestDesiredOperatorVersion() (*semver.Version, error) {

storageClientList := &v1alpha1.StorageClientList{}
if err := r.list(storageClientList); err != nil {
return nil, err
}

if len(storageClientList.Items) == 0 {
r.log.Info("no storageclients exist")
return nil, nil
}

// appending patchversion to pass semver checks
oldest, err := semver.Make(storageClientList.Items[0].Status.DesiredOperatorVersion + dummyPatchVersion)
if err != nil {
return nil, err
}
Comment on lines +212 to +215
Copy link
Contributor

Choose a reason for hiding this comment

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

You will run on this item again in the for loop. The usual way to solve this is to have a hard-coded value here which is the highest or a special value with special meaning. Personally, I would use the hard coded value approach.

Suggested change
oldest, err := semver.Make(storageClientList.Items[0].Status.DesiredOperatorVersion + dummyPatchVersion)
if err != nil {
return nil, err
}
oldest, err := semver.Make("999.999.999")
if err != nil {
panic(err)
}


for idx := range storageClientList.Items {
current, err := semver.Make(storageClientList.Items[idx].Status.DesiredOperatorVersion + dummyPatchVersion)
if err != nil {
return nil, err
}
if current.LE(oldest) {
oldest = current
}
}

return &oldest, nil
}

func (r *UpgradeReconciler) list(list client.ObjectList, opts ...client.ListOption) error {
return r.Client.List(r.ctx, list, opts...)
}
Loading
Loading