EVEREST-1247 | Re-design of operator upgrades (#452)

api/v1alpha1/databaseengine_test.go

@@ -0,0 +1,66 @@
+// everest-operator
+// Copyright (C) 2022 Percona LLC
+//
+// 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 v1alpha1
+
+import "testing"
+
+func TestGetNextUpgradeVersion(t *testing.T) {
+	t.Parallel()
+	testCases := []struct {
+		status *DatabaseEngineStatus
+		want   string
+	}{
+		{
+			status: &DatabaseEngineStatus{
+				PendingOperatorUpgrades: []OperatorUpgrade{},
+			},
+			want: "",
+		},
+		{
+			status: &DatabaseEngineStatus{
+				PendingOperatorUpgrades: []OperatorUpgrade{
+					{TargetVersion: "1.0.0"},
+				},
+			},
+			want: "1.0.0",
+		},
+		{
+			status: &DatabaseEngineStatus{
+				PendingOperatorUpgrades: []OperatorUpgrade{
+					{TargetVersion: "1.0.0"},
+					{TargetVersion: "1.1.0"},
+				},
+			},
+			want: "1.0.0",
+		},
+		{
+			status: &DatabaseEngineStatus{
+				PendingOperatorUpgrades: []OperatorUpgrade{
+					{TargetVersion: "1.0.0"},
+					{TargetVersion: "1.0.1"},
+					{TargetVersion: "1.1.0"},
+				},
+			},
+			want: "1.0.1",
+		},
+	}
+
+	for _, tc := range testCases {
+		got := tc.status.GetNextUpgradeVersion()
+		if got != tc.want {
+			t.Errorf("got %s, want %s", got, tc.want)
+		}
+	}
+}

api/v1alpha1/databaseengine_types.go

@@ -16,6 +16,7 @@
 package v1alpha1
 
 import (
+	"slices"
 	"sort"
 
 	goversion "github.com/hashicorp/go-version"
@@ -51,16 +52,11 @@ const (
 )
 
 const (
-	// DatabaseOperatorUpgradeAnnotation indicates that the database operator needs to be upgraded.
-	// The value of the annotation is the version to upgrade to.
-	DatabaseOperatorUpgradeAnnotation = "everest.percona.com/upgrade-operator-to"
 	// DatabaseOperatorUpgradeLockAnnotation is an annotation set on the database engine.
-	// If present and set to "true", Everest backend will prevent any resources from being modified in the namespace.
-	// This annotation is typically set by the Everest backend right before starting the operator upgrade.
+	// If present, the value should contain the timestamp of when the lock was set.
+	// Everest operator automatically removes this annotation (lock) after 5mins.
+	// This is done to ensure that the namespace/engine is not locked indefinitely in case an upgrade fails.
 	DatabaseOperatorUpgradeLockAnnotation = "everest.percona.com/upgrade-lock"
-	// DatabaseOperatorUpgradeLockAnnotationValueTrue is the value of the DatabaseOperatorUpgradeLockAnnotation
-	// if the database engine must be locked.
-	DatabaseOperatorUpgradeLockAnnotationValueTrue = "true"
 )
 
 type (
@@ -89,15 +85,41 @@ type DatabaseEngineStatus struct {
 	PendingOperatorUpgrades []OperatorUpgrade `json:"pendingOperatorUpgrades,omitempty"`
 
 	// OperatorUpgrade contains the status of the operator upgrade.
-	// This is set only if the `everest.percona.com/upgrade-operator-to` annotation is present.
 	OperatorUpgrade *OperatorUpgradeStatus `json:"operatorUpgrade,omitempty"`
 }
 
+// GetNextUpgradeVersion gets the next version of the operator to upgrade to.
+func (s *DatabaseEngineStatus) GetNextUpgradeVersion() string {
+	if len(s.PendingOperatorUpgrades) == 0 {
+		return ""
+	}
+	if len(s.PendingOperatorUpgrades) == 1 {
+		return s.PendingOperatorUpgrades[0].TargetVersion
+	}
+	next := slices.MinFunc(s.PendingOperatorUpgrades, func(a, b OperatorUpgrade) int {
+		v1 := goversion.Must(goversion.NewVersion(a.TargetVersion))
+		v2 := goversion.Must(goversion.NewVersion(b.TargetVersion))
+		// If major minor are equal, we return the one with the higher patch version.
+		if v1.Segments()[0] == v2.Segments()[0] && v1.Segments()[1] == v2.Segments()[1] {
+			return v2.Core().Compare(v1.Core())
+		}
+		return v1.Core().Compare(v2.Core())
+	})
+	return next.TargetVersion
+}
+
 // OperatorUpgrade contains the information about the operator upgrade.
 type OperatorUpgrade struct {
 	// TargetVersion is the version to which the operator should be upgraded.
 	TargetVersion string `json:"targetVersion,omitempty"`
 	// InstallPlanRef is a reference to the InstallPlan object created for the operator upgrade.
+	//
+	// We do not recommended approving this InstallPlan directly from the Kubernetes API.
+	// This is because this InstallPlan may also upgrade other operators in the namespace and that
+	// can have unintended consequences.
+	// This behaviour is not a bug from Everest, but an unfortunate limitation of OLM.
+	// We suggest using the Everest API/UI to handle operator upgrades, which will perform a series
+	// of checks and safely upgrade all operators in the namespace.
 	InstallPlanRef corev1.LocalObjectReference `json:"installPlanRef,omitempty"`
 }
 

bundle/manifests/everest-operator.clusterserviceversion.yaml

@@ -103,7 +103,7 @@ metadata:
         }
       ]
     capabilities: Basic Install
-    createdAt: "2024-06-13T15:11:12Z"
+    createdAt: "2024-07-16T07:47:08Z"
     operators.operatorframework.io/builder: operator-sdk-v1.32.0
     operators.operatorframework.io/project_layout: go.kubebuilder.io/v3
   name: everest-operator.v0.0.0
@@ -354,6 +354,15 @@ spec:
           - delete
           - get
           - update
+        - apiGroups:
+          - operators.coreos.com
+          resources:
+          - clusterserviceversions
+          verbs:
+          - get
+          - list
+          - update
+          - watch
         - apiGroups:
           - operators.coreos.com
           resources:

bundle/manifests/everest.percona.com_databaseengines.yaml

@@ -152,13 +152,19 @@ spec:
                     type: object
                 type: object
               operatorUpgrade:
-                description: |-
-                  OperatorUpgrade contains the status of the operator upgrade.
-                  This is set only if the `everest.percona.com/upgrade-operator-to` annotation is present.
+                description: OperatorUpgrade contains the status of the operator upgrade.
                 properties:
                   installPlanRef:
-                    description: InstallPlanRef is a reference to the InstallPlan
-                      object created for the operator upgrade.
+                    description: |-
+                      InstallPlanRef is a reference to the InstallPlan object created for the operator upgrade.
+
+
+                      We do not recommended approving this InstallPlan directly from the Kubernetes API.
+                      This is because this InstallPlan may also upgrade other operators in the namespace and that
+                      can have unintended consequences.
+                      This behaviour is not a bug from Everest, but an unfortunate limitation of OLM.
+                      We suggest using the Everest API/UI to handle operator upgrades, which will perform a series
+                      of checks and safely upgrade all operators in the namespace.
                     properties:
                       name:
                         default: ""
@@ -195,8 +201,16 @@ spec:
                     operator upgrade.
                   properties:
                     installPlanRef:
-                      description: InstallPlanRef is a reference to the InstallPlan
-                        object created for the operator upgrade.
+                      description: |-
+                        InstallPlanRef is a reference to the InstallPlan object created for the operator upgrade.
+
+
+                        We do not recommended approving this InstallPlan directly from the Kubernetes API.
+                        This is because this InstallPlan may also upgrade other operators in the namespace and that
+                        can have unintended consequences.
+                        This behaviour is not a bug from Everest, but an unfortunate limitation of OLM.
+                        We suggest using the Everest API/UI to handle operator upgrades, which will perform a series
+                        of checks and safely upgrade all operators in the namespace.
                       properties:
                         name:
                           default: ""

config/crd/bases/everest.percona.com_databaseengines.yaml

@@ -152,13 +152,19 @@ spec:
                     type: object
                 type: object
               operatorUpgrade:
-                description: |-
-                  OperatorUpgrade contains the status of the operator upgrade.
-                  This is set only if the `everest.percona.com/upgrade-operator-to` annotation is present.
+                description: OperatorUpgrade contains the status of the operator upgrade.
                 properties:
                   installPlanRef:
-                    description: InstallPlanRef is a reference to the InstallPlan
-                      object created for the operator upgrade.
+                    description: |-
+                      InstallPlanRef is a reference to the InstallPlan object created for the operator upgrade.
+
+
+                      We do not recommended approving this InstallPlan directly from the Kubernetes API.
+                      This is because this InstallPlan may also upgrade other operators in the namespace and that
+                      can have unintended consequences.
+                      This behaviour is not a bug from Everest, but an unfortunate limitation of OLM.
+                      We suggest using the Everest API/UI to handle operator upgrades, which will perform a series
+                      of checks and safely upgrade all operators in the namespace.
                     properties:
                       name:
                         default: ""
@@ -195,8 +201,16 @@ spec:
                     operator upgrade.
                   properties:
                     installPlanRef:
-                      description: InstallPlanRef is a reference to the InstallPlan
-                        object created for the operator upgrade.
+                      description: |-
+                        InstallPlanRef is a reference to the InstallPlan object created for the operator upgrade.
+
+
+                        We do not recommended approving this InstallPlan directly from the Kubernetes API.
+                        This is because this InstallPlan may also upgrade other operators in the namespace and that
+                        can have unintended consequences.
+                        This behaviour is not a bug from Everest, but an unfortunate limitation of OLM.
+                        We suggest using the Everest API/UI to handle operator upgrades, which will perform a series
+                        of checks and safely upgrade all operators in the namespace.
                       properties:
                         name:
                           default: ""

config/rbac/role.yaml

@@ -205,6 +205,15 @@ rules:
   - delete
   - get
   - update
+- apiGroups:
+  - operators.coreos.com
+  resources:
+  - clusterserviceversions
+  verbs:
+  - get
+  - list
+  - update
+  - watch
 - apiGroups:
   - operators.coreos.com
   resources:

controllers/controllers_test.go

@@ -16,9 +16,15 @@
 package controllers
 
 import (
+	"fmt"
 	"testing"
+	"time"
 
+	opfwv1alpha1 "github.com/operator-framework/api/pkg/operators/v1alpha1"
 	"github.com/stretchr/testify/assert"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+
+	everestv1alpha1 "github.com/percona/everest-operator/api/v1alpha1"
 )
 
 func Test_ParseCSVName(t *testing.T) {
@@ -35,3 +41,86 @@ func Test_ParseCSVName(t *testing.T) {
 	assert.Equal(t, "", name)
 	assert.Equal(t, "", version)
 }
+
+func TestGetInstallPlanRefsForUpgrade(t *testing.T) {
+	t.Parallel()
+	dbEngine := &everestv1alpha1.DatabaseEngine{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-db-operator",
+			Namespace: "everest",
+		},
+		Spec: everestv1alpha1.DatabaseEngineSpec{},
+		Status: everestv1alpha1.DatabaseEngineStatus{
+			OperatorVersion: "0.0.1",
+		},
+	}
+
+	subscription := &opfwv1alpha1.Subscription{
+		TypeMeta: metav1.TypeMeta{
+			APIVersion: "operators.coreos.com/v1alpha1",
+			Kind:       "Subscription",
+		},
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "test-db-operator",
+			Namespace: "everest",
+			UID:       "uid-1234",
+		},
+	}
+
+	ownerRef := metav1.OwnerReference{
+		APIVersion: subscription.APIVersion,
+		Kind:       subscription.Kind,
+		Name:       subscription.GetName(),
+		UID:        subscription.GetUID(),
+	}
+
+	testCases := []struct {
+		installplans *opfwv1alpha1.InstallPlanList
+		expected     map[string]string
+	}{
+		{
+			installplans: &opfwv1alpha1.InstallPlanList{
+				Items: []opfwv1alpha1.InstallPlan{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "installplan-1",
+							CreationTimestamp: metav1.Time{
+								Time: time.Date(2024, 1, 1, 10, 30, 0, 0, time.UTC),
+							},
+							OwnerReferences: []metav1.OwnerReference{ownerRef},
+						},
+						Spec: opfwv1alpha1.InstallPlanSpec{
+							ClusterServiceVersionNames: []string{"test-db-operator.v0.0.2"},
+						},
+					},
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "installplan-2",
+							CreationTimestamp: metav1.Time{
+								Time: time.Date(2024, 1, 1, 10, 35, 0, 0, time.UTC),
+							},
+							OwnerReferences: []metav1.OwnerReference{ownerRef},
+						},
+						Spec: opfwv1alpha1.InstallPlanSpec{
+							ClusterServiceVersionNames: []string{
+								"test-db-operator.v0.0.2",
+								"test-anotherdb-operator.v0.0.1",
+							},
+						},
+					},
+				},
+			},
+			expected: map[string]string{
+				"0.0.2": "installplan-2",
+			},
+		},
+	}
+
+	for i, tc := range testCases {
+		t.Run(fmt.Sprintf("test-%d", i), func(t *testing.T) {
+			t.Parallel()
+			result := getInstallPlanRefsForUpgrade(dbEngine, subscription, tc.installplans)
+			assert.Equal(t, tc.expected, result)
+		})
+	}
+}