diff --git a/lib/kubernetes-deploy/kubernetes_resource.rb b/lib/kubernetes-deploy/kubernetes_resource.rb index 125ce6243..016ba3018 100644 --- a/lib/kubernetes-deploy/kubernetes_resource.rb +++ b/lib/kubernetes-deploy/kubernetes_resource.rb @@ -139,6 +139,17 @@ def exists? @instance_data.present? end + def current_generation + return -1 unless exists? # must be different default than observed_generation + @instance_data["metadata"]["generation"] + end + + def observed_generation + return -2 unless exists? + # populating this is a best practice, but not all controllers actually do it + @instance_data["status"]["observedGeneration"] + end + def status exists? ? "Exists" : "Unknown" end diff --git a/lib/kubernetes-deploy/kubernetes_resource/daemon_set.rb b/lib/kubernetes-deploy/kubernetes_resource/daemon_set.rb index c1f302af6..93ad7451c 100644 --- a/lib/kubernetes-deploy/kubernetes_resource/daemon_set.rb +++ b/lib/kubernetes-deploy/kubernetes_resource/daemon_set.rb @@ -24,7 +24,8 @@ def deploy_succeeded? end def deploy_failed? - pods.present? && pods.any?(&:deploy_failed?) + pods.present? && pods.any?(&:deploy_failed?) && + observed_generation == current_generation end def fetch_logs(kubectl) @@ -35,16 +36,6 @@ def fetch_logs(kubectl) private - def current_generation - return -1 unless exists? # must be different default than observed_generation - @instance_data["metadata"]["generation"] - end - - def observed_generation - return -2 unless exists? - @instance_data["status"]["observedGeneration"] - end - def rollout_data return { "currentNumberScheduled" => 0 } unless exists? @instance_data["status"] diff --git a/lib/kubernetes-deploy/kubernetes_resource/deployment.rb b/lib/kubernetes-deploy/kubernetes_resource/deployment.rb index c709a953e..6722f151e 100644 --- a/lib/kubernetes-deploy/kubernetes_resource/deployment.rb +++ b/lib/kubernetes-deploy/kubernetes_resource/deployment.rb @@ -31,6 +31,7 @@ def fetch_logs(kubectl) def deploy_succeeded? return false unless exists? && @latest_rs.present? + return false unless observed_generation == current_generation if required_rollout == 'full' @latest_rs.deploy_succeeded? && @@ -51,7 +52,8 @@ def deploy_succeeded? end def deploy_failed? - @latest_rs&.deploy_failed? + @latest_rs&.deploy_failed? && + observed_generation == current_generation end def failure_message diff --git a/lib/kubernetes-deploy/kubernetes_resource/pod_disruption_budget.rb b/lib/kubernetes-deploy/kubernetes_resource/pod_disruption_budget.rb index 8ad2a8f77..83630007b 100644 --- a/lib/kubernetes-deploy/kubernetes_resource/pod_disruption_budget.rb +++ b/lib/kubernetes-deploy/kubernetes_resource/pod_disruption_budget.rb @@ -8,7 +8,7 @@ def status end def deploy_succeeded? - exists? + exists? && observed_generation == current_generation end def deploy_method diff --git a/lib/kubernetes-deploy/kubernetes_resource/replica_set.rb b/lib/kubernetes-deploy/kubernetes_resource/replica_set.rb index c66211154..98eedd849 100644 --- a/lib/kubernetes-deploy/kubernetes_resource/replica_set.rb +++ b/lib/kubernetes-deploy/kubernetes_resource/replica_set.rb @@ -26,12 +26,15 @@ def status end def deploy_succeeded? + observed_generation == current_generation && desired_replicas == rollout_data["availableReplicas"].to_i && desired_replicas == rollout_data["readyReplicas"].to_i end def deploy_failed? - pods.present? && pods.all?(&:deploy_failed?) + pods.present? && + pods.all?(&:deploy_failed?) && + observed_generation == current_generation end def desired_replicas diff --git a/lib/kubernetes-deploy/kubernetes_resource/stateful_set.rb b/lib/kubernetes-deploy/kubernetes_resource/stateful_set.rb index b489233f6..4993e88cc 100644 --- a/lib/kubernetes-deploy/kubernetes_resource/stateful_set.rb +++ b/lib/kubernetes-deploy/kubernetes_resource/stateful_set.rb @@ -30,6 +30,7 @@ def deploy_succeeded? end true else + observed_generation == current_generation && status_data['currentRevision'] == status_data['updateRevision'] && desired_replicas == status_data['readyReplicas'].to_i && desired_replicas == status_data['currentReplicas'].to_i @@ -38,7 +39,8 @@ def deploy_succeeded? def deploy_failed? return false if update_strategy == ONDELETE - pods.present? && pods.any?(&:deploy_failed?) + pods.present? && pods.any?(&:deploy_failed?) && + observed_generation == current_generation end private diff --git a/test/fixtures/for_unit_tests/deployment_test.yml b/test/fixtures/for_unit_tests/deployment_test.yml index 115202557..3e1a0cf29 100644 --- a/test/fixtures/for_unit_tests/deployment_test.yml +++ b/test/fixtures/for_unit_tests/deployment_test.yml @@ -33,6 +33,7 @@ spec: image: busybox status: replicas: 3 + observedGeneration: 2 # current conditions: - type: Progressing status: True @@ -43,6 +44,7 @@ apiVersion: apps/v1beta1 kind: ReplicaSet metadata: name: web-1 + generation: 1 labels: name: web app: hello-cloud @@ -66,4 +68,5 @@ spec: - name: app image: busybox status: + observedGeneration: 1 replicas: 3 diff --git a/test/fixtures/for_unit_tests/pod_disruption_budget_test.yml b/test/fixtures/for_unit_tests/pod_disruption_budget_test.yml new file mode 100644 index 000000000..621acaca7 --- /dev/null +++ b/test/fixtures/for_unit_tests/pod_disruption_budget_test.yml @@ -0,0 +1,14 @@ +--- +apiVersion: policy/v1beta1 +kind: PodDisruptionBudget +metadata: + name: test + generation: 2 +spec: + minAvailable: 2 + selector: + matchLabels: + name: web + app: hello-cloud +status: + observedGeneration: 2 diff --git a/test/fixtures/for_unit_tests/replica_set_test.yml b/test/fixtures/for_unit_tests/replica_set_test.yml new file mode 100644 index 000000000..d1da16c12 --- /dev/null +++ b/test/fixtures/for_unit_tests/replica_set_test.yml @@ -0,0 +1,27 @@ +--- +apiVersion: extensions/v1beta1 +kind: ReplicaSet +metadata: + name: test + generation: 2 +spec: + replicas: 3 + selector: + matchLabels: + app: hello-cloud + name: test + template: + metadata: + labels: + app: hello-cloud + name: test + spec: + containers: + - name: app + image: busybox + imagePullPolicy: IfNotPresent + command: ["tail", "-f", "/dev/null"] +status: + observedGeneration: 2 + availableReplicas: 3 + readyReplicas: 3 diff --git a/test/fixtures/for_unit_tests/stateful_set_test.yml b/test/fixtures/for_unit_tests/stateful_set_test.yml new file mode 100644 index 000000000..efc7e62a1 --- /dev/null +++ b/test/fixtures/for_unit_tests/stateful_set_test.yml @@ -0,0 +1,32 @@ +apiVersion: apps/v1beta1 +kind: StatefulSet +metadata: + name: test-ss + generation: 2 +spec: + selector: + matchLabels: + name: test-ss + app: hello-cloud + serviceName: "test-ss" + updateStrategy: + type: RollingUpdate + replicas: 2 + template: + metadata: + labels: + app: hello-cloud + name: test-ss + spec: + containers: + - name: app + image: busybox + imagePullPolicy: IfNotPresent + command: ["tail", "-f", "/dev/null"] +status: + replicas: 2 + readyReplicas: 2 + currentReplicas: 2 + observedGeneration: 2 + currentRevision: 2 + updateRevision: 2 diff --git a/test/unit/kubernetes-deploy/kubernetes_resource/daemon_set_test.rb b/test/unit/kubernetes-deploy/kubernetes_resource/daemon_set_test.rb index 36b39603b..37f2e36b5 100644 --- a/test/unit/kubernetes-deploy/kubernetes_resource/daemon_set_test.rb +++ b/test/unit/kubernetes-deploy/kubernetes_resource/daemon_set_test.rb @@ -7,10 +7,29 @@ def setup KubernetesDeploy::Kubectl.any_instance.expects(:run).never end - def test_deploy_fails_when_updated_available_does_not_match + def test_deploy_not_successful_when_updated_available_does_not_match ds_template = build_ds_template ds = build_synced_ds(template: ds_template) - refute ds.deploy_succeeded? + refute_predicate ds, :deploy_succeeded? + end + + def test_deploy_succeeded_not_fooled_by_stale_status + status = { + "observedGeneration": 1, + "numberReady": 2, + "desiredNumberScheduled": 2, + "updatedNumberScheduled": 2, + } + ds_template = build_ds_template(status: status) + ds = build_synced_ds(template: ds_template) + refute_predicate ds, :deploy_succeeded? + end + + def test_deploy_failed_ensures_controller_has_observed_deploy + ds_template = build_ds_template(status: { "observedGeneration": 1 }) + ds = build_synced_ds(template: ds_template) + ds.stubs(:pods).returns([stub(deploy_failed?: true)]) + refute_predicate ds, :deploy_failed? end def test_deploy_passes_when_updated_available_does_match @@ -24,7 +43,7 @@ def test_deploy_passes_when_updated_available_does_match ds_template = build_ds_template(status: status) ds = build_synced_ds(template: ds_template) - assert ds.deploy_succeeded? + assert_predicate ds, :deploy_succeeded? end private diff --git a/test/unit/kubernetes-deploy/kubernetes_resource/deployment_test.rb b/test/unit/kubernetes-deploy/kubernetes_resource/deployment_test.rb index 069eacc72..27553eda9 100644 --- a/test/unit/kubernetes-deploy/kubernetes_resource/deployment_test.rb +++ b/test/unit/kubernetes-deploy/kubernetes_resource/deployment_test.rb @@ -238,24 +238,35 @@ def test_validation_works_with_no_strategy_and_max_unavailable_annotation assert deploy.validate_definition(kubectl) end - def test_deploy_succeeded_not_fooled_by_stale_rs_data_in_deploy_status + def test_deploy_succeeded_not_fooled_by_stale_status_data deployment_status = { "replicas" => 3, - "updatedReplicas" => 3, # stale -- hasn't been updated since new RS was created + "updatedReplicas" => 3, # stale -- hasn't been updated since deploy "unavailableReplicas" => 0, - "availableReplicas" => 3 + "availableReplicas" => 3, + "observedGeneration" => 1, # stale } - rs_status = { - "replicas" => 1, - "availableReplicas" => 0, - "readyReplicas" => 0 + rs_status = { # points to old RS, new one not created yet + "replicas" => 3, + "availableReplicas" => 3, + "readyReplicas" => 3, + "observedGeneration" => 1, } deploy = build_synced_deployment( template: build_deployment_template(status: deployment_status, rollout: 'full', max_unavailable: 1), replica_sets: [build_rs_template(status: rs_status)] ) - refute deploy.deploy_succeeded? + refute_predicate deploy, :deploy_succeeded? + end + + def test_deploy_failed_ensures_controller_has_observed_deploy + deploy = build_synced_deployment( + template: build_deployment_template(status: { "observedGeneration" => 1 }, rollout: 'full', max_unavailable: 1), + replica_sets: [build_rs_template] + ) + KubernetesDeploy::ReplicaSet.any_instance.stubs(:pods).returns([stub(deploy_failed?: true)]) + refute_predicate deploy, :deploy_failed? end def test_deploy_timed_out_with_hard_timeout diff --git a/test/unit/kubernetes-deploy/kubernetes_resource/pod_disruption_budget_test.rb b/test/unit/kubernetes-deploy/kubernetes_resource/pod_disruption_budget_test.rb new file mode 100644 index 000000000..11dcfbcc7 --- /dev/null +++ b/test/unit/kubernetes-deploy/kubernetes_resource/pod_disruption_budget_test.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true +require 'test_helper' + +class PodDisruptionBudgetTest < KubernetesDeploy::TestCase + def setup + KubernetesDeploy::Kubectl.any_instance.expects(:run).never + super + end + + def test_deploy_succeeded_is_true_as_soon_as_controller_observes_new_version + template = build_pdb_template(status: { "observedGeneration": 2 }) + pdb = build_synced_pdb(template: template) + assert_predicate pdb, :deploy_succeeded? + end + + def test_deploy_succeeded_not_fooled_by_stale_status + template = build_pdb_template(status: { "observedGeneration": 1 }) + pdb = build_synced_pdb(template: template) + refute_predicate pdb, :deploy_succeeded? + end + + private + + def build_pdb_template(status: {}) + pdb_fixture.dup.deep_merge("status" => status) + end + + def build_synced_pdb(template:) + pdb = KubernetesDeploy::PodDisruptionBudget.new(namespace: "test", context: "nope", + logger: logger, definition: template) + sync_mediator = KubernetesDeploy::SyncMediator.new(namespace: 'test', context: 'minikube', logger: logger) + sync_mediator.kubectl.expects(:run).with("get", "PodDisruptionBudget", "test", "-a", "--output=json").returns( + [template.to_json, "", SystemExit.new(0)] + ) + pdb.sync(sync_mediator) + pdb + end + + def pdb_fixture + @pdb_fixture ||= YAML.load_stream( + File.read(File.join(fixture_path('for_unit_tests'), 'pod_disruption_budget_test.yml')) + ).find { |fixture| fixture["kind"] == "PodDisruptionBudget" } + end +end diff --git a/test/unit/kubernetes-deploy/kubernetes_resource/replica_set_test.rb b/test/unit/kubernetes-deploy/kubernetes_resource/replica_set_test.rb new file mode 100644 index 000000000..6c9c3eb9d --- /dev/null +++ b/test/unit/kubernetes-deploy/kubernetes_resource/replica_set_test.rb @@ -0,0 +1,53 @@ +# frozen_string_literal: true +require 'test_helper' + +class ReplicaSetTest < KubernetesDeploy::TestCase + def setup + KubernetesDeploy::Kubectl.any_instance.expects(:run).never + super + end + + def test_deploy_succeeded_is_true_when_generation_and_replica_counts_match + template = build_rs_template(status: { "observedGeneration": 2 }) + rs = build_synced_rs(template: template) + assert_predicate rs, :deploy_succeeded? + end + + def test_deploy_succeeded_not_fooled_by_stale_status + template = build_rs_template(status: { "observedGeneration": 1 }) + rs = build_synced_rs(template: template) + refute_predicate rs, :deploy_succeeded? + end + + def test_deploy_failed_ensures_controller_has_observed_deploy + template = build_rs_template(status: { "observedGeneration": 1 }) + rs = build_synced_rs(template: template) + rs.stubs(:pods).returns([stub(deploy_failed?: true)]) + refute_predicate rs, :deploy_failed? + end + + private + + def build_rs_template(status: {}) + rs_fixture.dup.deep_merge("status" => status) + end + + def build_synced_rs(template:) + rs = KubernetesDeploy::ReplicaSet.new(namespace: "test", context: "nope", logger: logger, definition: template) + sync_mediator = KubernetesDeploy::SyncMediator.new(namespace: 'test', context: 'minikube', logger: logger) + sync_mediator.kubectl.expects(:run).with("get", "ReplicaSet", "test", "-a", "--output=json").returns( + [template.to_json, "", SystemExit.new(0)] + ) + sync_mediator.kubectl.expects(:run).with("get", "Pod", "-a", "--output=json", anything).returns( + ['{ "items": [] }', "", SystemExit.new(0)] + ) + rs.sync(sync_mediator) + rs + end + + def rs_fixture + @rs_fixture ||= YAML.load_stream( + File.read(File.join(fixture_path('for_unit_tests'), 'replica_set_test.yml')) + ).find { |fixture| fixture["kind"] == "ReplicaSet" } + end +end diff --git a/test/unit/kubernetes-deploy/kubernetes_resource/stateful_set_test.rb b/test/unit/kubernetes-deploy/kubernetes_resource/stateful_set_test.rb new file mode 100644 index 000000000..486c871e7 --- /dev/null +++ b/test/unit/kubernetes-deploy/kubernetes_resource/stateful_set_test.rb @@ -0,0 +1,58 @@ +# frozen_string_literal: true +require 'test_helper' + +class StatefulSetTest < KubernetesDeploy::TestCase + def setup + KubernetesDeploy::Kubectl.any_instance.expects(:run).never + super + end + + def test_deploy_succeeded_is_true_when_revision_and_replica_counts_match + template = build_ss_template(status: { "observedGeneration": 2 }) + ss = build_synced_ss(template: template) + assert_predicate ss, :deploy_succeeded? + end + + def test_deploy_failed_ensures_controller_has_observed_deploy + template = build_ss_template(status: { "observedGeneration": 1 }) + ss = build_synced_ss(template: template) + refute_predicate ss, :deploy_succeeded? + end + + def test_deploy_failed_not_fooled_by_stale_status + status = { + "observedGeneration": 1, + "readyReplicas": 0, + } + template = build_ss_template(status: status) + ss = build_synced_ss(template: template) + ss.stubs(:pods).returns([stub(deploy_failed?: true)]) + refute_predicate ss, :deploy_failed? + end + + private + + def build_ss_template(status: {}) + ss_fixture.dup.deep_merge("status" => status) + end + + def build_synced_ss(template:) + ss = KubernetesDeploy::StatefulSet.new(namespace: "test", context: "nope", logger: logger, definition: template) + sync_mediator = KubernetesDeploy::SyncMediator.new(namespace: 'test', context: 'minikube', logger: logger) + sync_mediator.kubectl.expects(:run).with("get", "StatefulSet", "test-ss", "-a", "--output=json").returns( + [template.to_json, "", SystemExit.new(0)] + ) + sync_mediator.kubectl.expects(:run).with("get", "Pod", "-a", "--output=json", anything).returns( + ['{ "items": [] }', "", SystemExit.new(0)] + ) + sync_mediator.kubectl.expects(:server_version).returns(Gem::Version.new("1.8")) + ss.sync(sync_mediator) + ss + end + + def ss_fixture + @ss_fixture ||= YAML.load_stream( + File.read(File.join(fixture_path('for_unit_tests'), 'stateful_set_test.yml')) + ).find { |fixture| fixture["kind"] == "StatefulSet" } + end +end