new controller for restricting upgrade of operator #60
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: leelavg 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 |
leelavg
force-pushed
the
stop-client-upgrade
branch
from
8736511 to
1edf4ea
Compare
|
Depends on red-hat-storage/ocs-operator#2329 |
leelavg
force-pushed
the
stop-client-upgrade
branch
from
1edf4ea to
348cc6a
Compare
Signed-off-by: Leela Venkaiah G <[email protected]>
| 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") |
There was a problem hiding this comment.
Please log in the original error
| // and custom channels also might exist, either get this data as part of a configmap | ||
| // or cut the name by '-' | ||
| subscriptionChannelNamePrefix = "stable-" | ||
| controllerName = "upgrade" |
There was a problem hiding this comment.
| controllerName = "upgrade" | |
| controllerName = "upgrade-controller" |
| subscriptionChannelNamePrefix = "stable-" | ||
| controllerName = "upgrade" | ||
| upgradeConditionReason = "ConstraintsSatisfied" | ||
| dummyPatchVersion = ".99" |
There was a problem hiding this comment.
I don't see a need for an arbitrary value that can be different in any call site without any impact
| log logr.Logger | ||
|
|
||
| Scheme *runtime.Scheme | ||
| OperatorCondition conditions.Condition |
There was a problem hiding this comment.
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
| OperatorCondition conditions.Condition | |
| UpgradeCondition conditions.Condition |
|
|
||
| Scheme *runtime.Scheme | ||
| OperatorCondition conditions.Condition | ||
| OperatorNamespace string |
There was a problem hiding this comment.
| OperatorNamespace string | |
| Namespace string |
| if err != nil { | ||
| r.log.Error(err, "an error occurred during reconcilePhases") | ||
| } | ||
| r.log.Info("reconciling completed") |
There was a problem hiding this comment.
Reconciliation is not completed in the case of an error
| } | ||
|
|
||
| // returns current version of the operator based on the subscription | ||
| func (r *UpgradeReconciler) getCurrentOperatorVersion() (*semver.Version, error) { |
There was a problem hiding this comment.
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.
| } | ||
|
|
||
| // returns desired version of the operator based on storageclients | ||
| func (r *UpgradeReconciler) getDesiredOperatorVersion() (*semver.Version, error) { |
There was a problem hiding this comment.
| func (r *UpgradeReconciler) getDesiredOperatorVersion() (*semver.Version, error) { | |
| func (r *UpgradeReconciler) getLowestDesiredOperatorVersion() (*semver.Version, error) { |
| oldest, err := semver.Make(storageClientList.Items[0].Status.DesiredOperatorVersion + dummyPatchVersion) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
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.
| 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) | |
| } |
| @@ -60,3 +67,30 @@ func ValidateStausReporterImage() error { | |||
|
|
|||
| return nil | |||
| } | |||
|
|
|||
| // return csv matching the prefix or else error | |||
| func GetCSVByPrefix(ctx context.Context, c client.Client, prefix, namespace string) (*opv1a1.ClusterServiceVersion, error) { | |||
There was a problem hiding this comment.
| func GetCSVByPrefix(ctx context.Context, c client.Client, prefix, namespace string) (*opv1a1.ClusterServiceVersion, error) { | |
| func GetNonPendingCSVByPrefix(ctx context.Context, c client.Client, prefix, namespace string) (*opv1a1.ClusterServiceVersion, error) { |
leelavg
force-pushed
the
stop-client-upgrade
branch
from
348cc6a to
29cc3db
Compare
|
/hold |
|
not be revived |
As part of heartbeat response provider sends desired version that it wants client operator to match and this PR introduces a new (upgrade) controller to stop upgrades if the current version is ahead of desired version.