kubectl: implement kubectl apply --prune #33075

mikedanese · 2016-09-20T08:18:25Z

Closes #19805

Depends on #32599

@errordeveloper @lukemarsden

This change is

errordeveloper · 2016-09-20T08:25:37Z

Awesome! Thanks, @mikedanese.

errordeveloper

Few nits on UX and readability...

errordeveloper · 2016-09-20T08:32:32Z

pkg/kubectl/cmd/apply.go

 	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")
+	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).")
+	cmd.Flags().IntVar(&options.GracePeriod, "grace-period", -1, "Period of time in seconds given to pruned resources to terminate gracefully. Ignored if negative.")


Would it be more user-friendly to say "Wait forever if negative."? (Given I even understood this correctly).

It's actually defaulted by the apiserver, not wait forever but I can say something to that effect

errordeveloper · 2016-09-20T08:34:40Z

pkg/kubectl/cmd/apply.go

 	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")
+	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).")


When a user may need to use this? May be --no-cascaded-prune would be a better name for this.

--cascade is already used in multiple other commands in kubectl. I think continuity of experience outweighs a nice descriptive flag.

When a user may need to use this?

This is the default behavior. When we prime a deployment, we should also prune it's replicasets or they will become orphaned and unmanaged.

errordeveloper · 2016-09-20T08:39:31Z

pkg/kubectl/resource/builder.go

@@ -538,6 +538,41 @@ func (b *Builder) visitorResult() *Result {
 		b.selector = labels.Everything()
 	}

+	// visit items specified by paths


Is this code simply moved from down below, or it also contains some changes? May be good idea to make a separate PR for moving this first. Also, this looks like a ginormous func and some parts of it can be broken out,

errordeveloper · 2016-09-20T08:44:52Z

pkg/kubectl/cmd/apply.go

+		return nil
+	}
+
+	selector, err := labels.Parse(options.Selector)


Something like return visitAndPruneIfNeeded(options.Selector) could be easier to read, wdyt?

👍 for separate function, but I'd rather have it return err and check it here, than what you propose.

errordeveloper · 2016-09-20T08:47:46Z

pkg/kubectl/cmd/apply.go

+	return nil
+}
+
+type pruner struct {


To me it'd be perfectly reasonable to put have this in a separate file, when I was trying to read apply.go last time, it seemed rather big and not quite structured.

mikedanese · 2016-09-20T15:29:31Z

@kubernetes/kubectl

deads2k · 2016-09-21T13:25:44Z

@fabianofranz fyi

fabianofranz · 2016-09-21T18:35:59Z

pkg/kubectl/cmd/apply.go

@@ -158,6 +188,12 @@ func RunApply(f *cmdutil.Factory, cmd *cobra.Command, out io.Writer, options *re
 			if err := createAndRefresh(info); err != nil {
 				return cmdutil.AddSourceToErr("creating", info.Source, err)
 			}
+			if uid, err := info.Mapping.UID(info.Object); err != nil {
+				glog.Infof("here")


fabianofranz · 2016-09-21T18:36:11Z

pkg/kubectl/cmd/apply.go

@@ -183,6 +219,12 @@ func RunApply(f *cmdutil.Factory, cmd *cobra.Command, out io.Writer, options *re
 			}
 		}

+		if uid, err := info.Mapping.UID(info.Object); err != nil {
+			glog.Infof("here")


fabianofranz · 2016-09-21T18:37:05Z

pkg/kubectl/cmd/apply.go

+	for _, obj := range objs {
+		uid, err := mapping.UID(obj)
+		if err != nil {
+			glog.Infof("here")


fabianofranz · 2016-09-21T18:37:12Z

pkg/kubectl/cmd/apply.go

+
+		name, err := mapping.Name(obj)
+		if err != nil {
+			glog.Infof("here")


MrHohn · 2016-09-27T01:31:42Z

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?

mikedanese · 2016-09-27T01:38:57Z

@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.

MrHohn · 2016-09-27T01:41:26Z

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

MrHohn · 2016-09-27T01:44:01Z

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

mikedanese · 2016-09-27T01:45:51Z

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.

MrHohn · 2016-09-27T22:22:12Z

pkg/kubectl/cmd/apply.go

+		if err := p.prune(api.NamespaceNone, m, shortOutput); err != nil {
+			return fmt.Errorf("error pruning objects: %v", err)
+		}
+	}


Is using visitedRESTMappings here enough to cover all cases?

Given a YAML file which has a ReplicationController before, and now it is replaced with a Deployment. If we need to prune the RC, I think this logic will fail? Because resource kind RC will not be visited this time -> RCs will not be listed by pruner -> fail to prune existing RC?

Good point. I think we might need to add a --types flag to explicitly override visitedRESTMappings.

pwittrock · 2016-10-05T18:21:58Z

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

pwittrock · 2016-10-05T18:23:29Z

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

mikedanese · 2016-10-05T18:57:28Z

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?

k8s-ci-robot · 2016-10-06T22:04:37Z

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.

MrHohn · 2016-10-06T22:19:55Z

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.

pwittrock · 2016-10-07T01:13:23Z

pkg/kubectl/cmd/apply.go

-		JSON and YAML formats are accepted.`)
+		JSON and YAML formats are accepted.
+
+		Alpha Disclaimer: the --prune functionality is not yet complete. Do not use unless you are aware of what the current state is. See https://issues.k8s.io/34274.`)


pwittrock · 2016-10-07T01:14:43Z

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.

pwittrock · 2016-10-07T02:43:40Z

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

pwittrock · 2016-10-07T02:44:10Z

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-github-robot · 2016-10-07T03:15:14Z

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

k8s-github-robot · 2016-10-07T03:53:00Z

Automatic merge from submit-queue

mikedanese · 2016-10-07T06:53:35Z

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

bgrant0607 · 2016-10-07T21:52:26Z

@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.

MrHohn · 2016-10-07T22:08:59Z

@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.

@mikedanese

Automatic merge from submit-queue Upgrade addon-manager with kubectl apply The first step of #33698. Use `kubectl apply` to replace addon-manager's previous logic. The most important issue this PR is targeting is the upgrade from 1.4 to 1.5. Procedure as below: 1. Precondition: After the master is upgraded, new addon-manager starts and all the old resources on nodes are running normally. 2. Annotate the old ReplicationController resources with kubectl.kubernetes.io/last-applied-configuration="" 3. Call `kubectl apply --prune=false` on addons folder to create new addons, including the new Deployments. 4. Wait for one minute for new addons to be spinned up. 5. Enter the periodical loop of `kubectl apply --prune=true`. The old RCs will be pruned at the first call. Procedure of a normal startup: 1. Addon-manager starts and no addon resources are running. 2. Annotate nothing. 3. Call `kubectl apply --prune=false` to create all new addons. 4. No need to explain the remain. Remained Issues: - Need to add `--type` flag to `kubectl apply --prune`, mentioned [here](#33075 (comment)). - This addon manager is not working properly with the current Deployment heapster, which runs [addon-resizer](https://github.com/kubernetes/contrib/tree/master/addon-resizer) in the same pod and changes resource limit configuration through the apiserver. `kubectl apply` fights with the addon-resizers. May be we should remove the initial resource limit field in the configuration file for this specific Deployment as we removed the replica count. @mikedanese @thockin @bprashanth --- Below are some logical things that may need to be clarified, feel free to **OMIT** them as they are too verbose: - For upgrade, the old RCs will not fight with the new Deployments during the overlap period even if they use the same label in template: - Deployment will not recognize the old pods because it need to match an additional "pod-template-hash" label. - ReplicationController will not manage the new pods (created by deployment) because the [`controllerRef`](https://github.com/kubernetes/kubernetes/blob/master/docs/proposals/controller-ref.md) feature. - As we are moving all addons to Deployment, all old RCs would be removed. Attach empty annotation to RCs is only for letting `kubectl apply --prune` to recognize them, the content does not matter. - We might need to also annotate other resource types if we plan to upgrade them in 1.5 release: - They don't need to be attached this fake annotation if they remain in the same name. `kubectl apply` can recognize them by name/type/namespace. - In the other case, attaching empty annotations to them will still work. As the plan is to use label selector for annotate, some innocence old resources may also be attached empty annotations, they work as below two cases: - Resources that need to be bumped up to a newer version (mainly due to some significant update --- change disallowed fields --- that could not be managed by the update feature of `kubectl apply`) are good to go with this fake annotation, as old resources will be deleted and new sources will be created. The content in annotation does not matter. - Resources that need to stay inside the management of `kubectl apply` is also good to go. As `kubectl apply` will [generate a 3-way merge patch](https://github.com/kubernetes/kubernetes/blob/master/pkg/util/strategicpatch/patch.go#L1202-L1226). This empty annotation is harmless enough.

ghodss · 2016-10-19T00:48:56Z

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?

pwittrock · 2016-10-19T01:26:12Z

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?

bgrant0607 · 2016-10-19T04:07:24Z

@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.

pwittrock · 2016-10-19T04:47:57Z

Yup I meant --prune. Thanks.

pwittrock · 2016-10-19T04:55:42Z

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.

googlebot added the cla: yes label Sep 20, 2016

k8s-github-robot assigned smarterclayton Sep 20, 2016

k8s-github-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. release-note-label-needed labels Sep 20, 2016

mikedanese force-pushed the kubectl-prune branch from 9745ed6 to a279a04 Compare September 20, 2016 08:25

errordeveloper reviewed Sep 20, 2016

View reviewed changes

fabianofranz reviewed Sep 21, 2016

View reviewed changes

mikedanese force-pushed the kubectl-prune branch 2 times, most recently from 53498d9 to 05d68d2 Compare September 21, 2016 20:28

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2016

mikedanese mentioned this pull request Sep 26, 2016

Proposal: Enable purging resources in kubectl apply #29551

Closed

MrHohn reviewed Sep 27, 2016

View reviewed changes

MrHohn mentioned this pull request Sep 28, 2016

Migrating addons from RCs to Deployments. #33698

Closed

bgrant0607 assigned bgrant0607 and pwittrock Oct 3, 2016

mikedanese force-pushed the kubectl-prune branch from 05d68d2 to 8e8f5ff Compare October 5, 2016 19:41

mikedanese force-pushed the kubectl-prune branch from a344d1d to 2eebb30 Compare October 7, 2016 00:46

mikedanese added 2 commits October 6, 2016 17:49

implement kubectl apply --prune

ea5ecc4

add a test for kubectl apply --prune

62960aa

mikedanese force-pushed the kubectl-prune branch from 2eebb30 to 62960aa Compare October 7, 2016 00:49

pwittrock approved these changes Oct 7, 2016

View reviewed changes

pwittrock added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 7, 2016

k8s-github-robot merged commit 56a22f9 into kubernetes:master Oct 7, 2016

mikedanese deleted the kubectl-prune branch October 7, 2016 03:54

mikedanese restored the kubectl-prune branch October 7, 2016 17:14

mikedanese deleted the kubectl-prune branch October 7, 2016 17:14

MrHohn mentioned this pull request Oct 11, 2016

Upgrade addon-manager with kubectl apply #34513

Merged

smarterclayton mentioned this pull request Oct 11, 2016

Support deleting previous objects in kubectl apply kubernetes/enhancements#128

Closed

23 tasks

aaronlevy mentioned this pull request Nov 17, 2016

Start using kube-addon-manager coreos/coreos-kubernetes#494

Open

mengqiy mentioned this pull request Jan 27, 2017

Unable to delete stale objects with kubectl apply --prune when no object passed in. #40635

Closed

kubectl: implement kubectl apply --prune #33075

kubectl: implement kubectl apply --prune #33075

Conversation

mikedanese commented Sep 20, 2016 • edited

errordeveloper commented Sep 20, 2016

errordeveloper left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

errordeveloper Sep 20, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikedanese commented Sep 20, 2016

deads2k commented Sep 21, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MrHohn commented Sep 27, 2016 • edited

mikedanese commented Sep 27, 2016

MrHohn commented Sep 27, 2016

MrHohn commented Sep 27, 2016

mikedanese commented Sep 27, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pwittrock commented Oct 5, 2016

pwittrock commented Oct 5, 2016

mikedanese commented Oct 5, 2016

k8s-ci-robot commented Oct 6, 2016

MrHohn commented Oct 6, 2016 • edited

Choose a reason for hiding this comment

pwittrock commented Oct 7, 2016

pwittrock commented Oct 7, 2016

pwittrock commented Oct 7, 2016

k8s-github-robot commented Oct 7, 2016

k8s-github-robot commented Oct 7, 2016

mikedanese commented Oct 7, 2016

bgrant0607 commented Oct 7, 2016

MrHohn commented Oct 7, 2016

ghodss commented Oct 19, 2016

pwittrock commented Oct 19, 2016

bgrant0607 commented Oct 19, 2016

pwittrock commented Oct 19, 2016

pwittrock commented Oct 19, 2016

mikedanese commented Sep 20, 2016 •

edited

errordeveloper Sep 20, 2016 •

edited

MrHohn commented Sep 27, 2016 •

edited

mikedanese commented Sep 27, 2016 •

edited

MrHohn commented Oct 6, 2016 •

edited