diff --git a/pkg/cvo/sync_test.go b/pkg/cvo/sync_test.go index dec9a48d3..f960d0da5 100644 --- a/pkg/cvo/sync_test.go +++ b/pkg/cvo/sync_test.go @@ -465,7 +465,7 @@ func (pf *testPrecondition) Name() string { return fmt.Sprintf("TestPrecondition SuccessAfter: %d", pf.SuccessAfter) } -func (pf *testPrecondition) Run(_ context.Context, _ precondition.ReleaseContext, cv *configv1.ClusterVersion) error { +func (pf *testPrecondition) Run(_ context.Context, _ precondition.ReleaseContext) error { if pf.SuccessAfter == 0 { return nil } diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go index b753e9adc..6e9f4a9bc 100644 --- a/pkg/cvo/sync_worker.go +++ b/pkg/cvo/sync_worker.go @@ -630,12 +630,12 @@ func (w *SyncWorker) syncOnce(ctx context.Context, work *SyncWork, maxWorkers in Actual: desired, Verified: info.Verified, }) - if err := precondition.Summarize(w.preconditions.RunAll(ctx, precondition.ReleaseContext{DesiredVersion: payloadUpdate.Release.Version}, clusterVersion)); err != nil { - if work.Desired.Force { - klog.V(4).Infof("Forcing past precondition failures: %s", err) - w.eventRecorder.Eventf(cvoObjectRef, corev1.EventTypeWarning, "PreconditionsForced", "preconditions forced for payload loaded version=%q image=%q failures=%v", desired.Version, desired.Image, err) - } else { - w.eventRecorder.Eventf(cvoObjectRef, corev1.EventTypeWarning, "PreconditionsFailed", "preconditions failed for payload loaded version=%q image=%q failures=%v", desired.Version, desired.Image, err) + if block, err := precondition.Summarize(w.preconditions.RunAll(ctx, precondition.ReleaseContext{ + DesiredVersion: payloadUpdate.Release.Version, + }), work.Desired.Force); err != nil { + klog.V(4).Infof("Precondition error (force %t, block %t): %v", work.Desired.Force, block, err) + if block { + w.eventRecorder.Eventf(cvoObjectRef, corev1.EventTypeWarning, "PreconditionBlock", "preconditions failed for payload loaded version=%q image=%q: %v", desired.Version, desired.Image, err) reporter.Report(SyncWorkerStatus{ Generation: work.Generation, Failure: err, @@ -646,6 +646,8 @@ func (w *SyncWorker) syncOnce(ctx context.Context, work *SyncWork, maxWorkers in Verified: info.Verified, }) return err + } else { + w.eventRecorder.Eventf(cvoObjectRef, corev1.EventTypeWarning, "PreconditionWarn", "precondition warning for payload loaded version=%q image=%q: %v", desired.Version, desired.Image, err) } } w.eventRecorder.Eventf(cvoObjectRef, corev1.EventTypeNormal, "PreconditionsPassed", "preconditions passed for payload loaded version=%q image=%q", desired.Version, desired.Image) diff --git a/pkg/payload/precondition/clusterversion/etcdbackup.go b/pkg/payload/precondition/clusterversion/etcdbackup.go new file mode 100644 index 000000000..563f7d3f7 --- /dev/null +++ b/pkg/payload/precondition/clusterversion/etcdbackup.go @@ -0,0 +1,90 @@ +package clusterversion + +import ( + "context" + "fmt" + + configv1 "github.com/openshift/api/config/v1" + configv1listers "github.com/openshift/client-go/config/listers/config/v1" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" + + "github.com/openshift/cluster-version-operator/lib/resourcemerge" + precondition "github.com/openshift/cluster-version-operator/pkg/payload/precondition" +) + +const backupConditionType = "RecentBackup" + +// RecentEtcdBackup checks if a recent etcd backup has been taken. +type RecentEtcdBackup struct { + key string + cvLister configv1listers.ClusterVersionLister + coLister configv1listers.ClusterOperatorLister +} + +// NewRecentEtcdBackup returns a new RecentEtcdBackup precondition check. +func NewRecentEtcdBackup(cvLister configv1listers.ClusterVersionLister, coLister configv1listers.ClusterOperatorLister) *RecentEtcdBackup { + return &RecentEtcdBackup{ + key: "version", + cvLister: cvLister, + coLister: coLister, + } +} + +func recentEtcdBackupCondition(lister configv1listers.ClusterOperatorLister) (reason string, message string) { + var msgDetail string + ops, err := lister.Get("etcd") + if err == nil { + backupCondition := resourcemerge.FindOperatorStatusCondition(ops.Status.Conditions, backupConditionType) + if backupCondition == nil { + reason = "EtcdRecentBackupNotSet" + msgDetail = "etcd backup condition is not set." + } else if backupCondition.Status != configv1.ConditionTrue { + reason = backupCondition.Reason + msgDetail = backupCondition.Message + } + } else { + reason = "UnableToGetEtcdOperator" + msgDetail = fmt.Sprintf("Unable to get etcd operator, err=%v.", err) + } + if len(msgDetail) > 0 { + message = fmt.Sprintf("%s: %s", backupConditionType, msgDetail) + } + return reason, message +} + +// Run runs the RecentEtcdBackup precondition. It returns a PreconditionError until Etcd indicates that a +// recent etcd backup has been taken. +func (pf *RecentEtcdBackup) Run(ctx context.Context, releaseContext precondition.ReleaseContext) error { + cv, err := pf.cvLister.Get(pf.key) + if apierrors.IsNotFound(err) || meta.IsNoMatchError(err) { + return nil + } + if err != nil { + return &precondition.Error{ + Nested: err, + Reason: "UnknownError", + Message: err.Error(), + Name: pf.Name(), + } + } + + currentVersion := GetCurrentVersion(cv.Status.History) + currentMinor := GetEffectiveMinor(currentVersion) + desiredMinor := GetEffectiveMinor(releaseContext.DesiredVersion) + + if minorVersionUpgrade(currentMinor, desiredMinor) { + reason, message := recentEtcdBackupCondition(pf.coLister) + if len(reason) > 0 { + return &precondition.Error{ + Reason: reason, + Message: message, + Name: pf.Name(), + } + } + } + return nil +} + +// Name returns Name for the precondition. +func (pf *RecentEtcdBackup) Name() string { return "EtcdRecentBackup" } diff --git a/pkg/payload/precondition/clusterversion/upgradeable.go b/pkg/payload/precondition/clusterversion/upgradeable.go index 0e316d2ad..1d4422e5a 100644 --- a/pkg/payload/precondition/clusterversion/upgradeable.go +++ b/pkg/payload/precondition/clusterversion/upgradeable.go @@ -2,7 +2,6 @@ package clusterversion import ( "context" - "fmt" "strconv" "strings" @@ -16,8 +15,6 @@ import ( precondition "github.com/openshift/cluster-version-operator/pkg/payload/precondition" ) -const backupConditionType = "RecentBackup" - // Upgradeable checks if clusterversion is upgradeable currently. type Upgradeable struct { key string @@ -51,7 +48,7 @@ func ClusterVersionOverridesCondition(cv *configv1.ClusterVersion) *configv1.Clu // Run runs the Upgradeable precondition. // If the feature gate `key` is not found, or the api for clusterversion doesn't exist, this check is inert and always returns nil error. // Otherwise, if Upgradeable condition is set to false in the object, it returns an PreconditionError when possible. -func (pf *Upgradeable) Run(ctx context.Context, releaseContext precondition.ReleaseContext, clusterVersion *configv1.ClusterVersion) error { +func (pf *Upgradeable) Run(ctx context.Context, releaseContext precondition.ReleaseContext) error { cv, err := pf.lister.Get(pf.key) if apierrors.IsNotFound(err) || meta.IsNoMatchError(err) { return nil @@ -91,7 +88,7 @@ func (pf *Upgradeable) Run(ctx context.Context, releaseContext precondition.Rele // if no cluster overrides have been set if currentMinor == desiredMinor { klog.V(4).Infof("Precondition %q passed: minor from the current %s matches minor from the target %s (both %s).", pf.Name(), currentVersion, releaseContext.DesiredVersion, currentMinor) - if condition := ClusterVersionOverridesCondition(clusterVersion); condition != nil { + if condition := ClusterVersionOverridesCondition(cv); condition != nil { klog.V(4).Infof("Update from %s to %s blocked by %s: %s", currentVersion, releaseContext.DesiredVersion, condition.Reason, condition.Message) return &precondition.Error{ @@ -115,80 +112,6 @@ func (pf *Upgradeable) Run(ctx context.Context, releaseContext precondition.Rele // Name returns Name for the precondition. func (pf *Upgradeable) Name() string { return "ClusterVersionUpgradeable" } -// RecentEtcdBackup checks if a recent etcd backup has been taken. -type RecentEtcdBackup struct { - key string - cvLister configv1listers.ClusterVersionLister - coLister configv1listers.ClusterOperatorLister -} - -// NewRecentEtcdBackup returns a new RecentEtcdBackup precondition check. -func NewRecentEtcdBackup(cvLister configv1listers.ClusterVersionLister, coLister configv1listers.ClusterOperatorLister) *RecentEtcdBackup { - return &RecentEtcdBackup{ - key: "version", - cvLister: cvLister, - coLister: coLister, - } -} - -func recentEtcdBackupCondition(lister configv1listers.ClusterOperatorLister) (reason string, message string) { - var msgDetail string - ops, err := lister.Get("etcd") - if err == nil { - backupCondition := resourcemerge.FindOperatorStatusCondition(ops.Status.Conditions, backupConditionType) - if backupCondition == nil { - reason = "EtcdRecentBackupNotSet" - msgDetail = "etcd backup condition is not set." - } else if backupCondition.Status != configv1.ConditionTrue { - reason = backupCondition.Reason - msgDetail = backupCondition.Message - } - } else { - reason = "UnableToGetEtcdOperator" - msgDetail = fmt.Sprintf("Unable to get etcd operator, err=%v.", err) - } - if len(msgDetail) > 0 { - message = fmt.Sprintf("%s: %s", backupConditionType, msgDetail) - } - return reason, message -} - -// Run runs the RecentEtcdBackup precondition. It returns a PreconditionError until Etcd indicates that a -// recent etcd backup has been taken. -func (pf *RecentEtcdBackup) Run(ctx context.Context, releaseContext precondition.ReleaseContext, clusterVersion *configv1.ClusterVersion) error { - cv, err := pf.cvLister.Get(pf.key) - if apierrors.IsNotFound(err) || meta.IsNoMatchError(err) { - return nil - } - if err != nil { - return &precondition.Error{ - Nested: err, - Reason: "UnknownError", - Message: err.Error(), - Name: pf.Name(), - } - } - - currentVersion := GetCurrentVersion(cv.Status.History) - currentMinor := GetEffectiveMinor(currentVersion) - desiredMinor := GetEffectiveMinor(releaseContext.DesiredVersion) - - if minorVersionUpgrade(currentMinor, desiredMinor) { - reason, message := recentEtcdBackupCondition(pf.coLister) - if len(reason) > 0 { - return &precondition.Error{ - Reason: reason, - Message: message, - Name: pf.Name(), - } - } - } - return nil -} - -// Name returns Name for the precondition. -func (pf *RecentEtcdBackup) Name() string { return "EtcdRecentBackup" } - // GetCurrentVersion determines and returns the cluster's current version by iterating through the // provided update history until it finds the first version with update State of Completed. If a // Completed version is not found the version of the oldest history entry, which is the originally diff --git a/pkg/payload/precondition/clusterversion/upgradable_test.go b/pkg/payload/precondition/clusterversion/upgradeable_test.go similarity index 96% rename from pkg/payload/precondition/clusterversion/upgradable_test.go rename to pkg/payload/precondition/clusterversion/upgradeable_test.go index 53af4b888..5230f6bae 100644 --- a/pkg/payload/precondition/clusterversion/upgradable_test.go +++ b/pkg/payload/precondition/clusterversion/upgradeable_test.go @@ -54,6 +54,7 @@ func TestGetEffectiveMinor(t *testing.T) { } func TestUpgradeableRun(t *testing.T) { + ctx := context.Background() ptr := func(status configv1.ConditionStatus) *configv1.ConditionStatus { return &status } @@ -128,7 +129,9 @@ func TestUpgradeableRun(t *testing.T) { cvLister := fakeClusterVersionLister(t, clusterVersion) instance := NewUpgradeable(cvLister) - err := instance.Run(context.TODO(), precondition.ReleaseContext{DesiredVersion: tc.desiredVersion}, clusterVersion) + err := instance.Run(ctx, precondition.ReleaseContext{ + DesiredVersion: tc.desiredVersion, + }) switch { case err != nil && len(tc.expected) == 0: t.Error(err) diff --git a/pkg/payload/precondition/precondition.go b/pkg/payload/precondition/precondition.go index 6494591ba..5daa1d47e 100644 --- a/pkg/payload/precondition/precondition.go +++ b/pkg/payload/precondition/precondition.go @@ -5,7 +5,6 @@ import ( "fmt" "strings" - configv1 "github.com/openshift/api/config/v1" "k8s.io/klog/v2" "github.com/openshift/cluster-version-operator/pkg/payload" @@ -42,7 +41,7 @@ type ReleaseContext struct { // Precondition defines the precondition check for a payload. type Precondition interface { // Run executes the precondition checks ands returns an error when the precondition fails. - Run(ctx context.Context, releaseContext ReleaseContext, cv *configv1.ClusterVersion) error + Run(ctx context.Context, releaseContext ReleaseContext) error // Name returns a human friendly name for the precondition. Name() string @@ -53,10 +52,10 @@ type List []Precondition // RunAll runs all the reflight checks in order, returning a list of errors if any. // All checks are run, regardless if any one precondition fails. -func (pfList List) RunAll(ctx context.Context, releaseContext ReleaseContext, cv *configv1.ClusterVersion) []error { +func (pfList List) RunAll(ctx context.Context, releaseContext ReleaseContext) []error { var errs []error for _, pf := range pfList { - if err := pf.Run(ctx, releaseContext, cv); err != nil { + if err := pf.Run(ctx, releaseContext); err != nil { klog.Errorf("Precondition %q failed: %v", pf.Name(), err) errs = append(errs, err) } @@ -65,9 +64,11 @@ func (pfList List) RunAll(ctx context.Context, releaseContext ReleaseContext, cv } // Summarize summarizes all the precondition.Error from errs. -func Summarize(errs []error) error { +// Returns the consolidated error and a boolean for whether the error +// is blocking (true) or a warning (false). +func Summarize(errs []error, force bool) (bool, error) { if len(errs) == 0 { - return nil + return false, nil } var msgs []string for _, e := range errs { @@ -83,7 +84,12 @@ func Summarize(errs []error) error { } else { msg = fmt.Sprintf("Multiple precondition checks failed:\n* %s", strings.Join(msgs, "\n* ")) } - return &payload.UpdateError{ + + if force { + msg = fmt.Sprintf("Forced through blocking failures: %s", msg) + } + + return !force, &payload.UpdateError{ Nested: nil, Reason: "UpgradePreconditionCheckFailed", Message: msg, diff --git a/pkg/payload/precondition/precondition_test.go b/pkg/payload/precondition/precondition_test.go index 137f37d95..015a41cfd 100644 --- a/pkg/payload/precondition/precondition_test.go +++ b/pkg/payload/precondition/precondition_test.go @@ -7,30 +7,48 @@ import ( func TestSummarize(t *testing.T) { tests := []struct { - input []error - exp string + name string + errors []error + force bool + expectedBlock bool + expectedError string }{{ - input: nil, + name: "nil", + errors: nil, }, { - input: []error{}, + name: "empty error slice", + errors: []error{}, }, { - input: []error{fmt.Errorf("random error")}, - exp: "random error", + name: "unrecognized error type", + errors: []error{fmt.Errorf("random error")}, + expectedBlock: true, + expectedError: "random error", }, { - input: []error{&Error{ + name: "forced unrecognized error type", + errors: []error{fmt.Errorf("random error")}, + force: true, + expectedBlock: false, + expectedError: "Forced through blocking failures: random error", + }, { + name: "single feature-gate error", + errors: []error{&Error{ Nested: nil, Reason: "NotAllowedFeatureGateSet", Message: "Feature Gate random is set for the cluster. This Feature Gate turns on features that are not part of the normal supported platform.", Name: "FeatureGate", }}, - exp: `Precondition "FeatureGate" failed because of "NotAllowedFeatureGateSet": Feature Gate random is set for the cluster. This Feature Gate turns on features that are not part of the normal supported platform.`, + expectedBlock: true, + expectedError: `Precondition "FeatureGate" failed because of "NotAllowedFeatureGateSet": Feature Gate random is set for the cluster. This Feature Gate turns on features that are not part of the normal supported platform.`, }, { - input: []error{fmt.Errorf("random error"), fmt.Errorf("random error 2")}, - exp: `Multiple precondition checks failed: + name: "two unrecognized error types", + errors: []error{fmt.Errorf("random error"), fmt.Errorf("random error 2")}, + expectedBlock: true, + expectedError: `Multiple precondition checks failed: * random error * random error 2`, }, { - input: []error{&Error{ + name: "two feature gate errors", + errors: []error{&Error{ Nested: nil, Reason: "NotAllowedFeatureGateSet", Message: "Feature Gate random is set for the cluster. This Feature Gate turns on features that are not part of the normal supported platform.", @@ -41,11 +59,13 @@ func TestSummarize(t *testing.T) { Message: "Feature Gate random-2 is set for the cluster. This Feature Gate turns on features that are not part of the normal supported platform.", Name: "FeatureGate", }}, - exp: `Multiple precondition checks failed: + expectedBlock: true, + expectedError: `Multiple precondition checks failed: * Precondition "FeatureGate" failed because of "NotAllowedFeatureGateSet": Feature Gate random is set for the cluster. This Feature Gate turns on features that are not part of the normal supported platform. * Precondition "FeatureGate" failed because of "NotAllowedFeatureGateSet": Feature Gate random-2 is set for the cluster. This Feature Gate turns on features that are not part of the normal supported platform.`, }, { - input: []error{ + name: "unrecognized type and a feature-gate error", + errors: []error{ fmt.Errorf("random error"), &Error{ Nested: nil, @@ -53,22 +73,28 @@ func TestSummarize(t *testing.T) { Message: "Feature Gate random is set for the cluster. This Feature Gate turns on features that are not part of the normal supported platform.", Name: "FeatureGate", }}, - exp: `Multiple precondition checks failed: + expectedBlock: true, + expectedError: `Multiple precondition checks failed: * random error * Precondition "FeatureGate" failed because of "NotAllowedFeatureGateSet": Feature Gate random is set for the cluster. This Feature Gate turns on features that are not part of the normal supported platform.`, }} for _, test := range tests { - t.Run("", func(t *testing.T) { - err := Summarize(test.input) - if test.exp == "" { + t.Run(test.name, func(t *testing.T) { + block, err := Summarize(test.errors, test.force) + + if block != test.expectedBlock { + t.Errorf("expected block %t, but got %t", test.expectedBlock, block) + } + + if test.expectedError == "" { if err != nil { t.Fatalf("expected no error, got %v", err) } } else { if err == nil { - t.Fatalf("expected err %s, got nil", test.exp) - } else if err.Error() != test.exp { - t.Fatalf("expected err %s, got %s", test.exp, err.Error()) + t.Fatalf("expected err %s, got nil", test.expectedError) + } else if err.Error() != test.expectedError { + t.Fatalf("expected err %s, got %s", test.expectedError, err.Error()) } } })