Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Fix StatefulSet#deploy_succeeded? for OnDelete #950

Merged
merged 3 commits into from
Apr 3, 2024
Merged

Fix StatefulSet#deploy_succeeded? for OnDelete #950

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2db9122
Fix StatefulSet#deploy_succeeded? for OnDelete
JamesOwenHall Apr 2, 2024
d4218a9
Don't check currentRevision or currentReplicas
JamesOwenHall Apr 2, 2024
0af220c
Version 3.5.1
JamesOwenHall Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -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)

Expand Down
23 changes: 6 additions & 17 deletions lib/krane/kubernetes_resource/stateful_set.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines -25 to -27
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the check I've removed because it's not needed as far as I can tell. This previously passed in some of our deploys due to kubernetes/kubernetes#106055 not reliably setting .status.currentRevision properly.

I don't believe it should be necessary to look up the pods. The StatefulSet's status should be enough.


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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there known cases of update_strategy == DELETE AND required_rollout == "full"? If those conditions hold, krane will time out unless something external deletes the pods, right? I suppose that's not a problem if that's what's been encoded in the manifest since that would signal that as the desired behaviour 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do know of one such case in kafka-infrastructure, where this script in zonal-deploy-controller is used to delete the pods. I don't know that this change would break anything there, but tagging @Shopify/streaming-foundations for visibility

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If those conditions hold, krane will time out unless something external deletes the pods, right?

Yes, that's the desired behaviour as I understand it. You opt in with the annotation set to full to wait until something else rolls the pods.

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
Expand Down
2 changes: 1 addition & 1 deletion lib/krane/version.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# frozen_string_literal: true
module Krane
VERSION = "3.5.0"
VERSION = "3.5.1"
end
37 changes: 13 additions & 24 deletions test/unit/krane/kubernetes_resource/stateful_set_test.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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?)
Expand Down Expand Up @@ -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}}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This annotation wasn't set correctly.

"metadata" => {"annotations" => {"krane.shopify.io/required-rollout" => rollout}}
)
end

Expand Down
Loading