fix: rollout health could incorrectly report v0.9 rollouts as Progres…

…sing (#4949) Signed-off-by: Jesse Suen <Jesse_Suen@intuit.com>
argoproj · Dec 2, 2020 · f479639 · f479639
1 parent 0f0b6ce
commit f479639
Show file tree

Hide file tree

Showing 5 changed files with 81 additions and 9 deletions.
diff --git a/resource_customizations/argoproj.io/Rollout/actions/discovery.lua b/resource_customizations/argoproj.io/Rollout/actions/discovery.lua
@@ -16,7 +16,7 @@ actions["retry"] = {["disabled"] = fullyPromoted or not(obj.status.abort)}
 actions["promote-full"] = {["disabled"] = true}
 if obj.status ~= nil and not(fullyPromoted) then
     generation = tonumber(obj.status.observedGeneration)
-    if generation == nil then
+    if generation == nil or generation > obj.metadata.generation then
         -- rollouts v0.9 - full promotion only supported for canary
         actions["promote-full"] = {["disabled"] = obj.spec.strategy.blueGreen ~= nil}
     else

diff --git a/resource_customizations/argoproj.io/Rollout/actions/promote-full/action.lua b/resource_customizations/argoproj.io/Rollout/actions/promote-full/action.lua
@@ -1,6 +1,6 @@
 if obj.status ~= nil then
     generation = tonumber(obj.status.observedGeneration)
-    if generation == nil then
+    if generation == nil or generation > obj.metadata.generation then
         -- rollouts v0.9 and below
         obj.status.abort = nil
         if obj.spec.strategy.canary.steps ~= nil then

diff --git a/resource_customizations/argoproj.io/Rollout/health.lua b/resource_customizations/argoproj.io/Rollout/health.lua
@@ -51,15 +51,23 @@ function checkPaused(obj)
   return nil
 end
 
-hs = {}
-if obj.status == nil or obj.status.observedGeneration == nil then
-  hs.status = "Progressing"
-  hs.message = "Waiting for rollout spec update to be observed"
-  return hs
+-- isGenerationObserved determines if the rollout spec has been observed by the controller. This
+-- only applies to v0.10 rollout which uses a numeric status.observedGeneration. For v0.9 rollouts
+-- and below this function always returns true.
+function isGenerationObserved(obj)
+  if obj.status == nil then
+    return false
+  end
+  observedGeneration = tonumber(obj.status.observedGeneration)
+  if observedGeneration == nil or observedGeneration > obj.metadata.generation then
+    -- if we get here, the rollout is a v0.9 rollout
+    return true
+  end
+  return observedGeneration == obj.metadata.generation
 end
 
-generation = tonumber(obj.status.observedGeneration)
-if generation ~= nil and generation ~= obj.metadata.generation then
+hs = {}
+if not isGenerationObserved(obj) then
   hs.status = "Progressing"
   hs.message = "Waiting for rollout spec update to be observed"
   return hs

diff --git a/resource_customizations/argoproj.io/Rollout/health_test.yaml b/resource_customizations/argoproj.io/Rollout/health_test.yaml
@@ -11,6 +11,10 @@ tests:
     status: Healthy
     message: ""
   inputPath: testdata/healthy_legacy_v0.9_observedGeneration.yaml
+- healthStatus:
+    status: Healthy
+    message: ""
+  inputPath: testdata/healthy_legacy_v0.9_observedGeneration_numeric.yaml
 - healthStatus:
     status: Degraded
     message: "The Rollout \"basic\" is invalid: spec.strategy.strategy: Required value: Rollout has missing field '.spec.strategy.canary or .spec.strategy.blueGreen'"

diff --git a/...izations/argoproj.io/Rollout/testdata/healthy_legacy_v0.9_observedGeneration_numeric.yaml b/...izations/argoproj.io/Rollout/testdata/healthy_legacy_v0.9_observedGeneration_numeric.yaml
@@ -0,0 +1,60 @@
+apiVersion: argoproj.io/v1alpha1
+kind: Rollout
+metadata:
+  creationTimestamp: "2020-11-13T00:44:55Z"
+  generation: 1
+  name: basic
+  namespace: argocd-e2e
+  resourceVersion: "182108"
+  selfLink: /apis/argoproj.io/v1alpha1/namespaces/argocd-e2e/rollouts/basic
+  uid: 34e4bbfc-222c-4968-bd60-2b30ae81110d
+spec:
+  replicas: 1
+  selector:
+    matchLabels:
+      app: basic
+  strategy:
+    canary:
+      steps:
+      - setWeight: 50
+      - pause: {}
+  template:
+    metadata:
+      creationTimestamp: null
+      labels:
+        app: basic
+    spec:
+      containers:
+      - image: nginx:1.19-alpine
+        name: basic
+        resources:
+          requests:
+            cpu: 1m
+            memory: 16Mi
+status:
+  HPAReplicas: 1
+  availableReplicas: 1
+  blueGreen: {}
+  canary: {}
+  conditions:
+  - lastTransitionTime: "2020-11-13T00:48:20Z"
+    lastUpdateTime: "2020-11-13T00:48:22Z"
+    message: ReplicaSet "basic-754cb84d5" has successfully progressed.
+    reason: NewReplicaSetAvailable
+    status: "True"
+    type: Progressing
+  - lastTransitionTime: "2020-11-13T00:48:22Z"
+    lastUpdateTime: "2020-11-13T00:48:22Z"
+    message: Rollout has minimum availability
+    reason: AvailableReason
+    status: "True"
+    type: Available
+  currentPodHash: 754cb84d5
+  currentStepHash: 757f5f97b
+  currentStepIndex: 2
+  observedGeneration: "8575574967"  ##  <---- uses legacy observedGeneration hash which are numbers
+  readyReplicas: 1
+  replicas: 1
+  selector: app=basic
+  stableRS: 754cb84d5
+  updatedReplicas: 1