Awesome! Thanks, @mikedanese.

@kubernetes/kubectl

@fabianofranz fyi

It seems like the kubectl apply --prune with current commits will mistakenly prune the pods created by deployment/RC? As below:

$ kubectl apply -f apply-test.yaml
pod "jessie1" created
pod "jessie2" created
deployment "my-nginx1" created
replicationcontroller "my-nginx2" created
$ kubectl get pods
NAME                         READY     STATUS    RESTARTS   AGE
jessie1                      1/1       Running   0          10s
jessie2                      1/1       Running   0          10s
my-nginx1-3491544093-oz0sc   1/1       Running   0          10s
my-nginx1-3491544093-yjbix   1/1       Running   0          10s
my-nginx2-dhi03              1/1       Running   0          10s
my-nginx2-r51j1              1/1       Running   0          10s
# change nothing
$ kubectl apply -f apply-test.yaml --prune
pod "jessie1" configured
pod "jessie2" configured
deployment "my-nginx1" configured
replicationcontroller "my-nginx2" configured
pod "my-nginx1-3491544093-oz0sc" pruned
pod "my-nginx1-3491544093-yjbix" pruned
pod "my-nginx2-dhi03" pruned
pod "my-nginx2-r51j1" pruned
$ kubectl get pods
NAME                         READY     STATUS    RESTARTS   AGE
jessie1                      1/1       Running   0          48s
jessie2                      1/1       Running   0          48s
my-nginx1-3491544093-ysfta   1/1       Running   0          7s
my-nginx1-3491544093-zm6ue   1/1       Running   0          7s
my-nginx2-41f0p              1/1       Running   0          7s
my-nginx2-s5n06              1/1       Running   0          7s

Or should the proper way to do it be to utilize the labels?

@MrHohn can you show me the config you used? You likely want a label selector that only matches the top level objects you want to manage. e.g.

$ kubectl apply -l k8s-managed-addon=true -f apply-test.yaml

And add k8s-managed-addon=true to the labels of the top level objects you want to manage but not to the pods.

Sure. Config as below:

apiVersion: v1
kind: Pod
metadata:
  name: jessie1
spec:
  containers:
  - name: jessie
    image: girishkalele/jessie-full:1.0
    command: ["sh", "-c"]
    args: ["while true ; do sleep 3600 ; done"]
  restartPolicy: Never
---
apiVersion: v1
kind: Pod
metadata:
  name: jessie2
spec:
  containers:
  - name: jessie
    image: girishkalele/jessie-full:1.0
    command: ["sh", "-c"]
    args: ["while true ; do sleep 3600 ; done"]
  restartPolicy: Never
---
apiVersion: extensions/v1beta1
kind: Deployment
metadata:
  name: my-nginx1
spec:
  replicas: 2
  template:
    metadata:
      labels:
        run: my-nginx1
    spec:
      containers:
      - name: my-nginx
        image: nginx
        ports:
        - containerPort: 80
---
apiVersion: v1
kind: ReplicationController
metadata:
  name: my-nginx2
spec:
  replicas: 2
  template:
    metadata:
      labels:
        run: my-nginx2
    spec:
      containers:
      - name: my-nginx
        image: nginx
        ports:
        - containerPort: 80

I see now. But why not making the label mandatory for --prune? As if label is not set it will prune innocence resources?

Empty label selector is valid (matches all) which is the default. I think we should add a flag --all to this before merging so users don't shoot themselves in the foot.

General comment - this will result in the canonical result of apply to no longer live in source control since the label selectors define what will be deleted. Seems like this is very error prone - running apply on a subdirectory with prune will cause it to delete all sorts of things. At a minimum I think we need to print out what will be deleted and force the user to confirm.

Review status: 0 of 3 files reviewed at latest revision, 14 unresolved discussions, some commit checks failed.

pkg/kubectl/cmd/apply.go, line 94 at r4 (raw file):

  cmd.MarkFlagRequired("filename")
  cmd.Flags().Bool("overwrite", true, "Automatically resolve conflicts between the modified and live configuration by using values from the modified configuration")
  cmd.Flags().BoolVar(&options.Prune, "prune", false, "Automatically delete missing objects from supplied config")

Help message is a bit confusing - I read it as it modifies the configs to remove objects that are not found in the sever. Maybe "Automatically delete resource objects that do not appear in the configs."

pkg/kubectl/cmd/apply.go, line 95 at r4 (raw file):

  cmd.Flags().Bool("overwrite", true, "Automatically resolve conflicts between the modified and live configuration by using values from the modified configuration")
  cmd.Flags().BoolVar(&options.Prune, "prune", false, "Automatically delete missing objects from supplied config")
  cmd.Flags().BoolVar(&options.Cascade, "cascade", true, "Only relevant during a prune. If true, cascade the deletion of the resources managed by pruned resources (e.g. Pods created by a ReplicationController).")

Ug. Does the garbage collector handle this for us yet?

pkg/kubectl/cmd/apply.go, line 98 at r4 (raw file):

  cmd.Flags().IntVar(&options.GracePeriod, "grace-period", -1, "Period of time in seconds given to pruned resources to terminate gracefully. Ignored if negative.")
  cmdutil.AddValidateFlags(cmd)
  cmd.Flags().StringVarP(&options.Selector, "selector", "l", "", "Selector (label query) to filter on")

What does an empty value mean? Delete everything by default? We probably shouldn't permit using the default in that case

pkg/kubectl/cmd/apply.go, line 318 at r4 (raw file):

          return err
      }
      if p.cascade {

Consider pulling this whole logic into its own function - e.g. 'Delete' and it figures out to delete the thing

Comments from Reviewable

Review status: 0 of 3 files reviewed at latest revision, 15 unresolved discussions, some commit checks failed.

pkg/kubectl/resource/visitor.go, line 644 at r4 (raw file):

}

type FilterFunc func(info *Info, err error) (bool, error)

Comments for all this stuff to say what it does

Comments from Reviewable

General comment - this will result in the canonical result of apply to no longer live in source control since the label selectors define what will be deleted. Seems like this is very error prone - running apply on a subdirectory with prune will cause it to delete all sorts of things. At a minimum I think we need to print out what will be deleted and force the user to confirm.

@bgrant0607 @ghodss can you respond to @pwittrock 's above comment?

Jenkins unit/integration failed for commit a344d1d. Full PR test history.

The magic incantation to run this job again is @k8s-bot unit test this. Please help us cut down flakes by linking to an open flake issue when you hit one in your PR.

This should only prune resources directly created by apply, which it should be possible to deduce by looking for the configuration annotation.

But is there a way to do so if user does want to prune those resources?

Again take the Addon Manager as an use case. After upgrade the Addon Manager may try to use apply --prune for pruning the old resources, which is not created through apply nor create --save-config. It would be great if this command also supports this.

We should add a test that makes sure when --prune is not supplied, that pruning doesn't actually happen. Also would be good to test that --all is actually required when no label selectors are used.

Reviewed 3 of 3 files at r6, 1 of 4 files at r9, 3 of 3 files at r10.
Review status: 1 of 4 files reviewed at latest revision, 16 unresolved discussions.

Comments from Reviewable

Reviewed 1 of 3 files at r1, 1 of 2 files at r5, 3 of 3 files at r11.
Review status: all files reviewed at latest revision, 15 unresolved discussions.

Comments from Reviewable

@k8s-bot test this [submit-queue is verifying that this PR is safe to merge]

Automatic merge from submit-queue

There are a few outstanding review comments that need to be fixed in followups.

@MrHohn Deletion of resources that were never managed by kubectl apply is not the responsibility of kubectl apply. The addon manager will need to deal with that during the transition from the previous approach to the new approach.

@bgrant0607 Thanks for reply. Yes, in theory kubectl apply should not be responsible for this.

I'm now thinking about faking the kubectl.kubernetes.io/last-applied-configuration annotation and still utilize this feature to prune the old things.

General comment - this will result in the canonical result of apply to no longer live in source control since the label selectors define what will be deleted. Seems like this is very error prone - running apply on a subdirectory with prune will cause it to delete all sorts of things. At a minimum I think we need to print out what will be deleted and force the user to confirm.

@bgrant0607 @ghodss can you respond to @pwittrock 's above comment?

I don't understand this - apply --prune should always only delete objects with the last-applied-configuration annotation, right? So how would it delete things that it shouldn't?

Correct me if I am wrong on either of these:

Case 1: Applying a sub-directory accidentally

IIUC this will delete everything from the root config dir except the things in this sub dir

cd /root/config/dir
kubectl apply -f ./
# Sometime later...
cd /root/config/dir/sub
kubectl apply -f .

Case 2: Applying 2 different directories in the same namespace

IIUC this will delete everything owned by team 1

# Person 1
kubectl apply -f /team/1/config/dir
# Person 2
kubectl apply -f /team/2/config/dir

Am I miss understanding the results of these actions?

@pwittrock I assume that you meant those commands to specify --prune.

You are not wrong. However:

Your first case is just user error.

I'd expect different teams to use different namespaces. That was the original motivation for namespaces. There are many ways users can step on each other when multiple teams share the same namespace.

I'd recommend users use prune at the namespace level, without a label selector. However, I'd like all commands to support filtering via label selector, for additional control.

Additionally, this is not the only case of kubectl commands depending on command-line arguments. All of the generators do, as well: run, create secret, create configmap, etc. Users have asked for declarative (apply-compatible) ways of specifying such commands, despite the fact that it's easy to work around in most cases, such as using a script, make, nulecule, or some other tool. We'll need to decide whether to support that. I added a bullet in the roadmap about this a while ago. Perhaps the Kubefiles idea could be extended to support this if it looks compelling enough.

Yup I meant --prune. Thanks.

The consequences of the first user error could be quite severe. IMHO it is plausible that a user executes an apply command thinking they are in one directory when they are in fact in another directory.

I think it would be pretty simple to require a safeguard .applyconfig file to be present in the root directory and to only prune a directory if this file was found. The config could also contain a specific annotation to apply to each resource it created. This would prevent either case from accidentally occurring. Applying a directory without this config would give an error.

Example workflow:

kubectl apply init /root/config directory --config-annotation "project=myproject"
# Do some work
kubectl apply -f /root/config directory
# All resources created with annotation "project=myproject"
# Delete some resources...
kubectl apply --prune -f /root/config directory
# All resources with the "project=myproject" annotation are deleted.  No messy label selectors need to be specified.