-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Wait for pending deletions to complete before a deploy #4531
Wait for pending deletions to complete before a deploy #4531
Conversation
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 this makes sense.
pkg/skaffold/deploy/kubectl/cli.go
Outdated
// Find resources with `metadata.deletionTimestamp` field. | ||
for _, r := range result.Items { | ||
if r.Metadata.DeletionTimestamp != "" { | ||
list = append(list, r.Metadata.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.
I think it's worth appending the group/kind too. I think there's some code in the pkg/diag
that can be used here.
1fc3468
to
e6322d6
Compare
Codecov Report
@@ Coverage Diff @@
## master #4531 +/- ##
==========================================
+ Coverage 72.69% 72.71% +0.02%
==========================================
Files 335 335
Lines 12971 13014 +43
==========================================
+ Hits 9429 9463 +34
- Misses 2949 2954 +5
- Partials 593 597 +4
Continue to review full report at Codecov.
|
5ee591c
to
cc26713
Compare
360a6da
to
0b22ec0
Compare
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.
Sorry, I didn't make it all the way through.
de0ebd6
to
5bd6bd8
Compare
MaxRetry: 30, | ||
Delay: 2 * time.Second, |
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 this something a user would ever want to configure? maxDeletionWaitTime
or something?
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.
probably, yes. I can add that in a second PR
previousList := "" | ||
previousCount := 0 | ||
|
||
for try := 0; try < WaitDeletion.MaxRetry; try++ { |
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.
Would it make sense to instead use a child context with a timeout? (I'm really just concerned about infinite loops waiting for an object that will never die.)
Fixes GoogleContainerTools#4519 Signed-off-by: David Gageot <[email protected]>
5bd6bd8
to
5d203de
Compare
When Skaffold tries to deploy resources, it now checks that none of those resources are marked for deletion. In case, there are at least one, it'll wait for the deletion to complete.
If it doesn't do that, it's going to update resources that are marked for deletion and which are going to disappear in the background.
Fixes #4519
Signed-off-by: David Gageot [email protected]