-
Notifications
You must be signed in to change notification settings - Fork 40.1k
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
Introduce new flag "--include-uninitialized" to kubectl #50497
Conversation
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 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. |
4698d59
to
9cda6b1
Compare
pkg/kubectl/cmd/describe.go
Outdated
r := builder. | ||
ContinueOnError(). | ||
NamespaceParam(cmdNamespace).DefaultNamespace().AllNamespaces(allNamespaces). | ||
FilenameParam(enforceNamespace, options). | ||
SelectorParam(selector). | ||
IncludeUninitializedParam(includeUninitialized). |
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 the proposal #49035 (comment) intends to disable --include-uninitialized for batch commands. For example:
kubectl describe pods/foo
: must include uninitializedkubectl describe pods
: must NOT include uninitialized because it lists pods.
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.
@ahmetb As @caesarxuchao commented in #49035
Both should includes uninitialized objects. --include-uninitialized doesn't affect them at all.
So I keep it unchanged.
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.
If user explicitly set --include-uninitialized=false
when kubectl describe pods
, then we should skip uninitialized pods.
pkg/kubectl/cmd/get.go
Outdated
@@ -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) |
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.
forgot to remove this?
pkg/kubectl/cmd/annotate.go
Outdated
@@ -190,10 +190,12 @@ func (o AnnotateOptions) RunAnnotate(f cmdutil.Factory, cmd *cobra.Command) erro | |||
return err | |||
} | |||
|
|||
includeUninitialized := cmdutil.GetFlagBool(cmd, "include-uninitialized") |
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.
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.
pkg/kubectl/cmd/label.go
Outdated
@@ -185,10 +185,12 @@ func (o *LabelOptions) RunLabel(f cmdutil.Factory, cmd *cobra.Command) error { | |||
return err | |||
} | |||
|
|||
includeUninitialized := cmdutil.GetFlagBool(cmd, "include-uninitialized") |
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.
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.
pkg/kubectl/cmd/set/set_image.go
Outdated
@@ -137,10 +137,12 @@ func (o *ImageOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []st | |||
return err | |||
} | |||
|
|||
includeUninitialized := cmdutil.GetFlagBool(cmd, "include-uninitialized") |
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.
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.
pkg/kubectl/resource/builder.go
Outdated
@@ -77,6 +77,8 @@ type Builder struct { | |||
|
|||
export bool | |||
|
|||
includeUninitialized bool |
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 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"}, |
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.
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 documentationobject
should beobject(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.
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.
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?
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.
Does this mean
kubectl get pods --include-uninitialized
still meansincludeUnintialized=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
.
9cda6b1
to
b948b85
Compare
Updated. PTAL. |
I'll take a look today. |
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.
pkg/kubectl/cmd/annotate.go
Outdated
// unless explicitly set --include-uninitialized=false | ||
includeUninitialized = true | ||
} | ||
if o.selector != "" { |
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.
When both specified, does o.selector
overwrite o.all
?
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.
@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.
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 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.
pkg/kubectl/cmd/apply.go
Outdated
@@ -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) |
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.
If name
is specified, we should always return the object to the user, no matter if it's initialized. We shouldn't change Get()
.
pkg/kubectl/cmd/describe.go
Outdated
r := builder. | ||
ContinueOnError(). | ||
NamespaceParam(cmdNamespace).DefaultNamespace().AllNamespaces(allNamespaces). | ||
FilenameParam(enforceNamespace, options). | ||
SelectorParam(selector). | ||
IncludeUninitializedParam(includeUninitialized). |
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.
If user explicitly set --include-uninitialized=false
when kubectl describe pods
, then we should skip uninitialized pods.
pkg/kubectl/cmd/get.go
Outdated
@@ -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") |
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 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.)".
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.
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.
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.
@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?
pkg/kubectl/resource/helper.go
Outdated
@@ -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) { |
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.
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.
pkg/kubectl/resource/visitor.go
Outdated
Namespace: namespace, | ||
Name: name, | ||
Export: export, | ||
IncludeUninitialized: includeUninitialized, |
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.
Same here. If name
is specified, we always show user the object.
Context clientcmdapi.Context | ||
CurrentContext string | ||
Timeout string | ||
IncludeUninitialized bool |
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.
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.
pkg/kubectl/cmd/annotate.go
Outdated
@@ -190,10 +190,26 @@ func (o AnnotateOptions) RunAnnotate(f cmdutil.Factory, cmd *cobra.Command) erro | |||
return err | |||
} | |||
|
|||
var includeUninitialized bool | |||
if o.all { |
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.
We need to update the flag comment of --all
.
b948b85
to
562f099
Compare
562f099
to
4c1c366
Compare
pkg/kubectl/cmd/set/set_resources.go
Outdated
// unless explicitly set --include-uninitialized=false | ||
includeUninitialized = true | ||
} | ||
if o.Selector != "" { |
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 Selector != ""
when I do kubectl set pods/foo ...
? Trying to understand what this clause is for.
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 so, "foo" will be a name, not a selector in this case.
@dixudx @caesarxuchao I will review it today. |
4c1c366
to
a4f0b47
Compare
pkg/kubectl/cmd/apply.go
Outdated
r := builder. | ||
Schema(schema). | ||
ContinueOnError(). | ||
NamespaceParam(cmdNamespace).DefaultNamespace(). | ||
FilenameParam(enforceNamespace, &options.FilenameOptions). | ||
SelectorParam(options.Selector). | ||
IncludeUninitializedParam(includeUninitialized). |
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.
This is not a param if you're doing parsing yourself. It would just be IncludeUninitialized
.
pkg/kubectl/cmd/set/set_image.go
Outdated
@@ -137,10 +138,26 @@ func (o *ImageOptions) Complete(f cmdutil.Factory, cmd *cobra.Command, args []st | |||
return err | |||
} | |||
|
|||
var includeUninitialized bool |
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.
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.
hack/make-rules/test-cmd-util.sh
Outdated
# 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[@]}" |
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.
Why double "${kube_flags[@]}"
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.
Add a test case with --prune --include-uninitialized --all.
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.
@mengqiy Updated. PTAL.
hack/make-rules/test-cmd-util.sh
Outdated
# 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[@]}" |
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.
why double "${kube_flags[@]}"
hack/make-rules/test-cmd-util.sh
Outdated
# 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[@]}") |
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.
--include-uninitialized=true
is identical to --include-uninitialized
.
I don't think we need this line.
Release note is not very accurate.
This is not a global flag, even though it has been added to a lot of flags. |
93feafc
to
eab8c1d
Compare
Add another testcase for @caesarxuchao @mengqiy @pwittrock PTAL. Thanks. |
eab8c1d
to
d80ff0f
Compare
@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 |
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.
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 ...
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 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.
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 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.
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.
Make sense.
/test pull-kubernetes-e2e-gce-bazel |
@dixudx Please squash some unnecessary commits. Don't block on this, it's code freeze today. |
RE. release note, I think we need to provide more information.
We need to update the linked doc later. |
/lgtm |
/approve |
[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 |
/test all [submit-queue is verifying that this PR is safe to merge] |
Automatic merge from submit-queue (batch tested with PRs 51301, 50497, 50112, 48184, 50993) |
@dixudx: The following test failed, say
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. |
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 ```
What this PR does / why we need it:
Introduce
--include-uninitialized
as a global flag to kubectlWhich issue this PR fixes (optional, in
fixes #<issue number>(, fixes #<issue_number>, ...)
format, will close that issue when PR gets merged): fixes #49035Special notes for your reviewer:
/assign @caesarxuchao @smarterclayton @ahmetb @deads2k
Release note: