Merge pull request #950 from Shopify/fix-statefulset-on-delete

Fix StatefulSet#deploy_succeeded? for OnDelete

CHANGELOG.md

@@ -1,20 +1,24 @@
 ## next
 
-# 3.5.0
+## 3.5.1
+
+- Fixed successful deployment check for StatefulSets introduced in v3.3.0.
+
+## 3.5.0
 
 - Test against k8s 1.28
 - Drop support for k8s 1.23
 
-# 3.4.2
+## 3.4.2
 
 - Remove flag `--skip-dry-run` (see [#946](https://github.com/Shopify/krane/pull/946))
 - Remove support for batched server-side dry-run ([#946](https://github.com/Shopify/krane/pull/946))
 
-# 3.4.1
+## 3.4.1
 
 - Added flag `--skip-dry-run` to completely opt out of dry run validation.
 
-# 3.4.0
+## 3.4.0
 
 - Use `prune-allowlist` instead of `prune-whitelist` for 1.26+ clusters. Clusters running 1.25 or less will continue to use `--prune-whitelist`. [#940](https://github.com/Shopify/krane/pull/940)
 

lib/krane/kubernetes_resource/stateful_set.rb

@@ -22,26 +22,15 @@ 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 &= desired_replicas == status_data['readyReplicas'].to_i
+        success &= desired_replicas == status_data['updatedReplicas'].to_i
       end
 
       success

lib/krane/version.rb

@@ -1,4 +1,4 @@
 # frozen_string_literal: true
 module Krane
-  VERSION = "3.5.0"
+  VERSION = "3.5.1"
 end

test/unit/krane/kubernetes_resource/stateful_set_test.rb

@@ -4,49 +4,38 @@
 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)
-    ss = build_synced_ss(ss_template: ss_template)
+  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_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_no_rollout_annotation
+    # OnDelete strategy without rollout annotation should always succeed.
+    # Change updatedReplicas to ensure it's not being used to determine success.
+    ss_template = build_ss_template(status: { "updatedReplicas": 0 }, updateStrategy: "OnDelete", rollout: nil)
     ss = build_synced_ss(ss_template: ss_template)
     assert_predicate(ss, :deploy_succeeded?)
   end
 
-  def test_deploy_does_not_succeed_when_revision_does_not_match_without_annotation
-    ss_template = build_ss_template(status: { "updateRevision": 1 }, rollout: nil)
-    ss = build_synced_ss(ss_template: ss_template)
-    refute_predicate(ss, :deploy_succeeded?)
-  end
-
-  def test_deploy_succeeded_when_replica_counts_match_for_ondelete_strategy_with_full_annotation
+  def test_deploy_succeeded_true_with_on_delete_strategy_and_full_rollout_annotation
     ss_template = build_ss_template(updateStrategy: "OnDelete", rollout: "full")
     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_ondelete_strategy_with_full_annotation
-    ss_template = build_ss_template(status: { "readyReplicas": 1 }, updateStrategy: "OnDelete", rollout: "full")
+  def test_deploy_succeeded_false_when_updated_replicas_dont_match_desired
+    ss_template = build_ss_template(status: { "updatedReplicas": 1 })
     ss = build_synced_ss(ss_template: ss_template)
     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)
+  def test_deploy_does_not_succeed_when_replica_counts_do_not_match_for_ondelete_strategy_with_full_annotation
+    ss_template = build_ss_template(status: { "updatedReplicas": 1 }, updateStrategy: "OnDelete", rollout: "full")
     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 +69,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