Merge pull request #325 from Shopify/observed_generation

Compare generations when available

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

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"]

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

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

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

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

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

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

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

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

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

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

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

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