From 5d343a503d4f4b46e8a3493b82f2673556021d6b Mon Sep 17 00:00:00 2001 From: "W. Trevor King" Date: Fri, 10 Dec 2021 09:27:17 -0800 Subject: [PATCH] pkg/payload: Log manifest exclusion We have a number of different annotations all working together now, so logging exclusions with our reasoning will help folks who are debugging "why is my manifest not being reconciled?". And while this will create a number of logs for some profiles, we we already log each reconciled manifest with every sync cycle, so the volume increase isn't huge compared to our existing log rate. This is primarily a dev-time issue, which argues against log spew for production clusters, but since 72599d323d (Exclude featuregate.release.openshift/tech-preview=true manifests, 2021-04-13, #694), there's been the dynamic tech-preview business, so there is now more utility in production logging around this than there was before. --- pkg/payload/payload.go | 35 ++++++++++++++++++++--------------- pkg/payload/payload_test.go | 28 ++++++++++++++-------------- 2 files changed, 34 insertions(+), 29 deletions(-) diff --git a/pkg/payload/payload.go b/pkg/payload/payload.go index 49104c4b4..ac931eb1f 100644 --- a/pkg/payload/payload.go +++ b/pkg/payload/payload.go @@ -179,19 +179,21 @@ func LoadUpdate(dir, releaseImage, excludeIdentifier string, includeTechPreview errs = append(errs, fmt.Errorf("parse %s: %w", file.Name(), err)) continue } + originalFilename := filepath.Base(file.Name()) + for i := range ms { + ms[i].OriginalFilename = originalFilename + } + // Filter out manifests that should be excluded based on annotation filteredMs := []manifest.Manifest{} for _, manifest := range ms { - if shouldExclude(excludeIdentifier, includeTechPreview, profile, &manifest) { + if err := include(excludeIdentifier, includeTechPreview, profile, &manifest); err != nil { + klog.V(5).Infof("excluding %s group=%s kind=%s namespace=%s name=%s: %v\n", manifest.OriginalFilename, manifest.GVK.Group, manifest.GVK.Kind, manifest.Obj.GetNamespace(), manifest.Obj.GetName(), err) continue } filteredMs = append(filteredMs, manifest) } - ms = filteredMs - for i := range ms { - ms[i].OriginalFilename = filepath.Base(file.Name()) - } - manifests = append(manifests, ms...) + manifests = append(manifests, filteredMs...) } } @@ -213,31 +215,34 @@ func LoadUpdate(dir, releaseImage, excludeIdentifier string, includeTechPreview return payload, nil } -func shouldExclude(excludeIdentifier string, includeTechPreview bool, profile string, manifest *manifest.Manifest) bool { +func include(excludeIdentifier string, includeTechPreview bool, profile string, manifest *manifest.Manifest) error { annotations := manifest.Obj.GetAnnotations() if annotations == nil { - return true + return errors.New("no annotations") } excludeAnnotation := fmt.Sprintf("exclude.release.openshift.io/%s", excludeIdentifier) - if annotations[excludeAnnotation] == "true" { - return true + if v := annotations[excludeAnnotation]; v == "true" { + return fmt.Errorf("%s=%s", excludeAnnotation, v) } - featureGateAnnotationValue, featureGateAnnotationExists := annotations["release.openshift.io/feature-gate"] + featureGateAnnotation := "release.openshift.io/feature-gate" + featureGateAnnotationValue, featureGateAnnotationExists := annotations[featureGateAnnotation] if featureGateAnnotationValue == "TechPreviewNoUpgrade" && !includeTechPreview { - return true + return fmt.Errorf("tech-preview excluded, and %s=%s", featureGateAnnotation, featureGateAnnotationValue) } // never include the manifest if the feature-gate annotation is outside of allowed values (only TechPreviewNoUpgrade is currently allowed) if featureGateAnnotationExists && featureGateAnnotationValue != "TechPreviewNoUpgrade" { - return true + return fmt.Errorf("unrecognized value %s=%s", featureGateAnnotation, featureGateAnnotationValue) } profileAnnotation := fmt.Sprintf("include.release.openshift.io/%s", profile) if val, ok := annotations[profileAnnotation]; ok && val == "true" { - return false + return nil + } else if ok { + return fmt.Errorf("unrecognized value %s=%s", profileAnnotation, val) } - return true + return fmt.Errorf("%s unset", profileAnnotation) } // ValidateDirectory checks if a directory can be a candidate update by diff --git a/pkg/payload/payload_test.go b/pkg/payload/payload_test.go index b723152d0..08ce1e479 100644 --- a/pkg/payload/payload_test.go +++ b/pkg/payload/payload_test.go @@ -1,6 +1,7 @@ package payload import ( + "errors" "io/ioutil" "path/filepath" "reflect" @@ -17,7 +18,7 @@ import ( "github.com/openshift/library-go/pkg/manifest" ) -func Test_loadUpdatePayload(t *testing.T) { +func TestLoadUpdate(t *testing.T) { type args struct { dir string releaseImage string @@ -132,7 +133,7 @@ func mustRead(path string) []byte { return data } -func Test_Exclude(t *testing.T) { +func Test_include(t *testing.T) { tests := []struct { name string exclude string @@ -140,7 +141,7 @@ func Test_Exclude(t *testing.T) { profile string annotations map[string]interface{} - isExcluded bool + expected error }{ { name: "exclusion identifier set", @@ -149,13 +150,13 @@ func Test_Exclude(t *testing.T) { annotations: map[string]interface{}{ "exclude.release.openshift.io/identifier": "true", "include.release.openshift.io/self-managed-high-availability": "true"}, - isExcluded: true, + expected: errors.New("exclude.release.openshift.io/identifier=true"), }, { name: "profile selection works", profile: "single-node", annotations: map[string]interface{}{"include.release.openshift.io/self-managed-high-availability": "true"}, - isExcluded: true, + expected: errors.New("include.release.openshift.io/single-node unset"), }, { name: "profile selection works included", @@ -169,7 +170,7 @@ func Test_Exclude(t *testing.T) { "include.release.openshift.io/self-managed-high-availability": "true", "release.openshift.io/feature-gate": "TechPreviewNoUpgrade", }, - isExcluded: true, + expected: errors.New("tech-preview excluded, and release.openshift.io/feature-gate=TechPreviewNoUpgrade"), }, { name: "correct techpreview value is included if techpreview on", @@ -179,7 +180,6 @@ func Test_Exclude(t *testing.T) { "include.release.openshift.io/self-managed-high-availability": "true", "release.openshift.io/feature-gate": "TechPreviewNoUpgrade", }, - isExcluded: false, }, { name: "incorrect techpreview value is not excluded if techpreview off", @@ -188,7 +188,7 @@ func Test_Exclude(t *testing.T) { "include.release.openshift.io/self-managed-high-availability": "true", "release.openshift.io/feature-gate": "Other", }, - isExcluded: true, + expected: errors.New("unrecognized value release.openshift.io/feature-gate=Other"), }, { name: "incorrect techpreview value is not excluded if techpreview on", @@ -198,19 +198,19 @@ func Test_Exclude(t *testing.T) { "include.release.openshift.io/self-managed-high-availability": "true", "release.openshift.io/feature-gate": "Other", }, - isExcluded: true, + expected: errors.New("unrecognized value release.openshift.io/feature-gate=Other"), }, { name: "default profile selection excludes without annotation", profile: DefaultClusterProfile, annotations: map[string]interface{}{}, - isExcluded: true, + expected: errors.New("include.release.openshift.io/self-managed-high-availability unset"), }, { name: "default profile selection excludes with no annotation", profile: DefaultClusterProfile, annotations: nil, - isExcluded: true, + expected: errors.New("no annotations"), }, } for _, tt := range tests { @@ -219,15 +219,15 @@ func Test_Exclude(t *testing.T) { if tt.annotations != nil { metadata["annotations"] = tt.annotations } - ret := shouldExclude(tt.exclude, tt.includeTechPreview, tt.profile, &manifest.Manifest{ + err := include(tt.exclude, tt.includeTechPreview, tt.profile, &manifest.Manifest{ Obj: &unstructured.Unstructured{ Object: map[string]interface{}{ "metadata": metadata, }, }, }) - if ret != tt.isExcluded { - t.Errorf("(exclude: %v, profile: %v, annotations: %v) %v != %v", tt.exclude, tt.profile, tt.annotations, tt.isExcluded, ret) + if !reflect.DeepEqual(err, tt.expected) { + t.Errorf("(exclude: %v, profile: %v, annotations: %v) expected %v, but got %v", tt.exclude, tt.profile, tt.annotations, tt.expected, err) } }) }