Prune should respect namespace option #83084

tedyu · 2019-09-24T20:10:41Z

What type of PR is this?
/kind bug

What this PR does / why we need it:
As #74414 stated, the prune should respect specified namespace.

Which issue(s) this PR fixes:
Fixes #74414

Does this PR introduce a user-facing change?:

NONE

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

tedyu · 2019-09-24T20:37:44Z

/test pull-kubernetes-node-e2e

alejandrox1

Thank you for working on this @tedyu

iirc there is a kep related to this kubernetes/enhancements#810
/assign @Liujingfang1 @pwittrock @soltysh @seans3

/cc @kubernetes/sig-cli-pr-reviews
ptal

/priority important-longterm
/milestone v1.17

alejandrox1 · 2019-09-24T21:01:26Z

/retest

tedyu · 2019-09-25T16:15:09Z

@Liujingfang1
Can you review this ?

Liujingfang1 · 2019-09-25T17:07:30Z

@tedyu Can you add some tests?

tedyu · 2019-09-25T17:30:56Z

@Liujingfang1
I ran through apply_test.go which doesn't cover the func modified.

If you have some pointer for the new test, please share.

Thanks

Liujingfang1 · 2019-09-25T18:21:49Z

@tedyu You can add those test similar like this one

kubernetes/test/cmd/apply.sh

Line 127 in 7acb066

## kubectl apply --prune

k8s-ci-robot · 2019-10-03T01:40:49Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: seans3, tedyu

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~hack/testdata/OWNERS~~ [seans3]
~~staging/src/k8s.io/kubectl/OWNERS~~ [seans3]
~~test/cmd/OWNERS~~ [seans3]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

seans3 · 2019-10-03T21:19:59Z

/hold cancel

seans3 · 2019-10-04T01:28:55Z

/retest

tedyu · 2019-10-04T01:50:44Z

/test pull-kubernetes-kubemark-e2e-gce-big

tedyu · 2019-10-04T02:19:19Z

/test pull-kubernetes-kubemark-e2e-gce-big

tedyu · 2019-10-04T02:50:46Z

/test pull-kubernetes-e2e-gce-100-performance

tedyu · 2019-10-04T03:31:33Z

/test pull-kubernetes-e2e-gce-100-performance

tedyu · 2019-10-04T03:54:15Z

/test pull-kubernetes-e2e-gce-100-performance

fejta-bot · 2019-10-04T06:43:36Z

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel or /hold comment for consistent failures.

Before kubernetes#83084, `kubectl apply --prune` can prune resources in all namespaces specified in config files. After that PR got merged, only a single namespace is considered for pruning. It is OK if namespace is explicitly specified by --namespace option, but what the PR does is use the default namespace (or from kubeconfig) if not overridden by command line flag. That breaks the existing usage of `kubectl apply --prune` without --namespace option. If --namespace is not used, there is no error, and no one notices this issue unless they actually check that pruning happens. This issue also prevents resources in multiple namespaces in config file from being pruned. kubectl 1.16 does not have this bug. Let's see the difference between kubectl 1.16 and kubectl 1.17. Suppose the following config file: ```yaml apiVersion: v1 kind: ConfigMap metadata: creationTimestamp: null name: foo namespace: a labels: pl: foo data: foo: bar --- apiVersion: v1 kind: ConfigMap metadata: creationTimestamp: null name: bar namespace: a labels: pl: foo data: foo: bar ``` Apply it with `kubectl apply -f file`. Then comment out ConfigMap foo in this file. kubectl 1.16 prunes ConfigMap foo with the following command: $ kubectl-1.16 apply -f file -l pl=foo --prune configmap/bar configured configmap/foo pruned But kubectl 1.17 does not prune ConfigMap foo with the same command: $ kubectl-1.17 apply -f file -l pl=foo --prune configmap/bar configured With this patch, kubectl once again can prune the resource as before.

Before kubernetes#83084, `kubectl apply --prune` can prune resources in all namespaces specified in config files. After that PR got merged, only a single namespace is considered for pruning. It is OK if namespace is explicitly specified by --namespace option, but what the PR does is use the default namespace (or from kubeconfig) if not overridden by command line flag. That breaks the existing usage of `kubectl apply --prune` without --namespace option. If --namespace is not used, there is no error, and no one notices this issue unless they actually check that pruning happens. This issue also prevents resources in multiple namespaces in config file from being pruned. kubectl 1.16 does not have this bug. Let's see the difference between kubectl 1.16 and kubectl 1.17. Suppose the following config file: ```yaml apiVersion: v1 kind: ConfigMap metadata: creationTimestamp: null name: foo namespace: a labels: pl: foo data: foo: bar

Before kubernetes/kubernetes#83084, `kubectl apply --prune` can prune resources in all namespaces specified in config files. After that PR got merged, only a single namespace is considered for pruning. It is OK if namespace is explicitly specified by --namespace option, but what the PR does is use the default namespace (or from kubeconfig) if not overridden by command line flag. That breaks the existing usage of `kubectl apply --prune` without --namespace option. If --namespace is not used, there is no error, and no one notices this issue unless they actually check that pruning happens. This issue also prevents resources in multiple namespaces in config file from being pruned. kubectl 1.16 does not have this bug. Let's see the difference between kubectl 1.16 and kubectl 1.17. Suppose the following config file: ```yaml apiVersion: v1 kind: ConfigMap metadata: creationTimestamp: null name: foo namespace: a labels: pl: foo data: foo: bar --- apiVersion: v1 kind: ConfigMap metadata: creationTimestamp: null name: bar namespace: a labels: pl: foo data: foo: bar ``` Apply it with `kubectl apply -f file`. Then comment out ConfigMap foo in this file. kubectl 1.16 prunes ConfigMap foo with the following command: $ kubectl-1.16 apply -f file -l pl=foo --prune configmap/bar configured configmap/foo pruned But kubectl 1.17 does not prune ConfigMap foo with the same command: $ kubectl-1.17 apply -f file -l pl=foo --prune configmap/bar configured With this patch, kubectl once again can prune the resource as before. Kubernetes-commit: 7af3b01f24edfde34e42640ee565a5a6bb66ce26

Before kubernetes#83084, `kubectl apply --prune` can prune resources in all namespaces specified in config files. After that PR got merged, only a single namespace is considered for pruning. It is OK if namespace is explicitly specified by --namespace option, but what the PR does is use the default namespace (or from kubeconfig) if not overridden by command line flag. That breaks the existing usage of `kubectl apply --prune` without --namespace option. If --namespace is not used, there is no error, and no one notices this issue unless they actually check that pruning happens. This issue also prevents resources in multiple namespaces in config file from being pruned. kubectl 1.16 does not have this bug. Let's see the difference between kubectl 1.16 and kubectl 1.17. Suppose the following config file: ```yaml apiVersion: v1 kind: ConfigMap metadata: creationTimestamp: null name: foo namespace: a labels: pl: foo data: foo: bar

Before kubernetes/kubernetes#83084, `kubectl apply --prune` can prune resources in all namespaces specified in config files. After that PR got merged, only a single namespace is considered for pruning. It is OK if namespace is explicitly specified by --namespace option, but what the PR does is use the default namespace (or from kubeconfig) if not overridden by command line flag. That breaks the existing usage of `kubectl apply --prune` without --namespace option. If --namespace is not used, there is no error, and no one notices this issue unless they actually check that pruning happens. This issue also prevents resources in multiple namespaces in config file from being pruned. kubectl 1.16 does not have this bug. Let's see the difference between kubectl 1.16 and kubectl 1.17. Suppose the following config file: ```yaml apiVersion: v1 kind: ConfigMap metadata: creationTimestamp: null name: foo namespace: a labels: pl: foo data: foo: bar Kubernetes-commit: 4e644d0b4842c851b18d846d1ce56ef5c73204a1

Before kubernetes/kubernetes#83084, `kubectl apply --prune` can prune resources in all namespaces specified in config files. After that PR got merged, only a single namespace is considered for pruning. It is OK if namespace is explicitly specified by --namespace option, but what the PR does is use the default namespace (or from kubeconfig) if not overridden by command line flag. That breaks the existing usage of `kubectl apply --prune` without --namespace option. If --namespace is not used, there is no error, and no one notices this issue unless they actually check that pruning happens. This issue also prevents resources in multiple namespaces in config file from being pruned. kubectl 1.16 does not have this bug. Let's see the difference between kubectl 1.16 and kubectl 1.17. Suppose the following config file: ```yaml apiVersion: v1 kind: ConfigMap metadata: creationTimestamp: null name: foo namespace: a labels: pl: foo data: foo: bar Kubernetes-commit: bb3b084bba8c01bbdd577d61a0e6ec03cd503976

Prune should respect namespace option Kubernetes-commit: 791bfac

Before kubernetes/kubernetes#83084, `kubectl apply --prune` can prune resources in all namespaces specified in config files. After that PR got merged, only a single namespace is considered for pruning. It is OK if namespace is explicitly specified by --namespace option, but what the PR does is use the default namespace (or from kubeconfig) if not overridden by command line flag. That breaks the existing usage of `kubectl apply --prune` without --namespace option. If --namespace is not used, there is no error, and no one notices this issue unless they actually check that pruning happens. This issue also prevents resources in multiple namespaces in config file from being pruned. kubectl 1.16 does not have this bug. Let's see the difference between kubectl 1.16 and kubectl 1.17. Suppose the following config file: ```yaml apiVersion: v1 kind: ConfigMap metadata: creationTimestamp: null name: foo namespace: a labels: pl: foo data: foo: bar Kubernetes-commit: 4e644d0b4842c851b18d846d1ce56ef5c73204a1

k8s-ci-robot requested review from dims and mengqiy September 24, 2019 20:12

k8s-ci-robot added area/kubectl sig/cli Categorizes an issue or PR as relevant to SIG CLI. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 24, 2019

tedyu changed the title ~~Prune should respect namespace~~ Prune should respect namespace option Sep 24, 2019

alejandrox1 reviewed Sep 24, 2019

View reviewed changes

k8s-ci-robot assigned Liujingfang1, pwittrock, seans3 and soltysh Sep 24, 2019

k8s-ci-robot added the priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. label Sep 24, 2019

k8s-ci-robot added this to the v1.17 milestone Sep 24, 2019

k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Sep 24, 2019

yutedz force-pushed the ns-prune branch from a321b69 to eae495a Compare September 25, 2019 18:56

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. area/test and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 25, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 3, 2019

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 3, 2019

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 3, 2019

k8s-ci-robot merged commit 791bfac into kubernetes:master Oct 4, 2019

tedyu mentioned this pull request Dec 8, 2019

kubectl apply --prune --all fails to remove existing resources #66430

Closed

howardjohn mentioned this pull request Feb 2, 2020

kubectl apply --prune namespace interaction #87756

Closed

clarketm mentioned this pull request Mar 25, 2020

Refine prune logic istio/test-infra#2549

Merged

tatsuhiro-t mentioned this pull request Mar 27, 2020

Restore the ability to kubectl apply --prune without -n flag #89551

Merged

yhack mentioned this pull request May 7, 2020

kubectl --namespace apply --prune should prune within the namespace only #74414

Closed

wking pushed a commit to wking/kubernetes that referenced this pull request Jul 21, 2020

Merge pull request kubernetes#83084 from yutedz/ns-prune

e442f91

Prune should respect namespace option Kubernetes-commit: 791bfac

Prune should respect namespace option #83084

Prune should respect namespace option #83084

Uh oh!

Conversation

tedyu commented Sep 24, 2019

Uh oh!

tedyu commented Sep 24, 2019

Uh oh!

alejandrox1 left a comment

Choose a reason for hiding this comment

Uh oh!

alejandrox1 commented Sep 24, 2019

Uh oh!

tedyu commented Sep 25, 2019

Uh oh!

Liujingfang1 commented Sep 25, 2019

Uh oh!

tedyu commented Sep 25, 2019

Uh oh!

Liujingfang1 commented Sep 25, 2019

Uh oh!

k8s-ci-robot commented Oct 3, 2019

Uh oh!

seans3 commented Oct 3, 2019

Uh oh!

seans3 commented Oct 4, 2019

Uh oh!

tedyu commented Oct 4, 2019

Uh oh!

tedyu commented Oct 4, 2019

Uh oh!

tedyu commented Oct 4, 2019

Uh oh!

tedyu commented Oct 4, 2019

Uh oh!

tedyu commented Oct 4, 2019

Uh oh!

fejta-bot commented Oct 4, 2019

Uh oh!

Uh oh!