From ec9903dba891dc5fd4e42bd06b240a7a82f9283c Mon Sep 17 00:00:00 2001 From: Tyler Slaton Date: Fri, 15 Jul 2022 12:23:50 -0400 Subject: [PATCH] fix(openshift): use env var instead of clusterversion status Get the current OpenShift release version from the RELEASE_VERSION environment variable since the behavior of the original source -- the ClusterVersion desired release status field -- has changed. Signed-off-by: Tyler Slaton Co-authored-by: Nick Hale --- .../clusteroperator_controller_test.go | 65 +++++++-------- pkg/controller/operators/openshift/helpers.go | 62 +++++++++++--- .../operators/openshift/helpers_test.go | 83 +++---------------- pkg/controller/operators/openshift/options.go | 4 +- 4 files changed, 90 insertions(+), 124 deletions(-) diff --git a/pkg/controller/operators/openshift/clusteroperator_controller_test.go b/pkg/controller/operators/openshift/clusteroperator_controller_test.go index 5f8cb1dfd8..72a0552ff2 100644 --- a/pkg/controller/operators/openshift/clusteroperator_controller_test.go +++ b/pkg/controller/operators/openshift/clusteroperator_controller_test.go @@ -2,6 +2,7 @@ package openshift import ( "fmt" + "os" semver "github.com/blang/semver/v4" . "github.com/onsi/ginkgo/v2" @@ -19,7 +20,6 @@ import ( var _ = Describe("ClusterOperator controller", func() { var ( clusterOperatorName types.NamespacedName - cv *configv1.ClusterVersion csv *operatorsv1alpha1.ClusterServiceVersion ) @@ -55,33 +55,11 @@ var _ = Describe("ClusterOperator controller", func() { }) BeforeEach(func() { - // "version" singleton is available in OpenShift by default - cv = &configv1.ClusterVersion{} - cv.SetName("version") - - Eventually(func() error { - return k8sClient.Create(ctx, cv) - }).Should(Succeed()) - - cv.Status = configv1.ClusterVersionStatus{ - Desired: configv1.Release{ - Version: clusterVersion, - }, - } - - Eventually(func() error { - return k8sClient.Status().Update(ctx, cv) - }).Should(Succeed()) + os.Setenv(releaseEnvVar, clusterVersion) }) AfterEach(func() { - Eventually(func() error { - err := k8sClient.Delete(ctx, cv) - if err != nil && apierrors.IsNotFound(err) { - err = nil - } - return err - }).Should(Succeed()) + resetCurrentReleaseTo(clusterVersion) }) It("should ensure the ClusterOperator always exists", func() { @@ -143,10 +121,14 @@ var _ = Describe("ClusterOperator controller", func() { })) By("setting upgradeable=false when there's an error determining compatibility") - cv.Status = configv1.ClusterVersionStatus{} - + // Reset the ClusterOperator with an invalid version set + Expect(resetCurrentReleaseTo("")).To(Succeed()) Eventually(func() error { - return k8sClient.Status().Update(ctx, cv) + err := k8sClient.Delete(ctx, co) + if err != nil && !apierrors.IsNotFound(err) { + return err + } + return nil }).Should(Succeed()) Eventually(func() ([]configv1.ClusterOperatorStatusCondition, error) { @@ -156,18 +138,18 @@ var _ = Describe("ClusterOperator controller", func() { Type: configv1.OperatorUpgradeable, Status: configv1.ConditionFalse, Reason: ErrorCheckingOperatorCompatibility, - Message: "Encountered errors while checking compatibility with the next minor version of OpenShift: desired release version missing from ClusterVersion", + Message: fmt.Sprintf("Encountered errors while checking compatibility with the next minor version of OpenShift: desired release version missing from %v env variable", releaseEnvVar), LastTransitionTime: fixedNow(), })) - cv.Status = configv1.ClusterVersionStatus{ - Desired: configv1.Release{ - Version: clusterVersion, - }, - } - + // Reset the ClusterOperator with a valid version set + Expect(resetCurrentReleaseTo(clusterVersion)).To(Succeed()) Eventually(func() error { - return k8sClient.Status().Update(ctx, cv) + err := k8sClient.Delete(ctx, co) + if err != nil && !apierrors.IsNotFound(err) { + return err + } + return nil }).Should(Succeed()) By("setting upgradeable=false when incompatible operators exist") @@ -327,3 +309,14 @@ var _ = Describe("ClusterOperator controller", func() { })) }) }) + +// resetCurrentRelease thread safely updates the currentRelease.version and then sets the openshift release +// env var to the desired version. WARNING: This function should only be used for testing purposes as it +// goes around the desired logic of only setting the version of the cluster for this operator once. +func resetCurrentReleaseTo(version string) error { + currentRelease.mu.Lock() + defer currentRelease.mu.Unlock() + + currentRelease.version = nil + return os.Setenv(releaseEnvVar, version) +} diff --git a/pkg/controller/operators/openshift/helpers.go b/pkg/controller/operators/openshift/helpers.go index 22ae894a32..d13f1cc150 100644 --- a/pkg/controller/operators/openshift/helpers.go +++ b/pkg/controller/operators/openshift/helpers.go @@ -4,7 +4,9 @@ import ( "context" "errors" "fmt" + "os" "strings" + "sync" semver "github.com/blang/semver/v4" configv1 "github.com/openshift/api/config/v1" @@ -119,17 +121,17 @@ func transientErrors(err error) error { } func incompatibleOperators(ctx context.Context, cli client.Client) (skews, error) { - desired, err := desiredRelease(ctx, cli) + current, err := getCurrentRelease() if err != nil { return nil, err } - if desired == nil { + if current == nil { // Note: This shouldn't happen return nil, fmt.Errorf("failed to determine current OpenShift Y-stream release") } - next, err := nextY(*desired) + next, err := nextY(*current) if err != nil { return nil, err } @@ -168,24 +170,58 @@ func incompatibleOperators(ctx context.Context, cli client.Client) (skews, error return incompatible, nil } -func desiredRelease(ctx context.Context, cli client.Client) (*semver.Version, error) { - cv := configv1.ClusterVersion{} - if err := cli.Get(ctx, client.ObjectKey{Name: "version"}, &cv); err != nil { // "version" is the name of OpenShift's ClusterVersion singleton - return nil, &transientError{fmt.Errorf("failed to get ClusterVersion: %w", err)} +type openshiftRelease struct { + version *semver.Version + mu sync.Mutex +} + +var ( + currentRelease = &openshiftRelease{} +) + +const ( + releaseEnvVar = "RELEASE_VERSION" // OpenShift's env variable for defining the current release +) + +// getCurrentRelease thread safely retrieves the current version of OCP at the time of this operator starting. +// This is defined by an environment variable that our release manifests define (and get dynamically updated) +// by OCP. For the purposes of this package, that environment variable is a constant under the name of releaseEnvVar. +// +// Note: currentRelease is designed to be a singleton that only gets updated the first time that this function +// is called. As a result, all calls to this will return the same value even if the releaseEnvVar gets +// changed during runtime somehow. +func getCurrentRelease() (*semver.Version, error) { + currentRelease.mu.Lock() + defer currentRelease.mu.Unlock() + + if currentRelease.version != nil { + /* + If the version is already set, we don't want to set it again as the currentRelease + is designed to be a singleton. If a new version is set, we are making an assumption + that this controller will be restarted and thus pull in the new version from the + environment into memory. + + Note: sync.Once is not used here as it was difficult to reliably test without hitting + race conditions. + */ + return currentRelease.version, nil } - v := cv.Status.Desired.Version - if v == "" { - // The release version hasn't been set yet - return nil, fmt.Errorf("desired release version missing from ClusterVersion") + // Get the raw version from the releaseEnvVar environment variable + raw, ok := os.LookupEnv(releaseEnvVar) + if !ok || raw == "" { + // No env var set, try again later + return nil, fmt.Errorf("desired release version missing from %v env variable", releaseEnvVar) } - desired, err := semver.ParseTolerant(v) + release, err := semver.ParseTolerant(raw) if err != nil { return nil, fmt.Errorf("cluster version has invalid desired release version: %w", err) } - return &desired, nil + currentRelease.version = &release + + return currentRelease.version, nil } func nextY(v semver.Version) (semver.Version, error) { diff --git a/pkg/controller/operators/openshift/helpers_test.go b/pkg/controller/operators/openshift/helpers_test.go index 653620f804..8e08991e05 100644 --- a/pkg/controller/operators/openshift/helpers_test.go +++ b/pkg/controller/operators/openshift/helpers_test.go @@ -221,22 +221,13 @@ func TestIncompatibleOperators(t *testing.T) { } for _, tt := range []struct { description string - cv configv1.ClusterVersion + version string in skews expect expect }{ { description: "Compatible", - cv: configv1.ClusterVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "version", - }, - Status: configv1.ClusterVersionStatus{ - Desired: configv1.Release{ - Version: "1.0.0", - }, - }, - }, + version: "1.0.0", in: skews{ { name: "almond", @@ -261,16 +252,7 @@ func TestIncompatibleOperators(t *testing.T) { }, { description: "Incompatible", - cv: configv1.ClusterVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "version", - }, - Status: configv1.ClusterVersionStatus{ - Desired: configv1.Release{ - Version: "1.0.0", - }, - }, - }, + version: "1.0.0", in: skews{ { name: "almond", @@ -331,16 +313,7 @@ func TestIncompatibleOperators(t *testing.T) { }, { description: "Mixed", - cv: configv1.ClusterVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "version", - }, - Status: configv1.ClusterVersionStatus{ - Desired: configv1.Release{ - Version: "1.0.0", - }, - }, - }, + version: "1.0.0", in: skews{ { name: "almond", @@ -376,16 +349,7 @@ func TestIncompatibleOperators(t *testing.T) { }, { description: "Mixed/BadVersion", - cv: configv1.ClusterVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "version", - }, - Status: configv1.ClusterVersionStatus{ - Desired: configv1.Release{ - Version: "1.0.0", - }, - }, - }, + version: "1.0.0", in: skews{ { name: "almond", @@ -424,16 +388,7 @@ func TestIncompatibleOperators(t *testing.T) { }, { description: "EmptyVersion", - cv: configv1.ClusterVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "version", - }, - Status: configv1.ClusterVersionStatus{ - Desired: configv1.Release{ - Version: "", // This should result in an transient error - }, - }, - }, + version: "", // This should result in an transient error in: skews{ { name: "almond", @@ -453,16 +408,7 @@ func TestIncompatibleOperators(t *testing.T) { }, { description: "ClusterZ", - cv: configv1.ClusterVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "version", - }, - Status: configv1.ClusterVersionStatus{ - Desired: configv1.Release{ - Version: "1.0.1", // Next Y-stream is 1.1.0, NOT 1.1.1 - }, - }, - }, + version: "1.0.1", // Next Y-stream is 1.1.0, NOT 1.1.1 in: skews{ { name: "beech", @@ -477,16 +423,7 @@ func TestIncompatibleOperators(t *testing.T) { }, { description: "ClusterPre", - cv: configv1.ClusterVersion{ - ObjectMeta: metav1.ObjectMeta{ - Name: "version", - }, - Status: configv1.ClusterVersionStatus{ - Desired: configv1.Release{ - Version: "1.1.0-pre", // Next Y-stream is 1.1.0, NOT 1.2.0 - }, - }, - }, + version: "1.1.0-pre", // Next Y-stream is 1.1.0, NOT 1.2.0 in: skews{ { name: "almond", @@ -501,7 +438,9 @@ func TestIncompatibleOperators(t *testing.T) { }, } { t.Run(tt.description, func(t *testing.T) { - objs := []client.Object{tt.cv.DeepCopy()} + objs := []client.Object{} + + resetCurrentReleaseTo(tt.version) for _, s := range tt.in { csv := &operatorsv1alpha1.ClusterServiceVersion{} diff --git a/pkg/controller/operators/openshift/options.go b/pkg/controller/operators/openshift/options.go index 59f8533ab2..b74e6fcc91 100644 --- a/pkg/controller/operators/openshift/options.go +++ b/pkg/controller/operators/openshift/options.go @@ -163,7 +163,6 @@ func WithOLMOperator() ReconcilerOption { enqueue := handler.EnqueueRequestsFromMapFunc(config.mapClusterOperator) - name := "version" originalCSV := predicate.NewPredicateFuncs(func(obj client.Object) bool { csv, ok := obj.(*operatorsv1alpha1.ClusterServiceVersion) if !ok { @@ -174,8 +173,7 @@ func WithOLMOperator() ReconcilerOption { return !csv.IsCopied() // Keep original CSVs only }) config.TweakBuilder = func(bldr *builder.Builder) *builder.Builder { - return bldr.Watches(&source.Kind{Type: &operatorsv1alpha1.ClusterServiceVersion{}}, enqueue, builder.WithPredicates(originalCSV)). - Watches(&source.Kind{Type: &configv1.ClusterVersion{}}, enqueue, builder.WithPredicates(watchName(&name))) + return bldr.Watches(&source.Kind{Type: &operatorsv1alpha1.ClusterServiceVersion{}}, enqueue, builder.WithPredicates(originalCSV)) } } }