-
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
Add Daemon Set resource #133
Conversation
I have another branch ready for stateful sets but I'll wait for review on this before opening that one since they might have similar comments |
01a4e2d
to
8bd237d
Compare
|
||
if @found | ||
daemonset_data = JSON.parse(raw_json) | ||
@desired_number = daemonset_data["status"]["desiredNumberScheduled"] |
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.
Nit: since this is coming from the status data for this type, no need to store it separately--use the copy in the @rollout_data
slice.
end | ||
|
||
def deploy_succeeded? | ||
@desired_number == @rollout_data["desiredNumberScheduled"].to_i && |
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.
This is comparing @rollout_data["desiredNumberScheduled"]
to itself, no?
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.
🤦♂️
|
||
def deploy_succeeded? | ||
@desired_number == @rollout_data["desiredNumberScheduled"].to_i && | ||
@desired_number == @rollout_data["numberReady"].to_i |
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.
Looking at what's available to us, I think what we want to verify is desiredNumberScheduled == updatedNumberScheduled == numberAvailable. In other words, all required have been updated and are available (which means ready for at least minReadySeconds).
container_names.each_with_object({}) do |container_name, container_logs| | ||
out, _err, _st = kubectl.run( | ||
"logs", | ||
id, |
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.
This command isn't implemented for daemonsets unfortunately.
༶ kubectl logs ds/dd-agent
error: cannot get the logs from extensions/__internal, Kind=DaemonSet
Ref kubernetes/kubernetes#40927, which implemented it for deployments, jobs and statefulsets. We'll need to pick a pod ourselves and use its logs. The most_useful_pod
logic from fetch_events
is probably good enough for now, though it isn't as smart as what kubectl does. Note that there are tradeoffs to prioritizing failing pods, which I discuss in #138.
|
||
private | ||
|
||
def unmanaged? |
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.
This concept doesn't apply to DaemonSets--they aren't generated by other resources.
template: | ||
metadata: | ||
labels: | ||
app: busybox |
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.
Nit: the "app" label for this fixture set is usually set to "hello-cloud"
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.
I fixed that but still need to push it :P
test/helpers/fixture_set.rb
Outdated
daemon_sets.each do |ds| | ||
found = true if ds.metadata.name == name | ||
end | ||
assert found |
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.
We should also have an availability assertion like we do for deployments and replicasets.
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.
I'd also add the assertion message
|
||
assert_logs_match_all([ | ||
'Successfully deployed 1 resource', | ||
'1 currentNumberSchedule, 1 desiredNumberSchedule, 1 numberRead' |
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.
Since these tests are expensive, I'd recommend adding this assertion to the group at L17 (including a resource type prefix like the others there have) instead of having a separate test.
]) | ||
end | ||
|
||
def test_timed_out_daemon_set_deploy |
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.
Since the timeout has not special logic for this resource type, I'd rather swap this out for a test of a failure scenario. There are a few deployment tests you can use as a starting point (no need to duplicate all of them, since they're ultimately using the same pod logic). One thing it is important to show is that we're successfully fetching events from both the DS and a pod, as well as logs.
|
||
all_pods = JSON.parse(raw_json)["items"] | ||
all_pods.each_with_object([]) do |pod_data, relevant_pods| | ||
next unless pod_data["metadata"]["ownerReferences"].any? { |ref| ref["uid"] == ds_data["metadata"]["uid"] } |
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.
Seemingly this will select all pods belonging to the DaemonSet, both old and updated. Is there a way to select only the ones in the updated generation? It's the intermediate ReplicaSet layer that provides this guarantee for Deployments. If it isn't possible to filter down to the relevant pods, then we'll need to change our logic for success/failure.
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.
Thought I added the revision check too. I'll add that
Made all the requested changes. Should be good for another set of 👀 . Will rebase the commits before merging |
|
||
def deploy_succeeded? | ||
@rollout_data["desiredNumberScheduled"] == @rollout_data["currentNumberScheduled"].to_i && | ||
@rollout_data["desiredNumberScheduled"] == @rollout_data["numberAvailable"].to_i |
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.
I believe this could generate a false positive if we look right before the rollout of the new pods begins. I.e. at that point desired == current == available, and all of the pods in question are from the old generation. The status includes a updatedNumberScheduled
number, doesn't it? Do you think we should also look at @pods
at all?
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.
I'll look into the updatedNumberScheduled
. I skipped checking the pods since numberAvailable
should handle the case when a pod deploy fails. If you see a reason for it to be there, I can look into adding it.
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.
I skipped checking the pods since numberAvailable should handle the case when a pod deploy fails. If you see a reason for it to be there, I can look into adding it.
Theoretically you should be right. However, in debugging a test failure that happened about 1% of the time locally and more often on CI, I discovered that the following is possible for deployments:
- Deployment template is up to date
- New replica set exists and is scaled to zero (so its desired/current/available are equal)
- Deployment's status still describes the old replica set, which is fully scaled still (so the overall desired/current/available and even updated are also equal)
That's why Deployment's success condition looks at the latest replicaSet too, not just the status fields. I don't know that DS have the same issue, but it seems plausible. An in-between option would be to include @pods.length == desiredNumberScheduled
. WDYT?
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.
One more note on this: don't forget to convert all of them to integers (you've got a mix right now)
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.
Looking at the code you linked in your other comment, we should be safe without looking at pods as long as we also check status.observedGeneration == metadata.Generation
. Looks like that strategy would be available for Deployment too actually.
private | ||
|
||
def container_names | ||
@definition["spec"]["template"]["spec"]["containers"].map { |c| c["name"] } |
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.
Can DaemonSets have init containers too?
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.
It can. Sorry forgot to add it back while jumping between too many branches last week
|
||
latest_pods = all_pods.find_all do |pods| | ||
pods["metadata"]["ownerReferences"].any? { |ref| ref["uid"] == ds_data["metadata"]["uid"] } && | ||
pods["metadata"]["labels"]["pod-template-generation"] == current_generation |
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.
This sounds like the right thing to look at, but for my education (since this is different from Deployment) do you have a doc or PR link that describes Generation?
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.
I mostly figured this out by testing it and seeing what we had access too. 👀 at the k8s repo I did find isPodUpdated and rollout status ref. Hopefully that helps. If not I can investigate more tomorrow
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.
I started from those links and poked around a bit. tl;dr I think what you have here is correct for 1.6, but it'll change to controller-revision-hash
in 1.7 (deprecation)/1.8(final). Some of the stuff I just read:
- Daemonset Update Proposal with some relevant details about the field changes (looks like the old one will be kept and the new one added when a DS rolls in 1.7)
- pod-template-generation is deprecated
- Issue mentioning the DS/deploy/SS labels for 1.7 (it was apparently going to be
daemonset-controller-hash
for DS but was changed before release for consistency)
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.
Since 1.6 we don't have access to the revision, I'll make an issue to update this when we look at migrating to 1.7
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.
Works for me--that'll make our lives easier when the time comes. The test coverage should also prevent us from doing an upgrade without fixing it if it comes down to it.
assert_logs_match_all([ | ||
"DaemonSet/nginx: FAILED", | ||
"The following containers are in a state that is unlikely to be recoverable:", | ||
"Logs from container 'nginx' (last 250 lines shown):", |
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.
Add an assertion on an event too please
], in_order: true) | ||
|
||
assert_logs_match_all([ | ||
%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{Service/web\s+Selects at least 1 pod}, | ||
%r{DaemonSet/nginx\s+1 currentNumberSchedule, 1 desiredNumberSchedule, 1 numberRead, 1 numberAvailabl} |
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.
Why are "numberRead" and "numberAvailabl" truncated?
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.
Fixing now :)
@@ -51,7 +52,7 @@ def test_pruning_works | |||
'deployment "web"', | |||
'ingress "web"' | |||
] # not necessarily listed in this order | |||
expected_msgs = [/Pruned 5 resources and successfully deployed 3 resources/] | |||
expected_msgs = [/Pruned 6 resources and successfully deployed 3 resources/] |
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.
let's also add a message to the expected_prune
list above
|
||
if @found | ||
daemonset_data = JSON.parse(raw_json) | ||
@metadata = daemonset_data["metadata"] |
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.
Nit: Since all we want out of this is the generation, I'd suggest storing @current_generation
instead of the whole big blob. Similarly, observedGeneration
is a kinda different from the rest of the "rollout data", so I'd prefer to store that on its own as well. Either way, we need to make the else
reset these new values.
@metadata = daemonset_data["metadata"] | ||
@rollout_data = daemonset_data["status"] | ||
.slice("currentNumberScheduled", "desiredNumberScheduled", "numberReady", "numberAvailable", "observedGeneration") | ||
@status = @rollout_data.map { |state_replicas, num| "#{num} #{state_replicas}" }.join(", ") |
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.
Even if you keep observedGeneration
as part of the rollout data, I don't think it is useful to include it in the status, which is end-user-facing.
|
||
def fetch_logs | ||
most_useful_pod = @pods.find(&:deploy_failed?) || @pods.find(&:deploy_timed_out?) || @pods.first | ||
container_names.each_with_object({}) do |container_name, container_logs| |
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 there a reason we can't do most_useful_pod.fetch_logs
for this?
|
||
def container_names | ||
regular_containers = @definition["spec"]["template"]["spec"]["containers"].map { |c| c["name"] } | ||
init_containers = @definition["spec"]["template"]["spec"].fetch("initContainers", {}).map { |c| c["name"] } |
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.
Nit: .fetch("initContainers", {})
should technically be .fetch("initContainers", [])
since it'd be an array if it were present (the behaviour is the same though). I just fixed this for Pod in another PR. (then again, we don't need this method if we can use Pod's version directly)
return [] unless st.success? | ||
|
||
all_pods = JSON.parse(raw_json)["items"] | ||
current_generation = ds_data["metadata"]["generation"].to_s |
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.
Why to_s
--isn't this already a string, which really contains an integer?
|
||
latest_pods = all_pods.find_all do |pods| | ||
pods["metadata"]["ownerReferences"].any? { |ref| ref["uid"] == ds_data["metadata"]["uid"] } && | ||
pods["metadata"]["labels"]["pod-template-generation"] == current_generation |
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.
Works for me--that'll make our lives easier when the time comes. The test coverage should also prevent us from doing an upgrade without fixing it if it comes down to it.
assert_logs_match_all([ | ||
"DaemonSet/nginx: FAILED", | ||
"The following containers are in a state that is unlikely to be recoverable:", | ||
"Events: None found. Please check your usual logging service (e.g. Splunk).", |
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.
Hm, really? I'd expect there to be some events here. Indeed, I see two when I run this test locally:
[Pod/nginx-cnkzq] BackOff: Back-off restarting failed container (2 events)
[Pod/nginx-cnkzq] FailedSync: Error syncing pod, skipping: failed to "StartContainer" for "nginx" with CrashLoopBackOff: "Back-off 10s restarting failed container=nginx pod=nginx-cnkzq_k8sdeploy-test-bad-container-on-daemon-sets-fa-be1043b5b97dfe2b(969dd56c-7152-11e7-be12-76b0ec54df39)" (2 events)
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.
My local tests for some reason is not printing any logs or events so I wanted to see if buildkite would do it
@@ -14,7 +14,7 @@ def teardown | |||
def test_auth_use_default_gcp_success | |||
config = KubernetesDeploy::KubeclientBuilder::GoogleFriendlyConfig.new(kubeconfig, "") | |||
|
|||
stub_request(:post, 'https://www.googleapis.com/oauth2/v3/token') | |||
stub_request(:post, 'https://www.googleapis.com/oauth2/v4/token') |
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.
This is on master if you rebase FYI
8e44ad3
to
8eda84c
Compare
end | ||
|
||
def deploy_failed? | ||
@pods.present? && @pods.all?(&:deploy_failed?) |
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.
Why .all?
and not any?
? What if daemon set has one crashing and one starting pod?
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.
Good point. I'll fix that
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.
I think all?
is correct actually. Since these are clones, we don't want to fail the deploy if a large deployment has a single bad pod, which will automatically be rescheduled. We're trying to catch the case where something is systemically wrong. ReplicaSets also use all?
for this reason.
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.
I feel like all?
makes sense for replica_sets since they are redundant whereas a daemon set is meant to make sure there is a pod on each node. I don't have strong feelings either way though
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.
Very true. And when a single pod is bad, it is often because of a problem with the node, so in the case of a DS pod it is pretty doomed. I'm especially nervous that we'll wrongly fail deploys due to transient registry errors that unfortunately can surface as 404s, but I definitely see the logic. We can always try any?
if we feel it's the strictly correct solution as long as we keep an eye on our DaemonSet success metrics and switch to all?
if it is an actual problem.
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.
Sounds good 👍
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.
👍
I'll merge this first thing tomorrow since I don't want to ship it EOD |
What?
cc/ @Shopify/cloudplatform