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

Introduce new flag "--include-uninitialized" to kubectl #50497

Merged

Conversation

dixudx
Copy link
Member

@dixudx dixudx commented Aug 11, 2017

What this PR does / why we need it:

Introduce --include-uninitialized as a global flag to kubectl

Which issue this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close that issue when PR gets merged): fixes #49035

Special notes for your reviewer:
/assign @caesarxuchao @smarterclayton @ahmetb @deads2k

Release note:

Add flag "--include-uninitialized" to kubectl annotate, apply, edit-last-applied, delete, describe, edit, get, label, set. "--include-uninitialized=true" makes kubectl commands apply to uninitialized objects, which by default are ignored if the names of the objects are not provided. "--all" also makes kubectl commands apply to uninitialized objects. Please see the [initializer](https://kubernetes.io/docs/admin/extensible-admission-controllers/) doc for more details.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Aug 11, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @dixudx. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 11, 2017
@k8s-github-robot k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Aug 11, 2017
@dixudx dixudx force-pushed the kubectl-include-uninitialized branch from 4698d59 to 9cda6b1 Compare August 12, 2017 01:57
@dixudx dixudx changed the title [WIP] Kubectl include uninitialized Kubectl include uninitialized Aug 12, 2017
@dixudx dixudx changed the title Kubectl include uninitialized Introduce --include-uninitialized as a global flag to kubectl Aug 12, 2017
r := builder.
ContinueOnError().
NamespaceParam(cmdNamespace).DefaultNamespace().AllNamespaces(allNamespaces).
FilenameParam(enforceNamespace, options).
SelectorParam(selector).
IncludeUninitializedParam(includeUninitialized).
Copy link
Member

Choose a reason for hiding this comment

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

I think the proposal #49035 (comment) intends to disable --include-uninitialized for batch commands. For example:

  • kubectl describe pods/foo: must include uninitialized
  • kubectl describe pods: must NOT include uninitialized because it lists pods.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ahmetb As @caesarxuchao commented in #49035

Both should includes uninitialized objects. --include-uninitialized doesn't affect them at all.

So I keep it unchanged.

Copy link
Member

Choose a reason for hiding this comment

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

If user explicitly set --include-uninitialized=false when kubectl describe pods, then we should skip uninitialized pods.

@@ -118,6 +118,9 @@ func NewCmdGet(f cmdutil.Factory, out io.Writer, errOut io.Writer) *cobra.Comman
Long: getLong,
Example: getExample,
Run: func(cmd *cobra.Command, args []string) {
includeUninitialized := cmdutil.GetFlagBool(cmd, "include-uninitialized")
fmt.Printf("L122 %+v\n", includeUninitialized)
Copy link
Member

Choose a reason for hiding this comment

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

forgot to remove this?

@@ -190,10 +190,12 @@ func (o AnnotateOptions) RunAnnotate(f cmdutil.Factory, cmd *cobra.Command) erro
return err
}

includeUninitialized := cmdutil.GetFlagBool(cmd, "include-uninitialized")
Copy link
Member

Choose a reason for hiding this comment

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

See my comment at #49035 (comment) I think annotate works with individual resources (and doesn't work in batch mode) so it should not have this flag.

@@ -185,10 +185,12 @@ func (o *LabelOptions) RunLabel(f cmdutil.Factory, cmd *cobra.Command) error {
return err
}

includeUninitialized := cmdutil.GetFlagBool(cmd, "include-uninitialized")
Copy link
Member

Choose a reason for hiding this comment

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

See my comment at #49035 (comment) I think label works with individual resources (and doesn't work in batch mode) so it should not have this flag.

@@ -137,10 +137,12 @@ func (o *ImageOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []st
return err
}

includeUninitialized := cmdutil.GetFlagBool(cmd, "include-uninitialized")
Copy link
Member

Choose a reason for hiding this comment

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

See my comment at #49035 (comment) I think set works with individual resources (and doesn't work in batch mode) so it should not have this flag.

ditto for other set_*.go changes.

@@ -77,6 +77,8 @@ type Builder struct {

export bool

includeUninitialized bool
Copy link
Member

Choose a reason for hiding this comment

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

I think you can group this next to selectAll above.

Timeout: FlagInfo{prefix + FlagTimeout, "", "0", "The length of time to wait before giving up on a single server request. Non-zero values should contain a corresponding time unit (e.g. 1s, 2m, 3h). A value of zero means don't timeout requests."},
CurrentContext: FlagInfo{prefix + FlagContext, "", "", "The name of the kubeconfig context to use"},
Timeout: FlagInfo{prefix + FlagTimeout, "", "0", "The length of time to wait before giving up on a single server request. Non-zero values should contain a corresponding time unit (e.g. 1s, 2m, 3h). A value of zero means don't timeout requests."},
IncludeUninitialized: FlagInfo{prefix + FlagIncludeUninitialized, "", "false", "If true, will include the objects which have a non-empty initializer list"},
Copy link
Member

Choose a reason for hiding this comment

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

If true, will include the objects which have a non-empty initializer list

I think this captures the implementation detail, and not necessarily user-friendly.

Also:

  • will word doesn't match the rest of the flags documentation
  • object should be object(s)
  • sentence should be completed with a period (e.g. see kubectl get --help)

How about:

If true, include the object(s) that have not been realized yet. Such object(s) have a non-empty initializer list.

Copy link
Member

Choose a reason for hiding this comment

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

Also question:

I see the default value is false. Does this mean kubectl get pods --include-uninitialized still means includeUnintialized=false? Should the user explicitly set =true or does this work like --all-namespaces already with this code?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ahmetb

Does this mean kubectl get pods --include-uninitialized still means includeUnintialized=false?

No, they are totally different. kubectl get pods --include-uninitialized means includeUnintialized is set to True.

kubectl get pods --include-uninitialized is identical to kubectl get pods --include-uninitialized=true.

does this work like --all-namespaces already with this code

Yes, you're right. This works like other cobra-based CLIs, including our kubectl commands, like you mentioned --all-namespaces.

@dixudx dixudx force-pushed the kubectl-include-uninitialized branch from 9cda6b1 to b948b85 Compare August 15, 2017 06:40
@dixudx
Copy link
Member Author

dixudx commented Aug 15, 2017

Updated. PTAL.

@caesarxuchao
Copy link
Member

I'll take a look today.

Copy link
Member

@caesarxuchao caesarxuchao left a comment

Choose a reason for hiding this comment

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

Thanks @dixudx. We also need to add either test-cmd.sh or e2e tests. @mengqiy do you know where we should add tests?

@mengqiy could you also take a look at this PR or find someone from sig-cli to review it? I'm not that familiar with kubectl code. Thanks.

// unless explicitly set --include-uninitialized=false
includeUninitialized = true
}
if o.selector != "" {
Copy link
Member

Choose a reason for hiding this comment

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

When both specified, does o.selector overwrite o.all?

Copy link
Member Author

Choose a reason for hiding this comment

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

@caesarxuchao Yes, this is current implementation.

But we have to discuss on such combinations. I think o.all is the dominant one. When other flags combine with o.all, includeUninitialized should be set to true.

ping @ahmetb @smarterclayton @pwittrock, PTAL.

Copy link
Member

Choose a reason for hiding this comment

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

I think it's strange if kubectl annotate pod -l foo=bar returns different results from kubectl annotate pod -l foo=bar --all. I think the current implementation is good.

@@ -623,7 +634,7 @@ func (p *patcher) patch(current runtime.Object, modified []byte, source, namespa
if i > triesBeforeBackOff {
p.backOff.Sleep(backOffPeriod)
}
current, getErr = p.helper.Get(namespace, name, false)
current, getErr = p.helper.Get(namespace, name, false, false)
Copy link
Member

Choose a reason for hiding this comment

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

If name is specified, we should always return the object to the user, no matter if it's initialized. We shouldn't change Get().

r := builder.
ContinueOnError().
NamespaceParam(cmdNamespace).DefaultNamespace().AllNamespaces(allNamespaces).
FilenameParam(enforceNamespace, options).
SelectorParam(selector).
IncludeUninitializedParam(includeUninitialized).
Copy link
Member

Choose a reason for hiding this comment

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

If user explicitly set --include-uninitialized=false when kubectl describe pods, then we should skip uninitialized pods.

@@ -168,6 +168,7 @@ func RunGet(f cmdutil.Factory, out, errOut io.Writer, cmd *cobra.Command, args [
selector := cmdutil.GetFlagString(cmd, "selector")
allNamespaces := cmdutil.GetFlagBool(cmd, "all-namespaces")
showKind := cmdutil.GetFlagBool(cmd, "show-kind")
showAll := cmdutil.GetFlagBool(cmd, "show-all")
Copy link
Member

Choose a reason for hiding this comment

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

I just noticed --show-all exists in many other commands. I don't know if it's alright we only special case kubectl get <resource> --show-all.

On the other hand, kubectl get is quite special, it doesn't accept --all.

It seems we can't update the help text of --show-all for only kubectl get.

@mengqiy @pwittrock do you know if --show-all is supposed to only affect pods? The helper text reads "When printing, show all resources (default hide terminated pods.)".

Copy link
Member

Choose a reason for hiding this comment

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

Discussed with @pwittrock, let's don't specialize --show-all for kubectl get. That is, kubectl get pods --show-all doesn't include uninitialized objects. Users have to use kubectl get pods --include-uninitialized=true.

The root problem is that kubectl get doesn't have a --all flag, while many other kubectl commands have. We need to unify the flags, but that's another PR.

Copy link
Member Author

Choose a reason for hiding this comment

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

@caesarxuchao What about selector? Seems we have not taken selector into consideration.

For selector, we should make it consistent with kubectl label. That is "Does not include the uninitialized objects by default, unless user explicitly set --include-uninitialized=true". Any comments?

@@ -52,7 +52,7 @@ func NewHelper(client RESTClient, mapping *meta.RESTMapping) *Helper {
}
}

func (m *Helper) Get(namespace, name string, export bool) (runtime.Object, error) {
func (m *Helper) Get(namespace, name string, export, includeUninitialized bool) (runtime.Object, error) {
Copy link
Member

Choose a reason for hiding this comment

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

Let's leave this function as is. I don't see any use case to hide an uninitialized object if user specifically asks for it.

Namespace: namespace,
Name: name,
Export: export,
IncludeUninitialized: includeUninitialized,
Copy link
Member

Choose a reason for hiding this comment

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

Same here. If name is specified, we always show user the object.

Context clientcmdapi.Context
CurrentContext string
Timeout string
IncludeUninitialized bool
Copy link
Member

Choose a reason for hiding this comment

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

The config here is not specific for kubectl, so we shouldn't change it. Probably you should add it in helpers.go like other flags.

@@ -190,10 +190,26 @@ func (o AnnotateOptions) RunAnnotate(f cmdutil.Factory, cmd *cobra.Command) erro
return err
}

var includeUninitialized bool
if o.all {
Copy link
Member

Choose a reason for hiding this comment

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

We need to update the flag comment of --all.

@dixudx dixudx force-pushed the kubectl-include-uninitialized branch from b948b85 to 562f099 Compare August 16, 2017 14:31
@k8s-github-robot k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2017
@dixudx dixudx force-pushed the kubectl-include-uninitialized branch from 562f099 to 4c1c366 Compare August 16, 2017 14:40
@k8s-github-robot k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 16, 2017
// unless explicitly set --include-uninitialized=false
includeUninitialized = true
}
if o.Selector != "" {
Copy link
Member

Choose a reason for hiding this comment

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

is Selector != "" when I do kubectl set pods/foo ...? Trying to understand what this clause is for.

Copy link
Member

Choose a reason for hiding this comment

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

I think so, "foo" will be a name, not a selector in this case.

@mengqiy
Copy link
Member

mengqiy commented Aug 16, 2017

@dixudx @caesarxuchao I will review it today.
/assign
/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Aug 16, 2017
@dixudx dixudx force-pushed the kubectl-include-uninitialized branch from 4c1c366 to a4f0b47 Compare August 17, 2017 01:55
r := builder.
Schema(schema).
ContinueOnError().
NamespaceParam(cmdNamespace).DefaultNamespace().
FilenameParam(enforceNamespace, &options.FilenameOptions).
SelectorParam(options.Selector).
IncludeUninitializedParam(includeUninitialized).
Copy link
Contributor

Choose a reason for hiding this comment

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

This is not a param if you're doing parsing yourself. It would just be IncludeUninitialized.

@@ -137,10 +138,26 @@ func (o *ImageOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []st
return err
}

var includeUninitialized bool
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a lot of duplicated code that is effectively the same. It needs to be unified - copying and pasting this code into many places (with some differences) is a maintenance problem.

# Pre-Condition: no POD exists
kube::test::get_object_assert pods "{{range.items}}{{$id_field}}:{{end}}" ''
# apply pod a
kubectl apply --prune --request-timeout=20 --include-uninitialized=false --all -f hack/testdata/prune/a.yaml "${kube_flags[@]}" 2>&1 "${kube_flags[@]}"
Copy link
Member

Choose a reason for hiding this comment

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

Why double "${kube_flags[@]}"

Copy link
Member

Choose a reason for hiding this comment

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

Add a test case with --prune --include-uninitialized --all.

Copy link
Member Author

Choose a reason for hiding this comment

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

@mengqiy Updated. PTAL.

# cleanup
kubectl delete pod a
# apply pod a and prune uninitialized deployments web
kubectl apply --prune --request-timeout=20 --all -f hack/testdata/prune/a.yaml "${kube_flags[@]}" 2>&1 "${kube_flags[@]}"
Copy link
Member

Choose a reason for hiding this comment

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

why double "${kube_flags[@]}"

# Post-condition: I assume "web" is the deployment name
kube::test::if_has_string "${output_message}" 'web'
# Command
output_message=$(kubectl get deployments --include-uninitialized=true 2>&1 "${kube_flags[@]}")
Copy link
Member

Choose a reason for hiding this comment

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

--include-uninitialized=true is identical to --include-uninitialized.
I don't think we need this line.

@mengqiy
Copy link
Member

mengqiy commented Sep 1, 2017

Release note is not very accurate.

add flag "--include-uninitialized" to kubectl [command list]

This is not a global flag, even though it has been added to a lot of flags.
global flag will appear in kubectl options.

@dixudx dixudx changed the title Introduce --include-uninitialized as a global flag to kubectl Introduce new flag "--include-uninitialized" to kubectl Sep 1, 2017
@dixudx dixudx force-pushed the kubectl-include-uninitialized branch from 93feafc to eab8c1d Compare September 1, 2017 07:01
@k8s-github-robot k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2017
@dixudx
Copy link
Member Author

dixudx commented Sep 1, 2017

Add another testcase for apply --prune --include-uninitialized --all.

@caesarxuchao @mengqiy @pwittrock PTAL. Thanks.

@dixudx dixudx force-pushed the kubectl-include-uninitialized branch from eab8c1d to d80ff0f Compare September 1, 2017 07:05
@ahmetb
Copy link
Member

ahmetb commented Sep 1, 2017

@dixudx consider adding spaces after the commas in the release note.

kube::log::status "Testing --include-uninitialized"

### Create a deployment
kubectl create --request-timeout=1 -f hack/testdata/initializer-deployments.yaml 2>&1 "${kube_flags[@]}" || true
Copy link
Member

Choose a reason for hiding this comment

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

From help text:

--request-timeout='0': The length of time to wait before giving up on a single server request. Non-zero values should contain a corresponding time unit (e.g. 1s, 2m, 3h). A value of zero means don't timeout requests.

I don't understand why you give --request-timeout=1. And it's not clear to me that timeout is 1s or 1m or ...

Copy link
Member

@ahmetb ahmetb Sep 1, 2017

Choose a reason for hiding this comment

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

I think it's b/c I left a comment about it. If we don't specify a timeout, the test spend 30s here for the blocked initializer. Now I'm suspecting this 1s might cause flakes, I'm not sure making it another value like 5s would help prevent the flake at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't understand why you give --request-timeout=1

@mengqiy I've added pending initializers in the yaml, which will keep blocking until timeout. Per discussion.

And it's not clear to me that timeout is 1s or 1m or ...

Just follow other commands in the test files. Actually --request-timeout=1 means 1sec.

Since the flag description of --request-timeout is quite confusing. We may consider to elaborate it.

Copy link
Member

Choose a reason for hiding this comment

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

Make sense.

@dixudx
Copy link
Member Author

dixudx commented Sep 1, 2017

/test pull-kubernetes-e2e-gce-bazel

@mengqiy
Copy link
Member

mengqiy commented Sep 1, 2017

@dixudx Please squash some unnecessary commits.

Don't block on this, it's code freeze today.

@caesarxuchao
Copy link
Member

caesarxuchao commented Sep 1, 2017

RE. release note, I think we need to provide more information. How about:
[edit] I updated the release note, since today is code freeze.

Add flag "--include-uninitialized" to kubectl annotate, apply, edit-last-applied, delete, describe, edit, get, label, set. "--include-uninitialized=true" makes kubectl commands apply to uninitialized objects, which by default are ignored if the names of the objects are not provided. "--all" also makes kubectl commands apply to uninitialized objects. Please see the [initializer](https://kubernetes.io/docs/admin/extensible-admission-controllers/) doc for more details.

We need to update the linked doc later.

@caesarxuchao
Copy link
Member

caesarxuchao commented Sep 1, 2017

@deads2k could you approve? Thanks.

(We had Clayton and Phil approved the flags in the issue #49035)

@mengqiy
Copy link
Member

mengqiy commented Sep 1, 2017

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 1, 2017
@deads2k
Copy link
Contributor

deads2k commented Sep 1, 2017

/approve

@k8s-github-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: caesarxuchao, deads2k, dixudx, mengqiy

Associated issue: 49035

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@k8s-github-robot k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 1, 2017
@abgworrall abgworrall modified the milestone: v1.8 Sep 2, 2017
@k8s-github-robot
Copy link

/test all [submit-queue is verifying that this PR is safe to merge]

@k8s-github-robot
Copy link

Automatic merge from submit-queue (batch tested with PRs 51301, 50497, 50112, 48184, 50993)

@k8s-github-robot k8s-github-robot merged commit 5c0b265 into kubernetes:master Sep 3, 2017
@dixudx dixudx deleted the kubectl-include-uninitialized branch September 3, 2017 06:52
@k8s-ci-robot
Copy link
Contributor

@dixudx: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-e2e-kops-aws d80ff0f link /test pull-kubernetes-e2e-kops-aws

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

vdemeester pushed a commit to vdemeester/kubernetes that referenced this pull request Feb 6, 2018
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>.

kubectl should return an error if "-l" and "--all" are both specified

**What this PR does / why we need it**:
Per discussion in [kubernetes#50497](kubernetes#50497 (comment))

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

**Special notes for your reviewer**:
/assign @caesarxuchao @mengqiy 

**Release note**:

```release-note
kubectl should return an error if "-l" and "--all" are both specified
```
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

kubectl delete --all: does not include uninitialized resources
9 participants