-
Notifications
You must be signed in to change notification settings - Fork 115
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.
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
Fix crashes with Service of type ExternalName #211
Changes from 1 commit
9cdf0e2
761775d
7fb623b
066138b
0b297cf
6e91242
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,16 +11,19 @@ def sync | |
end | ||
|
||
def status | ||
if @num_pods_selected.blank? | ||
if !requires_endpoints? | ||
"Doesn't require any endpoint" | ||
elsif @num_pods_selected.blank? | ||
"Failed to count related pods" | ||
elsif selects_some_pods? | ||
"Selects at least 1 pod" | ||
else | ||
"Selects 0 pods" | ||
"Unknown" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The elsif condition selects_some_pods? applies to when pods are found. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, it seems like we'll only arrive here if we expected to select some pods but saw <= 0. If that's true, There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I replaced the string with |
||
end | ||
end | ||
|
||
def deploy_succeeded? | ||
return exists? unless requires_endpoints? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "The service must be found no matter if the service is external or not. Line 26 says that if the service is external then it is deployed successfully. It may not be true." There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Do you mean L28 should be
What do you mean? If the service is external, afaik there's no good way to check whether it is in fact available, so server-side existence is the best indicator we have. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Please ignore my comment about line 26. I was confused. The code is right. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 |
||
# We can't use endpoints if we want the service to be able to fail fast when the pods are down | ||
exposes_zero_replica_deployment? || selects_some_pods? | ||
end | ||
|
@@ -44,28 +47,39 @@ def exposes_zero_replica_deployment? | |
@related_deployment_replicas == 0 | ||
end | ||
|
||
def requires_endpoints? | ||
return false if external_name_svc? || @related_deployment_replicas.blank? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you disagree with my reasoning that this should be return false if external_name_svc?
return true if @related_deployment_replicas.blank? # problem counting replicas - by default, assume endpoints are required |
||
@related_deployment_replicas > 0 | ||
end | ||
|
||
def selects_some_pods? | ||
return false unless @num_pods_selected | ||
@num_pods_selected > 0 | ||
end | ||
|
||
def selector | ||
@selector ||= @definition["spec"]["selector"].map { |k, v| "#{k}=#{v}" }.join(",") | ||
@selector ||= @definition["spec"].fetch("selector", []).map { |k, v| "#{k}=#{v}" }.join(",") | ||
end | ||
|
||
def fetch_related_pod_count | ||
return 0 unless selector.present? | ||
raw_json, _err, st = kubectl.run("get", "pods", "--selector=#{selector}", "--output=json") | ||
return unless st.success? | ||
JSON.parse(raw_json)["items"].length | ||
end | ||
|
||
def fetch_related_replica_count | ||
return 0 unless selector.present? | ||
raw_json, _err, st = kubectl.run("get", "deployments", "--selector=#{selector}", "--output=json") | ||
return unless st.success? | ||
|
||
deployments = JSON.parse(raw_json)["items"] | ||
return unless deployments.length == 1 | ||
deployments.first["spec"]["replicas"].to_i | ||
end | ||
|
||
def external_name_svc? | ||
@definition["spec"]["type"] == "ExternalName" | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,7 +11,7 @@ def test_full_hello_cloud_set_deploy_succeeds | |
"Deploying ConfigMap/hello-cloud-configmap-data (timeout: 30s)", | ||
"Hello from the command runner!", # unmanaged pod logs | ||
"Result: SUCCESS", | ||
"Successfully deployed 14 resources" | ||
"Successfully deployed 15 resources" | ||
], in_order: true) | ||
|
||
assert_logs_match_all([ | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you please add an assertion for the logs about your new fixture here? (ie. |
||
|
@@ -59,7 +59,7 @@ def test_pruning_works | |
'daemonset "ds-app"', | ||
'statefulset "stateful-busybox"' | ||
] # not necessarily listed in this order | ||
expected_msgs = [/Pruned 7 resources and successfully deployed 3 resources/] | ||
expected_msgs = [/Pruned 7 resources and successfully deployed 4 resources/] | ||
expected_pruned.map do |resource| | ||
expected_msgs << /The following resources were pruned:.*#{resource}/ | ||
end | ||
|
@@ -627,7 +627,7 @@ def test_can_deploy_deployment_with_zero_replicas | |
assert_equal 0, pods.length, "Pods were running from zero-replica deployment" | ||
|
||
assert_logs_match_all([ | ||
%r{Service/web\s+Selects 0 pods}, | ||
%r{Service/web\s+Doesn't require any endpoint}, | ||
%r{Deployment/web\s+0 replicas} | ||
]) | ||
end | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it true that an endpoint is not required when the service is in type ExternalName or deployment replica is 0?
The if statement here is not quite straightforward, some comments will help to clarify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, that's exactly it, and that already seems pretty clear to me by reading this method's name and definition. Here, making the status say "Doesn't require any endpoints" when
!requires_endpoints?
also seems straightforward. I wouldn't add any more comments.