Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: improve migration from label to annotation tracking #7899

Merged
merged 1 commit into from
Dec 11, 2021
Merged

fix: improve migration from label to annotation tracking #7899

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
26 changes: 15 additions & 11 deletions reposerver/cache/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,15 +10,15 @@ import (
"strings"
"time"

"github.com/argoproj/argo-cd/v2/util/argo"

"github.com/argoproj/gitops-engine/pkg/utils/text"
"github.com/go-git/go-git/v5/plumbing"
"github.com/go-redis/redis/v8"
log "github.com/sirupsen/logrus"
"github.com/spf13/cobra"

appv1 "github.com/argoproj/argo-cd/v2/pkg/apis/application/v1alpha1"
"github.com/argoproj/argo-cd/v2/reposerver/apiclient"
"github.com/argoproj/argo-cd/v2/util/argo"
cacheutil "github.com/argoproj/argo-cd/v2/util/cache"
"github.com/argoproj/argo-cd/v2/util/env"
"github.com/argoproj/argo-cd/v2/util/hash"
Expand Down Expand Up @@ -138,12 +138,16 @@ func (c *Cache) GetGitReferences(repo string, references *[]*plumbing.Reference)
return nil
}

func manifestCacheKey(revision string, appSrc *appv1.ApplicationSource, namespace string, appLabelKey string, appName string, info ClusterRuntimeInfo) string {
return fmt.Sprintf("mfst|%s|%s|%s|%s|%d", appLabelKey, appName, revision, namespace, appSourceKey(appSrc)+clusterRuntimeInfoKey(info))
func manifestCacheKey(revision string, appSrc *appv1.ApplicationSource, namespace string, trackingMethod string, appLabelKey string, appName string, info ClusterRuntimeInfo) string {
trackingKey := appLabelKey
if text.FirstNonEmpty(trackingMethod, string(argo.TrackingMethodLabel)) != string(argo.TrackingMethodLabel) {
trackingKey = trackingMethod + ":" + trackingKey
}
return fmt.Sprintf("mfst|%s|%s|%s|%s|%d", trackingKey, appName, revision, namespace, appSourceKey(appSrc)+clusterRuntimeInfoKey(info))
}

func (c *Cache) GetManifests(revision string, appSrc *appv1.ApplicationSource, clusterInfo ClusterRuntimeInfo, namespace string, appLabelKey string, appName string, res *CachedManifestResponse) error {
err := c.cache.GetItem(manifestCacheKey(revision, appSrc, namespace, appLabelKey, appName, clusterInfo), res)
func (c *Cache) GetManifests(revision string, appSrc *appv1.ApplicationSource, clusterInfo ClusterRuntimeInfo, namespace string, trackingMethod string, appLabelKey string, appName string, res *CachedManifestResponse) error {
err := c.cache.GetItem(manifestCacheKey(revision, appSrc, namespace, trackingMethod, appLabelKey, appName, clusterInfo), res)

if err != nil {
return err
Expand All @@ -158,7 +162,7 @@ func (c *Cache) GetManifests(revision string, appSrc *appv1.ApplicationSource, c
if hash != res.CacheEntryHash || res.ManifestResponse == nil && res.MostRecentError == "" {
log.Warnf("Manifest hash did not match expected value or cached manifests response is empty, treating as a cache miss: %s", appName)

err = c.DeleteManifests(revision, appSrc, clusterInfo, namespace, appLabelKey, appName)
err = c.DeleteManifests(revision, appSrc, clusterInfo, namespace, trackingMethod, appLabelKey, appName)
if err != nil {
return fmt.Errorf("Unable to delete manifest after hash mismatch, %v", err)
}
Expand All @@ -173,7 +177,7 @@ func (c *Cache) GetManifests(revision string, appSrc *appv1.ApplicationSource, c
return nil
}

func (c *Cache) SetManifests(revision string, appSrc *appv1.ApplicationSource, clusterInfo ClusterRuntimeInfo, namespace string, appLabelKey string, appName string, res *CachedManifestResponse) error {
func (c *Cache) SetManifests(revision string, appSrc *appv1.ApplicationSource, clusterInfo ClusterRuntimeInfo, namespace string, trackingMethod string, appLabelKey string, appName string, res *CachedManifestResponse) error {

// Generate and apply the cache entry hash, before writing
if res != nil {
Expand All @@ -185,11 +189,11 @@ func (c *Cache) SetManifests(revision string, appSrc *appv1.ApplicationSource, c
res.CacheEntryHash = hash
}

return c.cache.SetItem(manifestCacheKey(revision, appSrc, namespace, appLabelKey, appName, clusterInfo), res, c.repoCacheExpiration, res == nil)
return c.cache.SetItem(manifestCacheKey(revision, appSrc, namespace, trackingMethod, appLabelKey, appName, clusterInfo), res, c.repoCacheExpiration, res == nil)
}

func (c *Cache) DeleteManifests(revision string, appSrc *appv1.ApplicationSource, clusterInfo ClusterRuntimeInfo, namespace string, appLabelKey string, appName string) error {
return c.cache.SetItem(manifestCacheKey(revision, appSrc, namespace, appLabelKey, appName, clusterInfo), "", c.repoCacheExpiration, true)
func (c *Cache) DeleteManifests(revision string, appSrc *appv1.ApplicationSource, clusterInfo ClusterRuntimeInfo, namespace string, trackingMethod string, appLabelKey string, appName string) error {
return c.cache.SetItem(manifestCacheKey(revision, appSrc, namespace, trackingMethod, appLabelKey, appName, clusterInfo), "", c.repoCacheExpiration, true)
}

func appDetailsCacheKey(revision string, appSrc *appv1.ApplicationSource, trackingMethod appv1.TrackingMethod) string {
Expand Down
22 changes: 11 additions & 11 deletions reposerver/cache/cache_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,29 +73,29 @@ func TestCache_GetManifests(t *testing.T) {
// cache miss
q := &apiclient.ManifestRequest{}
value := &CachedManifestResponse{}
err := cache.GetManifests("my-revision", &ApplicationSource{}, q, "my-namespace", "my-app-label-key", "my-app-label-value", value)
err := cache.GetManifests("my-revision", &ApplicationSource{}, q, "my-namespace", "", "my-app-label-key", "my-app-label-value", value)
assert.Equal(t, ErrCacheMiss, err)
// populate cache
res := &CachedManifestResponse{ManifestResponse: &apiclient.ManifestResponse{SourceType: "my-source-type"}}
err = cache.SetManifests("my-revision", &ApplicationSource{}, q, "my-namespace", "my-app-label-key", "my-app-label-value", res)
err = cache.SetManifests("my-revision", &ApplicationSource{}, q, "my-namespace", "", "my-app-label-key", "my-app-label-value", res)
assert.NoError(t, err)
// cache miss
err = cache.GetManifests("other-revision", &ApplicationSource{}, q, "my-namespace", "my-app-label-key", "my-app-label-value", value)
err = cache.GetManifests("other-revision", &ApplicationSource{}, q, "my-namespace", "", "my-app-label-key", "my-app-label-value", value)
assert.Equal(t, ErrCacheMiss, err)
// cache miss
err = cache.GetManifests("my-revision", &ApplicationSource{Path: "other-path"}, q, "my-namespace", "my-app-label-key", "my-app-label-value", value)
err = cache.GetManifests("my-revision", &ApplicationSource{Path: "other-path"}, q, "my-namespace", "", "my-app-label-key", "my-app-label-value", value)
assert.Equal(t, ErrCacheMiss, err)
// cache miss
err = cache.GetManifests("my-revision", &ApplicationSource{}, q, "other-namespace", "my-app-label-key", "my-app-label-value", value)
err = cache.GetManifests("my-revision", &ApplicationSource{}, q, "other-namespace", "", "my-app-label-key", "my-app-label-value", value)
assert.Equal(t, ErrCacheMiss, err)
// cache miss
err = cache.GetManifests("my-revision", &ApplicationSource{}, q, "my-namespace", "other-app-label-key", "my-app-label-value", value)
err = cache.GetManifests("my-revision", &ApplicationSource{}, q, "my-namespace", "", "other-app-label-key", "my-app-label-value", value)
assert.Equal(t, ErrCacheMiss, err)
// cache miss
err = cache.GetManifests("my-revision", &ApplicationSource{}, q, "my-namespace", "my-app-label-key", "other-app-label-value", value)
err = cache.GetManifests("my-revision", &ApplicationSource{}, q, "my-namespace", "", "my-app-label-key", "other-app-label-value", value)
assert.Equal(t, ErrCacheMiss, err)
// cache hit
err = cache.GetManifests("my-revision", &ApplicationSource{}, q, "my-namespace", "my-app-label-key", "my-app-label-value", value)
err = cache.GetManifests("my-revision", &ApplicationSource{}, q, "my-namespace", "", "my-app-label-key", "my-app-label-value", value)
assert.NoError(t, err)
assert.Equal(t, &CachedManifestResponse{ManifestResponse: &apiclient.ManifestResponse{SourceType: "my-source-type"}}, value)
}
Expand Down Expand Up @@ -154,7 +154,7 @@ func TestCachedManifestResponse_HashBehavior(t *testing.T) {
NumberOfCachedResponsesReturned: 0,
NumberOfConsecutiveFailures: 0,
}
err := repoCache.SetManifests(response.Revision, appSrc, &apiclient.ManifestRequest{}, response.Namespace, appKey, appValue, store)
err := repoCache.SetManifests(response.Revision, appSrc, &apiclient.ManifestRequest{}, response.Namespace, "", appKey, appValue, store)
if err != nil {
t.Fatal(err)
}
Expand Down Expand Up @@ -185,7 +185,7 @@ func TestCachedManifestResponse_HashBehavior(t *testing.T) {

// Retrieve the value using 'GetManifests' and confirm it works
retrievedVal := &CachedManifestResponse{}
err = repoCache.GetManifests(response.Revision, appSrc, &apiclient.ManifestRequest{}, response.Namespace, appKey, appValue, retrievedVal)
err = repoCache.GetManifests(response.Revision, appSrc, &apiclient.ManifestRequest{}, response.Namespace, "", appKey, appValue, retrievedVal)
if err != nil {
t.Fatal(err)
}
Expand All @@ -208,7 +208,7 @@ func TestCachedManifestResponse_HashBehavior(t *testing.T) {

// Retrieve the value using GetManifests and confirm it returns a cache miss
retrievedVal = &CachedManifestResponse{}
err = repoCache.GetManifests(response.Revision, appSrc, &apiclient.ManifestRequest{}, response.Namespace, appKey, appValue, retrievedVal)
err = repoCache.GetManifests(response.Revision, appSrc, &apiclient.ManifestRequest{}, response.Namespace, "", appKey, appValue, retrievedVal)

assert.True(t, err == cacheutil.ErrCacheMiss)

Expand Down
14 changes: 7 additions & 7 deletions reposerver/repository/repository.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -338,7 +338,7 @@ func (s *Service) runManifestGen(repoRoot, commitSHA, cacheKey string, ctxSrc op
// Retrieve a new copy (if available) of the cached response: this ensures we are updating the latest copy of the cache,
// rather than a copy of the cache that occurred before (a potentially lengthy) manifest generation.
innerRes := &cache.CachedManifestResponse{}
cacheErr := s.cache.GetManifests(cacheKey, q.ApplicationSource, q, q.Namespace, q.AppLabelKey, q.AppName, innerRes)
cacheErr := s.cache.GetManifests(cacheKey, q.ApplicationSource, q, q.Namespace, q.TrackingMethod, q.AppLabelKey, q.AppName, innerRes)
if cacheErr != nil && cacheErr != reposervercache.ErrCacheMiss {
log.Warnf("manifest cache set error %s: %v", q.ApplicationSource.String(), cacheErr)
return nil, cacheErr
Expand All @@ -353,7 +353,7 @@ func (s *Service) runManifestGen(repoRoot, commitSHA, cacheKey string, ctxSrc op
// Update the cache to include failure information
innerRes.NumberOfConsecutiveFailures++
innerRes.MostRecentError = err.Error()
cacheErr = s.cache.SetManifests(cacheKey, q.ApplicationSource, q, q.Namespace, q.AppLabelKey, q.AppName, innerRes)
cacheErr = s.cache.SetManifests(cacheKey, q.ApplicationSource, q, q.Namespace, q.TrackingMethod, q.AppLabelKey, q.AppName, innerRes)
if cacheErr != nil {
log.Warnf("manifest cache set error %s: %v", q.ApplicationSource.String(), cacheErr)
return nil, cacheErr
Expand All @@ -372,7 +372,7 @@ func (s *Service) runManifestGen(repoRoot, commitSHA, cacheKey string, ctxSrc op
}
manifestGenResult.Revision = commitSHA
manifestGenResult.VerifyResult = ctx.verificationResult
err = s.cache.SetManifests(cacheKey, q.ApplicationSource, q, q.Namespace, q.AppLabelKey, q.AppName, &manifestGenCacheEntry)
err = s.cache.SetManifests(cacheKey, q.ApplicationSource, q, q.Namespace, q.TrackingMethod, q.AppLabelKey, q.AppName, &manifestGenCacheEntry)
if err != nil {
log.Warnf("manifest cache set error %s/%s: %v", q.ApplicationSource.String(), cacheKey, err)
}
Expand All @@ -387,7 +387,7 @@ func (s *Service) runManifestGen(repoRoot, commitSHA, cacheKey string, ctxSrc op
// If true is returned, either the second or third parameter (but not both) will contain a value from the cache (a ManifestResponse, or error, respectively)
func (s *Service) getManifestCacheEntry(cacheKey string, q *apiclient.ManifestRequest, firstInvocation bool) (bool, *apiclient.ManifestResponse, error) {
res := cache.CachedManifestResponse{}
err := s.cache.GetManifests(cacheKey, q.ApplicationSource, q, q.Namespace, q.AppLabelKey, q.AppName, &res)
err := s.cache.GetManifests(cacheKey, q.ApplicationSource, q, q.Namespace, q.TrackingMethod, q.AppLabelKey, q.AppName, &res)
if err == nil {

// The cache contains an existing value
Expand All @@ -406,7 +406,7 @@ func (s *Service) getManifestCacheEntry(cacheKey string, q *apiclient.ManifestRe
// After X minutes, reset the cache and retry the operation (e.g. perhaps the error is ephemeral and has passed)
if elapsedTimeInMinutes >= s.initConstants.PauseGenerationOnFailureForMinutes {
// We can now try again, so reset the cache state and run the operation below
err = s.cache.DeleteManifests(cacheKey, q.ApplicationSource, q, q.Namespace, q.AppLabelKey, q.AppName)
err = s.cache.DeleteManifests(cacheKey, q.ApplicationSource, q, q.Namespace, q.TrackingMethod, q.AppLabelKey, q.AppName)
if err != nil {
log.Warnf("manifest cache set error %s/%s: %v", q.ApplicationSource.String(), cacheKey, err)
}
Expand All @@ -420,7 +420,7 @@ func (s *Service) getManifestCacheEntry(cacheKey string, q *apiclient.ManifestRe

if res.NumberOfCachedResponsesReturned >= s.initConstants.PauseGenerationOnFailureForRequests {
// We can now try again, so reset the error cache state and run the operation below
err = s.cache.DeleteManifests(cacheKey, q.ApplicationSource, q, q.Namespace, q.AppLabelKey, q.AppName)
err = s.cache.DeleteManifests(cacheKey, q.ApplicationSource, q, q.Namespace, q.TrackingMethod, q.AppLabelKey, q.AppName)
if err != nil {
log.Warnf("manifest cache set error %s/%s: %v", q.ApplicationSource.String(), cacheKey, err)
}
Expand All @@ -438,7 +438,7 @@ func (s *Service) getManifestCacheEntry(cacheKey string, q *apiclient.ManifestRe
// Increment the number of returned cached responses and push that new value to the cache
// (if we have not already done so previously in this function)
res.NumberOfCachedResponsesReturned++
err = s.cache.SetManifests(cacheKey, q.ApplicationSource, q, q.Namespace, q.AppLabelKey, q.AppName, &res)
err = s.cache.SetManifests(cacheKey, q.ApplicationSource, q, q.Namespace, q.TrackingMethod, q.AppLabelKey, q.AppName, &res)
if err != nil {
log.Warnf("manifest cache set error %s/%s: %v", q.ApplicationSource.String(), cacheKey, err)
}
Expand Down
6 changes: 3 additions & 3 deletions reposerver/repository/repository_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ func TestGenerateManifests_K8SAPIResetCache(t *testing.T) {

cachedFakeResponse := &apiclient.ManifestResponse{Manifests: []string{"Fake"}}

err := service.cache.SetManifests(mock.Anything, &src, &q, "", "", "", &cache.CachedManifestResponse{ManifestResponse: cachedFakeResponse})
err := service.cache.SetManifests(mock.Anything, &src, &q, "", "", "", "", &cache.CachedManifestResponse{ManifestResponse: cachedFakeResponse})
assert.NoError(t, err)

res, err := service.GenerateManifest(context.Background(), &q)
Expand All @@ -178,7 +178,7 @@ func TestGenerateManifests_EmptyCache(t *testing.T) {
Repo: &argoappv1.Repository{}, ApplicationSource: &src,
}

err := service.cache.SetManifests(mock.Anything, &src, &q, "", "", "", &cache.CachedManifestResponse{ManifestResponse: nil})
err := service.cache.SetManifests(mock.Anything, &src, &q, "", "", "", "", &cache.CachedManifestResponse{ManifestResponse: nil})
assert.NoError(t, err)

res, err := service.GenerateManifest(context.Background(), &q)
Expand Down Expand Up @@ -310,7 +310,7 @@ func TestManifestGenErrorCacheByNumRequests(t *testing.T) {
assert.NotNil(t, manifestRequest)

cachedManifestResponse := &cache.CachedManifestResponse{}
err := service.cache.GetManifests(mock.Anything, manifestRequest.ApplicationSource, manifestRequest, manifestRequest.Namespace, manifestRequest.AppLabelKey, manifestRequest.AppName, cachedManifestResponse)
err := service.cache.GetManifests(mock.Anything, manifestRequest.ApplicationSource, manifestRequest, manifestRequest.Namespace, "", manifestRequest.AppLabelKey, manifestRequest.AppName, cachedManifestResponse)
assert.Nil(t, err)
return cachedManifestResponse
}
Expand Down
5 changes: 1 addition & 4 deletions util/argo/resource_tracking.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -170,10 +170,7 @@ func (rt *resourceTracking) Normalize(config, live *unstructured.Unstructured, l
return err
}

err = argokube.SetAppInstanceLabel(config, labelKey, label)
if err != nil {
return err
}
argokube.RemoveLabel(live, labelKey)

return nil
}
30 changes: 17 additions & 13 deletions util/argo/resource_tracking_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,29 +108,33 @@ func TestParseAppInstanceValueCorrectFormat(t *testing.T) {
}

func TestResourceIdNormalizer_Normalize(t *testing.T) {
rt := NewResourceTracking()

// live object is a resource that has old style tracking label
yamlBytes, err := ioutil.ReadFile("testdata/svc.yaml")
assert.Nil(t, err)
var obj *unstructured.Unstructured
err = yaml.Unmarshal(yamlBytes, &obj)
var liveObj *unstructured.Unstructured
err = yaml.Unmarshal(yamlBytes, &liveObj)
assert.Nil(t, err)

rt := NewResourceTracking()

err = rt.SetAppInstance(obj, common.LabelKeyAppInstance, "my-app", "", TrackingMethodLabel)
err = rt.SetAppInstance(liveObj, common.LabelKeyAppInstance, "my-app", "", TrackingMethodLabel)
assert.Nil(t, err)

// config object is a resource that has new style tracking annotation
yamlBytes, err = ioutil.ReadFile("testdata/svc.yaml")
assert.Nil(t, err)
var obj2 *unstructured.Unstructured
err = yaml.Unmarshal(yamlBytes, &obj2)
var configObj *unstructured.Unstructured
err = yaml.Unmarshal(yamlBytes, &configObj)
assert.Nil(t, err)

err = rt.SetAppInstance(obj2, common.AnnotationKeyAppInstance, "my-app2", "", TrackingMethodAnnotation)
err = rt.SetAppInstance(configObj, common.AnnotationKeyAppInstance, "my-app2", "", TrackingMethodAnnotation)
assert.Nil(t, err)

_ = rt.Normalize(obj2, obj, common.LabelKeyAppInstance, string(TrackingMethodAnnotation))
annotation := kube.GetAppInstanceAnnotation(obj2, common.AnnotationKeyAppInstance)
assert.Equal(t, obj.GetAnnotations()[common.AnnotationKeyAppInstance], annotation)
_ = rt.Normalize(configObj, liveObj, common.LabelKeyAppInstance, string(TrackingMethodAnnotation))

// the normalization should affect add the new style annotation and drop old tracking label from live object
annotation := kube.GetAppInstanceAnnotation(configObj, common.AnnotationKeyAppInstance)
assert.Equal(t, liveObj.GetAnnotations()[common.AnnotationKeyAppInstance], annotation)
_, hasOldLabel := liveObj.GetLabels()[common.LabelKeyAppInstance]
assert.False(t, hasOldLabel)
}

func TestIsOldTrackingMethod(t *testing.T) {
Expand Down
16 changes: 16 additions & 0 deletions util/kube/kube.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,3 +120,19 @@ func GetAppInstanceLabel(un *unstructured.Unstructured, key string) string {
}
return ""
}

// RemoveLabel removes label with the specified name
func RemoveLabel(un *unstructured.Unstructured, key string) {
labels := un.GetLabels()
if labels == nil {
return
}

for k := range labels {
if k == key {
delete(labels, k)
un.SetLabels(labels)
break
}
}
}
14 changes: 13 additions & 1 deletion util/kube/kube_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@ import (
"testing"

"github.com/ghodss/yaml"

"github.com/stretchr/testify/assert"
apiv1 "k8s.io/api/core/v1"
extv1beta1 "k8s.io/api/extensions/v1beta1"
Expand Down Expand Up @@ -207,3 +206,16 @@ func TestGetAppInstanceLabel(t *testing.T) {
assert.Nil(t, err)
assert.Equal(t, "my-app", GetAppInstanceLabel(&obj, common.LabelKeyAppInstance))
}

func TestRemoveLabel(t *testing.T) {
yamlBytes, err := ioutil.ReadFile("testdata/svc.yaml")
assert.Nil(t, err)
var obj unstructured.Unstructured
err = yaml.Unmarshal(yamlBytes, &obj)
assert.Nil(t, err)
obj.SetLabels(map[string]string{"test": "value"})

RemoveLabel(&obj, "test")

assert.Len(t, obj.GetLabels(), 0)
}