Skip to content

fix(openshift): use env var instead of clusterversion status #2817

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

Merged
merged 1 commit into from
Aug 2, 2022
Merged

fix(openshift): use env var instead of clusterversion status #2817

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
65 changes: 29 additions & 36 deletions pkg/controller/operators/openshift/clusteroperator_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package openshift

import (
"fmt"
"os"

semver "github.com/blang/semver/v4"
. "github.com/onsi/ginkgo/v2"
Expand All @@ -19,7 +20,6 @@ import (
var _ = Describe("ClusterOperator controller", func() {
var (
clusterOperatorName types.NamespacedName
cv *configv1.ClusterVersion
csv *operatorsv1alpha1.ClusterServiceVersion
)

Expand Down Expand Up @@ -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() {
Expand Down Expand Up @@ -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) {
Expand All @@ -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")
Expand Down Expand Up @@ -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)
}
62 changes: 49 additions & 13 deletions pkg/controller/operators/openshift/helpers.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ import (
"context"
"errors"
"fmt"
"os"
"strings"
"sync"

semver "github.com/blang/semver/v4"
configv1 "github.com/openshift/api/config/v1"
Expand Down Expand Up @@ -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
}
Expand Down Expand Up @@ -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) {
Expand Down
83 changes: 11 additions & 72 deletions pkg/controller/operators/openshift/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand All @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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",
Expand All @@ -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{}
Expand Down
4 changes: 1 addition & 3 deletions pkg/controller/operators/openshift/options.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand All @@ -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))
}
}
}