From 4b6ed6a31c2567add9ef427fbbf44090f633042c Mon Sep 17 00:00:00 2001 From: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Date: Thu, 29 Jun 2023 15:40:41 -0400 Subject: [PATCH 1/4] chore: add more logging for failures to get label metadata Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --- reposerver/repository/repository.go | 2 +- util/argo/resource_tracking.go | 33 ++++++++++++++++++++--------- util/kube/kube.go | 7 +++--- 3 files changed, 28 insertions(+), 14 deletions(-) diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index a35dabca58046..a6875789d68e1 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -1397,7 +1397,7 @@ func GenerateManifests(ctx context.Context, appPath, repoRoot, revision string, if q.AppLabelKey != "" && q.AppName != "" && !kube.IsCRD(target) { err = resourceTracking.SetAppInstance(target, q.AppLabelKey, q.AppName, q.Namespace, v1alpha1.TrackingMethod(q.TrackingMethod)) if err != nil { - return nil, err + return nil, fmt.Errorf("failed to set app instance tracking info on manifest: %w", err) } } manifestStr, err := json.Marshal(target.Object) diff --git a/util/argo/resource_tracking.go b/util/argo/resource_tracking.go index 92f3f69d6c1ea..a0ec6b302a9aa 100644 --- a/util/argo/resource_tracking.go +++ b/util/argo/resource_tracking.go @@ -4,12 +4,13 @@ import ( "fmt" "strings" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" + "github.com/argoproj/argo-cd/v2/common" "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1" "github.com/argoproj/argo-cd/v2/util/kube" argokube "github.com/argoproj/argo-cd/v2/util/kube" "github.com/argoproj/argo-cd/v2/util/settings" - "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" ) const ( @@ -31,7 +32,7 @@ type ResourceTracking interface { Normalize(config, live *unstructured.Unstructured, labelKey, trackingMethod string) error } -//AppInstanceValue store information about resource tracking info +// AppInstanceValue store information about resource tracking info type AppInstanceValue struct { ApplicationName string Group string @@ -140,7 +141,11 @@ func (rt *resourceTracking) SetAppInstance(un *unstructured.Unstructured, key, v } switch trackingMethod { case TrackingMethodLabel: - return argokube.SetAppInstanceLabel(un, key, val) + err := argokube.SetAppInstanceLabel(un, key, val) + if err != nil { + return fmt.Errorf("failed to set app instance label: %w", err) + } + return nil case TrackingMethodAnnotation: return setAppInstanceAnnotation() case TrackingMethodAnnotationAndLabel: @@ -151,18 +156,26 @@ func (rt *resourceTracking) SetAppInstance(un *unstructured.Unstructured, key, v if len(val) > LabelMaxLength { val = val[:LabelMaxLength] } - return argokube.SetAppInstanceLabel(un, key, val) + err = argokube.SetAppInstanceLabel(un, key, val) + if err != nil { + return fmt.Errorf("failed to set app instance label: %w", err) + } + return nil default: - return argokube.SetAppInstanceLabel(un, key, val) + err := argokube.SetAppInstanceLabel(un, key, val) + if err != nil { + return fmt.Errorf("failed to set app instance label: %w", err) + } + return nil } } -//BuildAppInstanceValue build resource tracking id in format ;/// +// BuildAppInstanceValue build resource tracking id in format ;/// func (rt *resourceTracking) BuildAppInstanceValue(value AppInstanceValue) string { return fmt.Sprintf("%s:%s/%s:%s/%s", value.ApplicationName, value.Group, value.Kind, value.Namespace, value.Name) } -//ParseAppInstanceValue parse resource tracking id from format :/:/ to struct +// ParseAppInstanceValue parse resource tracking id from format :/:/ to struct func (rt *resourceTracking) ParseAppInstanceValue(value string) (*AppInstanceValue, error) { var appInstanceValue AppInstanceValue parts := strings.Split(value, ":") @@ -198,7 +211,7 @@ func (rt *resourceTracking) Normalize(config, live *unstructured.Unstructured, l label, err := kube.GetAppInstanceLabel(live, labelKey) if err != nil { - return err + return fmt.Errorf("failed to get app instance label: %w", err) } if label == "" { return nil @@ -215,12 +228,12 @@ func (rt *resourceTracking) Normalize(config, live *unstructured.Unstructured, l label, err = argokube.GetAppInstanceLabel(config, labelKey) if err != nil { - return err + return fmt.Errorf("failed to get app instance label: %w", err) } if label == "" { err = argokube.RemoveLabel(live, labelKey) if err != nil { - return err + return fmt.Errorf("failed to remove app instance label: %w", err) } } diff --git a/util/kube/kube.go b/util/kube/kube.go index ad3dd47e804eb..269d3372077a3 100644 --- a/util/kube/kube.go +++ b/util/kube/kube.go @@ -1,6 +1,7 @@ package kube import ( + "fmt" "regexp" "github.com/argoproj/gitops-engine/pkg/utils/kube" @@ -23,7 +24,7 @@ func SetAppInstanceLabel(target *unstructured.Unstructured, key, val string) err // Do not use target.GetLabels(), https://github.com/argoproj/argo-cd/issues/13730 labels, _, err := unstructured.NestedStringMap(target.Object, "metadata", "labels") if err != nil { - return err + return fmt.Errorf("failed to get labels from target object %s %s/%s: %w", target.GroupVersionKind().String(), target.GetNamespace(), target.GetName(), err) } if labels == nil { labels = make(map[string]string) @@ -131,7 +132,7 @@ func GetAppInstanceLabel(un *unstructured.Unstructured, key string) (string, err // Do not use target.GetLabels(), https://github.com/argoproj/argo-cd/issues/13730 labels, _, err := unstructured.NestedStringMap(un.Object, "metadata", "labels") if err != nil { - return "", err + return "", fmt.Errorf("failed to get labels for %s %s/%s: %w", un.GroupVersionKind().String(), un.GetNamespace(), un.GetName(), err) } if labels != nil { return labels[key], nil @@ -144,7 +145,7 @@ func RemoveLabel(un *unstructured.Unstructured, key string) error { // Do not use target.GetLabels(), https://github.com/argoproj/argo-cd/issues/13730 labels, _, err := unstructured.NestedStringMap(un.Object, "metadata", "labels") if err != nil { - return err + return fmt.Errorf("failed to get labels for %s %s/%s: %w", un.GroupVersionKind().String(), un.GetNamespace(), un.GetName(), err) } if labels == nil { return nil From 3be1b41b4604e9cec0503362eac68ad22826b949 Mon Sep 17 00:00:00 2001 From: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Date: Thu, 29 Jun 2023 15:58:32 -0400 Subject: [PATCH 2/4] context Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --- reposerver/repository/repository.go | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/reposerver/repository/repository.go b/reposerver/repository/repository.go index a6875789d68e1..016ae24f49ac9 100644 --- a/reposerver/repository/repository.go +++ b/reposerver/repository/repository.go @@ -784,6 +784,11 @@ func (s *Service) runManifestGenAsync(ctx context.Context, repoRoot, commitSHA, } } if err != nil { + logCtx := log.WithFields(log.Fields{ + "application": q.AppName, + "appNamespace": q.Namespace, + }) + // If manifest generation error caching is enabled if s.initConstants.PauseGenerationAfterFailedGenerationAttempts > 0 { cache.LogDebugManifestCacheKeyFields("getting manifests cache", "GenerateManifests error", cacheKey, q.ApplicationSource, q.RefSources, q, q.Namespace, q.TrackingMethod, q.AppLabelKey, q.AppName, refSourceCommitSHAs) @@ -793,7 +798,7 @@ func (s *Service) runManifestGenAsync(ctx context.Context, repoRoot, commitSHA, innerRes := &cache.CachedManifestResponse{} cacheErr := s.cache.GetManifests(cacheKey, appSourceCopy, q.RefSources, q, q.Namespace, q.TrackingMethod, q.AppLabelKey, q.AppName, innerRes, refSourceCommitSHAs) if cacheErr != nil && cacheErr != reposervercache.ErrCacheMiss { - log.Warnf("manifest cache set error %s: %v", appSourceCopy.String(), cacheErr) + logCtx.Warnf("manifest cache get error %s: %v", appSourceCopy.String(), cacheErr) ch.errCh <- cacheErr return } @@ -811,7 +816,7 @@ func (s *Service) runManifestGenAsync(ctx context.Context, repoRoot, commitSHA, innerRes.MostRecentError = err.Error() cacheErr = s.cache.SetManifests(cacheKey, appSourceCopy, q.RefSources, q, q.Namespace, q.TrackingMethod, q.AppLabelKey, q.AppName, innerRes, refSourceCommitSHAs) if cacheErr != nil { - log.Warnf("manifest cache set error %s: %v", appSourceCopy.String(), cacheErr) + logCtx.Warnf("manifest cache set error %s: %v", appSourceCopy.String(), cacheErr) ch.errCh <- cacheErr return } From e54d71c24bef7d3e0ad229a49a0f4b7b90511ea4 Mon Sep 17 00:00:00 2001 From: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Date: Thu, 29 Jun 2023 16:16:01 -0400 Subject: [PATCH 3/4] fix test Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --- util/kube/kube_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/kube/kube_test.go b/util/kube/kube_test.go index f7fc1607aaa39..e273c16894b5e 100644 --- a/util/kube/kube_test.go +++ b/util/kube/kube_test.go @@ -242,7 +242,7 @@ func TestGetAppInstanceLabelWithInvalidData(t *testing.T) { assert.Nil(t, err) _, err = GetAppInstanceLabel(&obj, "valid-label") assert.Error(t, err) - assert.Equal(t, ".metadata.labels accessor error: contains non-string key in the map: is of the type , expected string", err.Error()) + assert.Equal(t, "failed to get labels for /v1, Kind=Service /my-service: .metadata.labels accessor error: contains non-string key in the map: is of the type , expected string", err.Error()) } func TestRemoveLabel(t *testing.T) { From 3a4553029b56715c4db885c90c3b6c3ebcab9e22 Mon Sep 17 00:00:00 2001 From: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> Date: Thu, 29 Jun 2023 17:14:52 -0400 Subject: [PATCH 4/4] fix test Signed-off-by: Michael Crenshaw <350466+crenshaw-dev@users.noreply.github.com> --- util/kube/kube_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/util/kube/kube_test.go b/util/kube/kube_test.go index e273c16894b5e..94fd0faeeef00 100644 --- a/util/kube/kube_test.go +++ b/util/kube/kube_test.go @@ -268,5 +268,5 @@ func TestRemoveLabelWithInvalidData(t *testing.T) { err = RemoveLabel(&obj, "valid-label") assert.Error(t, err) - assert.Equal(t, ".metadata.labels accessor error: contains non-string key in the map: is of the type , expected string", err.Error()) + assert.Equal(t, "failed to get labels for /v1, Kind=Service /my-service: .metadata.labels accessor error: contains non-string key in the map: is of the type , expected string", err.Error()) }