Skip to content

Commit

Permalink
feat: Prune resources in reverse order of syncwave during sync (argop…
Browse files Browse the repository at this point in the history
…roj#15074) (argoproj#16748)

* Add e2e & doc for prune order during sync

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Point gitops-engine to fork with reverse prune changes

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Fix ci linting failures

Signed-off-by: Siddhesh Ghadi <[email protected]>

* Update gitops-engine commit ref

Signed-off-by: Siddhesh Ghadi <[email protected]>

---------

Signed-off-by: Siddhesh Ghadi <[email protected]>
  • Loading branch information
svghadi authored and Julien Fuix committed Feb 6, 2024
1 parent 2365392 commit d6ba047
Show file tree
Hide file tree
Showing 7 changed files with 144 additions and 4 deletions.
4 changes: 3 additions & 1 deletion docs/user-guide/sync-waves.md
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ Hooks and resources are assigned to wave zero by default. The wave can be negati
When Argo CD starts a sync, it orders the resources in the following precedence:
* The phase
* The wave they are in (lower values first)
* The wave they are in (lower values first for creation & updation and higher values first for deletion)
* By kind (e.g. [namespaces first and then other Kubernetes resources, followed by custom resources](https://github.com/argoproj/gitops-engine/blob/bc9ce5764fa306f58cf59199a94f6c968c775a2d/pkg/sync/sync_tasks.go#L27-L66))
* By name
Expand All @@ -49,6 +49,8 @@ It repeats this process until all phases and waves are in-sync and healthy.
Because an application can have resources that are unhealthy in the first wave, it may be that the app can never get to healthy.
During pruning of resources, resources from higher waves are processed first before moving to lower waves. If, for any reason, a resource isn't removed/pruned in a wave, the resources in next waves won't be processed. This is to ensure proper resource cleanup between waves.
Note that there's currently a delay between each sync wave in order give other controllers a chance to react to the spec change
that we just applied. This also prevent Argo CD from assessing resource health too quickly (against the stale object), causing
hooks to fire prematurely. The current delay between each sync wave is 2 seconds and can be configured via environment
Expand Down
2 changes: 1 addition & 1 deletion go.mod
Original file line number Diff line number Diff line change
Expand Up @@ -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.20240122213038-792124280fcc
github.com/argoproj/gitops-engine v0.7.1-0.20240124052710-5fd9f449e757
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
Expand Down
4 changes: 2 additions & 2 deletions go.sum
Original file line number Diff line number Diff line change
Expand Up @@ -694,8 +694,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.20240122213038-792124280fcc h1:Fv94Mi2WvtvPkEH5WoWC3iy/VoQRLeSsE0hyg0n2UkY=
github.com/argoproj/gitops-engine v0.7.1-0.20240122213038-792124280fcc/go.mod h1:gWE8uROi7hIkWGNAVM+8FWkMfo0vZ03SLx/aFw/DBzg=
github.com/argoproj/gitops-engine v0.7.1-0.20240124052710-5fd9f449e757 h1:5fKAhTQcTBom0vin56cz/UTPx2GMuvdb+lJRAUOPbHA=
github.com/argoproj/gitops-engine v0.7.1-0.20240124052710-5fd9f449e757/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=
Expand Down
45 changes: 45 additions & 0 deletions test/e2e/sync_waves_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,8 @@ import (

"github.com/argoproj/gitops-engine/pkg/health"
. "github.com/argoproj/gitops-engine/pkg/sync/common"

v1 "k8s.io/api/core/v1"
)

func TestFixingDegradedApp(t *testing.T) {
Expand Down Expand Up @@ -100,3 +102,46 @@ func TestDegradedDeploymentIsSucceededAndSynced(t *testing.T) {
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(ResourceResultNumbering(1))
}

// resources should be pruned in reverse of creation order(syncwaves order)
func TestSyncPruneOrderWithSyncWaves(t *testing.T) {
ctx := Given(t).Timeout(60)

// remove finalizer to ensure proper cleanup if test fails at early stage
defer func() {
_, _ = RunCli("app", "patch-resource", ctx.AppQualifiedName(),
"--kind", "Pod",
"--resource-name", "pod-with-finalizers",
"--patch", `[{"op": "remove", "path": "/metadata/finalizers"}]`,
"--patch-type", "application/json-patch+json", "--all",
)
}()

ctx.Path("syncwaves-prune-order").
When().
CreateApp().
// creation order: sa & role -> rolebinding -> pod
Sync().
Wait().
Then().
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy)).
When().
// delete files to remove resources
DeleteFile("pod.yaml").
DeleteFile("rbac.yaml").
Refresh(RefreshTypeHard).
IgnoreErrors().
Then().
Expect(SyncStatusIs(SyncStatusCodeOutOfSync)).
When().
// prune order: pod -> rolebinding -> sa & role
Sync("--prune").
Wait().
Then().
Expect(OperationPhaseIs(OperationSucceeded)).
Expect(SyncStatusIs(SyncStatusCodeSynced)).
Expect(HealthIs(health.HealthStatusHealthy)).
Expect(NotPod(func(p v1.Pod) bool { return p.Name == "pod-with-finalizers" })).
Expect(ResourceResultNumbering(4))
}
15 changes: 15 additions & 0 deletions test/e2e/testdata/syncwaves-prune-order/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
## Test Scenario

This test example is for testing the reverse pruning of resources with syncwaves during sync operation.

Resource creation happens in below order
- wave 0: sa & role
- wave 1: rolebinding
- wave 2: pod

They are setup in such a way that the resources will be cleaned up properly only if they are deleted in the reverse order of creation i.e
- wave 0: pod
- wave 1: rolebinding
- wave 2: sa & role

If above delete order is not followed the pod gets stuck in terminating state due to a finalizer which is supposed to be removed by k8s container lifecycle hook on delete if delete order is correct.
41 changes: 41 additions & 0 deletions test/e2e/testdata/syncwaves-prune-order/pod.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
apiVersion: v1
kind: Pod
metadata:
name: pod-with-finalizers
annotations:
argocd.argoproj.io/sync-wave: "2"
# remove this finalizers using container preStop lifecycle hook on delete
finalizers:
- example.com/block-delete
spec:
serviceAccountName: modify-pods-sa # sa with permissions to modify pods
terminationGracePeriodSeconds: 15
containers:
- name: container
image: nginx:alpine
command: ["/bin/sh", "-c"]
args: ["sleep 10h"]
env:
- name: POD_NAME
valueFrom:
fieldRef:
fieldPath: metadata.name
- name: NAMESPACE
valueFrom:
fieldRef:
fieldPath: metadata.namespace
lifecycle:
# remove finalizers for successful delete of pod
preStop:
exec:
command:
- /bin/sh
- -c
- |
set -e
SERVICE_ACCOUNT_TOKEN=$(cat /var/run/secrets/kubernetes.io/serviceaccount/token)
POD_URL="https://kubernetes.default.svc/api/v1/namespaces/$NAMESPACE/pods/$POD_NAME"
PATCH_PAYLOAD='[{"op": "remove", "path": "/metadata/finalizers"}]'
curl -k -v -H "Authorization: Bearer $SERVICE_ACCOUNT_TOKEN" -H "Content-Type: application/json-patch+json" -X PATCH --data "$PATCH_PAYLOAD" $POD_URL
37 changes: 37 additions & 0 deletions test/e2e/testdata/syncwaves-prune-order/rbac.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
apiVersion: v1
kind: ServiceAccount
metadata:
name: modify-pods-sa
annotations:
argocd.argoproj.io/sync-wave: "0"
---
apiVersion: rbac.authorization.k8s.io/v1
kind: Role
metadata:
name: modify-pods-role
annotations:
argocd.argoproj.io/sync-wave: "0"
rules:
- apiGroups: [""]
resources:
- pods
verbs:
- get
- list
- delete
- update
- patch
---
apiVersion: rbac.authorization.k8s.io/v1
kind: RoleBinding
metadata:
name: modify-pods-rolebinding
annotations:
argocd.argoproj.io/sync-wave: "1"
subjects:
- kind: ServiceAccount
name: modify-pods-sa
roleRef:
kind: Role
name: modify-pods-role
apiGroup: rbac.authorization.k8s.io

0 comments on commit d6ba047

Please sign in to comment.