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: treat pod deletion as if its containers were terminated #6587

Merged
merged 5 commits into from
Sep 14, 2021

Conversation

briandealwis
Copy link
Member

Fixes: #6586

Description

  • Treat pod-deletions as equivalent to terminating all containers within the pod.
  • Rewrite the debug container-manager tests to test different sequences of events to ensure that we handle container terminations and pod deletions equivalently

@briandealwis briandealwis requested a review from a team as a code owner September 10, 2021 16:14
@google-cla google-cla bot added the cla: yes label Sep 10, 2021
@codecov
Copy link

codecov bot commented Sep 10, 2021

Codecov Report

Merging #6587 (cbe631e) into main (290280e) will decrease coverage by 0.19%.
The diff coverage is 53.03%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6587      +/-   ##
==========================================
- Coverage   70.48%   70.29%   -0.20%     
==========================================
  Files         515      517       +2     
  Lines       23150    23292     +142     
==========================================
+ Hits        16317    16372      +55     
- Misses       5776     5850      +74     
- Partials     1057     1070      +13     
Impacted Files Coverage Δ
cmd/skaffold/app/cmd/flags.go 90.81% <ø> (ø)
cmd/skaffold/skaffold.go 0.00% <ø> (ø)
pkg/skaffold/build/buildpacks/logger.go 0.00% <ø> (ø)
pkg/skaffold/build/cluster/logs.go 0.00% <ø> (-16.67%) ⬇️
pkg/skaffold/config/options.go 100.00% <ø> (ø)
pkg/skaffold/debug/apply_transforms.go 22.64% <0.00%> (-0.44%) ⬇️
pkg/skaffold/debug/transform.go 95.91% <ø> (ø)
pkg/skaffold/docker/logger/log.go 22.64% <0.00%> (-0.44%) ⬇️
pkg/skaffold/kubectl/exec.go 100.00% <ø> (ø)
pkg/skaffold/kubernetes/debugging/transform.go 84.44% <0.00%> (ø)
... and 40 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 51b296e...cbe631e. Read the comment docs.

@@ -114,6 +129,24 @@ func (w *podWatcher) Start(kubeContext string, namespaces []string) (func(), err
continue
}

if l.Logger.IsLevelEnabled(logrus.TraceLevel) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this check l.Logger.IsLevelEnabled(logrus.TraceLevel) required? Doesn't l.Trace already handle it? Is it to save the cost of constructing the string?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, probably premature optimization: I thought the podWatcher fired more events than it actually does. This dereferencing seemed a bit distasteful but our output logger just returns a logrus object anyways.

Copy link
Member Author

Choose a reason for hiding this comment

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

pkg/skaffold/kubernetes/watcher.go:25:2: Do not use github.com/sirupsen/logrus package, use output.log.Entry instead 😭

Copy link
Contributor

Choose a reason for hiding this comment

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

wait you can add a allowlist of files here hack/tools/linters/logrus.go
Don't let the technology stop you!

// Ignore files
var ignoreFileSuffixes = []string{
        // always ignore test files
        "_test.go",
        "pkg/skaffold/output/log/log.go",
        "pkg/skaffold/event/v2/logger.go",
        "pkg/skaffold/build/buildpacks/logger.go",
}

@tejal29 tejal29 enabled auto-merge (squash) September 13, 2021 18:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Debug container manager must treat pod deletion as container termination
3 participants