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

fix: Make change of tracking method work at runtime #9820

Merged
merged 4 commits into from
Jul 6, 2022
Merged

fix: Make change of tracking method work at runtime #9820

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
d43fae7
fix: Make change of tracking method work at runtime
jannfis Jun 29, 2022
962fb69
GetAppName() will figure tracking label or annotation on its own
jannfis Jun 29, 2022
2aa1ce8
Correct test comments and add another test
jannfis Jun 29, 2022
2df1fc9
Add a read lock before getting cache settings
jannfis Jul 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions controller/cache/cache.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ func NewLiveStateCache(
type cacheSettings struct {
clusterSettings clustercache.Settings
appInstanceLabelKey string
trackingMethod appv1.TrackingMethod
}

type liveStateCache struct {
Expand Down Expand Up @@ -210,7 +211,7 @@ func (c *liveStateCache) loadCacheSettings() (*cacheSettings, error) {
ResourceHealthOverride: lua.ResourceHealthOverrides(resourceOverrides),
ResourcesFilter: resourcesFilter,
}
return &cacheSettings{clusterSettings, appInstanceLabelKey}, nil
return &cacheSettings{clusterSettings, appInstanceLabelKey, argo.GetTrackingMethod(c.settingsMgr)}, nil
}

func asResourceNode(r *clustercache.Resource) appv1.ResourceNode {
Expand Down Expand Up @@ -387,7 +388,6 @@ func (c *liveStateCache) getCluster(server string) (clustercache.ClusterCache, e
return nil, fmt.Errorf("controller is configured to ignore cluster %s", cluster.Server)
}

trackingMethod := argo.GetTrackingMethod(c.settingsMgr)
clusterCacheOpts := []clustercache.UpdateSettingsFunc{
clustercache.SetListSemaphore(semaphore.NewWeighted(clusterCacheListSemaphoreSize)),
clustercache.SetListPageSize(clusterCacheListPageSize),
Expand All @@ -400,9 +400,12 @@ func (c *liveStateCache) getCluster(server string) (clustercache.ClusterCache, e
clustercache.SetPopulateResourceInfoHandler(func(un *unstructured.Unstructured, isRoot bool) (interface{}, bool) {
res := &ResourceInfo{}
populateNodeInfo(un, res)
c.lock.RLock()
cacheSettings := c.cacheSettings
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was wondering, do I need to acquire a lock on the c.lock mutex here?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it happening already on line 374?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but this is a call-back function, which will be called outside the scope of the lock on line 374

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, looking at the code between lines 365 and 368, the assignment of c.cacheSettings is also done in a locked way. So probably makes sense to lock it here as well, just for the assignment to cacheSettings.

However, since the PopulateResourceInfoHandler will be called on every resource on the cluster, I'm wondering about the performance impact of this lock.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Mutexes break my brain. What problem would the mutex solve?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The lock would span the copy operation of assigning c.cacheSettings to the local variable cacheSettings, to perform the copy as one atomic operation.

Maybe in the current context, this doesn't seem to be important. However, when the cache settings get expanded at some point in time, and another process modifies the settings while we copy it, we may end up with non-working data.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, thanks!

Copy link
Member Author

@jannfis jannfis Jul 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alexmt Can you please have a look - I know that you raised some performance concerns in this particular part of the code in the past.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agree with https://github.com/argoproj/argo-cd/pull/9820/files#r911632043 . The lock is required to get a copy of c.cacheSettings . Read lock (c.lock.RLock() & c.lock.RUnlock()) should minimize performance impact. Cannot think of any other way to achieve it.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I added the read locks.

c.lock.RUnlock()
res.Health, _ = health.GetResourceHealth(un, cacheSettings.clusterSettings.ResourceHealthOverride)

appName := c.resourceTracking.GetAppName(un, cacheSettings.appInstanceLabelKey, trackingMethod)
appName := c.resourceTracking.GetAppName(un, cacheSettings.appInstanceLabelKey, cacheSettings.trackingMethod)
if isRoot && appName != "" {
res.AppName = appName
}
Expand Down
187 changes: 187 additions & 0 deletions test/e2e/app_management_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
projectFixture "github.com/argoproj/argo-cd/v2/test/e2e/fixture/project"
repoFixture "github.com/argoproj/argo-cd/v2/test/e2e/fixture/repos"
"github.com/argoproj/argo-cd/v2/test/e2e/testdata"
"github.com/argoproj/argo-cd/v2/util/argo"
. "github.com/argoproj/argo-cd/v2/util/argo"
. "github.com/argoproj/argo-cd/v2/util/errors"
"github.com/argoproj/argo-cd/v2/util/io"
Expand Down Expand Up @@ -2063,3 +2064,189 @@ func TestDisableManifestGeneration(t *testing.T) {
assert.Equal(t, app.Status.SourceType, ApplicationSourceTypeDirectory)
})
}

func TestSwitchTrackingMethod(t *testing.T) {
ctx := Given(t)

ctx.
SetTrackingMethod(string(argo.TrackingMethodAnnotation)).
Path("deployment").
When().
CreateApp().
Sync().
Refresh(RefreshTypeNormal).
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy)).
When().
And(func() {
// Add resource with tracking annotation. This should put the
// application OutOfSync.
FailOnErr(KubeClientset.CoreV1().ConfigMaps(DeploymentNamespace()).Create(context.Background(), &v1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "other-configmap",
Annotations: map[string]string{
common.AnnotationKeyAppInstance: fmt.Sprintf("%s:/ConfigMap:%s/other-configmap", Name(), DeploymentNamespace()),
},
},
}, metav1.CreateOptions{}))
}).
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeOutOfSync)).
Expect(HealthIs(health.HealthStatusHealthy)).
When().
And(func() {
// Delete resource to bring application back in sync
FailOnErr(nil, KubeClientset.CoreV1().ConfigMaps(DeploymentNamespace()).Delete(context.Background(), "other-configmap", metav1.DeleteOptions{}))
}).
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy)).
When().
SetTrackingMethod(string(argo.TrackingMethodLabel)).
Sync().
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy)).
When().
And(func() {
// Add a resource with a tracking annotation. This should not
// affect the application, because we now use the tracking method
// "label".
FailOnErr(KubeClientset.CoreV1().ConfigMaps(DeploymentNamespace()).Create(context.Background(), &v1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "other-configmap",
Annotations: map[string]string{
common.AnnotationKeyAppInstance: fmt.Sprintf("%s:/ConfigMap:%s/other-configmap", Name(), DeploymentNamespace()),
},
},
}, metav1.CreateOptions{}))
}).
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy)).
When().
And(func() {
// Add a resource with the tracking label. The app should become
// OutOfSync.
FailOnErr(KubeClientset.CoreV1().ConfigMaps(DeploymentNamespace()).Create(context.Background(), &v1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "extra-configmap",
Labels: map[string]string{
common.LabelKeyAppInstance: Name(),
},
},
}, metav1.CreateOptions{}))
}).
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeOutOfSync)).
Expect(HealthIs(health.HealthStatusHealthy)).
When().
And(func() {
// Delete resource to bring application back in sync
FailOnErr(nil, KubeClientset.CoreV1().ConfigMaps(DeploymentNamespace()).Delete(context.Background(), "extra-configmap", metav1.DeleteOptions{}))
}).
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy))
}

func TestSwitchTrackingLabel(t *testing.T) {
ctx := Given(t)

ctx.
Path("deployment").
When().
CreateApp().
Sync().
Refresh(RefreshTypeNormal).
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy)).
When().
And(func() {
// Add extra resource that carries the default tracking label
// We expect the app to go out of sync.
FailOnErr(KubeClientset.CoreV1().ConfigMaps(DeploymentNamespace()).Create(context.Background(), &v1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "other-configmap",
Labels: map[string]string{
common.LabelKeyAppInstance: Name(),
},
},
}, metav1.CreateOptions{}))
}).
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeOutOfSync)).
Expect(HealthIs(health.HealthStatusHealthy)).
When().
And(func() {
// Delete resource to bring application back in sync
FailOnErr(nil, KubeClientset.CoreV1().ConfigMaps(DeploymentNamespace()).Delete(context.Background(), "other-configmap", metav1.DeleteOptions{}))
}).
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy)).
When().
// Change tracking label
SetTrackingLabel("argocd.tracking").
Sync().
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy)).
When().
And(func() {
// Create resource with the new tracking label, the application
// is expected to go out of sync
FailOnErr(KubeClientset.CoreV1().ConfigMaps(DeploymentNamespace()).Create(context.Background(), &v1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "other-configmap",
Labels: map[string]string{
"argocd.tracking": Name(),
},
},
}, metav1.CreateOptions{}))
}).
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeOutOfSync)).
Expect(HealthIs(health.HealthStatusHealthy)).
When().
And(func() {
// Delete resource to bring application back in sync
FailOnErr(nil, KubeClientset.CoreV1().ConfigMaps(DeploymentNamespace()).Delete(context.Background(), "other-configmap", metav1.DeleteOptions{}))
}).
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy)).
When().
And(func() {
// Add extra resource that carries the default tracking label
// We expect the app to stay in sync, because the configured
// label is different.
FailOnErr(KubeClientset.CoreV1().ConfigMaps(DeploymentNamespace()).Create(context.Background(), &v1.ConfigMap{
ObjectMeta: metav1.ObjectMeta{
Name: "other-configmap",
Labels: map[string]string{
common.LabelKeyAppInstance: Name(),
},
},
}, metav1.CreateOptions{}))
}).
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy))
}
10 changes: 10 additions & 0 deletions test/e2e/fixture/app/actions.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -341,3 +341,13 @@ func (a *Actions) verifyAction() {
a.Then().Expect(Success(""))
}
}

func (a *Actions) SetTrackingMethod(trackingMethod string) *Actions {
fixture.SetTrackingMethod(trackingMethod)
return a
}

func (a *Actions) SetTrackingLabel(trackingLabel string) *Actions {
fixture.SetTrackingLabel(trackingLabel)
return a
}
5 changes: 5 additions & 0 deletions test/e2e/fixture/app/context.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,3 +308,8 @@ func (c *Context) HelmSkipCrds() *Context {
c.helmSkipCrds = true
return c
}

func (c *Context) SetTrackingMethod(trackingMethod string) *Context {
fixture.SetTrackingMethod(trackingMethod)
return c
}
7 changes: 7 additions & 0 deletions test/e2e/fixture/fixture.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -357,6 +357,13 @@ func SetTrackingMethod(trackingMethod string) {
})
}

func SetTrackingLabel(trackingLabel string) {
updateSettingConfigMap(func(cm *corev1.ConfigMap) error {
cm.Data["application.instanceLabelKey"] = trackingLabel
return nil
})
}

func SetResourceOverridesSplitKeys(overrides map[string]v1alpha1.ResourceOverride) {
updateSettingConfigMap(func(cm *corev1.ConfigMap) error {
for k, v := range overrides {
Expand Down
2 changes: 1 addition & 1 deletion util/argo/resource_tracking.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@ func NewResourceTracking() ResourceTracking {
// GetTrackingMethod retrieve tracking method from settings
func GetTrackingMethod(settingsMgr *settings.SettingsManager) v1alpha1.TrackingMethod {
tm, err := settingsMgr.GetTrackingMethod()
if err != nil {
if err != nil || tm == "" {
return TrackingMethodLabel
}
return v1alpha1.TrackingMethod(tm)
Expand Down
3 changes: 3 additions & 0 deletions util/settings/settings.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,8 @@ type ArgoCDSettings struct {
ServerRBACLogEnforceEnable bool `json:"serverRBACLogEnforceEnable"`
// ExecEnabled indicates whether the UI exec feature is enabled
ExecEnabled bool `json:"execEnabled"`
// TrackingMethod defines the resource tracking method to be used
TrackingMethod string `json:"application.resourceTrackingMethod,omitempty"`
}

type GoogleAnalytics struct {
Expand Down Expand Up @@ -1271,6 +1273,7 @@ func updateSettingsFromConfigMap(settings *ArgoCDSettings, argoCDCM *apiv1.Confi
}
settings.InClusterEnabled = argoCDCM.Data[inClusterEnabledKey] != "false"
settings.ExecEnabled = argoCDCM.Data[execEnabledKey] == "true"
settings.TrackingMethod = argoCDCM.Data[settingsResourceTrackingMethodKey]
crenshaw-dev marked this conversation as resolved.
Show resolved Hide resolved
}

// validateExternalURL ensures the external URL that is set on the configmap is valid
Expand Down