From 2db9122a4be96c2161a173001a0167bd76835862 Mon Sep 17 00:00:00 2001 From: James Hall Date: Tue, 2 Apr 2024 10:07:53 -0400 Subject: [PATCH 1/3] Fix StatefulSet#deploy_succeeded? for OnDelete --- lib/krane/kubernetes_resource/stateful_set.rb | 24 +++++------------ .../kubernetes_resource/stateful_set_test.rb | 27 ++++++++++--------- 2 files changed, 21 insertions(+), 30 deletions(-) diff --git a/lib/krane/kubernetes_resource/stateful_set.rb b/lib/krane/kubernetes_resource/stateful_set.rb index 4f06d7029..81de31056 100644 --- 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 index dd69d4bd2..e6b04284d 100644 --- 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 From d4218a97004ef719a43a77339a93662a93213967 Mon Sep 17 00:00:00 2001 From: James Hall Date: Tue, 2 Apr 2024 11:51:49 -0400 Subject: [PATCH 2/3] Don't check currentRevision or currentReplicas --- lib/krane/kubernetes_resource/stateful_set.rb | 1 - .../kubernetes_resource/stateful_set_test.rb | 24 +++++-------------- 2 files changed, 6 insertions(+), 19 deletions(-) diff --git a/lib/krane/kubernetes_resource/stateful_set.rb b/lib/krane/kubernetes_resource/stateful_set.rb index 81de31056..4961d6951 100644 --- a/lib/krane/kubernetes_resource/stateful_set.rb +++ b/lib/krane/kubernetes_resource/stateful_set.rb @@ -29,7 +29,6 @@ def deploy_succeeded? @success_assumption_warning_shown = true 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 diff --git a/test/unit/krane/kubernetes_resource/stateful_set_test.rb b/test/unit/krane/kubernetes_resource/stateful_set_test.rb index e6b04284d..99838f11b 100644 --- a/test/unit/krane/kubernetes_resource/stateful_set_test.rb +++ b/test/unit/krane/kubernetes_resource/stateful_set_test.rb @@ -11,38 +11,26 @@ def test_deploy_succeeded_true_with_rolling_update_strategy 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) + # 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_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 - - 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 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_does_not_succeed_when_replica_counts_do_not_match_for_rollingupdate_strategy - ss_template = build_ss_template(status: { "updatedReplicas": 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 From 0af220cfc29aa59fc711fce37243c58632e4a2d8 Mon Sep 17 00:00:00 2001 From: James Hall Date: Tue, 2 Apr 2024 10:39:41 -0400 Subject: [PATCH 3/3] Version 3.5.1 --- CHANGELOG.md | 12 ++++++++---- lib/krane/version.rb | 2 +- 2 files changed, 9 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 285b7fcc5..b10d3974f 100644 --- a/CHANGELOG.md +++ b/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) diff --git a/lib/krane/version.rb b/lib/krane/version.rb index 9eca3c640..826408985 100644 --- a/lib/krane/version.rb +++ b/lib/krane/version.rb @@ -1,4 +1,4 @@ # frozen_string_literal: true module Krane - VERSION = "3.5.0" + VERSION = "3.5.1" end