Fix StatefulSet#deploy_succeeded? for OnDelete

Shopify · Apr 2, 2024 · 2db9122 · 2db9122
1 parent d4ac4d0
commit 2db9122
Show file tree

Hide file tree

Showing 2 changed files with 21 additions and 30 deletions.
diff --git a/lib/krane/kubernetes_resource/stateful_set.rb b/lib/krane/kubernetes_resource/stateful_set.rb
@@ -22,26 +22,16 @@ def status
     def deploy_succeeded?
       success = observed_generation == current_generation
 
-      @pods.each do |pod|
-        success &= pod.definition["metadata"]["labels"]["controller-revision-hash"] == status_data['updateRevision']
-      end
-
-      if update_strategy == 'RollingUpdate'
-        success &= status_data['currentRevision'] == status_data['updateRevision']
-        success &= desired_replicas == status_data['readyReplicas'].to_i
-        success &= desired_replicas == status_data['currentReplicas'].to_i
-
-      elsif update_strategy == ONDELETE
+      if update_strategy == ONDELETE && required_rollout != "full"
         unless @success_assumption_warning_shown
-          @logger.warn("WARNING: Your StatefulSet's updateStrategy is set to OnDelete, "\
-                       "which means the deployment won't succeed until all pods are updated by deletion.")
+          @logger.warn("WARNING: Your StatefulSet's updateStrategy is set to #{update_strategy}, "\
+                       "which means updates will not be applied until its pods are deleted.")
           @success_assumption_warning_shown = true
         end
-
-        if required_rollout == 'full'
-          success &= desired_replicas == status_data['readyReplicas'].to_i
-          success &= desired_replicas == status_data['updatedReplicas'].to_i
-        end
+      else
+        success &= status_data['currentRevision'] == status_data['updateRevision']
+        success &= desired_replicas == status_data['readyReplicas'].to_i
+        success &= desired_replicas == status_data['updatedReplicas'].to_i
       end
 
       success

diff --git a/test/unit/krane/kubernetes_resource/stateful_set_test.rb b/test/unit/krane/kubernetes_resource/stateful_set_test.rb
@@ -4,14 +4,21 @@
 class StatefulSetTest < Krane::TestCase
   include ResourceCacheTestHelper
 
-  def test_deploy_succeeded_when_revision_matches_for_rollingupdate_strategy
-    ss_template = build_ss_template(updateStrategy: "RollingUpdate", rollout: nil)
+  def test_deploy_succeeded_true_with_rolling_update_strategy
+    ss = build_synced_ss(ss_template: build_ss_template)
+    assert_predicate(ss, :deploy_succeeded?)
+  end
+
+  def test_deploy_succeeded_true_with_on_delete_strategy_and_no_rollout_annotation
+    # OnDelete strategy without rollout annotation should always succeed.
+    # Change the updateRevision to ensure it's not being used to determine success.
+    ss_template = build_ss_template(status: { "updateRevision": 3 }, updateStrategy: "OnDelete", rollout: nil)
     ss = build_synced_ss(ss_template: ss_template)
     assert_predicate(ss, :deploy_succeeded?)
   end
 
-  def test_deploy_succeeded_when_revision_matches_for_ondelete_strategy_without_annotation
-    ss_template = build_ss_template(updateStrategy: "OnDelete", rollout: nil)
+  def test_deploy_succeeded_true_with_on_delete_strategy_and_full_rollout_annotation
+    ss_template = build_ss_template(status: { "updateRevision": 3 }, updateStrategy: "OnDelete", rollout: nil)
     ss = build_synced_ss(ss_template: ss_template)
     assert_predicate(ss, :deploy_succeeded?)
   end
@@ -34,19 +41,13 @@ def test_deploy_does_not_succeed_when_replica_counts_do_not_match_for_ondelete_s
     refute_predicate(ss, :deploy_succeeded?)
   end
 
-  def test_deploy_succeeded_when_replica_counts_match_for_rollingupdate_strategy
-    ss_template = build_ss_template(updateStrategy: "RollingUpdate", rollout: nil)
-    ss = build_synced_ss(ss_template: ss_template)
-    assert_predicate(ss, :deploy_succeeded?)
-  end
-
   def test_deploy_does_not_succeed_when_replica_counts_do_not_match_for_rollingupdate_strategy
-    ss_template = build_ss_template(status: { "currentReplicas": 1 }, updateStrategy: "RollingUpdate", rollout: nil)
+    ss_template = build_ss_template(status: { "updatedReplicas": 1 }, updateStrategy: "RollingUpdate", rollout: nil)
     ss = build_synced_ss(ss_template: ss_template)
     refute_predicate(ss, :deploy_succeeded?)
   end
 
-  def test_deploy_fails_when_current_and_observed_generations_do_not_match
+  def test_deploy_does_not_succeed_when_current_and_observed_generations_do_not_match
     ss_template = build_ss_template(status: { "observedGeneration": 1 })
     ss = build_synced_ss(ss_template: ss_template)
     refute_predicate(ss, :deploy_succeeded?)
@@ -80,7 +81,7 @@ def build_ss_template(status: {}, updateStrategy: "RollingUpdate", rollout: "ful
     ss_fixture.dup.deep_merge(
       "status" => status,
       "spec" => {"updateStrategy" => {"type" => updateStrategy}},
-      "metadata" => {"annotations" => {"krane.shopify.io/rollout" => rollout}}
+      "metadata" => {"annotations" => {"krane.shopify.io/required-rollout" => rollout}}
     )
   end