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.

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

Merged
merged 6 commits into from
Dec 19, 2017
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
25 changes: 22 additions & 3 deletions lib/kubernetes-deploy/kubernetes_resource/service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,16 +11,19 @@ def sync
end

def status
if @num_pods_selected.blank?
if !requires_endpoints?
Copy link
Contributor

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.

Copy link
Contributor

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?

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.

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

Choose a reason for hiding this comment

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

The elsif condition selects_some_pods? applies to when pods are found.
The else condition should cover the case when no pods are found for the service selector while replica is > 0. Besides that, are there are cases that will fall into the else clause? Message "unknown" is not clear to me.

Copy link
Contributor

Choose a reason for hiding this comment

The 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, Selects 0 pods might still be a better message, but otoh I don't hate Unknown since this is a catch-all fallback case. I think you have a point, but I wouldn't block merging the PR on this if the author feels differently.

Copy link
Contributor Author

@maxlaverse maxlaverse Dec 19, 2017

Choose a reason for hiding this comment

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

I replaced the string with Unknown to be consistent with the other services, but I hadn't realized we could have this message for good reasons.

end
end

def deploy_succeeded?
return exists? unless requires_endpoints?
Copy link
Contributor

@peiranliushop peiranliushop Dec 19, 2017

Choose a reason for hiding this comment

The 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."
Just looked at the code again. The logic is correct. Please ignore my above comments.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Do you mean L28 should be exists? && (exposes_zero_replica_deployment? || selects_some_pods?)? It's true that the pod selection doesn't inherently prove the server-side service resource was created, though that problem would predate this PR. Come to think of it, this could now just be exists? && selects_some_pods? since exposes_zero_replica_deployment? is already covered by the requires_endpoints? check.

Line 26 says that if the service is external then it is deployed successfully. It may not be true.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The 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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The 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
Expand All @@ -44,28 +47,44 @@ def exposes_zero_replica_deployment?
@related_deployment_replicas == 0
end

def requires_endpoints?
# Service of type External don't have endpoints
return false if external_name_svc?

# problem counting replicas - by default, assume endpoints are required
return true if @related_deployment_replicas.blank?

@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
33 changes: 33 additions & 0 deletions test/fixtures/hello-cloud/redis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,39 @@ spec:
selector:
name: redis
---
apiVersion: v1
kind: Service
metadata:
name: redis-external
labels:
name: redis-external
app: hello-cloud
spec:
externalName: external-redis.shopify
type: ExternalName
---
apiVersion: v1
kind: Endpoints
metadata:
name: redis-endpoints
subsets:
- addresses:
- ip: 10.1.1.1
- ip: 10.1.1.2
ports:
- name: redis
port: 6379
---
apiVersion: v1
kind: Service
metadata:
name: redis-endpoints
spec:
clusterIP: None
ports:
- name: redis
port: 6379
---
kind: PersistentVolumeClaim
apiVersion: v1
metadata:
Expand Down
9 changes: 5 additions & 4 deletions test/integration/kubernetes_deploy_test.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,15 +11,16 @@ 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 17 resources"
], in_order: true)

assert_logs_match_all([
Copy link
Contributor

Choose a reason for hiding this comment

The 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. %r{Service/redis-external\s+Doesn't require any endpoint})

%r{ReplicaSet/bare-replica-set\s+1 replica, 1 availableReplica, 1 readyReplica},
%r{Deployment/web\s+1 replica, 1 updatedReplica, 1 availableReplica},
%r{Service/web\s+Selects at least 1 pod},
%r{DaemonSet/ds-app\s+1 currentNumberScheduled, 1 desiredNumberScheduled, 1 numberReady},
%r{StatefulSet/stateful-busybox}
%r{StatefulSet/stateful-busybox},
%r{Service/redis-external\s+Doesn't require any endpoint}
])

# Verify that success section isn't duplicated for predeployed resources
Expand Down Expand Up @@ -59,7 +60,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 6 resources/]
expected_pruned.map do |resource|
expected_msgs << /The following resources were pruned:.*#{resource}/
end
Expand Down Expand Up @@ -627,7 +628,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
Expand Down