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

add kubectl wait #64034

Merged
merged 3 commits into from May 23, 2018

Conversation

@deads2k
Copy link
Contributor

deads2k commented May 18, 2018

Fixes #1899 (that might be a record, 3.5 years)

Adds a kubectl wait --for=[delete|condition=condition-name] resource/string command. This allows generic waiting on well behaved conditions and for a resource or set of resources to be deleted.

This was requested for delete to do foreground deletion

WIP because I need to add test cases.

@kubernetes/sig-cli-maintainers this is using a separation of concerns made possible by the genericclioptions to make an easily unit testable command.
@smarterclayton

`kubectl wait` is a new command that allows waiting for one or more resources to be deleted or to reach a specific condition
@soltysh

This comment has been minimized.

Copy link
Contributor

soltysh commented May 18, 2018

xref #1899

@k8s-ci-robot k8s-ci-robot added size/XL and removed size/L labels May 18, 2018

@deads2k deads2k changed the title [WIP] add kubectl wait add kubectl wait May 18, 2018

@deads2k deads2k force-pushed the deads2k:cli-62-wait branch from 74c48d2 to 25f0696 May 18, 2018

@k8s-ci-robot k8s-ci-robot removed the approved label May 18, 2018

@deads2k deads2k force-pushed the deads2k:cli-62-wait branch from 25f0696 to f5efc16 May 18, 2018

@k8s-ci-robot k8s-ci-robot added size/XXL and removed size/XL labels May 18, 2018

@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented May 18, 2018

/retest

@soltysh
Copy link
Contributor

soltysh left a comment

A few nits, mostly. But this is awesome!


// IsDeleted is a condition func for waiting for something to be deleted
func IsDeleted(info *resource.Info, o *WaitOptions) (runtime.Object, bool, error) {
endTime := time.Now().Add(o.Timeout)

This comment has been minimized.

@soltysh

soltysh May 18, 2018

Contributor

I'm not sure if I want to have indefinite wait. Why not cap it at some max, currently kubectl uses 5 min heavily, if you look for kubectl.Timeout.

This comment has been minimized.

@soltysh

soltysh May 18, 2018

Contributor

Here or eventually in the delete command itself.

@@ -23,13 +23,16 @@ import (

"github.com/spf13/cobra"

"github.com/golang/glog"

This comment has been minimized.

@soltysh

soltysh May 18, 2018

Contributor

Up.


finalObject, success, err := o.ConditionFn(info, o)
if success {
o.Printer.PrintObj(finalObject, o.Out)

This comment has been minimized.

@soltysh

soltysh May 18, 2018

Contributor

We need a quiet option, either by not specifying printer or explicitly setting Quiet flag in WaitOptions. It's needed for kubectl run command.

This comment has been minimized.

@deads2k

deads2k May 21, 2018

Author Contributor

We need a quiet option, either by not specifying printer or explicitly setting Quiet flag in WaitOptions. It's needed for kubectl run command.

Pass a discarding printer. Created in this pull.

}

func isDeleted(event watch.Event) (bool, error) {
if event.Type == watch.Deleted {

This comment has been minimized.

@soltysh

soltysh May 18, 2018

Contributor
return event.Type == watch.Deleted, nil
@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented May 18, 2018

/retest

@nilebox
Copy link
Member

nilebox left a comment

The change is really great, just two things (see the comments)

  • Please check if we actually have to reuse the existing internal WaitForDeletion flag
  • Not sure if condition on DynamicClient == nil is a special case (needs a comment clarifying it?) or a mistake
kubectl delete namespace my-namespace
kubectl delete namespace my-namespace --wait=false
# make sure that wait properly waits for finalization
kubectl wait --for=delete ns/my-namespace

This comment has been minimized.

@nilebox

nilebox May 19, 2018

Member

There is a chance that the deletion (as a result of kubectl delete above) will succeed before we run this kubectl wait command, i.e. this test could succeed even if the wait command is not implemented correctly.

To make this test case more reliable, we could start kubectl wait --for=delete first, and only then execute kubectl delete in a separate thread.
Not sure how hard it would be to implement with a shell script though, but we can write a similar unit test in Go (in addition to this change).

This comment has been minimized.

@deads2k

deads2k May 19, 2018

Author Contributor

It isn't super important, the unit tests on this command are quite thorough compared to others

This comment has been minimized.

@deads2k

deads2k May 19, 2018

Author Contributor

Also I chose a namespace because they tend to delete very slowly because of finalizers

@@ -138,6 +142,8 @@ func NewCmdDelete(f cmdutil.Factory, streams genericclioptions.IOStreams) *cobra

deleteFlags.AddFlags(cmd)

cmd.Flags().Bool("wait", true, `If true, wait for resources to be gone before returning. This waits for finalizers.`)

This comment has been minimized.

@nilebox

nilebox May 19, 2018

Member

Why not include this flag into deleteFlags.AddFlags(...) method above?

This comment has been minimized.

@nilebox

nilebox May 19, 2018

Member

Shall we also say in the description that this flag is on by default?

This comment has been minimized.

@deads2k

deads2k May 21, 2018

Author Contributor

Shall we also say in the description that this flag is on by default?

Help lists defaults already. Delete is a wasteland of weird delegation. I don't think this needs plumbing that far. DeleteFlags is really confused about what it is supposed to be.

@@ -167,6 +173,9 @@ func (o *DeleteOptions) Complete(f cmdutil.Factory, args []string, cmd *cobra.Co
o.WaitForDeletion = true
o.GracePeriod = 1
}
if b, err := cmd.Flags().GetBool("wait"); err == nil {
o.WaitForDeletion = b

This comment has been minimized.

@nilebox

nilebox May 19, 2018

Member

So this seems to be the reason for defining the "wait" flag differently from the rest - reusing the existing WaitForDeletion flag that is currently not exposed to the user?
The only place where the existing internal WaitForDeletion flag is used (for read) is in

if o.WaitForDeletion {
, i.e. in the ReapResult for resources with reapers only. It's not used for resources that don't have reapers.
Given the upcoming change to get rid of reapers completely in #63979, I would suggest not to bother with merging existing and new flags.
Instead, I would just rename the existing flag to get rid of it completely later as part of #63979, as I did in https://github.com/kubernetes/kubernetes/pull/63695/files#diff-7c126b9106a83157d89a336103eb3dbbR108

This comment has been minimized.

@nilebox

nilebox May 19, 2018

Member

Actually, once we add an explicit --wait flag and set it to true by default, I don't think we need to keep the existing logic for backward compatibility at all?

I mean the condition

if o.GracePeriod == 0 && !o.ForceDeletion {
// To preserve backwards compatibility, but prevent accidental data loss, we convert --grace-period=0
// into --grace-period=1 and wait until the object is successfully deleted. Users may provide --force
// to bypass this wait.
o.WaitForDeletion = true
o.GracePeriod = 1
(and 2 other similar places).
With the changes in this PR, WaitForDeletion is already true by default, i.e. it's consistent with the backward compatibility for grace period. And if the user explicitly sets --wait=false, we shouldn't override it there, I think?

This comment has been minimized.

@nilebox

nilebox May 19, 2018

Member

To be clear about the change I propose: Get rid of the existing logic with conditions for setting internal WaitForDeletion to true completely, and declare a new --wait flag that is exposed to the user in the same way as other flags in deleteFlags.AddFlags(...).

This comment has been minimized.

@deads2k

deads2k May 19, 2018

Author Contributor

Once reapers die, perhaps.

This comment has been minimized.

@soltysh

soltysh May 21, 2018

Contributor

Yes, that's the goal of getting rid of reapers. The old logic will die.

This comment has been minimized.

@deads2k

deads2k May 21, 2018

Author Contributor

So this seems to be the reason for defining the "wait" flag differently from the rest - reusing the existing WaitForDeletion flag that is currently not exposed to the user?

I think it falls out in the other direction. Reapers die and we remove this old codepath.

This comment has been minimized.

@nilebox

nilebox May 21, 2018

Member

Once the reapers are dead, we can move the wait flag to deleteFlags.AddFlags(...) to make it consistent with other flags, since the default will always be true, and there won't be internal conditional "graceful deletion" anymore.

@@ -167,6 +173,9 @@ func (o *DeleteOptions) Complete(f cmdutil.Factory, args []string, cmd *cobra.Co
o.WaitForDeletion = true

This comment has been minimized.

@nilebox

nilebox May 19, 2018

Member

As I noted in the other comment, this line becomes obsolete: WaitForDeletion is already true by default. And if user explicitly specifies --wait=false, it will be overwritten a few lines below.

@@ -167,6 +173,9 @@ func (o *DeleteOptions) Complete(f cmdutil.Factory, args []string, cmd *cobra.Co
o.WaitForDeletion = true
o.GracePeriod = 1
}
if b, err := cmd.Flags().GetBool("wait"); err == nil {

This comment has been minimized.

@nilebox

nilebox May 19, 2018

Member

If I understand correctly, there is no real case when err != nil (if there are no bugs in code)? If so, setting o.WaitForDeletion = true 2 lines above becomes useless with this change.

This comment has been minimized.

@deads2k

deads2k May 19, 2018

Author Contributor

It happens during bad delegation and this command is ripe with it

return nil
}
if o.DynamicClient == nil {
return nil

This comment has been minimized.

@nilebox

nilebox May 19, 2018

Member

The only place in code where we don't set DynamicClient is https://github.com/kubernetes/kubernetes/pull/64034/files#diff-7c126b9106a83157d89a336103eb3dbbR129 (apart from the unit tests).
Is that some special case? Shouldn't we always wait when o.WaitForDeletion == true (and therefore require DynamicClient to always be non-nil)?

watchOptions := metav1.ListOptions{}
watchOptions.FieldSelector = "metadata.name=" + info.Name
watchOptions.ResourceVersion = gottenObj.GetResourceVersion()
objWatch, err := o.DynamicClient.Resource(info.Mapping.Resource).Namespace(info.Namespace).Watch(watchOptions)

This comment has been minimized.

@nilebox

nilebox May 19, 2018

Member

There is a chance that there will be a delete event between Get and Watch? i.e. we might miss the delete event and timeout, if the delete event was after we checked that the object is still there, but before we started a watch.
We should probably setup watch before running Get - then we can guarantee that we won't miss a delete event that occurs after we check with Get?

This comment has been minimized.

@nilebox

nilebox May 19, 2018

Member

What happens if we try to subscribe for events on an object that already doesn't exist? Will there be an error? If so, then the code is fine.

This comment has been minimized.

@deads2k

deads2k May 19, 2018

Author Contributor

Watching from a resource version avoids the risk of missing the delete. It will send us the delete.

If you watch a thing that doesn't exist, it just waits until it does. The condition wait does this

@deads2k deads2k force-pushed the deads2k:cli-62-wait branch from f5efc16 to 244b3ca May 21, 2018

@@ -151,6 +151,7 @@ pkg/kubectl/cmd/util
pkg/kubectl/cmd/util/editor
pkg/kubectl/cmd/util/jsonmerge
pkg/kubectl/cmd/util/sanity
pkg/kubectl/cmd/wait

This comment has been minimized.

@deads2k

deads2k May 21, 2018

Author Contributor

the golang "standard" of everything being New or Interface or Flags sucks. Search for anything like that in our codebase and you're screwed. golint is trying to enforce that on this package, so I'm blacklisting it all.

Also, who thought that was ever a good idea???

@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented May 21, 2018

/retest

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 22, 2018

@deads2k deads2k added the lgtm label May 22, 2018

deads2k added some commits May 18, 2018

@deads2k deads2k force-pushed the deads2k:cli-62-wait branch from ed31d0c to 4925859 May 22, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label May 22, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented May 22, 2018

New changes are detected. LGTM label has been removed.

@deads2k deads2k added the lgtm label May 22, 2018

@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented May 22, 2018

/retest

@k8s-github-robot

This comment has been minimized.

Copy link
Contributor

k8s-github-robot commented May 23, 2018

Automatic merge from submit-queue (batch tested with PRs 64034, 64072, 64146, 64059, 64161). If you want to cherry-pick this change to another branch, please follow the instructions here.

@k8s-github-robot k8s-github-robot merged commit f9c8898 into kubernetes:master May 23, 2018

18 checks passed

Submit Queue Queued to run github e2e tests a second time.
Details
cla/linuxfoundation deads2k authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details

k8s-github-robot pushed a commit that referenced this pull request May 29, 2018

Kubernetes Submit Queue
Merge pull request #64375 from nilebox/delete-wait-cleanup
Automatic merge from submit-queue (batch tested with PRs 64300, 64375). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

Declare kubectl wait flag in a way consistent with other deletion flags

**What this PR does / why we need it**:
A follow up PR for #64034 and #63979 that makes declaring wait flag consistent with the other flags.

**Which issue(s) this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close the issue(s) when PR gets merged)*:
Fixes #64401

**Special notes for your reviewer**:

**Release note**:

```release-note

```
@bgrant0607

This comment has been minimized.

Copy link
Member

bgrant0607 commented Jun 5, 2018

@deads2k This is awesome! It deserves a better release note.

@liggitt

This comment has been minimized.

Copy link
Member

liggitt commented Jun 5, 2018

@deads2k This is awesome! It deserves a better release note.

bettered

@bgrant0607

This comment has been minimized.

Copy link
Member

bgrant0607 commented Jun 5, 2018

@liggitt Thanks!

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Jun 12, 2018

Can we make this experimental so we can change flags? I want to quibble with a bunch of them but I don't want to be locked in in 1.12

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Jun 12, 2018

For instance I want --for-condition=Available

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Jun 12, 2018

Also when we have jsonpath I'd rather have --for-jsonpath

@deads2k

This comment has been minimized.

Copy link
Contributor Author

deads2k commented Jun 12, 2018

Can we make this experimental so we can change flags? I want to quibble with a bunch of them but I don't want to be locked in in 1.12

For instance I want --for-condition=Available

Also when we have jsonpath I'd rather have --for-jsonpath

Having a single --for makes those conditions mutually exclusive. I think the mutual exclusivity is what we want.

I don't mind marking it experimental, but I think the mutual exclusivity is good and that this flag is comparable to -o for printing.

k8s-github-robot pushed a commit that referenced this pull request Jun 12, 2018

Kubernetes Submit Queue
Merge pull request #65030 from deads2k/cli-74-experimental
Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

mark kubectl wait as experimental

Per @smarterclayton comment in #64034

/assign @smarterclayton

@marpaia marpaia referenced this pull request Jul 2, 2018

Merged

1.12 Release Notes Draft #214

@deads2k deads2k deleted the deads2k:cli-62-wait branch Jul 3, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.