fix: fail Deployment tracking fast on ReplicaSet pod-create errors#399
Open
Sharvash wants to merge 2 commits into
Open
fix: fail Deployment tracking fast on ReplicaSet pod-create errors#399Sharvash wants to merge 2 commits into
Sharvash wants to merge 2 commits into
Conversation
Since 40930a9 (feat: greatly decrease Kubernetes apiserver load) the event informer requires a shared dynamic informer factory, but every legacy feed (deployment, statefulset, daemonset, job, pod, canary) still passed nil into NewTracker. As a result all `kubedog rollout track ...` and `kubedog follow ...` commands panicked with a nil pointer dereference as soon as the events informer started, and library consumers using multitrack hit the same panic because the dynamic client was never propagated down to the feeds: MultitrackOptions.DynamicClient shadows the embedded tracker.Options.DynamicClient, and newMultitrackOptions() built fresh tracker.Options without it. The fix: - add DynamicClient to tracker.Options and set it in the CLI's makeTrackerOptions so every follow/rollout command provides it; - build a concurrent informer factory from opts.DynamicClient in each legacy feed (deployment, statefulset, daemonset, job, pod, canary) and report factory watch errors through the feed error handling; - return an explicit error from the feeds when opts.DynamicClient is unset instead of failing later with a nil pointer dereference; - propagate MultitrackOptions.DynamicClient into the embedded tracker.Options in Multitrack() and pass it through newMultitrackOptions() so the werf/nelm code path gets a working factory as well; covered by a regression test. Verified with: go build ./..., go vet ./..., go test ./..., golangci-lint run (no new issues), and a manual run of `kubedog rollout track deployment` against a kind cluster. Signed-off-by: Alexey Gorovenko <[email protected]>
Problem: during a Deployment rollout, when the child ReplicaSet cannot create pods (for example `FailedCreate ... exceeded quota`), kubedog did not surface the failure and the deploy hung until timeout. Root cause: the ReplicaSet controller emits these Warning events with InvolvedObject set to the ReplicaSet, not the Deployment. The deployment tracker only ran an event informer for the Deployment object itself, so ReplicaSet-scoped failures were never observed. StatefulSet and DaemonSet are unaffected because their controllers emit FailedCreate on the controller object kubedog already watches. The fix: start an additional event informer for each new ReplicaSet as soon as it is discovered (tracked per RS name, so a new ReplicaSet appearing mid-rollout gets its own informer too), feeding the same resourceFailed channel the Deployment tracker already reacts to. Any "Failed*" reason (including FailedCreate: exceeded quota) now interrupts tracking, consistent with StatefulSet/DaemonSet behavior. Honors KUBEDOG_DISABLE_EVENTS=1. It does NOT change the event package, the failure-counting semantics of multitrack/dyntracker, or add a new immediate-abort bypass; it only widens what the Deployment tracker observes. Verified with: go build ./..., go vet ./..., golangci-lint run (clean), and a manual reproduction on an ephemeral kind cluster with a ResourceQuota (limits.cpu=500m) blocking a Deployment whose pod requests limits.cpu=750m. Before: `kubedog rollout track deployment` hung for the full 60s timeout. After: it failed within 4s printing `FailedCreate: ... exceeded quota: debug, requested: limits.cpu=750m`. Closes werf#398 Refs werf#216, werf#363 Signed-off-by: Alexey Gorovenko <[email protected]>
Member
|
Thanks! But the main branch is kinda freezed right now, the development is mostly in the "1" branch. A big portion of the kubedog codebase is a mess, so I don't want to bring features to both branches |
Contributor
Author
My plan:
Should I go ahead? |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR makes the Deployment tracker observe ReplicaSet events, so ReplicaSet-scoped failures like
FailedCreate ... exceeded quotainterrupt tracking within seconds instead of hanging until the timeout.The ReplicaSet controller emits
FailedCreateWarning events withInvolvedObjectset to the ReplicaSet, not the Deployment, while the deployment tracker only ran an event informer for the Deployment object itself - so these failures were never observed. StatefulSet and DaemonSet are unaffected because their controllers emitFailedCreateon the controller object kubedog already watches.The fix starts an additional event informer for each ReplicaSet as soon as it is discovered (tracked per RS name, so a new ReplicaSet appearing mid-rollout gets its own informer too), feeding the same
resourceFailedchannel the Deployment tracker already reacts to. AnyFailed*reason now interrupts tracking, consistent with StatefulSet/DaemonSet behavior. HonorsKUBEDOG_DISABLE_EVENTS=1. It does not change the event package or the failure-counting semantics of multitrack/dyntracker - it only widens what the Deployment tracker observes.While reproducing the issue I hit a pre-existing panic that had to be fixed first, so the PR is split into two commits. Since 40930a9 (
feat: greatly decrease Kubernetes apiserver load) the event informer requires a shared dynamic informer factory, but every legacy feed (deployment, statefulset, daemonset, job, pod, canary) still passednilintoNewTracker, so allkubedog rollout track .../kubedog follow ...commands panicked with a nil pointer dereference as soon as the events informer started. Library consumers using multitrack hit the same panic:MultitrackOptions.DynamicClientshadows the embeddedtracker.Options.DynamicClient, andnewMultitrackOptions()built freshtracker.Optionswithout it. The first commit threads the dynamic client down the stack:tracker.Optionsgains aDynamicClientfield which the CLI sets inmakeTrackerOptions; each legacy feed builds a concurrent informer factory from it and returns an explicit error when the client is unset, instead of failing later with a nil pointer dereference;Multitrack()andnewMultitrackOptions()propagate the client so the library code path gets a working factory as well. The propagation is covered by a small regression test (the first test in the repo - happy to drop it if you prefer).Verified with
go build ./...,go vet ./...,go test ./...andgolangci-lint run, plus a manual reproduction on an ephemeral kind cluster: a namespace withResourceQuota pods: 0and a 1-replica Deployment, producingWarning FailedCreate replicaset/... exceeded quota. Before:kubedog rollout track deploymenthung for the full timeout. After:rollout trackfails in ~1s andmultitrackin ~2s with theFailedCreatereason, no panic and no timeout.Closes #398
Refs #216, #363