From 04d69a989689594300e60110204d66ea709ee17e Mon Sep 17 00:00:00 2001 From: timothysmith0609 Date: Thu, 7 Nov 2019 15:17:18 -0500 Subject: [PATCH 1/7] ensure instance data is synced with pod state before reporting DS sucess --- lib/krane/kubernetes_resource/daemon_set.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/lib/krane/kubernetes_resource/daemon_set.rb b/lib/krane/kubernetes_resource/daemon_set.rb index 8e87ee9c3..a87d7eda9 100644 --- a/lib/krane/kubernetes_resource/daemon_set.rb +++ b/lib/krane/kubernetes_resource/daemon_set.rb @@ -59,7 +59,9 @@ def relevant_pods_ready? relevant_node_names = @nodes.map(&:name) considered_pods = @pods.select { |p| relevant_node_names.include?(p.node_name) } @logger.debug("Considered #{considered_pods.size} pods out of #{@pods.size} for #{@nodes.size} nodes") - considered_pods.present? && considered_pods.all?(&:deploy_succeeded?) + considered_pods.present? && + considered_pods.all?(&:deploy_succeeded?) && + rollout_data["numberReady"].to_i == considered_pods.length end def find_nodes(cache) From f5a1c155645249e38b74f25b677e683abac916d6 Mon Sep 17 00:00:00 2001 From: timothysmith0609 Date: Thu, 7 Nov 2019 15:28:46 -0500 Subject: [PATCH 2/7] Could have more numReady than consideredPods --- lib/krane/kubernetes_resource/daemon_set.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/krane/kubernetes_resource/daemon_set.rb b/lib/krane/kubernetes_resource/daemon_set.rb index a87d7eda9..859bcf8ae 100644 --- a/lib/krane/kubernetes_resource/daemon_set.rb +++ b/lib/krane/kubernetes_resource/daemon_set.rb @@ -61,7 +61,7 @@ def relevant_pods_ready? @logger.debug("Considered #{considered_pods.size} pods out of #{@pods.size} for #{@nodes.size} nodes") considered_pods.present? && considered_pods.all?(&:deploy_succeeded?) && - rollout_data["numberReady"].to_i == considered_pods.length + rollout_data["numberReady"].to_i >= considered_pods.length end def find_nodes(cache) From 5135c2b80f449b19e9f3142899b14d2b469b5ff0 Mon Sep 17 00:00:00 2001 From: timothysmith0609 Date: Thu, 7 Nov 2019 15:39:30 -0500 Subject: [PATCH 3/7] add test --- .../krane/kubernetes_resource/daemon_set_test.rb | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/unit/krane/kubernetes_resource/daemon_set_test.rb b/test/unit/krane/kubernetes_resource/daemon_set_test.rb index 221990a5f..c9b34e37d 100644 --- a/test/unit/krane/kubernetes_resource/daemon_set_test.rb +++ b/test/unit/krane/kubernetes_resource/daemon_set_test.rb @@ -107,6 +107,19 @@ def test_deploy_fails_when_not_all_pods_ready refute_predicate(ds, :deploy_succeeded?) end + def test_deploy_waits_for_daemonset_status_to_converge_to_pod_states + status = { + "desiredNumberScheduled": 1, + "updatedNumberScheduled": 1, + "numberReady": 0, + } + ds_template = build_ds_template(filename: 'daemon_set.yml', status: status) + ready_pod_template = load_fixtures(filenames: ['daemon_set_pods.yml']).first # should be a pod in `Ready` state + node_templates = load_fixtures(filenames: ['nodes.yml']) + ds = build_synced_ds(ds_template: ds_template, pod_templates: [ready_pod_template], node_templates: node_templates) + refute_predicate(ds, :deploy_succeeded?) + end + private def build_ds_template(filename:, status: {}) From c52192c2ce901b9a633ad64c8417bfc0aed36096 Mon Sep 17 00:00:00 2001 From: timothysmith0609 Date: Thu, 7 Nov 2019 15:59:49 -0500 Subject: [PATCH 4/7] take test to logical conclusion --- test/unit/krane/kubernetes_resource/daemon_set_test.rb | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/test/unit/krane/kubernetes_resource/daemon_set_test.rb b/test/unit/krane/kubernetes_resource/daemon_set_test.rb index c9b34e37d..6136bc3d8 100644 --- a/test/unit/krane/kubernetes_resource/daemon_set_test.rb +++ b/test/unit/krane/kubernetes_resource/daemon_set_test.rb @@ -113,11 +113,16 @@ def test_deploy_waits_for_daemonset_status_to_converge_to_pod_states "updatedNumberScheduled": 1, "numberReady": 0, } - ds_template = build_ds_template(filename: 'daemon_set.yml', status: status) + not_ready_ds_template = build_ds_template(filename: 'daemon_set.yml', status: status) ready_pod_template = load_fixtures(filenames: ['daemon_set_pods.yml']).first # should be a pod in `Ready` state node_templates = load_fixtures(filenames: ['nodes.yml']) - ds = build_synced_ds(ds_template: ds_template, pod_templates: [ready_pod_template], node_templates: node_templates) + ds = build_synced_ds(ds_template: not_ready_ds_template, pod_templates: [ready_pod_template], node_templates: node_templates) refute_predicate(ds, :deploy_succeeded?) + + status[:numberReady] = 1 + ready_ds_template = build_ds_template(filename: 'daemon_set.yml', status: status) + ds = build_synced_ds(ds_template: ready_ds_template, pod_templates: [ready_pod_template], node_templates: node_templates) + assert_predicate(ds, :deploy_succeeded?) end private From 218a82c3a9b8cad2d3594c1e38ab48af673ee18c Mon Sep 17 00:00:00 2001 From: timothysmith0609 Date: Thu, 7 Nov 2019 16:30:16 -0500 Subject: [PATCH 5/7] rubocop --- test/unit/krane/kubernetes_resource/daemon_set_test.rb | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/test/unit/krane/kubernetes_resource/daemon_set_test.rb b/test/unit/krane/kubernetes_resource/daemon_set_test.rb index 6136bc3d8..c759121fb 100644 --- a/test/unit/krane/kubernetes_resource/daemon_set_test.rb +++ b/test/unit/krane/kubernetes_resource/daemon_set_test.rb @@ -116,12 +116,14 @@ def test_deploy_waits_for_daemonset_status_to_converge_to_pod_states not_ready_ds_template = build_ds_template(filename: 'daemon_set.yml', status: status) ready_pod_template = load_fixtures(filenames: ['daemon_set_pods.yml']).first # should be a pod in `Ready` state node_templates = load_fixtures(filenames: ['nodes.yml']) - ds = build_synced_ds(ds_template: not_ready_ds_template, pod_templates: [ready_pod_template], node_templates: node_templates) + ds = build_synced_ds(ds_template: not_ready_ds_template, pod_templates: [ready_pod_template], + node_templates: node_templates) refute_predicate(ds, :deploy_succeeded?) status[:numberReady] = 1 ready_ds_template = build_ds_template(filename: 'daemon_set.yml', status: status) - ds = build_synced_ds(ds_template: ready_ds_template, pod_templates: [ready_pod_template], node_templates: node_templates) + ds = build_synced_ds(ds_template: ready_ds_template, pod_templates: [ready_pod_template], + node_templates: node_templates) assert_predicate(ds, :deploy_succeeded?) end From 9d0d0236cf69d5c647aad88f419a214c5c8f2c8a Mon Sep 17 00:00:00 2001 From: timothysmith0609 Date: Fri, 8 Nov 2019 10:30:07 -0500 Subject: [PATCH 6/7] PR suggestions --- lib/krane/kubernetes_resource/daemon_set.rb | 3 ++- .../krane/kubernetes_resource/daemon_set_test.rb | 12 ++++++------ 2 files changed, 8 insertions(+), 7 deletions(-) diff --git a/lib/krane/kubernetes_resource/daemon_set.rb b/lib/krane/kubernetes_resource/daemon_set.rb index 859bcf8ae..e22d98ee5 100644 --- a/lib/krane/kubernetes_resource/daemon_set.rb +++ b/lib/krane/kubernetes_resource/daemon_set.rb @@ -58,7 +58,8 @@ def relevant_pods_ready? return true if rollout_data["desiredNumberScheduled"].to_i == rollout_data["numberReady"].to_i # all pods ready relevant_node_names = @nodes.map(&:name) considered_pods = @pods.select { |p| relevant_node_names.include?(p.node_name) } - @logger.debug("Considered #{considered_pods.size} pods out of #{@pods.size} for #{@nodes.size} nodes") + @logger.debug("DaemonSet is reporting #{rollout_data['numberReady']} pods ready") + @logger.debug("Considered #{considered_pods.size} pods out of #{@pods.size} for #{@nodes.size} nodes.") considered_pods.present? && considered_pods.all?(&:deploy_succeeded?) && rollout_data["numberReady"].to_i >= considered_pods.length diff --git a/test/unit/krane/kubernetes_resource/daemon_set_test.rb b/test/unit/krane/kubernetes_resource/daemon_set_test.rb index c759121fb..2dcceee2e 100644 --- a/test/unit/krane/kubernetes_resource/daemon_set_test.rb +++ b/test/unit/krane/kubernetes_resource/daemon_set_test.rb @@ -113,17 +113,17 @@ def test_deploy_waits_for_daemonset_status_to_converge_to_pod_states "updatedNumberScheduled": 1, "numberReady": 0, } - not_ready_ds_template = build_ds_template(filename: 'daemon_set.yml', status: status) + ds_template = build_ds_template(filename: 'daemon_set.yml', status: status) ready_pod_template = load_fixtures(filenames: ['daemon_set_pods.yml']).first # should be a pod in `Ready` state node_templates = load_fixtures(filenames: ['nodes.yml']) - ds = build_synced_ds(ds_template: not_ready_ds_template, pod_templates: [ready_pod_template], - node_templates: node_templates) + ds = build_synced_ds(ds_template: ds_template, pod_templates: [ready_pod_template], node_templates: node_templates) refute_predicate(ds, :deploy_succeeded?) status[:numberReady] = 1 - ready_ds_template = build_ds_template(filename: 'daemon_set.yml', status: status) - ds = build_synced_ds(ds_template: ready_ds_template, pod_templates: [ready_pod_template], - node_templates: node_templates) + ds_template = build_ds_template(filename: 'daemon_set.yml', status: status) + stub_kind_get("DaemonSet", items: [ds_template]) + stub_kind_get("Pod", items: [ready_pod_template]) + ds.sync(build_resource_cache) assert_predicate(ds, :deploy_succeeded?) end From d8dc2f961073ae9e25450bb3685b8c4fea1a7ef4 Mon Sep 17 00:00:00 2001 From: timothysmith0609 Date: Mon, 11 Nov 2019 09:34:46 -0500 Subject: [PATCH 7/7] same line for debug --- lib/krane/kubernetes_resource/daemon_set.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/lib/krane/kubernetes_resource/daemon_set.rb b/lib/krane/kubernetes_resource/daemon_set.rb index e22d98ee5..8acf77e7a 100644 --- a/lib/krane/kubernetes_resource/daemon_set.rb +++ b/lib/krane/kubernetes_resource/daemon_set.rb @@ -58,8 +58,8 @@ def relevant_pods_ready? return true if rollout_data["desiredNumberScheduled"].to_i == rollout_data["numberReady"].to_i # all pods ready relevant_node_names = @nodes.map(&:name) considered_pods = @pods.select { |p| relevant_node_names.include?(p.node_name) } - @logger.debug("DaemonSet is reporting #{rollout_data['numberReady']} pods ready") - @logger.debug("Considered #{considered_pods.size} pods out of #{@pods.size} for #{@nodes.size} nodes.") + @logger.debug("DaemonSet is reporting #{rollout_data['numberReady']} pods ready." \ + " Considered #{considered_pods.size} pods out of #{@pods.size} for #{@nodes.size} nodes.") considered_pods.present? && considered_pods.all?(&:deploy_succeeded?) && rollout_data["numberReady"].to_i >= considered_pods.length