From 981712550e1ec9383e65ccbbd22590adfdd0553d Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Fri, 19 May 2023 12:05:30 -0400 Subject: [PATCH 01/24] feat: Implement Server-Side Diff Signed-off-by: Leonardo Luz Almeida --- Makefile | 1 + controller/state.go | 15 +++++++++++++++ controller/sync.go | 12 ++++++++++++ go.mod | 3 +++ util/argo/diff/diff.go | 39 ++++++++++++++++++++++++++++++++++++--- 5 files changed, 67 insertions(+), 3 deletions(-) diff --git a/Makefile b/Makefile index d4df041fa58e1..880622e7279a9 100644 --- a/Makefile +++ b/Makefile @@ -492,6 +492,7 @@ start-local: mod-vendor-local dep-ui-local cli-local ARGOCD_ZJWT_FEATURE_FLAG=always \ ARGOCD_IN_CI=false \ ARGOCD_GPG_ENABLED=$(ARGOCD_GPG_ENABLED) \ + BIN_MODE=$(ARGOCD_BIN_MODE) \ ARGOCD_E2E_TEST=false \ ARGOCD_APPLICATION_NAMESPACES=$(ARGOCD_APPLICATION_NAMESPACES) \ goreman -f $(ARGOCD_PROCFILE) start ${ARGOCD_START} diff --git a/controller/state.go b/controller/state.go index 59e7fa31248ae..77f548fae0d71 100644 --- a/controller/state.go +++ b/controller/state.go @@ -603,6 +603,21 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 diffConfigBuilder.WithGVKParser(gvkParser) diffConfigBuilder.WithManager(common.ArgoCDSSAManager) + // TODO (SSD): Make serverSideDiff configurable via argocd-cm + // and resource annotation + serverSideDiff := true + diffConfigBuilder.WithServerSideDiff(serverSideDiff) + + if serverSideDiff { + resourceOps, cleanup, err := m.getResourceOperations(app.Spec.Destination.Server) + if err != nil { + log.Errorf("CompareAppState error getting resource operations: %s", err) + conditions = append(conditions, v1alpha1.ApplicationCondition{Type: v1alpha1.ApplicationConditionUnknownError, Message: err.Error(), LastTransitionTime: &now}) + } + defer cleanup() + diffConfigBuilder.WithResourceOperations(resourceOps) + } + // enable structured merge diff if application syncs with server-side apply if app.Spec.SyncPolicy != nil && app.Spec.SyncPolicy.SyncOptions.HasOption("ServerSideApply=true") { diffConfigBuilder.WithStructuredMergeDiff(true) diff --git a/controller/sync.go b/controller/sync.go index 2b925b7782b9e..d48d9ee0ac83a 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -56,6 +56,18 @@ func (m *appStateManager) getGVKParser(server string) (*managedfields.GvkParser, } return cluster.GetGVKParser(), nil } +func (m *appStateManager) getResourceOperations(server string) (kube.ResourceOperations, func(), error) { + clusterCache, err := m.liveStateCache.GetClusterCache(server) + if err != nil { + return nil, nil, fmt.Errorf("error getting cluster cache: %w", err) + } + + cluster, err := m.db.GetCluster(context.Background(), server) + if err != nil { + return nil, nil, fmt.Errorf("error getting cluster: %w", err) + } + return clusterCache.GetKubectl().ManageResources(cluster.RawRestConfig(), clusterCache.GetOpenAPISchema()) +} func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha1.OperationState) { // Sync requests might be requested with ambiguous revisions (e.g. master, HEAD, v1.2.3). diff --git a/go.mod b/go.mod index 8b91fe20eef1b..7c939cfbacdbd 100644 --- a/go.mod +++ b/go.mod @@ -293,6 +293,9 @@ require ( ) replace ( + // TODO (SSD): Remove this before merging + github.com/argoproj/gitops-engine => /Users/lalmeida1/dev/git/intuit/gitops-engine + // https://github.com/golang/go/issues/33546#issuecomment-519656923 github.com/go-check/check => github.com/go-check/check v0.0.0-20180628173108-788fd7840127 diff --git a/util/argo/diff/diff.go b/util/argo/diff/diff.go index 9b104719c5616..246cf901b5df4 100644 --- a/util/argo/diff/diff.go +++ b/util/argo/diff/diff.go @@ -95,6 +95,16 @@ func (b *DiffConfigBuilder) WithManager(manager string) *DiffConfigBuilder { return b } +func (b *DiffConfigBuilder) WithResourceOperations(resourceOps kube.ResourceOperations) *DiffConfigBuilder { + b.diffConfig.resourceOperations = resourceOps + return b +} + +func (b *DiffConfigBuilder) WithServerSideDiff(ssd bool) *DiffConfigBuilder { + b.diffConfig.serverSideDiff = ssd + return b +} + // Build will first validate the current state of the diff config and return the // DiffConfig implementation if no errors are found. Will return nil and the error // details otherwise. @@ -140,6 +150,9 @@ type DiffConfig interface { // Manager returns the manager that should be used by the diff while // calculating the structured merge diff. Manager() string + + ResourceOperations() kube.ResourceOperations + ServerSideDiff() bool } // diffConfig defines the configurations used while applying diffs. @@ -156,6 +169,8 @@ type diffConfig struct { gvkParser *k8smanagedfields.GvkParser structuredMergeDiff bool manager string + serverSideDiff bool + resourceOperations kube.ResourceOperations } func (c *diffConfig) Ignores() []v1alpha1.ResourceIgnoreDifferences { @@ -194,6 +209,12 @@ func (c *diffConfig) StructuredMergeDiff() bool { func (c *diffConfig) Manager() string { return c.manager } +func (c *diffConfig) ResourceOperations() kube.ResourceOperations { + return c.resourceOperations +} +func (c *diffConfig) ServerSideDiff() bool { + return c.serverSideDiff +} // Validate will check the current state of this diffConfig and return // error if it finds any required configuration missing. @@ -213,6 +234,9 @@ func (c *diffConfig) Validate() error { return fmt.Errorf("%s: StateCache must be set when retrieving from cache", msg) } } + if c.serverSideDiff && c.resourceOperations == nil { + return fmt.Errorf("%s: restConfig must be set when using server side diff", msg) + } return nil } @@ -254,12 +278,17 @@ func StateDiffs(lives, configs []*unstructured.Unstructured, diffConfig DiffConf diff.WithStructuredMergeDiff(diffConfig.StructuredMergeDiff()), diff.WithGVKParser(diffConfig.GVKParser()), diff.WithManager(diffConfig.Manager()), + diff.WithServerSideDiff(diffConfig.ServerSideDiff()), + diff.WithKubeApplier(diffConfig.ResourceOperations()), } if diffConfig.Logger() != nil { diffOpts = append(diffOpts, diff.WithLogr(*diffConfig.Logger())) } + // TODO (SSD): ServerSideDiff needs to be configured to always rely on the + // cache to avoid hitting Kube API too frequently. Need to validate if the + // current cached diff is compatible with SSD. useCache, cachedDiff := diffConfig.DiffFromCache(diffConfig.AppName()) if useCache && cachedDiff != nil { cached, err := diffArrayCached(normResults.Targets, normResults.Lives, cachedDiff, diffOpts...) @@ -282,9 +311,8 @@ func diffArrayCached(configArray []*unstructured.Unstructured, liveArray []*unst } diffByKey := map[kube.ResourceKey]*v1alpha1.ResourceDiff{} - for i := range cachedDiff { - res := cachedDiff[i] - diffByKey[kube.NewResourceKey(res.Group, res.Kind, res.Namespace, res.Name)] = cachedDiff[i] + for _, res := range cachedDiff { + diffByKey[kube.NewResourceKey(res.Group, res.Kind, res.Namespace, res.Name)] = res } diffResultList := diff.DiffResultList{ @@ -335,6 +363,8 @@ func (c *diffConfig) DiffFromCache(appName string) (bool, []*v1alpha1.ResourceDi return false, nil } cachedDiff := make([]*v1alpha1.ResourceDiff, 0) + // TODO (SSD): Fix this code as it is swallowing errors if + // GetAppManagedResources fails to execute. if c.stateCache != nil && c.stateCache.GetAppManagedResources(appName, &cachedDiff) == nil { return true, cachedDiff } @@ -364,6 +394,9 @@ func preDiffNormalize(lives, targets []*unstructured.Unstructured, diffConfig Di gvk := target.GetObjectKind().GroupVersionKind() idc := NewIgnoreDiffConfig(diffConfig.Ignores(), diffConfig.Overrides()) ok, ignoreDiff := idc.HasIgnoreDifference(gvk.Group, gvk.Kind, target.GetName(), target.GetNamespace()) + // TODO (SSD): This is likely unnecessary when ServerSideDiff is enabled + // as it seems that Kubernetes already takes care of it during dryrun apply. + // Need to validate and add condition if confirmed. if ok && len(ignoreDiff.ManagedFieldsManagers) > 0 { pt := scheme.ResolveParseableType(gvk, diffConfig.GVKParser()) var err error From 1c58a6f3a1b05540d6096d690ce2597b7294ec9d Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Tue, 21 Nov 2023 15:37:05 -0500 Subject: [PATCH 02/24] propagate the refreshtype to the diff config Signed-off-by: Leonardo Luz Almeida --- controller/state.go | 4 +++- util/argo/diff/diff.go | 17 +++++++++++++---- 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/controller/state.go b/controller/state.go index 77f548fae0d71..ce58d5deed980 100644 --- a/controller/state.go +++ b/controller/state.go @@ -588,7 +588,9 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 diffConfigBuilder := argodiff.NewDiffConfigBuilder(). WithDiffSettings(app.Spec.IgnoreDifferences, resourceOverrides, compareOptions.IgnoreAggregatedRoles). - WithTracking(appLabelKey, string(trackingMethod)) + WithTracking(appLabelKey, string(trackingMethod)). + WithCache(m.cache, app.InstanceName(m.namespace)). + WithRefreshType(refreshType) if useDiffCache { diffConfigBuilder.WithCache(m.cache, app.InstanceName(m.namespace)) diff --git a/util/argo/diff/diff.go b/util/argo/diff/diff.go index 246cf901b5df4..1664c387a3b09 100644 --- a/util/argo/diff/diff.go +++ b/util/argo/diff/diff.go @@ -4,6 +4,7 @@ import ( "fmt" "github.com/go-logr/logr" + log "github.com/sirupsen/logrus" k8smanagedfields "k8s.io/apimachinery/pkg/util/managedfields" @@ -63,7 +64,6 @@ func (b *DiffConfigBuilder) WithNoCache() *DiffConfigBuilder { // WithCache sets the appstatecache.Cache and the appName in the diff config. Those the // are two objects necessary to retrieve a cached diff. func (b *DiffConfigBuilder) WithCache(s *appstatecache.Cache, appName string) *DiffConfigBuilder { - b.diffConfig.noCache = false b.diffConfig.stateCache = s b.diffConfig.appName = appName return b @@ -105,6 +105,11 @@ func (b *DiffConfigBuilder) WithServerSideDiff(ssd bool) *DiffConfigBuilder { return b } +func (b *DiffConfigBuilder) WithRefreshType(rt v1alpha1.RefreshType) *DiffConfigBuilder { + b.diffConfig.refreshType = rt + return b +} + // Build will first validate the current state of the diff config and return the // DiffConfig implementation if no errors are found. Will return nil and the error // details otherwise. @@ -171,6 +176,7 @@ type diffConfig struct { manager string serverSideDiff bool resourceOperations kube.ResourceOperations + refreshType v1alpha1.RefreshType } func (c *diffConfig) Ignores() []v1alpha1.ResourceIgnoreDifferences { @@ -363,9 +369,12 @@ func (c *diffConfig) DiffFromCache(appName string) (bool, []*v1alpha1.ResourceDi return false, nil } cachedDiff := make([]*v1alpha1.ResourceDiff, 0) - // TODO (SSD): Fix this code as it is swallowing errors if - // GetAppManagedResources fails to execute. - if c.stateCache != nil && c.stateCache.GetAppManagedResources(appName, &cachedDiff) == nil { + if c.stateCache != nil { + err := c.stateCache.GetAppManagedResources(appName, &cachedDiff) + if err != nil { + log.Errorf("DiffFromCache error: error getting managed resources: %s", err) + return false, nil + } return true, cachedDiff } return false, nil From 876627cd628790c89021393e9d0644a87d95d628 Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Thu, 30 Nov 2023 15:57:21 -0500 Subject: [PATCH 03/24] Create the serverSideDiff config Signed-off-by: Leonardo Luz Almeida --- common/common.go | 2 + controller/state.go | 34 ++-- controller/state_test.go | 11 +- .../operator-manual/argocd-cmd-params-cm.yaml | 4 + go.sum | 2 - ...ocd-application-controller-deployment.yaml | 156 +++++++++--------- ...cd-application-controller-statefulset.yaml | 150 +++++++++-------- util/argo/diff/diff.go | 9 - 8 files changed, 197 insertions(+), 171 deletions(-) diff --git a/common/common.go b/common/common.go index 5faedc2e344c4..d08260591abaa 100644 --- a/common/common.go +++ b/common/common.go @@ -260,6 +260,8 @@ const ( EnvRedisHaProxyName = "ARGOCD_REDIS_HAPROXY_NAME" // EnvGRPCKeepAliveMin defines the GRPCKeepAliveEnforcementMinimum, used in the grpc.KeepaliveEnforcementPolicy. Expects a "Duration" format (e.g. 10s). EnvGRPCKeepAliveMin = "ARGOCD_GRPC_KEEP_ALIVE_MIN" + // EnvServerSideDiff defines the env var used to enable ServerSide Diff feature + EnvServerSideDiff = "ARGOCD_APPLICATION_CONTROLLER_SERVER_SIDE_DIFF" ) // Config Management Plugin related constants diff --git a/controller/state.go b/controller/state.go index ce58d5deed980..569c73a89e2cf 100644 --- a/controller/state.go +++ b/controller/state.go @@ -114,6 +114,7 @@ type appStateManager struct { persistResourceHealth bool repoErrorCache goSync.Map repoErrorGracePeriod time.Duration + serverSideDiff bool } // getRepoObjs will generate the manifests for the given application delegating the @@ -584,13 +585,15 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 manifestRevisions = append(manifestRevisions, manifestInfo.Revision) } - useDiffCache := useDiffCache(noCache, manifestInfos, sources, app, manifestRevisions, m.statusRefreshTimeout, logCtx) + // TODO (SSD): Make serverSideDiff configurable via argocd-cm + // and resource annotation + serverSideDiff := true + + useDiffCache := useDiffCache(noCache, manifestInfos, sources, app, manifestRevisions, m.statusRefreshTimeout, serverSideDiff, logCtx) diffConfigBuilder := argodiff.NewDiffConfigBuilder(). WithDiffSettings(app.Spec.IgnoreDifferences, resourceOverrides, compareOptions.IgnoreAggregatedRoles). - WithTracking(appLabelKey, string(trackingMethod)). - WithCache(m.cache, app.InstanceName(m.namespace)). - WithRefreshType(refreshType) + WithTracking(appLabelKey, string(trackingMethod)) if useDiffCache { diffConfigBuilder.WithCache(m.cache, app.InstanceName(m.namespace)) @@ -605,9 +608,6 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 diffConfigBuilder.WithGVKParser(gvkParser) diffConfigBuilder.WithManager(common.ArgoCDSSAManager) - // TODO (SSD): Make serverSideDiff configurable via argocd-cm - // and resource annotation - serverSideDiff := true diffConfigBuilder.WithServerSideDiff(serverSideDiff) if serverSideDiff { @@ -819,18 +819,26 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 // useDiffCache will determine if the diff should be calculated based // on the existing live state cache or not. -func useDiffCache(noCache bool, manifestInfos []*apiclient.ManifestResponse, sources []v1alpha1.ApplicationSource, app *v1alpha1.Application, manifestRevisions []string, statusRefreshTimeout time.Duration, log *log.Entry) bool { +func useDiffCache(noCache bool, manifestInfos []*apiclient.ManifestResponse, sources []v1alpha1.ApplicationSource, app *v1alpha1.Application, manifestRevisions []string, statusRefreshTimeout time.Duration, serverSideDiff bool, log *log.Entry) bool { if noCache { log.WithField("useDiffCache", "false").Debug("noCache is true") return false } - _, refreshRequested := app.IsRefreshRequested() + refreshType, refreshRequested := app.IsRefreshRequested() if refreshRequested { - log.WithField("useDiffCache", "false").Debug("refreshRequested") - return false + if refreshType == v1alpha1.RefreshTypeHard { + log.WithField("useDiffCache", "false").Debug("hard refresh requested") + return false + } + // serverSideDiff should still use cache if normal refresh is requested + if !serverSideDiff && refreshType == v1alpha1.RefreshTypeNormal { + log.WithField("useDiffCache", "false").Debug("normal refresh Requested") + return false + } } - if app.Status.Expired(statusRefreshTimeout) { + // serverSideDiff should still use cache even if status is expired + if app.Status.Expired(statusRefreshTimeout) && !serverSideDiff { log.WithField("useDiffCache", "false").Debug("app.status.expired") return false } @@ -911,6 +919,7 @@ func NewAppStateManager( resourceTracking argo.ResourceTracking, persistResourceHealth bool, repoErrorGracePeriod time.Duration, + serverSideDiff bool, ) AppStateManager { return &appStateManager{ liveStateCache: liveStateCache, @@ -927,6 +936,7 @@ func NewAppStateManager( resourceTracking: resourceTracking, persistResourceHealth: persistResourceHealth, repoErrorGracePeriod: repoErrorGracePeriod, + serverSideDiff: serverSideDiff, } } diff --git a/controller/state_test.go b/controller/state_test.go index a240b30d688df..d33a6fd120777 100644 --- a/controller/state_test.go +++ b/controller/state_test.go @@ -1407,6 +1407,7 @@ func TestUseDiffCache(t *testing.T) { manifestRevisions []string statusRefreshTimeout time.Duration expectedUseCache bool + serverSideDiff bool } manifestInfos := func(revision string) []*apiclient.ManifestResponse { @@ -1505,6 +1506,7 @@ func TestUseDiffCache(t *testing.T) { manifestRevisions: []string{"rev1"}, statusRefreshTimeout: time.Hour * 24, expectedUseCache: true, + serverSideDiff: false, }, { testName: "will use diff cache for multisource", @@ -1548,6 +1550,7 @@ func TestUseDiffCache(t *testing.T) { manifestRevisions: []string{"rev1", "rev2"}, statusRefreshTimeout: time.Hour * 24, expectedUseCache: true, + serverSideDiff: false, }, { testName: "will return false if nocache is true", @@ -1558,6 +1561,7 @@ func TestUseDiffCache(t *testing.T) { manifestRevisions: []string{"rev1"}, statusRefreshTimeout: time.Hour * 24, expectedUseCache: false, + serverSideDiff: false, }, { testName: "will return false if requested refresh", @@ -1568,6 +1572,7 @@ func TestUseDiffCache(t *testing.T) { manifestRevisions: []string{"rev1"}, statusRefreshTimeout: time.Hour * 24, expectedUseCache: false, + serverSideDiff: false, }, { testName: "will return false if status expired", @@ -1578,6 +1583,7 @@ func TestUseDiffCache(t *testing.T) { manifestRevisions: []string{"rev1"}, statusRefreshTimeout: time.Minute, expectedUseCache: false, + serverSideDiff: false, }, { testName: "will return false if there is a new revision", @@ -1588,6 +1594,7 @@ func TestUseDiffCache(t *testing.T) { manifestRevisions: []string{"rev2"}, statusRefreshTimeout: time.Hour * 24, expectedUseCache: false, + serverSideDiff: false, }, { testName: "will return false if app spec repo changed", @@ -1604,6 +1611,7 @@ func TestUseDiffCache(t *testing.T) { manifestRevisions: []string{"rev1"}, statusRefreshTimeout: time.Hour * 24, expectedUseCache: false, + serverSideDiff: false, }, { testName: "will return false if app spec IgnoreDifferences changed", @@ -1626,6 +1634,7 @@ func TestUseDiffCache(t *testing.T) { manifestRevisions: []string{"rev1"}, statusRefreshTimeout: time.Hour * 24, expectedUseCache: false, + serverSideDiff: false, }, } @@ -1638,7 +1647,7 @@ func TestUseDiffCache(t *testing.T) { log := logrus.NewEntry(logger) // When - useDiffCache := useDiffCache(tc.noCache, tc.manifestInfos, tc.sources, tc.app, tc.manifestRevisions, tc.statusRefreshTimeout, log) + useDiffCache := useDiffCache(tc.noCache, tc.manifestInfos, tc.sources, tc.app, tc.manifestRevisions, tc.statusRefreshTimeout, tc.serverSideDiff, log) // Then assert.Equal(t, useDiffCache, tc.expectedUseCache) diff --git a/docs/operator-manual/argocd-cmd-params-cm.yaml b/docs/operator-manual/argocd-cmd-params-cm.yaml index 695119bf0a27f..4ceb9c1ba8f6f 100644 --- a/docs/operator-manual/argocd-cmd-params-cm.yaml +++ b/docs/operator-manual/argocd-cmd-params-cm.yaml @@ -68,6 +68,10 @@ data: controller.k8sclient.retry.base.backoff: "100" # Grace period in seconds for ignoring consecutive errors while communicating with repo server. controller.repo.error.grace.period.seconds: "180" + # Enables the server side diff feature at the application controller level. + # Diff calculation will be done by running a server side apply dryrun (when + # diff cache is unavailable). + controller.diff.server.side: "false" ## Server properties # Listen on given address for incoming connections (default "0.0.0.0") diff --git a/go.sum b/go.sum index dbe7107f59cba..fb4ef3abd94b9 100644 --- a/go.sum +++ b/go.sum @@ -696,8 +696,6 @@ github.com/apache/thrift v0.12.0/go.mod h1:cp2SuWMxlEZw2r+iP2GNCdIi4C1qmUzdZFSVb github.com/apache/thrift v0.13.0/go.mod h1:cp2SuWMxlEZw2r+iP2GNCdIi4C1qmUzdZFSVb+bacwQ= github.com/apache/thrift v0.16.0/go.mod h1:PHK3hniurgQaNMZYaCLEqXKsYK8upmhPbmdP2FXSqgU= github.com/appscode/go v0.0.0-20191119085241-0887d8ec2ecc/go.mod h1:OawnOmAL4ZX3YaPdN+8HTNwBveT1jMsqP74moa9XUbE= -github.com/argoproj/gitops-engine v0.7.1-0.20231102154024-c0c2dd1f6f48 h1:vnXMrNkBFC0H0KBkH1Jno31OVgJQR4KSd0ypEcfzQRA= -github.com/argoproj/gitops-engine v0.7.1-0.20231102154024-c0c2dd1f6f48/go.mod h1:1TchqKw9XmYYZluyEHa1dTJQoZgbV6PhabB/e8Wf3KY= github.com/argoproj/notifications-engine v0.4.1-0.20231027194313-a8d185ecc0a9 h1:1lt0VXzmLK7Vv0kaeal3S6/JIfzPyBORkUWXhiqF3l0= github.com/argoproj/notifications-engine v0.4.1-0.20231027194313-a8d185ecc0a9/go.mod h1:E/vv4+by868m0mmflaRfGBmKBtAupoF+mmyfekP8QCk= github.com/argoproj/pkg v0.13.7-0.20230626144333-d56162821bd1 h1:qsHwwOJ21K2Ao0xPju1sNuqphyMnMYkyB3ZLoLtxWpo= diff --git a/manifests/base/application-controller-deployment/argocd-application-controller-deployment.yaml b/manifests/base/application-controller-deployment/argocd-application-controller-deployment.yaml index 4862721961f21..0fbf979809c97 100644 --- a/manifests/base/application-controller-deployment/argocd-application-controller-deployment.yaml +++ b/manifests/base/application-controller-deployment/argocd-application-controller-deployment.yaml @@ -34,24 +34,30 @@ spec: name: argocd-cm key: timeout.hard.reconciliation optional: true + - name: ARGOCD_REPO_ERROR_GRACE_PERIOD_SECONDS + valueFrom: + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.repo.error.grace.period.seconds + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: repo.server - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: repo.server + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER_TIMEOUT_SECONDS valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.repo.server.timeout.seconds - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.repo.server.timeout.seconds + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_STATUS_PROCESSORS valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.status.processors - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.status.processors + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_OPERATION_PROCESSORS valueFrom: configMapKeyRef: @@ -78,22 +84,22 @@ spec: optional: true - name: ARGOCD_APPLICATION_CONTROLLER_SELF_HEAL_TIMEOUT_SECONDS valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.self.heal.timeout.seconds - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.self.heal.timeout.seconds + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER_PLAINTEXT valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.repo.server.plaintext - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.repo.server.plaintext + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER_STRICT_TLS valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.repo.server.strict.tls - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.repo.server.strict.tls + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_PERSIST_RESOURCE_HEALTH valueFrom: configMapKeyRef: @@ -102,16 +108,16 @@ spec: optional: true - name: ARGOCD_APP_STATE_CACHE_EXPIRATION valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.app.state.cache.expiration - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.app.state.cache.expiration + optional: true - name: REDIS_SERVER valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: redis.server - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: redis.server + optional: true - name: REDIS_COMPRESSION valueFrom: configMapKeyRef: @@ -120,70 +126,70 @@ spec: optional: true - name: REDISDB valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: redis.db - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: redis.db + optional: true - name: ARGOCD_DEFAULT_CACHE_EXPIRATION valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.default.cache.expiration - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.default.cache.expiration + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_OTLP_ADDRESS valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: otlp.address - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: otlp.address + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_OTLP_INSECURE valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: otlp.insecure - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: otlp.insecure + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_OTLP_HEADERS valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: otlp.headers - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: otlp.headers + optional: true - name: ARGOCD_APPLICATION_NAMESPACES valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: application.namespaces - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: application.namespaces + optional: true - name: ARGOCD_CONTROLLER_SHARDING_ALGORITHM valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.sharding.algorithm - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.sharding.algorithm + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_KUBECTL_PARALLELISM_LIMIT - valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.kubectl.parallelism.limit - optional: true - - name: ARGOCD_CONTROLLER_HEARTBEAT_TIME valueFrom: configMapKeyRef: name: argocd-cmd-params-cm - key: controller.heatbeat.time + key: controller.kubectl.parallelism.limit optional: true - name: ARGOCD_K8SCLIENT_RETRY_MAX valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.k8sclient.retry.max - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.k8sclient.retry.max + optional: true - name: ARGOCD_K8SCLIENT_RETRY_BASE_BACKOFF valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.k8sclient.retry.base.backoff - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.k8sclient.retry.base.backoff + optional: true + - name: ARGOCD_APPLICATION_CONTROLLER_SERVER_SIDE_DIFF + valueFrom: + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.diff.server.side + optional: true image: quay.io/argoproj/argocd:latest imagePullPolicy: Always name: argocd-application-controller diff --git a/manifests/base/application-controller/argocd-application-controller-statefulset.yaml b/manifests/base/application-controller/argocd-application-controller-statefulset.yaml index f5e5c759e0750..62f98a1449215 100644 --- a/manifests/base/application-controller/argocd-application-controller-statefulset.yaml +++ b/manifests/base/application-controller/argocd-application-controller-statefulset.yaml @@ -43,22 +43,22 @@ spec: optional: true - name: ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: repo.server - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: repo.server + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER_TIMEOUT_SECONDS valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.repo.server.timeout.seconds - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.repo.server.timeout.seconds + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_STATUS_PROCESSORS valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.status.processors - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.status.processors + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_OPERATION_PROCESSORS valueFrom: configMapKeyRef: @@ -85,22 +85,22 @@ spec: optional: true - name: ARGOCD_APPLICATION_CONTROLLER_SELF_HEAL_TIMEOUT_SECONDS valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.self.heal.timeout.seconds - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.self.heal.timeout.seconds + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER_PLAINTEXT valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.repo.server.plaintext - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.repo.server.plaintext + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_REPO_SERVER_STRICT_TLS valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.repo.server.strict.tls - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.repo.server.strict.tls + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_PERSIST_RESOURCE_HEALTH valueFrom: configMapKeyRef: @@ -109,16 +109,16 @@ spec: optional: true - name: ARGOCD_APP_STATE_CACHE_EXPIRATION valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.app.state.cache.expiration - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.app.state.cache.expiration + optional: true - name: REDIS_SERVER valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: redis.server - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: redis.server + optional: true - name: REDIS_COMPRESSION valueFrom: configMapKeyRef: @@ -127,64 +127,70 @@ spec: optional: true - name: REDISDB valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: redis.db - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: redis.db + optional: true - name: ARGOCD_DEFAULT_CACHE_EXPIRATION valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.default.cache.expiration - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.default.cache.expiration + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_OTLP_ADDRESS valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: otlp.address - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: otlp.address + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_OTLP_INSECURE valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: otlp.insecure - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: otlp.insecure + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_OTLP_HEADERS valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: otlp.headers - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: otlp.headers + optional: true - name: ARGOCD_APPLICATION_NAMESPACES valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: application.namespaces - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: application.namespaces + optional: true - name: ARGOCD_CONTROLLER_SHARDING_ALGORITHM valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.sharding.algorithm - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.sharding.algorithm + optional: true - name: ARGOCD_APPLICATION_CONTROLLER_KUBECTL_PARALLELISM_LIMIT valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.kubectl.parallelism.limit - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.kubectl.parallelism.limit + optional: true - name: ARGOCD_K8SCLIENT_RETRY_MAX valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.k8sclient.retry.max - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.k8sclient.retry.max + optional: true - name: ARGOCD_K8SCLIENT_RETRY_BASE_BACKOFF valueFrom: - configMapKeyRef: - name: argocd-cmd-params-cm - key: controller.k8sclient.retry.base.backoff - optional: true + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.k8sclient.retry.base.backoff + optional: true + - name: ARGOCD_APPLICATION_CONTROLLER_SERVER_SIDE_DIFF + valueFrom: + configMapKeyRef: + name: argocd-cmd-params-cm + key: controller.diff.server.side + optional: true image: quay.io/argoproj/argocd:latest imagePullPolicy: Always name: argocd-application-controller diff --git a/util/argo/diff/diff.go b/util/argo/diff/diff.go index 1664c387a3b09..2ab040bf5cd03 100644 --- a/util/argo/diff/diff.go +++ b/util/argo/diff/diff.go @@ -105,11 +105,6 @@ func (b *DiffConfigBuilder) WithServerSideDiff(ssd bool) *DiffConfigBuilder { return b } -func (b *DiffConfigBuilder) WithRefreshType(rt v1alpha1.RefreshType) *DiffConfigBuilder { - b.diffConfig.refreshType = rt - return b -} - // Build will first validate the current state of the diff config and return the // DiffConfig implementation if no errors are found. Will return nil and the error // details otherwise. @@ -176,7 +171,6 @@ type diffConfig struct { manager string serverSideDiff bool resourceOperations kube.ResourceOperations - refreshType v1alpha1.RefreshType } func (c *diffConfig) Ignores() []v1alpha1.ResourceIgnoreDifferences { @@ -292,9 +286,6 @@ func StateDiffs(lives, configs []*unstructured.Unstructured, diffConfig DiffConf diffOpts = append(diffOpts, diff.WithLogr(*diffConfig.Logger())) } - // TODO (SSD): ServerSideDiff needs to be configured to always rely on the - // cache to avoid hitting Kube API too frequently. Need to validate if the - // current cached diff is compatible with SSD. useCache, cachedDiff := diffConfig.DiffFromCache(diffConfig.AppName()) if useCache && cachedDiff != nil { cached, err := diffArrayCached(normResults.Targets, normResults.Lives, cachedDiff, diffOpts...) From 0e25b0b40851ac4be74c234fe840706c38c7c2f4 Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Fri, 1 Dec 2023 12:03:08 -0500 Subject: [PATCH 04/24] chore: add featureflag utility package Signed-off-by: Leonardo Luz Almeida --- util/featureflag/featureflag.go | 44 +++++++++++++++++++++++ util/featureflag/featureflag_test.go | 53 ++++++++++++++++++++++++++++ 2 files changed, 97 insertions(+) create mode 100644 util/featureflag/featureflag.go create mode 100644 util/featureflag/featureflag_test.go diff --git a/util/featureflag/featureflag.go b/util/featureflag/featureflag.go new file mode 100644 index 0000000000000..1957de055a571 --- /dev/null +++ b/util/featureflag/featureflag.go @@ -0,0 +1,44 @@ +package featureflag + +type featureFlags struct { + ExampleFeature *featureFlag +} + +// New will instantiate a new featureFlags object with all default values. +// Every new feature flag must be initialized in this function or the test +// will fail. +func New() featureFlags { + return featureFlags{ + ExampleFeature: &featureFlag{ + description: "this is just an example of how a feature flag must be created", + enabled: false, + }, + } +} + +type featureFlag struct { + description string + enabled bool +} + +func (ff *featureFlag) IsEnabled() bool { + if ff == nil { + return false + } + return ff.enabled +} + +func (ff *featureFlag) Enable() { + ff.enabled = true +} + +func (ff *featureFlag) Disable() { + ff.enabled = false +} + +func (ff *featureFlag) Description() string { + if ff == nil { + return "" + } + return ff.description +} diff --git a/util/featureflag/featureflag_test.go b/util/featureflag/featureflag_test.go new file mode 100644 index 0000000000000..cdf4ffddfb566 --- /dev/null +++ b/util/featureflag/featureflag_test.go @@ -0,0 +1,53 @@ +package featureflag_test + +import ( + "fmt" + "reflect" + "testing" + + "github.com/argoproj/argo-cd/v2/util/featureflag" + "github.com/stretchr/testify/assert" +) + +type FeatureFlager interface { + IsEnabled() bool + Enable() + Disable() + Description() string +} + +func TestFeatureFlags(t *testing.T) { + + t.Run("will validate a simple usage", func(t *testing.T) { + // Given + ff := featureflag.New() + + // Then + assert.False(t, ff.ExampleFeature.IsEnabled(), "ExampleFeature flags should always be disabled") + assert.NotEmpty(t, ff.ExampleFeature.Description(), "All feature flags must provide a description") + ff.ExampleFeature.Enable() + assert.True(t, ff.ExampleFeature.IsEnabled(), "ExampleFeature should be enabled") + }) + t.Run("will validate that all feature flags are properly initialized", func(t *testing.T) { + // Given + ff := featureflag.New() + value := reflect.ValueOf(ff) + types := value.Type() + fields := make(map[string]interface{}, value.NumField()) + + // When + for i := 0; i < value.NumField(); i++ { + if value.Field(i).CanInterface() { + fields[types.Field(i).Name] = value.Field(i).Interface() + } + } + + // Then + for k, v := range fields { + assert.NotNil(t, v, fmt.Sprintf("%s feature flag needs to be initialized in the New function", k)) + if ff, ok := v.(FeatureFlager); ok { + assert.NotEmpty(t, ff.Description(), fmt.Sprintf("%s feature flag needs to define its description", k)) + } + } + }) +} From 0fac9cc2d4b33eeada5eb48c676284a73a8fb022 Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Fri, 1 Dec 2023 15:07:14 -0500 Subject: [PATCH 05/24] remove featureflag package Signed-off-by: Leonardo Luz Almeida --- util/featureflag/featureflag.go | 44 ----------------------- util/featureflag/featureflag_test.go | 53 ---------------------------- 2 files changed, 97 deletions(-) delete mode 100644 util/featureflag/featureflag.go delete mode 100644 util/featureflag/featureflag_test.go diff --git a/util/featureflag/featureflag.go b/util/featureflag/featureflag.go deleted file mode 100644 index 1957de055a571..0000000000000 --- a/util/featureflag/featureflag.go +++ /dev/null @@ -1,44 +0,0 @@ -package featureflag - -type featureFlags struct { - ExampleFeature *featureFlag -} - -// New will instantiate a new featureFlags object with all default values. -// Every new feature flag must be initialized in this function or the test -// will fail. -func New() featureFlags { - return featureFlags{ - ExampleFeature: &featureFlag{ - description: "this is just an example of how a feature flag must be created", - enabled: false, - }, - } -} - -type featureFlag struct { - description string - enabled bool -} - -func (ff *featureFlag) IsEnabled() bool { - if ff == nil { - return false - } - return ff.enabled -} - -func (ff *featureFlag) Enable() { - ff.enabled = true -} - -func (ff *featureFlag) Disable() { - ff.enabled = false -} - -func (ff *featureFlag) Description() string { - if ff == nil { - return "" - } - return ff.description -} diff --git a/util/featureflag/featureflag_test.go b/util/featureflag/featureflag_test.go deleted file mode 100644 index cdf4ffddfb566..0000000000000 --- a/util/featureflag/featureflag_test.go +++ /dev/null @@ -1,53 +0,0 @@ -package featureflag_test - -import ( - "fmt" - "reflect" - "testing" - - "github.com/argoproj/argo-cd/v2/util/featureflag" - "github.com/stretchr/testify/assert" -) - -type FeatureFlager interface { - IsEnabled() bool - Enable() - Disable() - Description() string -} - -func TestFeatureFlags(t *testing.T) { - - t.Run("will validate a simple usage", func(t *testing.T) { - // Given - ff := featureflag.New() - - // Then - assert.False(t, ff.ExampleFeature.IsEnabled(), "ExampleFeature flags should always be disabled") - assert.NotEmpty(t, ff.ExampleFeature.Description(), "All feature flags must provide a description") - ff.ExampleFeature.Enable() - assert.True(t, ff.ExampleFeature.IsEnabled(), "ExampleFeature should be enabled") - }) - t.Run("will validate that all feature flags are properly initialized", func(t *testing.T) { - // Given - ff := featureflag.New() - value := reflect.ValueOf(ff) - types := value.Type() - fields := make(map[string]interface{}, value.NumField()) - - // When - for i := 0; i < value.NumField(); i++ { - if value.Field(i).CanInterface() { - fields[types.Field(i).Name] = value.Field(i).Interface() - } - } - - // Then - for k, v := range fields { - assert.NotNil(t, v, fmt.Sprintf("%s feature flag needs to be initialized in the New function", k)) - if ff, ok := v.(FeatureFlager); ok { - assert.NotEmpty(t, ff.Description(), fmt.Sprintf("%s feature flag needs to define its description", k)) - } - } - }) -} From 2fe68039530b913c9720279594dfc8129edd0c0c Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Fri, 1 Dec 2023 15:18:54 -0500 Subject: [PATCH 06/24] add param Signed-off-by: Leonardo Luz Almeida --- .../commands/argocd_application_controller.go | 3 +++ controller/appcontroller.go | 3 ++- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/cmd/argocd-application-controller/commands/argocd_application_controller.go b/cmd/argocd-application-controller/commands/argocd_application_controller.go index 74d0af45c9d7a..0b9708384a3b1 100644 --- a/cmd/argocd-application-controller/commands/argocd_application_controller.go +++ b/cmd/argocd-application-controller/commands/argocd_application_controller.go @@ -73,6 +73,7 @@ func NewCommand() *cobra.Command { persistResourceHealth bool shardingAlgorithm string enableDynamicClusterDistribution bool + serverSideDiff bool ) var command = cobra.Command{ Use: cliName, @@ -166,6 +167,7 @@ func NewCommand() *cobra.Command { clusterFilter, applicationNamespaces, &workqueueRateLimit, + serverSideDiff, ) errors.CheckError(err) cacheutil.CollectMetrics(redisClient, appController.GetMetricsServer()) @@ -224,6 +226,7 @@ func NewCommand() *cobra.Command { command.Flags().DurationVar(&workqueueRateLimit.MaxDelay, "wq-maxdelay-ns", time.Duration(env.ParseInt64FromEnv("WORKQUEUE_MAX_DELAY_NS", time.Second.Nanoseconds(), 1*time.Millisecond.Nanoseconds(), (24*time.Hour).Nanoseconds())), "Set Workqueue Per Item Rate Limiter Max Delay duration in nanoseconds, default 1000000000 (1s)") command.Flags().Float64Var(&workqueueRateLimit.BackoffFactor, "wq-backoff-factor", env.ParseFloat64FromEnv("WORKQUEUE_BACKOFF_FACTOR", 1.5, 0, math.MaxFloat64), "Set Workqueue Per Item Rate Limiter Backoff Factor, default is 1.5") command.Flags().BoolVar(&enableDynamicClusterDistribution, "dynamic-cluster-distribution-enabled", env.ParseBoolFromEnv(common.EnvEnableDynamicClusterDistribution, false), "Enables dynamic cluster distribution.") + command.Flags().BoolVar(&serverSideDiff, "server-side-diff", env.ParseBoolFromEnv(common.EnvServerSideDiff, false), "Feature flag to enable ServerSide diff") cacheSource = appstatecache.AddCacheFlagsToCmd(&command, func(client *redis.Client) { redisClient = client }) diff --git a/controller/appcontroller.go b/controller/appcontroller.go index 964bcc1aa0dec..cbaabf0f13e4f 100644 --- a/controller/appcontroller.go +++ b/controller/appcontroller.go @@ -152,6 +152,7 @@ func NewApplicationController( clusterFilter func(cluster *appv1.Cluster) bool, applicationNamespaces []string, rateLimiterConfig *ratelimiter.AppControllerRateLimiterConfig, + serverSideDiff bool, ) (*ApplicationController, error) { log.Infof("appResyncPeriod=%v, appHardResyncPeriod=%v", appResyncPeriod, appHardResyncPeriod) db := db.NewDB(namespace, settingsMgr, kubeClientset) @@ -260,7 +261,7 @@ func NewApplicationController( } } stateCache := statecache.NewLiveStateCache(db, appInformer, ctrl.settingsMgr, kubectl, ctrl.metricsServer, ctrl.handleObjectUpdated, clusterFilter, argo.NewResourceTracking()) - appStateManager := NewAppStateManager(db, applicationClientset, repoClientset, namespace, kubectl, ctrl.settingsMgr, stateCache, projInformer, ctrl.metricsServer, argoCache, ctrl.statusRefreshTimeout, argo.NewResourceTracking(), persistResourceHealth, repoErrorGracePeriod) + appStateManager := NewAppStateManager(db, applicationClientset, repoClientset, namespace, kubectl, ctrl.settingsMgr, stateCache, projInformer, ctrl.metricsServer, argoCache, ctrl.statusRefreshTimeout, argo.NewResourceTracking(), persistResourceHealth, repoErrorGracePeriod, serverSideDiff) ctrl.appInformer = appInformer ctrl.appLister = appLister ctrl.projInformer = projInformer From 38016961200686535203a9ec91d9f3a03d291618 Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Fri, 1 Dec 2023 16:39:01 -0500 Subject: [PATCH 07/24] make ssd configurable with app annotation Signed-off-by: Leonardo Luz Almeida --- controller/state.go | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/controller/state.go b/controller/state.go index 569c73a89e2cf..367b17fb52f7d 100644 --- a/controller/state.go +++ b/controller/state.go @@ -585,9 +585,8 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 manifestRevisions = append(manifestRevisions, manifestInfo.Revision) } - // TODO (SSD): Make serverSideDiff configurable via argocd-cm - // and resource annotation - serverSideDiff := true + serverSideDiff := m.serverSideDiff || + resourceutil.HasAnnotationOption(app, common.AnnotationCompareOptions, "ServerSideDiff=true") useDiffCache := useDiffCache(noCache, manifestInfos, sources, app, manifestRevisions, m.statusRefreshTimeout, serverSideDiff, logCtx) From ec518f9085cc33c0ca8df32f017e005356302f7a Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Fri, 1 Dec 2023 17:05:53 -0500 Subject: [PATCH 08/24] add server-side-diff flags Signed-off-by: Leonardo Luz Almeida --- cmd/argocd/commands/admin/app.go | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/cmd/argocd/commands/admin/app.go b/cmd/argocd/commands/admin/app.go index 10e4effe8797a..a68987a041ab9 100644 --- a/cmd/argocd/commands/admin/app.go +++ b/cmd/argocd/commands/admin/app.go @@ -243,6 +243,7 @@ func NewReconcileCommand(clientOpts *argocdclient.ClientOptions) *cobra.Command repoServerAddress string outputFormat string refresh bool + serverSideDiff bool ) var command = &cobra.Command{ @@ -280,7 +281,7 @@ func NewReconcileCommand(clientOpts *argocdclient.ClientOptions) *cobra.Command appClientset := appclientset.NewForConfigOrDie(cfg) kubeClientset := kubernetes.NewForConfigOrDie(cfg) - result, err = reconcileApplications(ctx, kubeClientset, appClientset, namespace, repoServerClient, selector, newLiveStateCache) + result, err = reconcileApplications(ctx, kubeClientset, appClientset, namespace, repoServerClient, selector, newLiveStateCache, serverSideDiff) errors.CheckError(err) } else { appClientset := appclientset.NewForConfigOrDie(cfg) @@ -295,6 +296,7 @@ func NewReconcileCommand(clientOpts *argocdclient.ClientOptions) *cobra.Command command.Flags().StringVar(&selector, "l", "", "Label selector") command.Flags().StringVar(&outputFormat, "o", "yaml", "Output format (yaml|json)") command.Flags().BoolVar(&refresh, "refresh", false, "If set to true then recalculates apps reconciliation") + command.Flags().BoolVar(&serverSideDiff, "serverSideDiff", false, "If set to true will use serverSideDiff while comparing resources") return command } @@ -344,6 +346,7 @@ func reconcileApplications( repoServerClient reposerverclient.Clientset, selector string, createLiveStateCache func(argoDB db.ArgoDB, appInformer kubecache.SharedIndexInformer, settingsMgr *settings.SettingsManager, server *metrics.MetricsServer) cache.LiveStateCache, + serverSideDiff bool, ) ([]appReconcileResult, error) { settingsMgr := settings.NewSettingsManager(ctx, kubeClientset, namespace) argoDB := db.NewDB(namespace, settingsMgr, kubeClientset) @@ -384,7 +387,7 @@ func reconcileApplications( ) appStateManager := controller.NewAppStateManager( - argoDB, appClientset, repoServerClient, namespace, kubeutil.NewKubectl(), settingsMgr, stateCache, projInformer, server, cache, time.Second, argo.NewResourceTracking(), false, 0) + argoDB, appClientset, repoServerClient, namespace, kubeutil.NewKubectl(), settingsMgr, stateCache, projInformer, server, cache, time.Second, argo.NewResourceTracking(), false, 0, serverSideDiff) appsList, err := appClientset.ArgoprojV1alpha1().Applications(namespace).List(ctx, v1.ListOptions{LabelSelector: selector}) if err != nil { From 76fcc0665b3f54b4f8cc10cd9ed2ea8e64e18955 Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Tue, 5 Dec 2023 17:44:56 -0500 Subject: [PATCH 09/24] apply the same logic regardless of the refresh type Signed-off-by: Leonardo Luz Almeida --- controller/state.go | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/controller/state.go b/controller/state.go index 367b17fb52f7d..d44aafd0e3349 100644 --- a/controller/state.go +++ b/controller/state.go @@ -826,14 +826,13 @@ func useDiffCache(noCache bool, manifestInfos []*apiclient.ManifestResponse, sou } refreshType, refreshRequested := app.IsRefreshRequested() if refreshRequested { - if refreshType == v1alpha1.RefreshTypeHard { - log.WithField("useDiffCache", "false").Debug("hard refresh requested") - return false - } - // serverSideDiff should still use cache if normal refresh is requested - if !serverSideDiff && refreshType == v1alpha1.RefreshTypeNormal { + switch refreshType { + case v1alpha1.RefreshTypeNormal: log.WithField("useDiffCache", "false").Debug("normal refresh Requested") return false + case v1alpha1.RefreshTypeHard: + log.WithField("useDiffCache", "false").Debug("hard refresh requested") + return false } } // serverSideDiff should still use cache even if status is expired From 415e264b71f1506e5c54f16399aa2f913f5cd780 Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Thu, 7 Dec 2023 15:07:50 -0500 Subject: [PATCH 10/24] fix gitops-engine reference Signed-off-by: Leonardo Luz Almeida --- go.mod | 5 +++-- go.sum | 6 ++++-- util/argo/diff/diff.go | 3 --- 3 files changed, 7 insertions(+), 7 deletions(-) diff --git a/go.mod b/go.mod index 7c939cfbacdbd..5c289e1fdaf15 100644 --- a/go.mod +++ b/go.mod @@ -104,7 +104,7 @@ require ( layeh.com/gopher-json v0.0.0-20190114024228-97fed8db8427 oras.land/oras-go/v2 v2.3.0 sigs.k8s.io/controller-runtime v0.14.6 - sigs.k8s.io/structured-merge-diff/v4 v4.3.0 + sigs.k8s.io/structured-merge-diff/v4 v4.4.1 sigs.k8s.io/yaml v1.3.0 ) @@ -294,7 +294,8 @@ require ( replace ( // TODO (SSD): Remove this before merging - github.com/argoproj/gitops-engine => /Users/lalmeida1/dev/git/intuit/gitops-engine + // github.com/argoproj/gitops-engine => /Users/lalmeida1/dev/git/intuit/gitops-engine + github.com/argoproj/gitops-engine => github.com/leoluz/gitops-engine v0.4.1-0.20231207195844-158a833350fc // https://github.com/golang/go/issues/33546#issuecomment-519656923 github.com/go-check/check => github.com/go-check/check v0.0.0-20180628173108-788fd7840127 diff --git a/go.sum b/go.sum index fb4ef3abd94b9..ddd49124b1511 100644 --- a/go.sum +++ b/go.sum @@ -1355,6 +1355,8 @@ github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0 github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw= github.com/leodido/go-urn v1.2.0 h1:hpXL4XnriNwQ/ABnpepYM/1vCLWNDfUNts8dX3xTG6Y= github.com/leodido/go-urn v1.2.0/go.mod h1:+8+nEpDfqqsY+g338gtMEUOtuK+4dEMhiQEgxpxOKII= +github.com/leoluz/gitops-engine v0.4.1-0.20231207195844-158a833350fc h1:KEgIxOcBVL1jp7Bcti2qnmHdILLBTKsB6P+K15AaIiQ= +github.com/leoluz/gitops-engine v0.4.1-0.20231207195844-158a833350fc/go.mod h1:gWE8uROi7hIkWGNAVM+8FWkMfo0vZ03SLx/aFw/DBzg= github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de h1:9TO3cAIGXtEhnIaL+V+BEER86oLrvS+kWobKpbJuye0= github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de/go.mod h1:zAbeS9B/r2mtpb6U+EI2rYA5OAXxsYw6wTamcNW+zcE= github.com/lightstep/lightstep-tracer-common/golang/gogo v0.0.0-20190605223551-bc2310a04743/go.mod h1:qklhhLq1aX+mtWk9cPHPzaBjWImj5ULL6C7HFJtXQMM= @@ -2711,8 +2713,8 @@ sigs.k8s.io/kustomize/api v0.12.1/go.mod h1:y3JUhimkZkR6sbLNwfJHxvo1TCLwuwm14sCY sigs.k8s.io/kustomize/kyaml v0.13.9 h1:Qz53EAaFFANyNgyOEJbT/yoIHygK40/ZcvU3rgry2Tk= sigs.k8s.io/kustomize/kyaml v0.13.9/go.mod h1:QsRbD0/KcU+wdk0/L0fIp2KLnohkVzs6fQ85/nOXac4= sigs.k8s.io/structured-merge-diff/v4 v4.2.3/go.mod h1:qjx8mGObPmV2aSZepjQjbmb2ihdVs8cGKBraizNC69E= -sigs.k8s.io/structured-merge-diff/v4 v4.3.0 h1:UZbZAZfX0wV2zr7YZorDz6GXROfDFj6LvqCRm4VUVKk= -sigs.k8s.io/structured-merge-diff/v4 v4.3.0/go.mod h1:N8hJocpFajUSSeSJ9bOZ77VzejKZaXsTtZo4/u7Io08= +sigs.k8s.io/structured-merge-diff/v4 v4.4.1 h1:150L+0vs/8DA78h1u02ooW1/fFq/Lwr+sGiqlzvrtq4= +sigs.k8s.io/structured-merge-diff/v4 v4.4.1/go.mod h1:N8hJocpFajUSSeSJ9bOZ77VzejKZaXsTtZo4/u7Io08= sigs.k8s.io/yaml v1.1.0/go.mod h1:UJmg0vDUVViEyp3mgSv9WPwZCDxu4rQW1olrI1uml+o= sigs.k8s.io/yaml v1.2.0/go.mod h1:yfXDCHCao9+ENCvLSE62v9VSji2MKu5jeNfTrofGhJc= sigs.k8s.io/yaml v1.3.0 h1:a2VclLzOGrwOHDiV8EfBGhvjHvP46CtW5j6POvhYGGo= diff --git a/util/argo/diff/diff.go b/util/argo/diff/diff.go index 2ab040bf5cd03..b5e7922f42cd7 100644 --- a/util/argo/diff/diff.go +++ b/util/argo/diff/diff.go @@ -394,9 +394,6 @@ func preDiffNormalize(lives, targets []*unstructured.Unstructured, diffConfig Di gvk := target.GetObjectKind().GroupVersionKind() idc := NewIgnoreDiffConfig(diffConfig.Ignores(), diffConfig.Overrides()) ok, ignoreDiff := idc.HasIgnoreDifference(gvk.Group, gvk.Kind, target.GetName(), target.GetNamespace()) - // TODO (SSD): This is likely unnecessary when ServerSideDiff is enabled - // as it seems that Kubernetes already takes care of it during dryrun apply. - // Need to validate and add condition if confirmed. if ok && len(ignoreDiff.ManagedFieldsManagers) > 0 { pt := scheme.ResolveParseableType(gvk, diffConfig.GVKParser()) var err error From 3ead6d8150bdc319536b79d84b401c3bf7b8df39 Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Mon, 11 Dec 2023 12:05:20 -0500 Subject: [PATCH 11/24] address review comments Signed-off-by: Leonardo Luz Almeida --- .../commands/argocd_application_controller.go | 2 +- cmd/argocd/commands/admin/app.go | 1 + common/common.go | 3 ++- controller/state.go | 18 ++++++++--------- controller/sync.go | 11 +++++++++- go.mod | 2 +- go.sum | 4 ++-- util/argo/diff/diff.go | 20 +++++++++---------- 8 files changed, 35 insertions(+), 26 deletions(-) diff --git a/cmd/argocd-application-controller/commands/argocd_application_controller.go b/cmd/argocd-application-controller/commands/argocd_application_controller.go index 0b9708384a3b1..796a645f03393 100644 --- a/cmd/argocd-application-controller/commands/argocd_application_controller.go +++ b/cmd/argocd-application-controller/commands/argocd_application_controller.go @@ -226,7 +226,7 @@ func NewCommand() *cobra.Command { command.Flags().DurationVar(&workqueueRateLimit.MaxDelay, "wq-maxdelay-ns", time.Duration(env.ParseInt64FromEnv("WORKQUEUE_MAX_DELAY_NS", time.Second.Nanoseconds(), 1*time.Millisecond.Nanoseconds(), (24*time.Hour).Nanoseconds())), "Set Workqueue Per Item Rate Limiter Max Delay duration in nanoseconds, default 1000000000 (1s)") command.Flags().Float64Var(&workqueueRateLimit.BackoffFactor, "wq-backoff-factor", env.ParseFloat64FromEnv("WORKQUEUE_BACKOFF_FACTOR", 1.5, 0, math.MaxFloat64), "Set Workqueue Per Item Rate Limiter Backoff Factor, default is 1.5") command.Flags().BoolVar(&enableDynamicClusterDistribution, "dynamic-cluster-distribution-enabled", env.ParseBoolFromEnv(common.EnvEnableDynamicClusterDistribution, false), "Enables dynamic cluster distribution.") - command.Flags().BoolVar(&serverSideDiff, "server-side-diff", env.ParseBoolFromEnv(common.EnvServerSideDiff, false), "Feature flag to enable ServerSide diff") + command.Flags().BoolVar(&serverSideDiff, "server-side-diff-enabled", env.ParseBoolFromEnv(common.EnvServerSideDiff, false), "Feature flag to enable ServerSide diff. Default (\"false\")") cacheSource = appstatecache.AddCacheFlagsToCmd(&command, func(client *redis.Client) { redisClient = client }) diff --git a/cmd/argocd/commands/admin/app.go b/cmd/argocd/commands/admin/app.go index a68987a041ab9..53513e50c4012 100644 --- a/cmd/argocd/commands/admin/app.go +++ b/cmd/argocd/commands/admin/app.go @@ -297,6 +297,7 @@ func NewReconcileCommand(clientOpts *argocdclient.ClientOptions) *cobra.Command command.Flags().StringVar(&outputFormat, "o", "yaml", "Output format (yaml|json)") command.Flags().BoolVar(&refresh, "refresh", false, "If set to true then recalculates apps reconciliation") command.Flags().BoolVar(&serverSideDiff, "serverSideDiff", false, "If set to true will use serverSideDiff while comparing resources") + command.Flags().BoolVar(&serverSideDiff, "server-side-diff-enabled", false, "If set to \"true\" will use server-side diff while comparing resources. Default (\"false\")") return command } diff --git a/common/common.go b/common/common.go index d08260591abaa..c5b9362f7f943 100644 --- a/common/common.go +++ b/common/common.go @@ -260,7 +260,8 @@ const ( EnvRedisHaProxyName = "ARGOCD_REDIS_HAPROXY_NAME" // EnvGRPCKeepAliveMin defines the GRPCKeepAliveEnforcementMinimum, used in the grpc.KeepaliveEnforcementPolicy. Expects a "Duration" format (e.g. 10s). EnvGRPCKeepAliveMin = "ARGOCD_GRPC_KEEP_ALIVE_MIN" - // EnvServerSideDiff defines the env var used to enable ServerSide Diff feature + // EnvServerSideDiff defines the env var used to enable ServerSide Diff feature. + // If defined, value must be "true" or "false". EnvServerSideDiff = "ARGOCD_APPLICATION_CONTROLLER_SERVER_SIDE_DIFF" ) diff --git a/controller/state.go b/controller/state.go index d44aafd0e3349..9f0141811c2f9 100644 --- a/controller/state.go +++ b/controller/state.go @@ -616,7 +616,7 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 conditions = append(conditions, v1alpha1.ApplicationCondition{Type: v1alpha1.ApplicationConditionUnknownError, Message: err.Error(), LastTransitionTime: &now}) } defer cleanup() - diffConfigBuilder.WithResourceOperations(resourceOps) + diffConfigBuilder.WithServerSideDryRunner(diff.NewK8sServerSideDryRunner(resourceOps)) } // enable structured merge diff if application syncs with server-side apply @@ -826,16 +826,14 @@ func useDiffCache(noCache bool, manifestInfos []*apiclient.ManifestResponse, sou } refreshType, refreshRequested := app.IsRefreshRequested() if refreshRequested { - switch refreshType { - case v1alpha1.RefreshTypeNormal: - log.WithField("useDiffCache", "false").Debug("normal refresh Requested") - return false - case v1alpha1.RefreshTypeHard: - log.WithField("useDiffCache", "false").Debug("hard refresh requested") - return false - } + log.WithField("useDiffCache", "false").Debugf("refresh type %s requested", string(refreshType)) + return false } - // serverSideDiff should still use cache even if status is expired + // serverSideDiff should still use cache even if status is expired. + // This is an attempt to avoid hitting k8s API server too frequently during + // app refresh with serverSideDiff is enabled. If there are negative side + // effects identified with this approach, the serverSideDiff should be removed + // from this condition. if app.Status.Expired(statusRefreshTimeout) && !serverSideDiff { log.WithField("useDiffCache", "false").Debug("app.status.expired") return false diff --git a/controller/sync.go b/controller/sync.go index d48d9ee0ac83a..5ae065690a7db 100644 --- a/controller/sync.go +++ b/controller/sync.go @@ -56,6 +56,11 @@ func (m *appStateManager) getGVKParser(server string) (*managedfields.GvkParser, } return cluster.GetGVKParser(), nil } + +// getResourceOperations will return the kubectl implementation of the ResourceOperations +// interface that provides functionality to manage kubernetes resources. Returns a +// cleanup function that must be called to remove the generated kube config for this +// server. func (m *appStateManager) getResourceOperations(server string) (kube.ResourceOperations, func(), error) { clusterCache, err := m.liveStateCache.GetClusterCache(server) if err != nil { @@ -66,7 +71,11 @@ func (m *appStateManager) getResourceOperations(server string) (kube.ResourceOpe if err != nil { return nil, nil, fmt.Errorf("error getting cluster: %w", err) } - return clusterCache.GetKubectl().ManageResources(cluster.RawRestConfig(), clusterCache.GetOpenAPISchema()) + ops, cleanup, err := m.kubectl.ManageResources(cluster.RawRestConfig(), clusterCache.GetOpenAPISchema()) + if err != nil { + return nil, nil, fmt.Errorf("error creating kubectl ResourceOperations: %w", err) + } + return ops, cleanup, nil } func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha1.OperationState) { diff --git a/go.mod b/go.mod index 5c289e1fdaf15..afed047ef1043 100644 --- a/go.mod +++ b/go.mod @@ -295,7 +295,7 @@ require ( replace ( // TODO (SSD): Remove this before merging // github.com/argoproj/gitops-engine => /Users/lalmeida1/dev/git/intuit/gitops-engine - github.com/argoproj/gitops-engine => github.com/leoluz/gitops-engine v0.4.1-0.20231207195844-158a833350fc + github.com/argoproj/gitops-engine => github.com/leoluz/gitops-engine v0.4.1-0.20231208220525-c1e3999317ff // https://github.com/golang/go/issues/33546#issuecomment-519656923 github.com/go-check/check => github.com/go-check/check v0.0.0-20180628173108-788fd7840127 diff --git a/go.sum b/go.sum index ddd49124b1511..8e42fc32c4076 100644 --- a/go.sum +++ b/go.sum @@ -1355,8 +1355,8 @@ github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0 github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw= github.com/leodido/go-urn v1.2.0 h1:hpXL4XnriNwQ/ABnpepYM/1vCLWNDfUNts8dX3xTG6Y= github.com/leodido/go-urn v1.2.0/go.mod h1:+8+nEpDfqqsY+g338gtMEUOtuK+4dEMhiQEgxpxOKII= -github.com/leoluz/gitops-engine v0.4.1-0.20231207195844-158a833350fc h1:KEgIxOcBVL1jp7Bcti2qnmHdILLBTKsB6P+K15AaIiQ= -github.com/leoluz/gitops-engine v0.4.1-0.20231207195844-158a833350fc/go.mod h1:gWE8uROi7hIkWGNAVM+8FWkMfo0vZ03SLx/aFw/DBzg= +github.com/leoluz/gitops-engine v0.4.1-0.20231208220525-c1e3999317ff h1:iL253TLkewFR7Ydt/NnC0S+1gWzmjx6oN2mYW4thxvI= +github.com/leoluz/gitops-engine v0.4.1-0.20231208220525-c1e3999317ff/go.mod h1:gWE8uROi7hIkWGNAVM+8FWkMfo0vZ03SLx/aFw/DBzg= github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de h1:9TO3cAIGXtEhnIaL+V+BEER86oLrvS+kWobKpbJuye0= github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de/go.mod h1:zAbeS9B/r2mtpb6U+EI2rYA5OAXxsYw6wTamcNW+zcE= github.com/lightstep/lightstep-tracer-common/golang/gogo v0.0.0-20190605223551-bc2310a04743/go.mod h1:qklhhLq1aX+mtWk9cPHPzaBjWImj5ULL6C7HFJtXQMM= diff --git a/util/argo/diff/diff.go b/util/argo/diff/diff.go index b5e7922f42cd7..55b6efadb283c 100644 --- a/util/argo/diff/diff.go +++ b/util/argo/diff/diff.go @@ -95,8 +95,8 @@ func (b *DiffConfigBuilder) WithManager(manager string) *DiffConfigBuilder { return b } -func (b *DiffConfigBuilder) WithResourceOperations(resourceOps kube.ResourceOperations) *DiffConfigBuilder { - b.diffConfig.resourceOperations = resourceOps +func (b *DiffConfigBuilder) WithServerSideDryRunner(ssdr diff.ServerSideDryRunner) *DiffConfigBuilder { + b.diffConfig.serverSideDryRunner = ssdr return b } @@ -151,8 +151,8 @@ type DiffConfig interface { // calculating the structured merge diff. Manager() string - ResourceOperations() kube.ResourceOperations ServerSideDiff() bool + ServerSideDryRunner() diff.ServerSideDryRunner } // diffConfig defines the configurations used while applying diffs. @@ -170,7 +170,7 @@ type diffConfig struct { structuredMergeDiff bool manager string serverSideDiff bool - resourceOperations kube.ResourceOperations + serverSideDryRunner diff.ServerSideDryRunner } func (c *diffConfig) Ignores() []v1alpha1.ResourceIgnoreDifferences { @@ -209,8 +209,8 @@ func (c *diffConfig) StructuredMergeDiff() bool { func (c *diffConfig) Manager() string { return c.manager } -func (c *diffConfig) ResourceOperations() kube.ResourceOperations { - return c.resourceOperations +func (c *diffConfig) ServerSideDryRunner() diff.ServerSideDryRunner { + return c.serverSideDryRunner } func (c *diffConfig) ServerSideDiff() bool { return c.serverSideDiff @@ -234,8 +234,8 @@ func (c *diffConfig) Validate() error { return fmt.Errorf("%s: StateCache must be set when retrieving from cache", msg) } } - if c.serverSideDiff && c.resourceOperations == nil { - return fmt.Errorf("%s: restConfig must be set when using server side diff", msg) + if c.serverSideDiff && c.serverSideDryRunner == nil { + return fmt.Errorf("%s: serverSideDryRunner must be set when using server side diff", msg) } return nil } @@ -279,7 +279,7 @@ func StateDiffs(lives, configs []*unstructured.Unstructured, diffConfig DiffConf diff.WithGVKParser(diffConfig.GVKParser()), diff.WithManager(diffConfig.Manager()), diff.WithServerSideDiff(diffConfig.ServerSideDiff()), - diff.WithKubeApplier(diffConfig.ResourceOperations()), + diff.WithServerSideDryRunner(diffConfig.ServerSideDryRunner()), } if diffConfig.Logger() != nil { @@ -363,7 +363,7 @@ func (c *diffConfig) DiffFromCache(appName string) (bool, []*v1alpha1.ResourceDi if c.stateCache != nil { err := c.stateCache.GetAppManagedResources(appName, &cachedDiff) if err != nil { - log.Errorf("DiffFromCache error: error getting managed resources: %s", err) + log.Errorf("DiffFromCache error: error getting managed resources for app %s: %s", appName, err) return false, nil } return true, cachedDiff From 4a24c5d1b376b288d57510ada465b426ceedae9c Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Wed, 13 Dec 2023 11:34:23 -0500 Subject: [PATCH 12/24] Address review comments Signed-off-by: Leonardo Luz Almeida --- cmd/argocd/commands/admin/app.go | 3 +-- controller/state.go | 6 ++++++ 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/cmd/argocd/commands/admin/app.go b/cmd/argocd/commands/admin/app.go index 53513e50c4012..096c92f9feb01 100644 --- a/cmd/argocd/commands/admin/app.go +++ b/cmd/argocd/commands/admin/app.go @@ -296,8 +296,7 @@ func NewReconcileCommand(clientOpts *argocdclient.ClientOptions) *cobra.Command command.Flags().StringVar(&selector, "l", "", "Label selector") command.Flags().StringVar(&outputFormat, "o", "yaml", "Output format (yaml|json)") command.Flags().BoolVar(&refresh, "refresh", false, "If set to true then recalculates apps reconciliation") - command.Flags().BoolVar(&serverSideDiff, "serverSideDiff", false, "If set to true will use serverSideDiff while comparing resources") - command.Flags().BoolVar(&serverSideDiff, "server-side-diff-enabled", false, "If set to \"true\" will use server-side diff while comparing resources. Default (\"false\")") + command.Flags().BoolVar(&serverSideDiff, "server-side-diff", false, "If set to \"true\" will use server-side diff while comparing resources. Default (\"false\")") return command } diff --git a/controller/state.go b/controller/state.go index 9f0141811c2f9..edf8f1efbe36e 100644 --- a/controller/state.go +++ b/controller/state.go @@ -588,6 +588,12 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 serverSideDiff := m.serverSideDiff || resourceutil.HasAnnotationOption(app, common.AnnotationCompareOptions, "ServerSideDiff=true") + // This allows turning SSD off for a given app if it is enabled at the + // controller level + if resourceutil.HasAnnotationOption(app, common.AnnotationCompareOptions, "ServerSideDiff=false") { + serverSideDiff = false + } + useDiffCache := useDiffCache(noCache, manifestInfos, sources, app, manifestRevisions, m.statusRefreshTimeout, serverSideDiff, logCtx) diffConfigBuilder := argodiff.NewDiffConfigBuilder(). From 0fa3955414ed5e36afb4fa416db414866b6beb88 Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Thu, 14 Dec 2023 16:12:23 -0500 Subject: [PATCH 13/24] docs: add docs related to server-side-diff Signed-off-by: Leonardo Luz Almeida --- docs/user-guide/diff-strategies.md | 86 ++++++++++++++++++++++++++++++ mkdocs.yml | 4 +- 2 files changed, 89 insertions(+), 1 deletion(-) create mode 100644 docs/user-guide/diff-strategies.md diff --git a/docs/user-guide/diff-strategies.md b/docs/user-guide/diff-strategies.md new file mode 100644 index 0000000000000..df124235c35fd --- /dev/null +++ b/docs/user-guide/diff-strategies.md @@ -0,0 +1,86 @@ +# Diff Strategies + +Argo CD calculates the diff between the desired state and the live +state in order to define if an Application is out-of-sync. This same +logic is also used in Argo CD UI to display the differences between +live and desired states for all resources belonging to an application. + +Argo CD currently has 3 different strategies to calculate diffs: + +- **Legacy**: This is the main diff strategy used by default. It + applies a 3-way diff based on live state, desired state and + last-applied-configuration (annotation). +- **Structured-Merge Diff**: Strategy automatically applied when + enabling Server-Side Apply sync option. +- **Server-Side Diff**: New strategy that invokes a Server-Side Apply + in dryrun mode delegating to Kube-API the action of calculating the + diff. + +## Structured-Merge Diff +*Current Status: [Beta][1] (Since v2.5.0)* + +This is diff strategy is automatically used when Server-Side Apply +sync option is enabled. It uses the [structured-merge-diff][2] library +used by kubernetes to calculate diffs based on fields ownership. There +are some challenges using this strategy to calculate diffs for CRDs +that define default values. After different issues were identified by +the community, this is strategy is being discontinued in favour of +Server-Side Diff. + +## Server-Side Diff +*Current Status: [Beta][1] (Since v2.10.0)* + +This diff strategy will execute a Server-Side Apply in dryrun mode for +each resource of the application. The response of this operation is then +compared with the live state in order to provide the diff results. The +diff results are cached and new Server-Side Apply requests to Kube API +are only triggered when: + +- An Application refresh or hard-refresh is requested. +- There is a new revision in the repo where Argo CD Application is + targeting. +- Argo CD Application spec changed. + +The advantages of Server-Side Diff is that + +### Enabling it + +Server-Side Diff can be enabled at the Argo CD Controller level or per +Application. + +**Enabling Server-Side Diff for all Applications** + +Add the following entry in the argocd-cmd-params-cm configmap: + +``` +controller.diff.server.side: "true" +``` + +**Enabling Server-Side Diff for one application** + +Add the following annotation in the Argo CD Application resource: + +``` +argocd.argoproj.io/compare-options: ServerSideDiff=true +``` + +### Mutation Webhooks + +Server-Side Diff will revert changes made by mutation webhooks by +default. If you want to include mutation webhooks in Argo CD diffs add +the following annotation in the Argo CD Application resource: + +``` +argocd.argoproj.io/compare-options: IncludeMutationWebhookInDiff=true +``` + +Note: This annoation is only effective with Server-Side Diff is +enabled. To enable both options for a given application add the +following annotation in the Argo CD Application resource: + +``` +argocd.argoproj.io/compare-options: ServerSideDiff=true,IncludeMutationWebhookInDiff=true +``` + +[1]: https://github.com/argoproj/argoproj/blob/main/community/feature-status.md#beta +[2]: https://github.com/kubernetes-sigs/structured-merge-diff diff --git a/mkdocs.yml b/mkdocs.yml index 4a58580e29619..d4c206a01c4d1 100644 --- a/mkdocs.yml +++ b/mkdocs.yml @@ -162,7 +162,9 @@ nav: - user-guide/multiple_sources.md - GnuPG verification: user-guide/gpg-verification.md - user-guide/auto_sync.md - - user-guide/diffing.md + - Diffing: + - Diff Strategies: user-guide/diff-strategies.md + - Diff Customization: user-guide/diffing.md - user-guide/orphaned-resources.md - user-guide/compare-options.md - user-guide/sync-options.md From bf9a6090d9c65235b0023e782453ff9f8c5cb43e Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Thu, 14 Dec 2023 16:29:15 -0500 Subject: [PATCH 14/24] docs: update doc Signed-off-by: Leonardo Luz Almeida --- docs/user-guide/diff-strategies.md | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/docs/user-guide/diff-strategies.md b/docs/user-guide/diff-strategies.md index df124235c35fd..3ab3babfb572f 100644 --- a/docs/user-guide/diff-strategies.md +++ b/docs/user-guide/diff-strategies.md @@ -41,7 +41,11 @@ are only triggered when: targeting. - Argo CD Application spec changed. -The advantages of Server-Side Diff is that +One advantage of Server-Side Diff is that Kubernetes Admission +Controllers will participate in the diff calculation. If for example +a validation webhook identifies a resource to be invalid, that will be +informed to Argo CD during the diff stage rather than during the sync +state. ### Enabling it From f3589ba88863da6aef7ef927810c20abcd599513 Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Thu, 14 Dec 2023 17:37:40 -0500 Subject: [PATCH 15/24] Add config to include mutation webhooks Signed-off-by: Leonardo Luz Almeida --- controller/state.go | 4 ++++ docs/user-guide/diff-strategies.md | 6 +++--- util/argo/diff/diff.go | 15 ++++++++++++++- 3 files changed, 21 insertions(+), 4 deletions(-) diff --git a/controller/state.go b/controller/state.go index edf8f1efbe36e..55ae7f1f14da7 100644 --- a/controller/state.go +++ b/controller/state.go @@ -606,6 +606,10 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *v1 diffConfigBuilder.WithNoCache() } + if resourceutil.HasAnnotationOption(app, common.AnnotationCompareOptions, "IncludeMutationWebhook=true") { + diffConfigBuilder.WithIgnoreMutationWebhook(false) + } + gvkParser, err := m.getGVKParser(app.Spec.Destination.Server) if err != nil { conditions = append(conditions, v1alpha1.ApplicationCondition{Type: v1alpha1.ApplicationConditionUnknownError, Message: err.Error(), LastTransitionTime: &now}) diff --git a/docs/user-guide/diff-strategies.md b/docs/user-guide/diff-strategies.md index 3ab3babfb572f..06859130683c4 100644 --- a/docs/user-guide/diff-strategies.md +++ b/docs/user-guide/diff-strategies.md @@ -75,15 +75,15 @@ default. If you want to include mutation webhooks in Argo CD diffs add the following annotation in the Argo CD Application resource: ``` -argocd.argoproj.io/compare-options: IncludeMutationWebhookInDiff=true +argocd.argoproj.io/compare-options: IncludeMutationWebhook=true ``` -Note: This annoation is only effective with Server-Side Diff is +Note: This annoation is only effective when Server-Side Diff is enabled. To enable both options for a given application add the following annotation in the Argo CD Application resource: ``` -argocd.argoproj.io/compare-options: ServerSideDiff=true,IncludeMutationWebhookInDiff=true +argocd.argoproj.io/compare-options: ServerSideDiff=true,IncludeMutationWebhook=true ``` [1]: https://github.com/argoproj/argoproj/blob/main/community/feature-status.md#beta diff --git a/util/argo/diff/diff.go b/util/argo/diff/diff.go index 55b6efadb283c..c99a04354c751 100644 --- a/util/argo/diff/diff.go +++ b/util/argo/diff/diff.go @@ -27,7 +27,9 @@ type DiffConfigBuilder struct { // NewDiffConfigBuilder create a new DiffConfigBuilder instance. func NewDiffConfigBuilder() *DiffConfigBuilder { return &DiffConfigBuilder{ - diffConfig: &diffConfig{}, + diffConfig: &diffConfig{ + ignoreMutationWebhook: true, + }, } } @@ -105,6 +107,11 @@ func (b *DiffConfigBuilder) WithServerSideDiff(ssd bool) *DiffConfigBuilder { return b } +func (b *DiffConfigBuilder) WithIgnoreMutationWebhook(m bool) *DiffConfigBuilder { + b.diffConfig.ignoreMutationWebhook = m + return b +} + // Build will first validate the current state of the diff config and return the // DiffConfig implementation if no errors are found. Will return nil and the error // details otherwise. @@ -153,6 +160,7 @@ type DiffConfig interface { ServerSideDiff() bool ServerSideDryRunner() diff.ServerSideDryRunner + IgnoreMutationWebhook() bool } // diffConfig defines the configurations used while applying diffs. @@ -171,6 +179,7 @@ type diffConfig struct { manager string serverSideDiff bool serverSideDryRunner diff.ServerSideDryRunner + ignoreMutationWebhook bool } func (c *diffConfig) Ignores() []v1alpha1.ResourceIgnoreDifferences { @@ -215,6 +224,9 @@ func (c *diffConfig) ServerSideDryRunner() diff.ServerSideDryRunner { func (c *diffConfig) ServerSideDiff() bool { return c.serverSideDiff } +func (c *diffConfig) IgnoreMutationWebhook() bool { + return c.ignoreMutationWebhook +} // Validate will check the current state of this diffConfig and return // error if it finds any required configuration missing. @@ -280,6 +292,7 @@ func StateDiffs(lives, configs []*unstructured.Unstructured, diffConfig DiffConf diff.WithManager(diffConfig.Manager()), diff.WithServerSideDiff(diffConfig.ServerSideDiff()), diff.WithServerSideDryRunner(diffConfig.ServerSideDryRunner()), + diff.WithIgnoreMutationWebhook(diffConfig.IgnoreMutationWebhook()), } if diffConfig.Logger() != nil { From bbb7343abba53f80f2a6cd4e95476b366c3a1e55 Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Fri, 15 Dec 2023 16:18:24 -0500 Subject: [PATCH 16/24] Address review comments Signed-off-by: Leonardo Luz Almeida --- Procfile | 2 +- docs/user-guide/diff-strategies.md | 50 ++++++++++++++++++++++++------ 2 files changed, 42 insertions(+), 10 deletions(-) diff --git a/Procfile b/Procfile index 2bb26a086fb1d..96916d10b84d6 100644 --- a/Procfile +++ b/Procfile @@ -1,4 +1,4 @@ -controller: [ "$BIN_MODE" = 'true' ] && COMMAND=./dist/argocd || COMMAND='go run ./cmd/main.go' && sh -c "FORCE_LOG_COLORS=1 ARGOCD_FAKE_IN_CLUSTER=true ARGOCD_TLS_DATA_PATH=${ARGOCD_TLS_DATA_PATH:-/tmp/argocd-local/tls} ARGOCD_SSH_DATA_PATH=${ARGOCD_SSH_DATA_PATH:-/tmp/argocd-local/ssh} ARGOCD_BINARY_NAME=argocd-application-controller $COMMAND --loglevel debug --redis localhost:${ARGOCD_E2E_REDIS_PORT:-6379} --repo-server localhost:${ARGOCD_E2E_REPOSERVER_PORT:-8081} --otlp-address=${ARGOCD_OTLP_ADDRESS} --application-namespaces=${ARGOCD_APPLICATION_NAMESPACES:-''}" +controller: [ "$BIN_MODE" = 'true' ] && COMMAND=./dist/argocd || COMMAND='go run ./cmd/main.go' && sh -c "FORCE_LOG_COLORS=1 ARGOCD_FAKE_IN_CLUSTER=true ARGOCD_TLS_DATA_PATH=${ARGOCD_TLS_DATA_PATH:-/tmp/argocd-local/tls} ARGOCD_SSH_DATA_PATH=${ARGOCD_SSH_DATA_PATH:-/tmp/argocd-local/ssh} ARGOCD_BINARY_NAME=argocd-application-controller $COMMAND --loglevel debug --redis localhost:${ARGOCD_E2E_REDIS_PORT:-6379} --repo-server localhost:${ARGOCD_E2E_REPOSERVER_PORT:-8081} --otlp-address=${ARGOCD_OTLP_ADDRESS} --application-namespaces=${ARGOCD_APPLICATION_NAMESPACES:-''} --server-side-diff-enabled=${ARGOCD_APPLICATION_CONTROLLER_SERVER_SIDE_DIFF:-''}" api-server: [ "$BIN_MODE" = 'true' ] && COMMAND=./dist/argocd || COMMAND='go run ./cmd/main.go' && sh -c "FORCE_LOG_COLORS=1 ARGOCD_FAKE_IN_CLUSTER=true ARGOCD_TLS_DATA_PATH=${ARGOCD_TLS_DATA_PATH:-/tmp/argocd-local/tls} ARGOCD_SSH_DATA_PATH=${ARGOCD_SSH_DATA_PATH:-/tmp/argocd-local/ssh} ARGOCD_BINARY_NAME=argocd-server $COMMAND --loglevel debug --redis localhost:${ARGOCD_E2E_REDIS_PORT:-6379} --disable-auth=${ARGOCD_E2E_DISABLE_AUTH:-'true'} --insecure --dex-server http://localhost:${ARGOCD_E2E_DEX_PORT:-5556} --repo-server localhost:${ARGOCD_E2E_REPOSERVER_PORT:-8081} --port ${ARGOCD_E2E_APISERVER_PORT:-8080} --otlp-address=${ARGOCD_OTLP_ADDRESS} --application-namespaces=${ARGOCD_APPLICATION_NAMESPACES:-''}" dex: sh -c "ARGOCD_BINARY_NAME=argocd-dex go run github.com/argoproj/argo-cd/v2/cmd gendexcfg -o `pwd`/dist/dex.yaml && (test -f dist/dex.yaml || { echo 'Failed to generate dex configuration'; exit 1; }) && docker run --rm -p ${ARGOCD_E2E_DEX_PORT:-5556}:${ARGOCD_E2E_DEX_PORT:-5556} -v `pwd`/dist/dex.yaml:/dex.yaml ghcr.io/dexidp/dex:$(grep "image: ghcr.io/dexidp/dex" manifests/base/dex/argocd-dex-server-deployment.yaml | cut -d':' -f3) dex serve /dex.yaml" redis: bash -c "if [ \"$ARGOCD_REDIS_LOCAL\" = 'true' ]; then redis-server --save '' --appendonly no --port ${ARGOCD_E2E_REDIS_PORT:-6379}; else docker run --rm --name argocd-redis -i -p ${ARGOCD_E2E_REDIS_PORT:-6379}:${ARGOCD_E2E_REDIS_PORT:-6379} docker.io/library/redis:$(grep "image: redis" manifests/base/redis/argocd-redis-deployment.yaml | cut -d':' -f3) --save '' --appendonly no --port ${ARGOCD_E2E_REDIS_PORT:-6379}; fi" diff --git a/docs/user-guide/diff-strategies.md b/docs/user-guide/diff-strategies.md index 06859130683c4..cd249f968a943 100644 --- a/docs/user-guide/diff-strategies.md +++ b/docs/user-guide/diff-strategies.md @@ -13,18 +13,17 @@ Argo CD currently has 3 different strategies to calculate diffs: - **Structured-Merge Diff**: Strategy automatically applied when enabling Server-Side Apply sync option. - **Server-Side Diff**: New strategy that invokes a Server-Side Apply - in dryrun mode delegating to Kube-API the action of calculating the - diff. + in dryrun mode in order to generate the predicted live state. ## Structured-Merge Diff *Current Status: [Beta][1] (Since v2.5.0)* This is diff strategy is automatically used when Server-Side Apply sync option is enabled. It uses the [structured-merge-diff][2] library -used by kubernetes to calculate diffs based on fields ownership. There +used by Kubernetes to calculate diffs based on fields ownership. There are some challenges using this strategy to calculate diffs for CRDs that define default values. After different issues were identified by -the community, this is strategy is being discontinued in favour of +the community, this strategy is being discontinued in favour of Server-Side Diff. ## Server-Side Diff @@ -37,7 +36,7 @@ diff results are cached and new Server-Side Apply requests to Kube API are only triggered when: - An Application refresh or hard-refresh is requested. -- There is a new revision in the repo where Argo CD Application is +- There is a new revision in the repo which the Argo CD Application is targeting. - Argo CD Application spec changed. @@ -65,17 +64,45 @@ controller.diff.server.side: "true" Add the following annotation in the Argo CD Application resource: ``` -argocd.argoproj.io/compare-options: ServerSideDiff=true +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + annotations: + argocd.argoproj.io/compare-options: ServerSideDiff=true +... ``` +**Disabling Server-Side Diff for one application** + +If Server-Side Diff is enabled globally in your Argo CD instance, it +is possible to disable it at the application level. In order to do so, +add the following annotation in the Application resource: + +``` +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + annotations: + argocd.argoproj.io/compare-options: ServerSideDiff=false +... +``` + +*Note: Please report any issues that forced you to disable the +Server-Side Diff feature* + ### Mutation Webhooks -Server-Side Diff will revert changes made by mutation webhooks by +Server-Side Diff does not include changes made by mutation webhooks by default. If you want to include mutation webhooks in Argo CD diffs add the following annotation in the Argo CD Application resource: ``` -argocd.argoproj.io/compare-options: IncludeMutationWebhook=true +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + annotations: + argocd.argoproj.io/compare-options: IncludeMutationWebhook=true +... ``` Note: This annoation is only effective when Server-Side Diff is @@ -83,7 +110,12 @@ enabled. To enable both options for a given application add the following annotation in the Argo CD Application resource: ``` -argocd.argoproj.io/compare-options: ServerSideDiff=true,IncludeMutationWebhook=true +apiVersion: argoproj.io/v1alpha1 +kind: Application +metadata: + annotations: + argocd.argoproj.io/compare-options: ServerSideDiff=true,IncludeMutationWebhook=true +... ``` [1]: https://github.com/argoproj/argoproj/blob/main/community/feature-status.md#beta From d1ddcece378ea07ec6e5b2af3912582aa2ffff12 Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Sun, 17 Dec 2023 15:06:26 -0500 Subject: [PATCH 17/24] go mod update Signed-off-by: Leonardo Luz Almeida --- go.mod | 2 +- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/go.mod b/go.mod index afed047ef1043..97d98f2a86f35 100644 --- a/go.mod +++ b/go.mod @@ -295,7 +295,7 @@ require ( replace ( // TODO (SSD): Remove this before merging // github.com/argoproj/gitops-engine => /Users/lalmeida1/dev/git/intuit/gitops-engine - github.com/argoproj/gitops-engine => github.com/leoluz/gitops-engine v0.4.1-0.20231208220525-c1e3999317ff + github.com/argoproj/gitops-engine => github.com/leoluz/gitops-engine v0.4.1-0.20231213210257-c4a60dce1e93 // https://github.com/golang/go/issues/33546#issuecomment-519656923 github.com/go-check/check => github.com/go-check/check v0.0.0-20180628173108-788fd7840127 diff --git a/go.sum b/go.sum index 8e42fc32c4076..f472dae05cfba 100644 --- a/go.sum +++ b/go.sum @@ -1355,8 +1355,8 @@ github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0 github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw= github.com/leodido/go-urn v1.2.0 h1:hpXL4XnriNwQ/ABnpepYM/1vCLWNDfUNts8dX3xTG6Y= github.com/leodido/go-urn v1.2.0/go.mod h1:+8+nEpDfqqsY+g338gtMEUOtuK+4dEMhiQEgxpxOKII= -github.com/leoluz/gitops-engine v0.4.1-0.20231208220525-c1e3999317ff h1:iL253TLkewFR7Ydt/NnC0S+1gWzmjx6oN2mYW4thxvI= -github.com/leoluz/gitops-engine v0.4.1-0.20231208220525-c1e3999317ff/go.mod h1:gWE8uROi7hIkWGNAVM+8FWkMfo0vZ03SLx/aFw/DBzg= +github.com/leoluz/gitops-engine v0.4.1-0.20231213210257-c4a60dce1e93 h1:SVGSimn24UZnAFWSuQw6HimgAjUTOav1xSQLCATw25Q= +github.com/leoluz/gitops-engine v0.4.1-0.20231213210257-c4a60dce1e93/go.mod h1:gWE8uROi7hIkWGNAVM+8FWkMfo0vZ03SLx/aFw/DBzg= github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de h1:9TO3cAIGXtEhnIaL+V+BEER86oLrvS+kWobKpbJuye0= github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de/go.mod h1:zAbeS9B/r2mtpb6U+EI2rYA5OAXxsYw6wTamcNW+zcE= github.com/lightstep/lightstep-tracer-common/golang/gogo v0.0.0-20190605223551-bc2310a04743/go.mod h1:qklhhLq1aX+mtWk9cPHPzaBjWImj5ULL6C7HFJtXQMM= From e5f22f860fa638f6b4096154b9122da62cdfdb3d Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Sun, 17 Dec 2023 15:33:47 -0500 Subject: [PATCH 18/24] Add sdd cache test case Signed-off-by: Leonardo Luz Almeida --- cmd/argocd/commands/admin/app_test.go | 1 + controller/appcontroller_test.go | 1 + controller/state_test.go | 11 +++++++++++ 3 files changed, 13 insertions(+) diff --git a/cmd/argocd/commands/admin/app_test.go b/cmd/argocd/commands/admin/app_test.go index 0cad2485e6696..a0284fe8ffa09 100644 --- a/cmd/argocd/commands/admin/app_test.go +++ b/cmd/argocd/commands/admin/app_test.go @@ -113,6 +113,7 @@ func TestGetReconcileResults_Refresh(t *testing.T) { func(argoDB db.ArgoDB, appInformer cache.SharedIndexInformer, settingsMgr *settings.SettingsManager, server *metrics.MetricsServer) statecache.LiveStateCache { return &liveStateCache }, + false, ) if !assert.NoError(t, err) { diff --git a/controller/appcontroller_test.go b/controller/appcontroller_test.go index 9c2933feeb6df..437a0de525708 100644 --- a/controller/appcontroller_test.go +++ b/controller/appcontroller_test.go @@ -133,6 +133,7 @@ func newFakeController(data *fakeData, repoErr error) *ApplicationController { nil, data.applicationNamespaces, nil, + false, ) if err != nil { panic(err) diff --git a/controller/state_test.go b/controller/state_test.go index d33a6fd120777..b7bfcc49a6ab9 100644 --- a/controller/state_test.go +++ b/controller/state_test.go @@ -1585,6 +1585,17 @@ func TestUseDiffCache(t *testing.T) { expectedUseCache: false, serverSideDiff: false, }, + { + testName: "will return true if status expired and server-side diff", + noCache: false, + manifestInfos: manifestInfos("rev1"), + sources: sources(), + app: app("httpbin", "rev1", false, nil), + manifestRevisions: []string{"rev1"}, + statusRefreshTimeout: time.Minute, + expectedUseCache: true, + serverSideDiff: false, + }, { testName: "will return false if there is a new revision", noCache: false, From af991951e6a717c5ab539ab16509060f64351951 Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Sun, 17 Dec 2023 15:35:33 -0500 Subject: [PATCH 19/24] fix ssd cache unit test Signed-off-by: Leonardo Luz Almeida --- controller/state_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/controller/state_test.go b/controller/state_test.go index b7bfcc49a6ab9..5d2342b601a77 100644 --- a/controller/state_test.go +++ b/controller/state_test.go @@ -1594,7 +1594,7 @@ func TestUseDiffCache(t *testing.T) { manifestRevisions: []string{"rev1"}, statusRefreshTimeout: time.Minute, expectedUseCache: true, - serverSideDiff: false, + serverSideDiff: true, }, { testName: "will return false if there is a new revision", From 27edfee2df2ef8df09586a2f72174c80176a4391 Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Sun, 17 Dec 2023 16:14:07 -0500 Subject: [PATCH 20/24] Update clidocs Signed-off-by: Leonardo Luz Almeida --- .../server-commands/argocd-application-controller.md | 1 + .../commands/argocd_admin_app_get-reconcile-results.md | 1 + 2 files changed, 2 insertions(+) diff --git a/docs/operator-manual/server-commands/argocd-application-controller.md b/docs/operator-manual/server-commands/argocd-application-controller.md index 434c30621b8bd..1d71fc6494445 100644 --- a/docs/operator-manual/server-commands/argocd-application-controller.md +++ b/docs/operator-manual/server-commands/argocd-application-controller.md @@ -67,6 +67,7 @@ argocd-application-controller [flags] --sentinel stringArray Redis sentinel hostname and port (e.g. argocd-redis-ha-announce-0:6379). --sentinelmaster string Redis sentinel master group name. (default "master") --server string The address and port of the Kubernetes API server + --server-side-diff-enabled Feature flag to enable ServerSide diff. Default ("false") --sharding-method string Enables choice of sharding method. Supported sharding methods are : [legacy, round-robin] (default "legacy") --status-processors int Number of application status processors (default 20) --tls-server-name string If provided, this name will be used to validate server certificate. If this is not provided, hostname used to contact the server is used. diff --git a/docs/user-guide/commands/argocd_admin_app_get-reconcile-results.md b/docs/user-guide/commands/argocd_admin_app_get-reconcile-results.md index 3290098999b7c..29fa5d54d9388 100644 --- a/docs/user-guide/commands/argocd_admin_app_get-reconcile-results.md +++ b/docs/user-guide/commands/argocd_admin_app_get-reconcile-results.md @@ -32,6 +32,7 @@ argocd admin app get-reconcile-results PATH [flags] --repo-server string Repo server address. --request-timeout string The length of time to wait before giving up on a single server request. Non-zero values should contain a corresponding time unit (e.g. 1s, 2m, 3h). A value of zero means don't timeout requests. (default "0") --server string The address and port of the Kubernetes API server + --server-side-diff If set to "true" will use server-side diff while comparing resources. Default ("false") --tls-server-name string If provided, this name will be used to validate server certificate. If this is not provided, hostname used to contact the server is used. --token string Bearer token for authentication to the API server --user string The name of the kubeconfig user to use From 20ecd85d453c93e63f090304662588d2ec748e3e Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Sun, 17 Dec 2023 16:29:13 -0500 Subject: [PATCH 21/24] update manifests Signed-off-by: Leonardo Luz Almeida --- manifests/core-install.yaml | 6 ++++++ manifests/ha/install.yaml | 6 ++++++ manifests/ha/namespace-install.yaml | 6 ++++++ manifests/install.yaml | 6 ++++++ manifests/namespace-install.yaml | 6 ++++++ 5 files changed, 30 insertions(+) diff --git a/manifests/core-install.yaml b/manifests/core-install.yaml index beda1e8a5103f..316db026ee944 100644 --- a/manifests/core-install.yaml +++ b/manifests/core-install.yaml @@ -21641,6 +21641,12 @@ spec: key: controller.k8sclient.retry.base.backoff name: argocd-cmd-params-cm optional: true + - name: ARGOCD_APPLICATION_CONTROLLER_SERVER_SIDE_DIFF + valueFrom: + configMapKeyRef: + key: controller.diff.server.side + name: argocd-cmd-params-cm + optional: true image: quay.io/argoproj/argocd:latest imagePullPolicy: Always name: argocd-application-controller diff --git a/manifests/ha/install.yaml b/manifests/ha/install.yaml index 50254b138d6ab..ebc3d6170717b 100644 --- a/manifests/ha/install.yaml +++ b/manifests/ha/install.yaml @@ -23468,6 +23468,12 @@ spec: key: controller.k8sclient.retry.base.backoff name: argocd-cmd-params-cm optional: true + - name: ARGOCD_APPLICATION_CONTROLLER_SERVER_SIDE_DIFF + valueFrom: + configMapKeyRef: + key: controller.diff.server.side + name: argocd-cmd-params-cm + optional: true image: quay.io/argoproj/argocd:latest imagePullPolicy: Always name: argocd-application-controller diff --git a/manifests/ha/namespace-install.yaml b/manifests/ha/namespace-install.yaml index 59aad0e49bda3..e219fccefe769 100644 --- a/manifests/ha/namespace-install.yaml +++ b/manifests/ha/namespace-install.yaml @@ -2855,6 +2855,12 @@ spec: key: controller.k8sclient.retry.base.backoff name: argocd-cmd-params-cm optional: true + - name: ARGOCD_APPLICATION_CONTROLLER_SERVER_SIDE_DIFF + valueFrom: + configMapKeyRef: + key: controller.diff.server.side + name: argocd-cmd-params-cm + optional: true image: quay.io/argoproj/argocd:latest imagePullPolicy: Always name: argocd-application-controller diff --git a/manifests/install.yaml b/manifests/install.yaml index 4fd267106eaf6..cfb02f5e40f4f 100644 --- a/manifests/install.yaml +++ b/manifests/install.yaml @@ -22512,6 +22512,12 @@ spec: key: controller.k8sclient.retry.base.backoff name: argocd-cmd-params-cm optional: true + - name: ARGOCD_APPLICATION_CONTROLLER_SERVER_SIDE_DIFF + valueFrom: + configMapKeyRef: + key: controller.diff.server.side + name: argocd-cmd-params-cm + optional: true image: quay.io/argoproj/argocd:latest imagePullPolicy: Always name: argocd-application-controller diff --git a/manifests/namespace-install.yaml b/manifests/namespace-install.yaml index fdd4eacd14efb..48f4e12870e97 100644 --- a/manifests/namespace-install.yaml +++ b/manifests/namespace-install.yaml @@ -1899,6 +1899,12 @@ spec: key: controller.k8sclient.retry.base.backoff name: argocd-cmd-params-cm optional: true + - name: ARGOCD_APPLICATION_CONTROLLER_SERVER_SIDE_DIFF + valueFrom: + configMapKeyRef: + key: controller.diff.server.side + name: argocd-cmd-params-cm + optional: true image: quay.io/argoproj/argocd:latest imagePullPolicy: Always name: argocd-application-controller From bdce1cd50916e6122369864ac2e3955a80ed3216 Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Mon, 18 Dec 2023 11:13:40 -0500 Subject: [PATCH 22/24] Fix procfile Signed-off-by: Leonardo Luz Almeida --- Procfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Procfile b/Procfile index 96916d10b84d6..a6a9c61068b78 100644 --- a/Procfile +++ b/Procfile @@ -1,4 +1,4 @@ -controller: [ "$BIN_MODE" = 'true' ] && COMMAND=./dist/argocd || COMMAND='go run ./cmd/main.go' && sh -c "FORCE_LOG_COLORS=1 ARGOCD_FAKE_IN_CLUSTER=true ARGOCD_TLS_DATA_PATH=${ARGOCD_TLS_DATA_PATH:-/tmp/argocd-local/tls} ARGOCD_SSH_DATA_PATH=${ARGOCD_SSH_DATA_PATH:-/tmp/argocd-local/ssh} ARGOCD_BINARY_NAME=argocd-application-controller $COMMAND --loglevel debug --redis localhost:${ARGOCD_E2E_REDIS_PORT:-6379} --repo-server localhost:${ARGOCD_E2E_REPOSERVER_PORT:-8081} --otlp-address=${ARGOCD_OTLP_ADDRESS} --application-namespaces=${ARGOCD_APPLICATION_NAMESPACES:-''} --server-side-diff-enabled=${ARGOCD_APPLICATION_CONTROLLER_SERVER_SIDE_DIFF:-''}" +controller: [ "$BIN_MODE" = 'true' ] && COMMAND=./dist/argocd || COMMAND='go run ./cmd/main.go' && sh -c "FORCE_LOG_COLORS=1 ARGOCD_FAKE_IN_CLUSTER=true ARGOCD_TLS_DATA_PATH=${ARGOCD_TLS_DATA_PATH:-/tmp/argocd-local/tls} ARGOCD_SSH_DATA_PATH=${ARGOCD_SSH_DATA_PATH:-/tmp/argocd-local/ssh} ARGOCD_BINARY_NAME=argocd-application-controller $COMMAND --loglevel debug --redis localhost:${ARGOCD_E2E_REDIS_PORT:-6379} --repo-server localhost:${ARGOCD_E2E_REPOSERVER_PORT:-8081} --otlp-address=${ARGOCD_OTLP_ADDRESS} --application-namespaces=${ARGOCD_APPLICATION_NAMESPACES:-''} --server-side-diff-enabled=${ARGOCD_APPLICATION_CONTROLLER_SERVER_SIDE_DIFF:-'false'}" api-server: [ "$BIN_MODE" = 'true' ] && COMMAND=./dist/argocd || COMMAND='go run ./cmd/main.go' && sh -c "FORCE_LOG_COLORS=1 ARGOCD_FAKE_IN_CLUSTER=true ARGOCD_TLS_DATA_PATH=${ARGOCD_TLS_DATA_PATH:-/tmp/argocd-local/tls} ARGOCD_SSH_DATA_PATH=${ARGOCD_SSH_DATA_PATH:-/tmp/argocd-local/ssh} ARGOCD_BINARY_NAME=argocd-server $COMMAND --loglevel debug --redis localhost:${ARGOCD_E2E_REDIS_PORT:-6379} --disable-auth=${ARGOCD_E2E_DISABLE_AUTH:-'true'} --insecure --dex-server http://localhost:${ARGOCD_E2E_DEX_PORT:-5556} --repo-server localhost:${ARGOCD_E2E_REPOSERVER_PORT:-8081} --port ${ARGOCD_E2E_APISERVER_PORT:-8080} --otlp-address=${ARGOCD_OTLP_ADDRESS} --application-namespaces=${ARGOCD_APPLICATION_NAMESPACES:-''}" dex: sh -c "ARGOCD_BINARY_NAME=argocd-dex go run github.com/argoproj/argo-cd/v2/cmd gendexcfg -o `pwd`/dist/dex.yaml && (test -f dist/dex.yaml || { echo 'Failed to generate dex configuration'; exit 1; }) && docker run --rm -p ${ARGOCD_E2E_DEX_PORT:-5556}:${ARGOCD_E2E_DEX_PORT:-5556} -v `pwd`/dist/dex.yaml:/dex.yaml ghcr.io/dexidp/dex:$(grep "image: ghcr.io/dexidp/dex" manifests/base/dex/argocd-dex-server-deployment.yaml | cut -d':' -f3) dex serve /dex.yaml" redis: bash -c "if [ \"$ARGOCD_REDIS_LOCAL\" = 'true' ]; then redis-server --save '' --appendonly no --port ${ARGOCD_E2E_REDIS_PORT:-6379}; else docker run --rm --name argocd-redis -i -p ${ARGOCD_E2E_REDIS_PORT:-6379}:${ARGOCD_E2E_REDIS_PORT:-6379} docker.io/library/redis:$(grep "image: redis" manifests/base/redis/argocd-redis-deployment.yaml | cut -d':' -f3) --save '' --appendonly no --port ${ARGOCD_E2E_REDIS_PORT:-6379}; fi" From cb37c193a6c6ba318918f9cbad44b301fa16961f Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Mon, 18 Dec 2023 13:17:54 -0500 Subject: [PATCH 23/24] additional doc changes Signed-off-by: Leonardo Luz Almeida --- docs/user-guide/diff-strategies.md | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/docs/user-guide/diff-strategies.md b/docs/user-guide/diff-strategies.md index cd249f968a943..a7b3216fa7ec7 100644 --- a/docs/user-guide/diff-strategies.md +++ b/docs/user-guide/diff-strategies.md @@ -38,13 +38,13 @@ are only triggered when: - An Application refresh or hard-refresh is requested. - There is a new revision in the repo which the Argo CD Application is targeting. -- Argo CD Application spec changed. +- The Argo CD Application spec changed. One advantage of Server-Side Diff is that Kubernetes Admission Controllers will participate in the diff calculation. If for example a validation webhook identifies a resource to be invalid, that will be informed to Argo CD during the diff stage rather than during the sync -state. +stage. ### Enabling it @@ -59,6 +59,9 @@ Add the following entry in the argocd-cmd-params-cm configmap: controller.diff.server.side: "true" ``` +Note: It is necessary to restart the `argocd-application-controller` +after applying this configuration. + **Enabling Server-Side Diff for one application** Add the following annotation in the Argo CD Application resource: From 579b1ed8217d0e826cb281d002273aaeabb8f9e9 Mon Sep 17 00:00:00 2001 From: Leonardo Luz Almeida Date: Mon, 18 Dec 2023 14:48:01 -0500 Subject: [PATCH 24/24] update gitops-engine version Signed-off-by: Leonardo Luz Almeida --- go.mod | 6 +----- go.sum | 4 ++-- 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/go.mod b/go.mod index 97d98f2a86f35..80cb0ef95375a 100644 --- a/go.mod +++ b/go.mod @@ -13,7 +13,7 @@ require ( github.com/TomOnTime/utfutil v0.0.0-20180511104225-09c41003ee1d github.com/alicebob/miniredis/v2 v2.30.4 github.com/antonmedv/expr v1.15.2 - github.com/argoproj/gitops-engine v0.7.1-0.20231102154024-c0c2dd1f6f48 + github.com/argoproj/gitops-engine v0.7.1-0.20231218194513-aba38192fb16 github.com/argoproj/notifications-engine v0.4.1-0.20231027194313-a8d185ecc0a9 github.com/argoproj/pkg v0.13.7-0.20230626144333-d56162821bd1 github.com/aws/aws-sdk-go v1.44.317 @@ -293,10 +293,6 @@ require ( ) replace ( - // TODO (SSD): Remove this before merging - // github.com/argoproj/gitops-engine => /Users/lalmeida1/dev/git/intuit/gitops-engine - github.com/argoproj/gitops-engine => github.com/leoluz/gitops-engine v0.4.1-0.20231213210257-c4a60dce1e93 - // https://github.com/golang/go/issues/33546#issuecomment-519656923 github.com/go-check/check => github.com/go-check/check v0.0.0-20180628173108-788fd7840127 diff --git a/go.sum b/go.sum index f472dae05cfba..c70791965dbd5 100644 --- a/go.sum +++ b/go.sum @@ -696,6 +696,8 @@ github.com/apache/thrift v0.12.0/go.mod h1:cp2SuWMxlEZw2r+iP2GNCdIi4C1qmUzdZFSVb github.com/apache/thrift v0.13.0/go.mod h1:cp2SuWMxlEZw2r+iP2GNCdIi4C1qmUzdZFSVb+bacwQ= github.com/apache/thrift v0.16.0/go.mod h1:PHK3hniurgQaNMZYaCLEqXKsYK8upmhPbmdP2FXSqgU= github.com/appscode/go v0.0.0-20191119085241-0887d8ec2ecc/go.mod h1:OawnOmAL4ZX3YaPdN+8HTNwBveT1jMsqP74moa9XUbE= +github.com/argoproj/gitops-engine v0.7.1-0.20231218194513-aba38192fb16 h1:kR15L8UsSVr7oitABKU88msQirMT0/RO/KRla1jkq/s= +github.com/argoproj/gitops-engine v0.7.1-0.20231218194513-aba38192fb16/go.mod h1:gWE8uROi7hIkWGNAVM+8FWkMfo0vZ03SLx/aFw/DBzg= github.com/argoproj/notifications-engine v0.4.1-0.20231027194313-a8d185ecc0a9 h1:1lt0VXzmLK7Vv0kaeal3S6/JIfzPyBORkUWXhiqF3l0= github.com/argoproj/notifications-engine v0.4.1-0.20231027194313-a8d185ecc0a9/go.mod h1:E/vv4+by868m0mmflaRfGBmKBtAupoF+mmyfekP8QCk= github.com/argoproj/pkg v0.13.7-0.20230626144333-d56162821bd1 h1:qsHwwOJ21K2Ao0xPju1sNuqphyMnMYkyB3ZLoLtxWpo= @@ -1355,8 +1357,6 @@ github.com/kylelemons/godebug v1.1.0 h1:RPNrshWIDI6G2gRW9EHilWtl7Z6Sb1BR0xunSBf0 github.com/kylelemons/godebug v1.1.0/go.mod h1:9/0rRGxNHcop5bhtWyNeEfOS8JIWk580+fNqagV/RAw= github.com/leodido/go-urn v1.2.0 h1:hpXL4XnriNwQ/ABnpepYM/1vCLWNDfUNts8dX3xTG6Y= github.com/leodido/go-urn v1.2.0/go.mod h1:+8+nEpDfqqsY+g338gtMEUOtuK+4dEMhiQEgxpxOKII= -github.com/leoluz/gitops-engine v0.4.1-0.20231213210257-c4a60dce1e93 h1:SVGSimn24UZnAFWSuQw6HimgAjUTOav1xSQLCATw25Q= -github.com/leoluz/gitops-engine v0.4.1-0.20231213210257-c4a60dce1e93/go.mod h1:gWE8uROi7hIkWGNAVM+8FWkMfo0vZ03SLx/aFw/DBzg= github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de h1:9TO3cAIGXtEhnIaL+V+BEER86oLrvS+kWobKpbJuye0= github.com/liggitt/tabwriter v0.0.0-20181228230101-89fcab3d43de/go.mod h1:zAbeS9B/r2mtpb6U+EI2rYA5OAXxsYw6wTamcNW+zcE= github.com/lightstep/lightstep-tracer-common/golang/gogo v0.0.0-20190605223551-bc2310a04743/go.mod h1:qklhhLq1aX+mtWk9cPHPzaBjWImj5ULL6C7HFJtXQMM=