From 6402e3d24805a0667107d4f67e46c52c808bc003 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Mon, 20 May 2019 11:17:28 -0700 Subject: [PATCH 1/6] pkg/payload: Rename Update to Payload Narrow pivot to start moving the focus away from upgrades and towards reconciliation. This is the main type out of this package, so just echo the package name. --- pkg/cvo/sync_test.go | 4 ++-- pkg/cvo/sync_worker.go | 4 ++-- pkg/payload/payload.go | 9 +++++---- pkg/payload/payload_test.go | 4 ++-- pkg/verify/verify.go | 2 +- pkg/verify/verify_test.go | 14 +++++++------- 6 files changed, 19 insertions(+), 18 deletions(-) diff --git a/pkg/cvo/sync_test.go b/pkg/cvo/sync_test.go index 6121d14898..1153b9c4ba 100644 --- a/pkg/cvo/sync_test.go +++ b/pkg/cvo/sync_test.go @@ -116,7 +116,7 @@ func Test_SyncWorker_apply(t *testing.T) { manifests = append(manifests, m) } - up := &payload.Update{ReleaseImage: "test", ReleaseVersion: "v0.0.0", Manifests: manifests} + up := &payload.Payload{ReleaseImage: "test", ReleaseVersion: "v0.0.0", Manifests: manifests} r := &recorder{} testMapper := resourcebuilder.NewResourceMapper() testMapper.RegisterGVK(schema.GroupVersionKind{"test.cvo.io", "v1", "TestA"}, newTestBuilder(r, test.reactors)) @@ -308,7 +308,7 @@ func Test_SyncWorker_apply_generic(t *testing.T) { dynamicScheme.AddKnownTypeWithName(schema.GroupVersionKind{Group: "test.cvo.io", Version: "v1", Kind: "TestB"}, &unstructured.Unstructured{}) dynamicClient := dynamicfake.NewSimpleDynamicClient(dynamicScheme) - up := &payload.Update{ReleaseImage: "test", ReleaseVersion: "v0.0.0", Manifests: manifests} + up := &payload.Payload{ReleaseImage: "test", ReleaseVersion: "v0.0.0", Manifests: manifests} worker := &SyncWorker{} worker.backoff.Steps = 1 worker.builder = &testResourceBuilder{ diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go index d0fe115e50..e0ed31a52b 100644 --- a/pkg/cvo/sync_worker.go +++ b/pkg/cvo/sync_worker.go @@ -145,7 +145,7 @@ type SyncWorker struct { status SyncWorkerStatus // updated by the run method only - payload *payload.Update + payload *payload.Payload } // NewSyncWorker initializes a ConfigSyncWorker that will retrieve payloads to disk, apply them via builder @@ -503,7 +503,7 @@ func (w *SyncWorker) syncOnce(ctx context.Context, work *SyncWork, maxWorkers in // apply updates the server with the contents of the provided image or returns an error. // Cancelling the context will abort the execution of the sync. Will be executed in parallel if // maxWorkers is set greater than 1. -func (w *SyncWorker) apply(ctx context.Context, payloadUpdate *payload.Update, work *SyncWork, maxWorkers int, reporter StatusReporter) error { +func (w *SyncWorker) apply(ctx context.Context, payloadUpdate *payload.Payload, work *SyncWork, maxWorkers int, reporter StatusReporter) error { update := configv1.Update{ Version: payloadUpdate.ReleaseVersion, Image: payloadUpdate.ReleaseImage, diff --git a/pkg/payload/payload.go b/pkg/payload/payload.go index 2e978526bf..fbfd3bb219 100644 --- a/pkg/payload/payload.go +++ b/pkg/payload/payload.go @@ -1,3 +1,4 @@ +// Package payload provides tools for interacting with the content of release images. package payload import ( @@ -87,7 +88,7 @@ const ( imageReferencesFile = "image-references" ) -type Update struct { +type Payload struct { ReleaseImage string ReleaseVersion string // XXX: cincinatti.json struct @@ -102,7 +103,7 @@ type Update struct { Manifests []lib.Manifest } -func LoadUpdate(dir, releaseImage string) (*Update, error) { +func LoadUpdate(dir, releaseImage string) (*Payload, error) { payload, tasks, err := loadUpdatePayloadMetadata(dir, releaseImage) if err != nil { return nil, err @@ -206,7 +207,7 @@ type payloadTasks struct { skipFiles sets.String } -func loadUpdatePayloadMetadata(dir, releaseImage string) (*Update, []payloadTasks, error) { +func loadUpdatePayloadMetadata(dir, releaseImage string) (*Payload, []payloadTasks, error) { klog.V(4).Infof("Loading updatepayload from %q", dir) if err := ValidateDirectory(dir); err != nil { return nil, nil, err @@ -240,5 +241,5 @@ func loadUpdatePayloadMetadata(dir, releaseImage string) (*Update, []payloadTask preprocess: nil, skipFiles: sets.NewString(cjf, irf), }} - return &Update{ImageRef: imageRef, ReleaseImage: releaseImage, ReleaseVersion: imageRef.Name}, tasks, nil + return &Payload{ImageRef: imageRef, ReleaseImage: releaseImage, ReleaseVersion: imageRef.Name}, tasks, nil } diff --git a/pkg/payload/payload_test.go b/pkg/payload/payload_test.go index 376d3f1586..9dc51bce0c 100644 --- a/pkg/payload/payload_test.go +++ b/pkg/payload/payload_test.go @@ -24,7 +24,7 @@ func Test_loadUpdatePayload(t *testing.T) { tests := []struct { name string args args - want *Update + want *Payload wantErr bool }{ { @@ -33,7 +33,7 @@ func Test_loadUpdatePayload(t *testing.T) { dir: filepath.Join("..", "cvo", "testdata", "payloadtest"), releaseImage: "image:1", }, - want: &Update{ + want: &Payload{ ReleaseImage: "image:1", ReleaseVersion: "1.0.0-abc", ImageRef: &imagev1.ImageStream{ diff --git a/pkg/verify/verify.go b/pkg/verify/verify.go index bd6f287b0c..458087927f 100644 --- a/pkg/verify/verify.go +++ b/pkg/verify/verify.go @@ -74,7 +74,7 @@ var Reject Interface = rejectVerifier{} // if each provided public key has signed the release image digest. The signature may be in any // store and the lookup order is internally defined. // -func LoadFromPayload(update *payload.Update) (Interface, error) { +func LoadFromPayload(update *payload.Payload) (Interface, error) { configMapGVK := corev1.SchemeGroupVersion.WithKind("ConfigMap") for _, manifest := range update.Manifests { if manifest.GVK != configMapGVK { diff --git a/pkg/verify/verify_test.go b/pkg/verify/verify_test.go index 32ee408a72..293bdb981b 100644 --- a/pkg/verify/verify_test.go +++ b/pkg/verify/verify_test.go @@ -171,18 +171,18 @@ func Test_loadReleaseVerifierFromPayload(t *testing.T) { tests := []struct { name string - update *payload.Update + update *payload.Payload want bool wantErr bool wantVerifiers int }{ { name: "is a no-op when no objects are found", - update: &payload.Update{}, + update: &payload.Payload{}, }, { name: "requires data", - update: &payload.Update{ + update: &payload.Payload{ Manifests: []lib.Manifest{ { GVK: schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"}, @@ -204,7 +204,7 @@ func Test_loadReleaseVerifierFromPayload(t *testing.T) { }, { name: "requires stores", - update: &payload.Update{ + update: &payload.Payload{ Manifests: []lib.Manifest{ { GVK: schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"}, @@ -229,7 +229,7 @@ func Test_loadReleaseVerifierFromPayload(t *testing.T) { }, { name: "requires verifiers", - update: &payload.Update{ + update: &payload.Payload{ Manifests: []lib.Manifest{ { GVK: schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"}, @@ -254,7 +254,7 @@ func Test_loadReleaseVerifierFromPayload(t *testing.T) { }, { name: "loads valid configuration", - update: &payload.Update{ + update: &payload.Payload{ Manifests: []lib.Manifest{ { GVK: schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"}, @@ -281,7 +281,7 @@ func Test_loadReleaseVerifierFromPayload(t *testing.T) { }, { name: "only the first valid configuration is used", - update: &payload.Update{ + update: &payload.Payload{ Manifests: []lib.Manifest{ { GVK: schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"}, From 73de37fa2569c81f2dc19738f2a397852ea4f159 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Mon, 20 May 2019 11:22:25 -0700 Subject: [PATCH 2/6] pkg/payload: Rename LoadUpdate to Load Narrow pivot moving the focus away from upgrades and towards reconciliation. This loader returns the main type out of this package, so drop the "what is being loaded" qualifier from the function name to avoid stuttering the package name. --- pkg/cvo/cvo.go | 2 +- pkg/cvo/sync_worker.go | 2 +- pkg/payload/image.go | 2 +- pkg/payload/payload.go | 9 +++++---- pkg/payload/payload_test.go | 8 ++++---- pkg/payload/task_graph_test.go | 2 +- 6 files changed, 13 insertions(+), 12 deletions(-) diff --git a/pkg/cvo/cvo.go b/pkg/cvo/cvo.go index f14b252085..a7bb2fc8ee 100644 --- a/pkg/cvo/cvo.go +++ b/pkg/cvo/cvo.go @@ -182,7 +182,7 @@ func New( // controller that loads and applies content to the cluster. It returns an error if the payload appears to // be in error rather than continuing. func (optr *Operator) InitializeFromPayload(restConfig *rest.Config, burstRestConfig *rest.Config) error { - update, err := payload.LoadUpdate(optr.defaultPayloadDir(), optr.releaseImage) + update, err := payload.Load(optr.defaultPayloadDir(), optr.releaseImage) if err != nil { return fmt.Errorf("the local release contents are invalid - no current version can be determined from disk: %v", err) } diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go index e0ed31a52b..df6219c649 100644 --- a/pkg/cvo/sync_worker.go +++ b/pkg/cvo/sync_worker.go @@ -477,7 +477,7 @@ func (w *SyncWorker) syncOnce(ctx context.Context, work *SyncWork, maxWorkers in return err } - payloadUpdate, err := payload.LoadUpdate(info.Directory, update.Image) + payloadUpdate, err := payload.Load(info.Directory, update.Image) if err != nil { reporter.Report(SyncWorkerStatus{ Generation: work.Generation, diff --git a/pkg/payload/image.go b/pkg/payload/image.go index f282a9d223..752557af70 100644 --- a/pkg/payload/image.go +++ b/pkg/payload/image.go @@ -9,7 +9,7 @@ import ( // ImageForShortName returns the image using the updatepayload embedded in // the Operator. func ImageForShortName(name string) (string, error) { - up, err := LoadUpdate(DefaultPayloadDir, "") + up, err := Load(DefaultPayloadDir, "") if err != nil { return "", errors.Wrapf(err, "error loading release manifests from %q", DefaultPayloadDir) } diff --git a/pkg/payload/payload.go b/pkg/payload/payload.go index fbfd3bb219..2515fbb6c7 100644 --- a/pkg/payload/payload.go +++ b/pkg/payload/payload.go @@ -103,8 +103,9 @@ type Payload struct { Manifests []lib.Manifest } -func LoadUpdate(dir, releaseImage string) (*Payload, error) { - payload, tasks, err := loadUpdatePayloadMetadata(dir, releaseImage) +// Load loads a Payload from the given release image. +func Load(dir, releaseImage string) (*Payload, error) { + payload, tasks, err := loadPayloadMetadata(dir, releaseImage) if err != nil { return nil, err } @@ -207,8 +208,8 @@ type payloadTasks struct { skipFiles sets.String } -func loadUpdatePayloadMetadata(dir, releaseImage string) (*Payload, []payloadTasks, error) { - klog.V(4).Infof("Loading updatepayload from %q", dir) +func loadPayloadMetadata(dir, releaseImage string) (*Payload, []payloadTasks, error) { + klog.V(4).Infof("Loading payload from %q", dir) if err := ValidateDirectory(dir); err != nil { return nil, nil, err } diff --git a/pkg/payload/payload_test.go b/pkg/payload/payload_test.go index 9dc51bce0c..7546959b07 100644 --- a/pkg/payload/payload_test.go +++ b/pkg/payload/payload_test.go @@ -16,7 +16,7 @@ import ( "github.com/openshift/cluster-version-operator/lib" ) -func Test_loadUpdatePayload(t *testing.T) { +func Test_loadPayload(t *testing.T) { type args struct { dir string releaseImage string @@ -104,13 +104,13 @@ func Test_loadUpdatePayload(t *testing.T) { } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, err := LoadUpdate(tt.args.dir, tt.args.releaseImage) + got, err := Load(tt.args.dir, tt.args.releaseImage) if (err != nil) != tt.wantErr { - t.Errorf("loadUpdatePayload() error = %v, wantErr %v", err, tt.wantErr) + t.Errorf("loadPayload() error = %v, wantErr %v", err, tt.wantErr) return } if !reflect.DeepEqual(got, tt.want) { - t.Errorf("loadUpdatePayload() = %s", diff.ObjectReflectDiff(tt.want, got)) + t.Errorf("loadPayload() = %s", diff.ObjectReflectDiff(tt.want, got)) } }) } diff --git a/pkg/payload/task_graph_test.go b/pkg/payload/task_graph_test.go index f5fc8afbad..e2496ee56e 100644 --- a/pkg/payload/task_graph_test.go +++ b/pkg/payload/task_graph_test.go @@ -375,7 +375,7 @@ func Test_TaskGraph_real(t *testing.T) { if len(path) == 0 { t.Skip("TEST_GRAPH_PATH unset") } - p, err := LoadUpdate(path, "arbitrary/image:1") + p, err := Load(path, "arbitrary/image:1") if err != nil { t.Fatal(err) } From 81f6445b8f1e5f1666f72ff986e0ab9dc84690aa Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Mon, 20 May 2019 14:33:35 -0700 Subject: [PATCH 3/6] pkg/payload: Rename UpdateError to Error This is the only error type in this package, so there's no need to be more specific. This also moving the focus away from upgrades, to avoid marginalizing reconciliation. --- pkg/cvo/cvo_scenarios_test.go | 14 ++++++------ pkg/cvo/cvo_test.go | 6 ++--- pkg/cvo/internal/operatorstatus.go | 8 +++---- pkg/cvo/internal/operatorstatus_test.go | 30 ++++++++++++------------- pkg/cvo/status.go | 4 ++-- pkg/cvo/sync_worker.go | 10 ++++----- pkg/cvo/updatepayload.go | 4 ++-- pkg/payload/image.go | 4 ++-- pkg/payload/payload.go | 2 +- pkg/payload/task.go | 18 +++++++-------- 10 files changed, 50 insertions(+), 50 deletions(-) diff --git a/pkg/cvo/cvo_scenarios_test.go b/pkg/cvo/cvo_scenarios_test.go index 3cc8d3c4ef..de25eff20f 100644 --- a/pkg/cvo/cvo_scenarios_test.go +++ b/pkg/cvo/cvo_scenarios_test.go @@ -357,7 +357,7 @@ func TestCVO_StartupAndSyncUnverifiedPayload(t *testing.T) { defer shutdownFn() // make the image report unverified - payloadErr := &payload.UpdateError{ + payloadErr := &payload.Error{ Reason: "ImageVerificationFailed", Message: fmt.Sprintf("The update cannot be verified: some random error"), Nested: fmt.Errorf("some random error"), @@ -661,7 +661,7 @@ func TestCVO_UpgradeUnverifiedPayload(t *testing.T) { defer shutdownFn() // make the image report unverified - payloadErr := &payload.UpdateError{ + payloadErr := &payload.Error{ Reason: "ImageVerificationFailed", Message: fmt.Sprintf("The update cannot be verified: some random error"), Nested: fmt.Errorf("some random error"), @@ -887,7 +887,7 @@ func TestCVO_UpgradeVerifiedPayload(t *testing.T) { defer shutdownFn() // make the image report unverified - payloadErr := &payload.UpdateError{ + payloadErr := &payload.Error{ Reason: "ImageVerificationFailed", Message: fmt.Sprintf("The update cannot be verified: some random error"), Nested: fmt.Errorf("some random error"), @@ -1385,7 +1385,7 @@ func TestCVO_ErrorDuringReconcile(t *testing.T) { Step: "ApplyResources", Fraction: float32(2) / 3, VersionHash: "6GC9TkkG9PA=", - Failure: &payload.UpdateError{ + Failure: &payload.Error{ Nested: fmt.Errorf("unable to proceed"), Reason: "UpdatePayloadFailed", Message: "Could not update test \"file-yml\" (3 of 3)", @@ -1440,12 +1440,12 @@ func TestCVO_ParallelError(t *testing.T) { defer shutdownFn() worker := o.configSync.(*SyncWorker) b := &errorResourceBuilder{errors: map[string]error{ - "0000_10_a_file.yaml": &payload.UpdateError{ + "0000_10_a_file.yaml": &payload.Error{ Reason: "ClusterOperatorNotAvailable", Name: "operator-1", }, "0000_20_a_file.yaml": nil, - "0000_20_b_file.yaml": &payload.UpdateError{ + "0000_20_b_file.yaml": &payload.Error{ Reason: "ClusterOperatorNotAvailable", Name: "operator-2", }, @@ -1539,7 +1539,7 @@ func TestCVO_ParallelError(t *testing.T) { continue } err := status.Failure - uErr, ok := err.(*payload.UpdateError) + uErr, ok := err.(*payload.Error) if !ok || uErr.Reason != "ClusterOperatorsNotAvailable" || uErr.Message != "Some cluster operators are still updating: operator-1, operator-2" { t.Fatalf("unexpected error: %v", err) } diff --git a/pkg/cvo/cvo_test.go b/pkg/cvo/cvo_test.go index 4a296c326c..2b0001894c 100644 --- a/pkg/cvo/cvo_test.go +++ b/pkg/cvo/cvo_test.go @@ -227,7 +227,7 @@ func TestOperator_sync(t *testing.T) { Step: "Moving", Reconciling: false, Actual: configv1.Update{Version: "0.0.1-abc", Image: "image/image:v4.0.1"}, - Failure: &payload.UpdateError{ + Failure: &payload.Error{ Reason: "UpdatePayloadIntegrity", Message: "unable to apply object", }, @@ -304,7 +304,7 @@ func TestOperator_sync(t *testing.T) { Step: "Moving", Reconciling: true, Actual: configv1.Update{Version: "0.0.1-abc", Image: "image/image:v4.0.1"}, - Failure: &payload.UpdateError{ + Failure: &payload.Error{ Reason: "UpdatePayloadIntegrity", Message: "unable to apply object", }, @@ -378,7 +378,7 @@ func TestOperator_sync(t *testing.T) { Reconciling: true, Completed: 2, Actual: configv1.Update{Version: "0.0.1-abc", Image: "image/image:v4.0.1"}, - Failure: &payload.UpdateError{ + Failure: &payload.Error{ Reason: "UpdatePayloadIntegrity", Message: "unable to apply object", }, diff --git a/pkg/cvo/internal/operatorstatus.go b/pkg/cvo/internal/operatorstatus.go index cda58cd74b..62554f989d 100644 --- a/pkg/cvo/internal/operatorstatus.go +++ b/pkg/cvo/internal/operatorstatus.go @@ -105,7 +105,7 @@ func waitForOperatorStatusToBeDone(ctx context.Context, interval time.Duration, err := wait.PollImmediateUntil(interval, func() (bool, error) { actual, err := client.Get(expected.Name) if err != nil { - lastErr = &payload.UpdateError{ + lastErr = &payload.Error{ Nested: err, Reason: "ClusterOperatorNotAvailable", Message: fmt.Sprintf("Cluster operator %s has not yet reported success", expected.Name), @@ -137,7 +137,7 @@ func waitForOperatorStatusToBeDone(ctx context.Context, interval time.Duration, sort.Strings(keys) message := fmt.Sprintf("Cluster operator %s is still updating", actual.Name) - lastErr = &payload.UpdateError{ + lastErr = &payload.Error{ Nested: errors.New(lowerFirst(message)), Reason: "ClusterOperatorNotAvailable", Message: message, @@ -197,7 +197,7 @@ func waitForOperatorStatusToBeDone(ctx context.Context, interval time.Duration, if len(condition.Message) > 0 { message = fmt.Sprintf("Cluster operator %s is reporting a failure: %s", actual.Name, condition.Message) } - lastErr = &payload.UpdateError{ + lastErr = &payload.Error{ Nested: errors.New(lowerFirst(message)), Reason: "ClusterOperatorDegraded", Message: message, @@ -206,7 +206,7 @@ func waitForOperatorStatusToBeDone(ctx context.Context, interval time.Duration, return false, nil } - lastErr = &payload.UpdateError{ + lastErr = &payload.Error{ Nested: fmt.Errorf("cluster operator %s is not done; it is available=%v, progressing=%v, degraded=%v", actual.Name, available, progressing, degraded, ), diff --git a/pkg/cvo/internal/operatorstatus_test.go b/pkg/cvo/internal/operatorstatus_test.go index 4ccbc90aca..14d045209e 100644 --- a/pkg/cvo/internal/operatorstatus_test.go +++ b/pkg/cvo/internal/operatorstatus_test.go @@ -42,7 +42,7 @@ func Test_waitForOperatorStatusToBeDone(t *testing.T) { }}, }, }, - expErr: &payload.UpdateError{ + expErr: &payload.Error{ Nested: apierrors.NewNotFound(schema.GroupResource{"", "clusteroperator"}, "test-co"), Reason: "ClusterOperatorNotAvailable", Message: "Cluster operator test-co has not yet reported success", @@ -61,7 +61,7 @@ func Test_waitForOperatorStatusToBeDone(t *testing.T) { }}, }, }, - expErr: &payload.UpdateError{ + expErr: &payload.Error{ Nested: fmt.Errorf("cluster operator test-co is still updating"), Reason: "ClusterOperatorNotAvailable", Message: "Cluster operator test-co is still updating", @@ -82,7 +82,7 @@ func Test_waitForOperatorStatusToBeDone(t *testing.T) { }}, }, }, - expErr: &payload.UpdateError{ + expErr: &payload.Error{ Nested: fmt.Errorf("cluster operator test-co is still updating"), Reason: "ClusterOperatorNotAvailable", Message: "Cluster operator test-co is still updating", @@ -108,7 +108,7 @@ func Test_waitForOperatorStatusToBeDone(t *testing.T) { }}, }, }, - expErr: &payload.UpdateError{ + expErr: &payload.Error{ Nested: fmt.Errorf("cluster operator test-co is still updating"), Reason: "ClusterOperatorNotAvailable", Message: "Cluster operator test-co is still updating", @@ -136,7 +136,7 @@ func Test_waitForOperatorStatusToBeDone(t *testing.T) { }}, }, }, - expErr: &payload.UpdateError{ + expErr: &payload.Error{ Nested: fmt.Errorf("cluster operator test-co is still updating"), Reason: "ClusterOperatorNotAvailable", Message: "Cluster operator test-co is still updating", @@ -164,7 +164,7 @@ func Test_waitForOperatorStatusToBeDone(t *testing.T) { }}, }, }, - expErr: &payload.UpdateError{ + expErr: &payload.Error{ Nested: fmt.Errorf("cluster operator test-co is still updating"), Reason: "ClusterOperatorNotAvailable", Message: "Cluster operator test-co is still updating", @@ -196,7 +196,7 @@ func Test_waitForOperatorStatusToBeDone(t *testing.T) { }}, }, }, - expErr: &payload.UpdateError{ + expErr: &payload.Error{ Nested: fmt.Errorf("cluster operator test-co is still updating"), Reason: "ClusterOperatorNotAvailable", Message: "Cluster operator test-co is still updating", @@ -228,7 +228,7 @@ func Test_waitForOperatorStatusToBeDone(t *testing.T) { }}, }, }, - expErr: &payload.UpdateError{ + expErr: &payload.Error{ Nested: fmt.Errorf("cluster operator test-co is still updating"), Reason: "ClusterOperatorNotAvailable", Message: "Cluster operator test-co is still updating", @@ -256,7 +256,7 @@ func Test_waitForOperatorStatusToBeDone(t *testing.T) { }}, }, }, - expErr: &payload.UpdateError{ + expErr: &payload.Error{ Nested: fmt.Errorf("cluster operator test-co is not done; it is available=false, progressing=true, degraded=true"), Reason: "ClusterOperatorNotAvailable", Message: "Cluster operator test-co has not yet reported success", @@ -285,7 +285,7 @@ func Test_waitForOperatorStatusToBeDone(t *testing.T) { }}, }, }, - expErr: &payload.UpdateError{ + expErr: &payload.Error{ Nested: fmt.Errorf("cluster operator test-co is not done; it is available=false, progressing=true, degraded=true"), Reason: "ClusterOperatorNotAvailable", Message: "Cluster operator test-co has not yet reported success", @@ -314,7 +314,7 @@ func Test_waitForOperatorStatusToBeDone(t *testing.T) { }}, }, }, - expErr: &payload.UpdateError{ + expErr: &payload.Error{ Nested: fmt.Errorf("cluster operator test-co is reporting a failure: random error"), Reason: "ClusterOperatorDegraded", Message: "Cluster operator test-co is reporting a failure: random error", @@ -343,7 +343,7 @@ func Test_waitForOperatorStatusToBeDone(t *testing.T) { }}, }, }, - expErr: &payload.UpdateError{ + expErr: &payload.Error{ Nested: fmt.Errorf("cluster operator test-co is not done; it is available=true, progressing=true, degraded=true"), Reason: "ClusterOperatorNotAvailable", Message: "Cluster operator test-co has not yet reported success", @@ -372,7 +372,7 @@ func Test_waitForOperatorStatusToBeDone(t *testing.T) { }}, }, }, - expErr: &payload.UpdateError{ + expErr: &payload.Error{ Nested: fmt.Errorf("cluster operator test-co is reporting a failure: random error"), Reason: "ClusterOperatorDegraded", Message: "Cluster operator test-co is reporting a failure: random error", @@ -401,7 +401,7 @@ func Test_waitForOperatorStatusToBeDone(t *testing.T) { }}, }, }, - expErr: &payload.UpdateError{ + expErr: &payload.Error{ Nested: fmt.Errorf("cluster operator test-co is reporting a failure: random error"), Reason: "ClusterOperatorDegraded", Message: "Cluster operator test-co is reporting a failure: random error", @@ -430,7 +430,7 @@ func Test_waitForOperatorStatusToBeDone(t *testing.T) { }}, }, }, - expErr: &payload.UpdateError{ + expErr: &payload.Error{ Nested: fmt.Errorf("cluster operator test-co is not done; it is available=true, progressing=true, degraded=true"), Reason: "ClusterOperatorNotAvailable", Message: "Cluster operator test-co has not yet reported success", diff --git a/pkg/cvo/status.go b/pkg/cvo/status.go index 25f2639bae..5690750782 100644 --- a/pkg/cvo/status.go +++ b/pkg/cvo/status.go @@ -221,7 +221,7 @@ func (optr *Operator) syncStatus(original, config *configv1.ClusterVersion, stat if err := status.Failure; err != nil && !skipFailure { var reason string msg := "an error occurred" - if uErr, ok := err.(*payload.UpdateError); ok { + if uErr, ok := err.(*payload.Error); ok { reason = uErr.Reason msg = payload.SummaryForReason(reason, uErr.Name) } @@ -330,7 +330,7 @@ func convertErrorToProgressing(history []configv1.UpdateHistory, now time.Time, if now.Sub(status.LastProgress) > 10*time.Minute || now.Sub(history[0].StartedTime.Time) > time.Hour { return "", "", false } - uErr, ok := status.Failure.(*payload.UpdateError) + uErr, ok := status.Failure.(*payload.Error) if !ok { return "", "", false } diff --git a/pkg/cvo/sync_worker.go b/pkg/cvo/sync_worker.go index df6219c649..1f8a507698 100644 --- a/pkg/cvo/sync_worker.go +++ b/pkg/cvo/sync_worker.go @@ -704,7 +704,7 @@ func isImageVerificationError(err error) bool { if err == nil { return false } - updateErr, ok := err.(*payload.UpdateError) + updateErr, ok := err.(*payload.Error) if !ok { return false } @@ -735,7 +735,7 @@ func summarizeTaskGraphErrors(errs []error) error { if klog.V(4) { klog.Infof("Summarizing %d errors", len(errs)) for _, err := range errs { - if uErr, ok := err.(*payload.UpdateError); ok { + if uErr, ok := err.(*payload.Error); ok { if uErr.Task != nil { klog.Infof("Update error %d/%d: %s %s (%T: %v)", uErr.Task.Index, uErr.Task.Total, uErr.Reason, uErr.Message, uErr.Nested, uErr.Nested) } else { @@ -762,7 +762,7 @@ func summarizeTaskGraphErrors(errs []error) error { func newClusterOperatorsNotAvailable(errs []error) error { names := make([]string, 0, len(errs)) for _, err := range errs { - uErr, ok := err.(*payload.UpdateError) + uErr, ok := err.(*payload.Error) if !ok || uErr.Reason != "ClusterOperatorNotAvailable" { return nil } @@ -780,7 +780,7 @@ func newClusterOperatorsNotAvailable(errs []error) error { } sort.Strings(names) name := strings.Join(names, ", ") - return &payload.UpdateError{ + return &payload.Error{ Nested: errors.NewAggregate(errs), Reason: "ClusterOperatorsNotAvailable", Message: fmt.Sprintf("Some cluster operators are still updating: %s", name), @@ -826,7 +826,7 @@ func newMultipleError(errs []error) error { if len(messages) == 0 { return errs[0] } - return &payload.UpdateError{ + return &payload.Error{ Nested: errors.NewAggregate(errs), Reason: "MultipleErrors", Message: fmt.Sprintf("Multiple errors are preventing progress:\n* %s", strings.Join(messages, "\n* ")), diff --git a/pkg/cvo/updatepayload.go b/pkg/cvo/updatepayload.go index abf490e5fe..7d74a6663c 100644 --- a/pkg/cvo/updatepayload.go +++ b/pkg/cvo/updatepayload.go @@ -85,7 +85,7 @@ func (r *payloadRetriever) RetrievePayload(ctx context.Context, update configv1. releaseDigest = update.Image[index+1:] } if err := r.verifier.Verify(ctx, releaseDigest); err != nil { - vErr := &payload.UpdateError{ + vErr := &payload.Error{ Reason: "ImageVerificationFailed", Message: fmt.Sprintf("The update cannot be verified: %v", err), Nested: err, @@ -103,7 +103,7 @@ func (r *payloadRetriever) RetrievePayload(ctx context.Context, update configv1. var err error info.Directory, err = r.targetUpdatePayloadDir(ctx, update) if err != nil { - return PayloadInfo{}, &payload.UpdateError{ + return PayloadInfo{}, &payload.Error{ Reason: "UpdatePayloadRetrievalFailed", Message: fmt.Sprintf("Unable to download and prepare the update: %v", err), } diff --git a/pkg/payload/image.go b/pkg/payload/image.go index 752557af70..0430fed70b 100644 --- a/pkg/payload/image.go +++ b/pkg/payload/image.go @@ -6,7 +6,7 @@ import ( "github.com/pkg/errors" ) -// ImageForShortName returns the image using the updatepayload embedded in +// ImageForShortName returns the image using the payload embedded in // the Operator. func ImageForShortName(name string) (string, error) { up, err := Load(DefaultPayloadDir, "") @@ -23,5 +23,5 @@ func ImageForShortName(name string) (string, error) { } } - return "", fmt.Errorf("error: Unknown name requested, could not find %s in UpdatePayload", name) + return "", fmt.Errorf("error: Unknown name requested, could not find %s in payload", name) } diff --git a/pkg/payload/payload.go b/pkg/payload/payload.go index 2515fbb6c7..b6e7440d96 100644 --- a/pkg/payload/payload.go +++ b/pkg/payload/payload.go @@ -160,7 +160,7 @@ func Load(dir, releaseImage string) (*Payload, error) { agg := utilerrors.NewAggregate(errs) if agg != nil { - return nil, &UpdateError{ + return nil, &Error{ Reason: "UpdatePayloadIntegrity", Message: fmt.Sprintf("Error loading manifests from %s: %v", dir, agg.Error()), } diff --git a/pkg/payload/task.go b/pkg/payload/task.go index 99da959453..27e5ad1913 100644 --- a/pkg/payload/task.go +++ b/pkg/payload/task.go @@ -89,15 +89,15 @@ func (st *Task) Run(ctx context.Context, version string, builder ResourceBuilder case <-time.After(d): continue case <-ctx.Done(): - if uerr, ok := lastErr.(*UpdateError); ok { + if uerr, ok := lastErr.(*Error); ok { uerr.Task = st.Copy() return uerr } - reason, cause := reasonForPayloadSyncError(lastErr) + reason, cause := reasonForError(lastErr) if len(cause) > 0 { cause = ": " + cause } - return &UpdateError{ + return &Error{ Nested: lastErr, Reason: reason, Message: fmt.Sprintf("Could not update %s%s", st, cause), @@ -108,8 +108,8 @@ func (st *Task) Run(ctx context.Context, version string, builder ResourceBuilder } } -// UpdateError is a wrapper for errors that occur during a payload sync. -type UpdateError struct { +// Error is a wrapper for errors that occur during a payload sync. +type Error struct { Nested error Reason string Message string @@ -118,18 +118,18 @@ type UpdateError struct { Task *Task } -func (e *UpdateError) Error() string { +func (e *Error) Error() string { return e.Message } -func (e *UpdateError) Cause() error { +func (e *Error) Cause() error { return e.Nested } -// reasonForUpdateError provides a succint explanation of a known error type for use in a human readable +// reasonForError provides a succint explanation of a known error type for use in a human readable // message during update. Since all objects in the image should be successfully applied, messages // should direct the reader (likely a cluster administrator) to a possible cause in their own config. -func reasonForPayloadSyncError(err error) (string, string) { +func reasonForError(err error) (string, string) { err = errors.Cause(err) switch { case apierrors.IsNotFound(err), apierrors.IsAlreadyExists(err): From 16e5a076f17f7d1c545c6d6cd81d173f6b8f0ccb Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Mon, 20 May 2019 14:46:52 -0700 Subject: [PATCH 4/6] pkg/payload: Move SummaryForReason to Error.Summary There's no need for a separate function here, and using a method gives us access to additional Error properties if we want them (although I haven't pivoted to take advantage of that yet). --- pkg/cvo/status.go | 2 +- pkg/payload/error.go | 112 +++++++++++++++++++++++++++++++++++++++++++ pkg/payload/task.go | 102 --------------------------------------- 3 files changed, 113 insertions(+), 103 deletions(-) create mode 100644 pkg/payload/error.go diff --git a/pkg/cvo/status.go b/pkg/cvo/status.go index 5690750782..b64e9062a5 100644 --- a/pkg/cvo/status.go +++ b/pkg/cvo/status.go @@ -223,7 +223,7 @@ func (optr *Operator) syncStatus(original, config *configv1.ClusterVersion, stat msg := "an error occurred" if uErr, ok := err.(*payload.Error); ok { reason = uErr.Reason - msg = payload.SummaryForReason(reason, uErr.Name) + msg = uErr.Summary() } // set the failing condition diff --git a/pkg/payload/error.go b/pkg/payload/error.go new file mode 100644 index 0000000000..785ab649de --- /dev/null +++ b/pkg/payload/error.go @@ -0,0 +1,112 @@ +package payload + +import ( + "fmt" + "strings" + + "github.com/pkg/errors" + apierrors "k8s.io/apimachinery/pkg/api/errors" + "k8s.io/apimachinery/pkg/api/meta" +) + +// Error is a wrapper for errors that occur during a payload sync. +type Error struct { + Nested error + Reason string + Message string + Name string + + Task *Task +} + +func (e *Error) Error() string { + return e.Message +} + +func (e *Error) Cause() error { + return e.Nested +} + +func (e *Error) Summary() string { + switch e.Reason { + + // likely temporary errors + case "UpdatePayloadResourceNotFound", "UpdatePayloadResourceConflict": + return "some resources could not be updated" + case "UpdatePayloadClusterDown": + return "the control plane is down or not responding" + case "UpdatePayloadClusterError": + return "the control plane is reporting an internal error" + case "UpdatePayloadClusterOverloaded": + return "the control plane is overloaded and is not accepting updates" + case "UpdatePayloadClusterUnauthorized": + return "could not authenticate to the server" + case "UpdatePayloadRetrievalFailed": + return "could not download the update" + + // likely a policy or other configuration error due to end user action + case "UpdatePayloadResourceForbidden": + return "the server is rejecting updates" + + // the image may not be correct, or the cluster may be in an unexpected + // state + case "UpdatePayloadResourceTypeMissing": + return "a required extension is not available to update" + case "UpdatePayloadResourceInvalid": + return "some cluster configuration is invalid" + case "UpdatePayloadIntegrity": + return "the contents of the update are invalid" + + case "ImageVerificationFailed": + return "the image may not be safe to use" + + case "ClusterOperatorDegraded": + if len(e.Name) > 0 { + return fmt.Sprintf("the cluster operator %s is degraded", e.Name) + } + return "a cluster operator is degraded" + case "ClusterOperatorNotAvailable": + if len(e.Name) > 0 { + return fmt.Sprintf("the cluster operator %s has not yet successfully rolled out", e.Name) + } + return "a cluster operator has not yet rolled out" + case "ClusterOperatorsNotAvailable": + return "some cluster operators have not yet rolled out" + } + + if strings.HasPrefix(e.Reason, "UpdatePayload") { + return "the update could not be applied" + } + return "an unknown error has occurred" +} + +// reasonForError provides a succint explanation of a known error type +// for use in a human readable message. Since all objects in the image +// should be successfully applied, messages should direct the reader +// (likely a cluster administrator) to a possible cause in their own +// config. +func reasonForError(err error) (string, string) { + err = errors.Cause(err) + switch { + case apierrors.IsNotFound(err), apierrors.IsAlreadyExists(err): + return "UpdatePayloadResourceNotFound", "resource may have been deleted" + case apierrors.IsConflict(err): + return "UpdatePayloadResourceConflict", "someone else is updating this resource" + case apierrors.IsTimeout(err), apierrors.IsServiceUnavailable(err), apierrors.IsUnexpectedServerError(err): + return "UpdatePayloadClusterDown", "the server is down or not responding" + case apierrors.IsInternalError(err): + return "UpdatePayloadClusterError", "the server is reporting an internal error" + case apierrors.IsInvalid(err): + return "UpdatePayloadResourceInvalid", "the object is invalid, possibly due to local cluster configuration" + case apierrors.IsUnauthorized(err): + return "UpdatePayloadClusterUnauthorized", "could not authenticate to the server" + case apierrors.IsForbidden(err): + return "UpdatePayloadResourceForbidden", "the server has forbidden updates to this resource" + case apierrors.IsServerTimeout(err), apierrors.IsTooManyRequests(err): + return "UpdatePayloadClusterOverloaded", "the server is overloaded and is not accepting updates" + case meta.IsNoMatchError(err): + return "UpdatePayloadResourceTypeMissing", "the server does not recognize this resource, check extension API servers" + default: + return "UpdatePayloadFailed", "" + } +} diff --git a/pkg/payload/task.go b/pkg/payload/task.go index 27e5ad1913..6dc39d6f8d 100644 --- a/pkg/payload/task.go +++ b/pkg/payload/task.go @@ -9,8 +9,6 @@ import ( "github.com/pkg/errors" "github.com/prometheus/client_golang/prometheus" - apierrors "k8s.io/apimachinery/pkg/api/errors" - "k8s.io/apimachinery/pkg/api/meta" utilruntime "k8s.io/apimachinery/pkg/util/runtime" "k8s.io/apimachinery/pkg/util/wait" @@ -107,103 +105,3 @@ func (st *Task) Run(ctx context.Context, version string, builder ResourceBuilder } } } - -// Error is a wrapper for errors that occur during a payload sync. -type Error struct { - Nested error - Reason string - Message string - Name string - - Task *Task -} - -func (e *Error) Error() string { - return e.Message -} - -func (e *Error) Cause() error { - return e.Nested -} - -// reasonForError provides a succint explanation of a known error type for use in a human readable -// message during update. Since all objects in the image should be successfully applied, messages -// should direct the reader (likely a cluster administrator) to a possible cause in their own config. -func reasonForError(err error) (string, string) { - err = errors.Cause(err) - switch { - case apierrors.IsNotFound(err), apierrors.IsAlreadyExists(err): - return "UpdatePayloadResourceNotFound", "resource may have been deleted" - case apierrors.IsConflict(err): - return "UpdatePayloadResourceConflict", "someone else is updating this resource" - case apierrors.IsTimeout(err), apierrors.IsServiceUnavailable(err), apierrors.IsUnexpectedServerError(err): - return "UpdatePayloadClusterDown", "the server is down or not responding" - case apierrors.IsInternalError(err): - return "UpdatePayloadClusterError", "the server is reporting an internal error" - case apierrors.IsInvalid(err): - return "UpdatePayloadResourceInvalid", "the object is invalid, possibly due to local cluster configuration" - case apierrors.IsUnauthorized(err): - return "UpdatePayloadClusterUnauthorized", "could not authenticate to the server" - case apierrors.IsForbidden(err): - return "UpdatePayloadResourceForbidden", "the server has forbidden updates to this resource" - case apierrors.IsServerTimeout(err), apierrors.IsTooManyRequests(err): - return "UpdatePayloadClusterOverloaded", "the server is overloaded and is not accepting updates" - case meta.IsNoMatchError(err): - return "UpdatePayloadResourceTypeMissing", "the server does not recognize this resource, check extension API servers" - default: - return "UpdatePayloadFailed", "" - } -} - -func SummaryForReason(reason, name string) string { - switch reason { - - // likely temporary errors - case "UpdatePayloadResourceNotFound", "UpdatePayloadResourceConflict": - return "some resources could not be updated" - case "UpdatePayloadClusterDown": - return "the control plane is down or not responding" - case "UpdatePayloadClusterError": - return "the control plane is reporting an internal error" - case "UpdatePayloadClusterOverloaded": - return "the control plane is overloaded and is not accepting updates" - case "UpdatePayloadClusterUnauthorized": - return "could not authenticate to the server" - case "UpdatePayloadRetrievalFailed": - return "could not download the update" - - // likely a policy or other configuration error due to end user action - case "UpdatePayloadResourceForbidden": - return "the server is rejecting updates" - - // the image may not be correct, or the cluster may be in an unexpected - // state - case "UpdatePayloadResourceTypeMissing": - return "a required extension is not available to update" - case "UpdatePayloadResourceInvalid": - return "some cluster configuration is invalid" - case "UpdatePayloadIntegrity": - return "the contents of the update are invalid" - - case "ImageVerificationFailed": - return "the image may not be safe to use" - - case "ClusterOperatorDegraded": - if len(name) > 0 { - return fmt.Sprintf("the cluster operator %s is degraded", name) - } - return "a cluster operator is degraded" - case "ClusterOperatorNotAvailable": - if len(name) > 0 { - return fmt.Sprintf("the cluster operator %s has not yet successfully rolled out", name) - } - return "a cluster operator has not yet rolled out" - case "ClusterOperatorsNotAvailable": - return "some cluster operators have not yet rolled out" - } - - if strings.HasPrefix(reason, "UpdatePayload") { - return "the update could not be applied" - } - return "an unknown error has occurred" -} From b3a96bc6ca5479da5ddbb6e059bc5e4faf1f4c1e Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Mon, 20 May 2019 14:57:36 -0700 Subject: [PATCH 5/6] pkg/payload: Improve Load error (UpdatePayloadIntegrity -> LoadManifestsError) The errors feeding this have little to do with integrity. And they aren't upgrade-specific; disk corruption or similar during regular reconciliation could also lead to this error. --- pkg/cvo/cvo_test.go | 24 ++++++++++++------------ pkg/cvo/status_test.go | 2 +- pkg/payload/error.go | 4 ++-- pkg/payload/payload.go | 2 +- 4 files changed, 16 insertions(+), 16 deletions(-) diff --git a/pkg/cvo/cvo_test.go b/pkg/cvo/cvo_test.go index 2b0001894c..a34c2a0765 100644 --- a/pkg/cvo/cvo_test.go +++ b/pkg/cvo/cvo_test.go @@ -228,7 +228,7 @@ func TestOperator_sync(t *testing.T) { Reconciling: false, Actual: configv1.Update{Version: "0.0.1-abc", Image: "image/image:v4.0.1"}, Failure: &payload.Error{ - Reason: "UpdatePayloadIntegrity", + Reason: "LoadManifestsError", Message: "unable to apply object", }, }, @@ -253,7 +253,7 @@ func TestOperator_sync(t *testing.T) { VersionHash: "", Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionFalse}, - {Type: ClusterStatusFailing, Status: configv1.ConditionTrue, Reason: "UpdatePayloadIntegrity", Message: "unable to apply object"}, + {Type: ClusterStatusFailing, Status: configv1.ConditionTrue, Reason: "LoadManifestsError", Message: "unable to apply object"}, {Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Message: "Working towards 4.0.1"}, {Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse}, }, @@ -284,8 +284,8 @@ func TestOperator_sync(t *testing.T) { VersionHash: "", Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionFalse}, - {Type: ClusterStatusFailing, Status: configv1.ConditionTrue, Reason: "UpdatePayloadIntegrity", Message: "unable to apply object"}, - {Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Reason: "UpdatePayloadIntegrity", Message: "Unable to apply 0.0.1-abc: the contents of the update are invalid"}, + {Type: ClusterStatusFailing, Status: configv1.ConditionTrue, Reason: "LoadManifestsError", Message: "unable to apply object"}, + {Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Reason: "LoadManifestsError", Message: "Unable to apply 0.0.1-abc: failed to load manifests from the release image"}, {Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse}, }, }, @@ -305,7 +305,7 @@ func TestOperator_sync(t *testing.T) { Reconciling: true, Actual: configv1.Update{Version: "0.0.1-abc", Image: "image/image:v4.0.1"}, Failure: &payload.Error{ - Reason: "UpdatePayloadIntegrity", + Reason: "LoadManifestsError", Message: "unable to apply object", }, }, @@ -326,7 +326,7 @@ func TestOperator_sync(t *testing.T) { VersionHash: "", Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionFalse}, - {Type: ClusterStatusFailing, Status: configv1.ConditionTrue, Reason: "UpdatePayloadIntegrity", Message: "unable to apply object"}, + {Type: ClusterStatusFailing, Status: configv1.ConditionTrue, Reason: "LoadManifestsError", Message: "unable to apply object"}, {Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Message: "Working towards 4.0.1"}, {Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse}, }, @@ -357,8 +357,8 @@ func TestOperator_sync(t *testing.T) { VersionHash: "", Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionFalse}, - {Type: ClusterStatusFailing, Status: configv1.ConditionTrue, Reason: "UpdatePayloadIntegrity", Message: "unable to apply object"}, - {Type: configv1.OperatorProgressing, Status: configv1.ConditionFalse, Reason: "UpdatePayloadIntegrity", Message: "Error while reconciling 0.0.1-abc: the contents of the update are invalid"}, + {Type: ClusterStatusFailing, Status: configv1.ConditionTrue, Reason: "LoadManifestsError", Message: "unable to apply object"}, + {Type: configv1.OperatorProgressing, Status: configv1.ConditionFalse, Reason: "LoadManifestsError", Message: "Error while reconciling 0.0.1-abc: failed to load manifests from the release image"}, {Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse}, }, }, @@ -379,7 +379,7 @@ func TestOperator_sync(t *testing.T) { Completed: 2, Actual: configv1.Update{Version: "0.0.1-abc", Image: "image/image:v4.0.1"}, Failure: &payload.Error{ - Reason: "UpdatePayloadIntegrity", + Reason: "LoadManifestsError", Message: "unable to apply object", }, }, @@ -400,7 +400,7 @@ func TestOperator_sync(t *testing.T) { VersionHash: "", Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionFalse}, - {Type: ClusterStatusFailing, Status: configv1.ConditionTrue, Reason: "UpdatePayloadIntegrity", Message: "unable to apply object"}, + {Type: ClusterStatusFailing, Status: configv1.ConditionTrue, Reason: "LoadManifestsError", Message: "unable to apply object"}, {Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Message: "Working towards 4.0.1"}, {Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse}, }, @@ -431,8 +431,8 @@ func TestOperator_sync(t *testing.T) { VersionHash: "", Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 0.0.1-abc"}, - {Type: ClusterStatusFailing, Status: configv1.ConditionTrue, Reason: "UpdatePayloadIntegrity", Message: "unable to apply object"}, - {Type: configv1.OperatorProgressing, Status: configv1.ConditionFalse, Reason: "UpdatePayloadIntegrity", Message: "Error while reconciling 0.0.1-abc: the contents of the update are invalid"}, + {Type: ClusterStatusFailing, Status: configv1.ConditionTrue, Reason: "LoadManifestsError", Message: "unable to apply object"}, + {Type: configv1.OperatorProgressing, Status: configv1.ConditionFalse, Reason: "LoadManifestsError", Message: "Error while reconciling 0.0.1-abc: failed to load manifests from the release image"}, {Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse}, }, }, diff --git a/pkg/cvo/status_test.go b/pkg/cvo/status_test.go index d8c2e75c2e..62392c94bd 100644 --- a/pkg/cvo/status_test.go +++ b/pkg/cvo/status_test.go @@ -177,7 +177,7 @@ func TestOperator_syncFailingStatus(t *testing.T) { VersionHash: "", Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionFalse}, - {Type: ClusterStatusFailing, Status: configv1.ConditionTrue, Reason: "UpdatePayloadIntegrity", Message: "unable to apply object"}, + {Type: ClusterStatusFailing, Status: configv1.ConditionTrue, Reason: "LoadManifestsError", Message: "unable to apply object"}, {Type: configv1.OperatorProgressing, Status: configv1.ConditionTrue, Message: "Working towards 4.0.1"}, {Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse}, }, diff --git a/pkg/payload/error.go b/pkg/payload/error.go index 785ab649de..ac8a88dd47 100644 --- a/pkg/payload/error.go +++ b/pkg/payload/error.go @@ -54,8 +54,8 @@ func (e *Error) Summary() string { return "a required extension is not available to update" case "UpdatePayloadResourceInvalid": return "some cluster configuration is invalid" - case "UpdatePayloadIntegrity": - return "the contents of the update are invalid" + case "LoadManifestsError": + return "failed to load manifests from the release image" case "ImageVerificationFailed": return "the image may not be safe to use" diff --git a/pkg/payload/payload.go b/pkg/payload/payload.go index b6e7440d96..d2f97fdff7 100644 --- a/pkg/payload/payload.go +++ b/pkg/payload/payload.go @@ -161,7 +161,7 @@ func Load(dir, releaseImage string) (*Payload, error) { agg := utilerrors.NewAggregate(errs) if agg != nil { return nil, &Error{ - Reason: "UpdatePayloadIntegrity", + Reason: "LoadManifestsError", Message: fmt.Sprintf("Error loading manifests from %s: %v", dir, agg.Error()), } } From 52424871983c85596cdb727e944318ac9c8116a6 Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Mon, 20 May 2019 22:22:49 -0700 Subject: [PATCH 6/6] pkg/payload: Change reasonForError to messageForError This allows us to avoid having two separate but very similar blocks of code for converting these errors to succint, admin-oriented descriptions. --- pkg/cvo/cvo_scenarios_test.go | 8 +++--- pkg/payload/error.go | 49 ++++++++++------------------------- pkg/payload/task.go | 9 ++----- 3 files changed, 20 insertions(+), 46 deletions(-) diff --git a/pkg/cvo/cvo_scenarios_test.go b/pkg/cvo/cvo_scenarios_test.go index de25eff20f..7acadf37f8 100644 --- a/pkg/cvo/cvo_scenarios_test.go +++ b/pkg/cvo/cvo_scenarios_test.go @@ -1387,8 +1387,8 @@ func TestCVO_ErrorDuringReconcile(t *testing.T) { VersionHash: "6GC9TkkG9PA=", Failure: &payload.Error{ Nested: fmt.Errorf("unable to proceed"), - Reason: "UpdatePayloadFailed", - Message: "Could not update test \"file-yml\" (3 of 3)", + Reason: "ApplyManifestError", + Message: "Could not apply test \"file-yml\" (3 of 3): unable to proceed", Task: &payload.Task{Index: 3, Total: 3, Manifest: &worker.payload.Manifests[2]}, }, Actual: configv1.Update{Version: "1.0.0-abc", Image: "image/image:1"}, @@ -1423,8 +1423,8 @@ func TestCVO_ErrorDuringReconcile(t *testing.T) { }, Conditions: []configv1.ClusterOperatorStatusCondition{ {Type: configv1.OperatorAvailable, Status: configv1.ConditionTrue, Message: "Done applying 1.0.0-abc"}, - {Type: ClusterStatusFailing, Status: configv1.ConditionTrue, Reason: "UpdatePayloadFailed", Message: "Could not update test \"file-yml\" (3 of 3)"}, - {Type: configv1.OperatorProgressing, Status: configv1.ConditionFalse, Reason: "UpdatePayloadFailed", Message: "Error while reconciling 1.0.0-abc: the update could not be applied"}, + {Type: ClusterStatusFailing, Status: configv1.ConditionTrue, Reason: "ApplyManifestError", Message: "Could not apply test \"file-yml\" (3 of 3): unable to proceed"}, + {Type: configv1.OperatorProgressing, Status: configv1.ConditionFalse, Reason: "ApplyManifestError", Message: "Error while reconciling 1.0.0-abc: Could not apply test \"file-yml\" (3 of 3): unable to proceed"}, {Type: configv1.RetrievedUpdates, Status: configv1.ConditionFalse}, }, }, diff --git a/pkg/payload/error.go b/pkg/payload/error.go index ac8a88dd47..22c8e84ba4 100644 --- a/pkg/payload/error.go +++ b/pkg/payload/error.go @@ -30,30 +30,9 @@ func (e *Error) Cause() error { func (e *Error) Summary() string { switch e.Reason { - // likely temporary errors - case "UpdatePayloadResourceNotFound", "UpdatePayloadResourceConflict": - return "some resources could not be updated" - case "UpdatePayloadClusterDown": - return "the control plane is down or not responding" - case "UpdatePayloadClusterError": - return "the control plane is reporting an internal error" - case "UpdatePayloadClusterOverloaded": - return "the control plane is overloaded and is not accepting updates" - case "UpdatePayloadClusterUnauthorized": - return "could not authenticate to the server" - case "UpdatePayloadRetrievalFailed": - return "could not download the update" - - // likely a policy or other configuration error due to end user action - case "UpdatePayloadResourceForbidden": - return "the server is rejecting updates" + case "ApplyManifestError": + return e.Message - // the image may not be correct, or the cluster may be in an unexpected - // state - case "UpdatePayloadResourceTypeMissing": - return "a required extension is not available to update" - case "UpdatePayloadResourceInvalid": - return "some cluster configuration is invalid" case "LoadManifestsError": return "failed to load manifests from the release image" @@ -80,33 +59,33 @@ func (e *Error) Summary() string { return "an unknown error has occurred" } -// reasonForError provides a succint explanation of a known error type +// messageForError provides a succint explanation of a known error type // for use in a human readable message. Since all objects in the image // should be successfully applied, messages should direct the reader // (likely a cluster administrator) to a possible cause in their own // config. -func reasonForError(err error) (string, string) { +func messageForError(err error) string { err = errors.Cause(err) switch { case apierrors.IsNotFound(err), apierrors.IsAlreadyExists(err): - return "UpdatePayloadResourceNotFound", "resource may have been deleted" + return "resource may have been deleted" case apierrors.IsConflict(err): - return "UpdatePayloadResourceConflict", "someone else is updating this resource" + return "someone else is updating this resource" case apierrors.IsTimeout(err), apierrors.IsServiceUnavailable(err), apierrors.IsUnexpectedServerError(err): - return "UpdatePayloadClusterDown", "the server is down or not responding" + return "the control plane is down or not responding" case apierrors.IsInternalError(err): - return "UpdatePayloadClusterError", "the server is reporting an internal error" + return "the control plane is reporting an internal error" case apierrors.IsInvalid(err): - return "UpdatePayloadResourceInvalid", "the object is invalid, possibly due to local cluster configuration" + return "the object is invalid, possibly due to local cluster configuration" case apierrors.IsUnauthorized(err): - return "UpdatePayloadClusterUnauthorized", "could not authenticate to the server" + return "could not authenticate to the control plane" case apierrors.IsForbidden(err): - return "UpdatePayloadResourceForbidden", "the server has forbidden updates to this resource" + return "the control plane has forbidden updates to this resource" case apierrors.IsServerTimeout(err), apierrors.IsTooManyRequests(err): - return "UpdatePayloadClusterOverloaded", "the server is overloaded and is not accepting updates" + return "the control plane is overloaded and is not accepting updates" case meta.IsNoMatchError(err): - return "UpdatePayloadResourceTypeMissing", "the server does not recognize this resource, check extension API servers" + return "the control plane does not recognize this resource, check extension API servers" default: - return "UpdatePayloadFailed", "" + return err.Error() } } diff --git a/pkg/payload/task.go b/pkg/payload/task.go index 6dc39d6f8d..f5f7d08ba8 100644 --- a/pkg/payload/task.go +++ b/pkg/payload/task.go @@ -91,15 +91,10 @@ func (st *Task) Run(ctx context.Context, version string, builder ResourceBuilder uerr.Task = st.Copy() return uerr } - reason, cause := reasonForError(lastErr) - if len(cause) > 0 { - cause = ": " + cause - } return &Error{ Nested: lastErr, - Reason: reason, - Message: fmt.Sprintf("Could not update %s%s", st, cause), - + Reason: "ApplyManifestError", + Message: fmt.Sprintf("Could not apply %s: %s", st, messageForError(lastErr)), Task: st.Copy(), } }