fix: requeue if failed to trigger an un-instrument rollout (#2406)

odigos-io · Feb 8, 2025 · fb4fa7f · fb4fa7f
1 parent 5cee798
commit fb4fa7f
Show file tree

Hide file tree

Showing 3 changed files with 37 additions and 70 deletions.
diff --git a/instrumentor/controllers/agentenabled/rollout/rollout.go b/instrumentor/controllers/agentenabled/rollout/rollout.go
@@ -42,11 +42,9 @@ func Do(ctx context.Context, c client.Client, ic *odigosv1alpha1.Instrumentation
 	if ic == nil {
 		// instrumentation config is deleted, trigger a rollout for the associated workload
 		// this should happen once per workload, as the instrumentation config is deleted
+		// and we want to rollout the workload to remove the instrumentation
 		rolloutErr := rolloutRestartWorkload(ctx, workloadObj, c, time.Now())
-		if rolloutErr != nil {
-			logger.Error(rolloutErr, "error rolling out workload", "name", pw.Name, "namespace", pw.Namespace)
-		}
-		return ctrl.Result{}, nil
+		return ctrl.Result{}, client.IgnoreNotFound(rolloutErr)
 	}
 
 	savedRolloutHash := ic.Status.WorkloadRolloutHash

diff --git a/tests/e2e/source/chainsaw-test.yaml b/tests/e2e/source/chainsaw-test.yaml
@@ -94,12 +94,7 @@ spec:
             file: 01-workloads.yaml
         - script:
             timeout: 70s
-            content: |
-              kubectl rollout status deployment -l app=coupon
-              kubectl rollout status deployment -l app=frontend
-              kubectl rollout status deployment -l app=inventory
-              kubectl rollout status deployment -l app=pricing
-              kubectl rollout status deployment -l app=membership
+            content: ./wait_for_rollout.sh
     - name: Generate Traffic
       try:
         - script:
@@ -181,12 +176,7 @@ spec:
       try:
         - script:
             timeout: 70s
-            content: |
-              kubectl rollout status deployment -l app=coupon
-              kubectl rollout status deployment -l app=frontend
-              kubectl rollout status deployment -l app=inventory
-              kubectl rollout status deployment -l app=pricing
-              kubectl rollout status deployment -l app=membership
+            content: ./wait_for_rollout.sh
     - name: Instrument Namespace
       try:
         - apply:
@@ -205,12 +195,7 @@ spec:
             file: ../../common/assert/simple-demo-instrumented.yaml
         - script:
             timeout: 70s
-            content: |
-              kubectl rollout status deployment -l app=coupon
-              kubectl rollout status deployment -l app=frontend
-              kubectl rollout status deployment -l app=inventory
-              kubectl rollout status deployment -l app=pricing
-              kubectl rollout status deployment -l app=membership
+            content: ./wait_for_rollout.sh
     - name: Generate Traffic
       try:
         - script:
@@ -290,12 +275,7 @@ spec:
             file: 04-workloads.yaml
         - script:
             timeout: 70s
-            content: |
-              kubectl rollout status deployment -l app=coupon
-              kubectl rollout status deployment -l app=frontend
-              kubectl rollout status deployment -l app=inventory
-              kubectl rollout status deployment -l app=pricing
-              kubectl rollout status deployment -l app=membership
+            content: ./wait_for_rollout.sh
     - name: Instrument frontend workload specifically
       try:
         - apply:
@@ -312,16 +292,16 @@ spec:
       try:
         - script:
             timeout: 70s
-            content: |
-              kubectl rollout status deployment -l app=coupon
-              kubectl rollout status deployment -l app=frontend
-              kubectl rollout status deployment -l app=inventory
-              kubectl rollout status deployment -l app=pricing
-              kubectl rollout status deployment -l app=membership
+            content: ./wait_for_rollout.sh
     - name: Instrument rest of Namespace
       try:
         - apply:
             file: ../../common/apply/instrument-default-ns.yaml
+    - name: Wait for workloads to roll out new revisions
+      try:
+        - script:
+            timeout: 70s
+            content: ./wait_for_rollout.sh
     - name: Assert Runtime Detected for all workloads
       try:
         - assert:
@@ -347,12 +327,7 @@ spec:
       try:
         - script:
             timeout: 70s
-            content: |
-              kubectl rollout status deployment -l app=coupon
-              kubectl rollout status deployment -l app=frontend
-              kubectl rollout status deployment -l app=inventory
-              kubectl rollout status deployment -l app=pricing
-              kubectl rollout status deployment -l app=membership
+            content: ./wait_for_rollout.sh
     - name: Assert Runtime still Detected for single workload
       try:
         - assert:
@@ -373,12 +348,7 @@ spec:
       try:
         - script:
             timeout: 70s
-            content: |
-              kubectl rollout status deployment -l app=coupon
-              kubectl rollout status deployment -l app=frontend
-              kubectl rollout status deployment -l app=inventory
-              kubectl rollout status deployment -l app=pricing
-              kubectl rollout status deployment -l app=membership
+            content: ./wait_for_rollout.sh
     - name: Assert runtime detected for all workloads except excluded (coupon)
       try:
         - assert:
@@ -403,12 +373,7 @@ spec:
       try:
         - script:
             timeout: 70s
-            content: |
-              kubectl rollout status deployment -l app=coupon
-              kubectl rollout status deployment -l app=frontend
-              kubectl rollout status deployment -l app=inventory
-              kubectl rollout status deployment -l app=pricing
-              kubectl rollout status deployment -l app=membership
+            content: ./wait_for_rollout.sh
     - name: Assert runtime detected for no-longer-excluded workload
       try:
         - assert:
@@ -425,12 +390,7 @@ spec:
       try:
         - script:
             timeout: 70s
-            content: |
-              kubectl rollout status deployment -l app=coupon
-              kubectl rollout status deployment -l app=frontend
-              kubectl rollout status deployment -l app=inventory
-              kubectl rollout status deployment -l app=pricing
-              kubectl rollout status deployment -l app=membership
+            content: ./wait_for_rollout.sh
     - name: Assert runtime detected for all workloads except newly excluded (membership)
       try:
         - assert:
@@ -454,12 +414,7 @@ spec:
       try:
         - script:
             timeout: 70s
-            content: |
-              kubectl rollout status deployment -l app=coupon
-              kubectl rollout status deployment -l app=frontend
-              kubectl rollout status deployment -l app=inventory
-              kubectl rollout status deployment -l app=pricing
-              kubectl rollout status deployment -l app=membership
+            content: ./wait_for_rollout.sh
     - name: Assert runtime detected for no-longer-excluded workload
       try:
         - assert:
@@ -477,12 +432,7 @@ spec:
       try:
         - script:
             timeout: 70s
-            content: |
-              kubectl rollout status deployment -l app=coupon
-              kubectl rollout status deployment -l app=frontend
-              kubectl rollout status deployment -l app=inventory
-              kubectl rollout status deployment -l app=pricing
-              kubectl rollout status deployment -l app=membership
+            content: ./wait_for_rollout.sh
     - name: Assert runtime not detected for re-excluded (membership)
       try:
         - script:

diff --git a/tests/e2e/source/wait_for_rollout.sh b/tests/e2e/source/wait_for_rollout.sh
@@ -0,0 +1,19 @@
+#!/bin/bash
+
+# some of the rollouts during the test are triggered by patching an annotation on the workload.
+# the annotation value is a timestamp. This timestamp has a resolution of seconds, so we need to wait
+# at least 1 second before we can be sure that the rollout has been triggered.
+#
+# The controller itself could check this timestamp and wait for the next second to trigger the rollout,
+# however for now we decided to not implement it as it seems extremely unlikely that this would happen in a real-world scenario.
+#
+# this is also consistent with the behavior of `kubectl rollout restart deployment ...` which also has a resolution of seconds.
+# trying to do a few consecutive `kubectl rollout restart deployment ...` may result in an error such as
+# "if restart has already been triggered within the past second, please wait before attempting to trigger another"
+sleep 1
+
+kubectl rollout status deployment -l app=coupon
+kubectl rollout status deployment -l app=frontend
+kubectl rollout status deployment -l app=inventory
+kubectl rollout status deployment -l app=pricing
+kubectl rollout status deployment -l app=membership