Skip to content

Ensure Cleanup is called if Deploy creates resources but fails #6345

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

Merged
merged 1 commit into from
Aug 3, 2021

Conversation

nkubala
Copy link
Contributor

@nkubala nkubala commented Aug 2, 2021

Related: #6285

Description

When skaffold dev is invoked, an initial Deploy() is called. If this call fails, Skaffold will exit, but will not attempt to clean up any deployed resources, because the Deploy failed. However, it is possible that some resources were successfully created during this initial deploy, so Skaffold should attempt to clean up in the event this happens so that it does not abandon resources.

Tried on https://github.com/ZeroVocabulary/skaffold-namespace-manifests-inconsistency.

Before:

➜  skaffold dev
Listing files to watch...
Generating tags...
Checking cache...
Starting test...
Tags used in deployment:
Starting deploy...
 - namespace/test created
 - Error from server (NotFound): error when creating "STDIN": namespaces "test" not found
 - Error from server (NotFound): error when creating "STDIN": namespaces "test" not found
kubectl apply: exit status 1

➜  kubectl get namespaces
NAME                     STATUS   AGE
test                     Active   3s

After:

➜  skaffold dev         
Listing files to watch...
Generating tags...
Checking cache...
Starting test...
Tags used in deployment:
Starting deploy...
 - namespace/test created
 - Error from server (NotFound): error when creating "STDIN": namespaces "test" not found
 - Error from server (NotFound): error when creating "STDIN": namespaces "test" not found
Cleaning up...
 - warning: deleting cluster-scoped resources, not scoped to the provided namespace
 - namespace "test" deleted
kubectl apply: exit status 1

➜  kubectl get namespaces
NAME                     STATUS   AGE

@codecov
Copy link

codecov bot commented Aug 3, 2021

Codecov Report

Merging #6345 (81db935) into main (47e12af) will increase coverage by 0.04%.
The diff coverage is 50.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #6345      +/-   ##
==========================================
+ Coverage   70.45%   70.49%   +0.04%     
==========================================
  Files         498      498              
  Lines       22539    22560      +21     
==========================================
+ Hits        15879    15903      +24     
+ Misses       5635     5627       -8     
- Partials     1025     1030       +5     
Impacted Files Coverage Δ
pkg/skaffold/runner/v1/apply.go 0.00% <0.00%> (ø)
pkg/skaffold/runner/v1/deploy.go 74.28% <100.00%> (ø)
pkg/skaffold/hooks/container.go 64.44% <0.00%> (-2.23%) ⬇️
pkg/skaffold/docker/image.go 65.44% <0.00%> (-1.00%) ⬇️
pkg/skaffold/docker/parse.go 87.39% <0.00%> (-0.85%) ⬇️
pkg/skaffold/kubernetes/logger/formatter.go 83.92% <0.00%> (ø)
.../skaffold/deploy/component/kubernetes/component.go 94.73% <0.00%> (ø)
pkg/skaffold/deploy/kpt/kpt.go 79.25% <0.00%> (+0.05%) ⬆️
pkg/skaffold/deploy/helm/deploy.go 76.82% <0.00%> (+0.07%) ⬆️
pkg/skaffold/deploy/kubectl/kubectl.go 68.01% <0.00%> (+0.13%) ⬆️
... and 6 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 47e12af...81db935. Read the comment docs.

Copy link
Contributor

@gsquared94 gsquared94 left a comment

Choose a reason for hiding this comment

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

LGTM

@nkubala nkubala merged commit b864260 into GoogleContainerTools:main Aug 3, 2021
@nkubala nkubala deleted the cleanup-partial-deploy branch August 3, 2021 16:06
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.

2 participants