add kep for better pruning in apply

[Liujingfang1]

This KEP proposes a different approach of kubectl apply --prune.

fixes kubernetes/kubectl#572

[Liujingfang1]

/assign @pwittrock @soltysh @seans3

[barney-s]

1. The conceptualized root-object seems to be a grouping construct that is similar to application object (https://github.com/kubernetes-sigs/application) albeit different purpose. I would prefer if it was application object.

2. Consider injecting an unique label when applying. Say apply introduces "label-applied-context" uuid. Its the same on all objects in that apply command. This could help with tracking objects that are removed from dir but exist in the server side. If the label exists for even 1 object server side it is reused for all new objects as well.

[barney-s]

Link to issues broken. Would you please add the issues you think need to be addressed.

[Liujingfang1]

Here is the issue kubernetes/kubectl#572. I'll update the link

[barney-s]

Is supporting working across directories an explicit goal.
If an user has conflicting labels across directories isn't that an user error ?

[Liujingfang1]

Is supporting working across directories an explicit goal.

Yes

If an user has conflicting labels across directories isn't that an user error ?

It could be, but conflicting labels are sometimes not easy to discover. The point here is that we need to provide a solution in kubectl, which will avoid errors that are easy to make and hard to detect.

[crimsonfaith91]

User error can be challenging to avoid. It is hard to differentiate whether the error is really an error or a user intent. IMO, we should at least express it clearly how prune works for some popular user errors.

[barney-s]

Could we leverage https://github.com/kubernetes-sigs/application for this ?

[smarterclayton]

I would prefer a resource 100% of clusters are guaranteed to have installed.

[barney-s]

ack

[bgrant0607]

Another issue is that apply isn't just for applications. It's for arbitrary resource types and groupings.

[barney-s]

Is this a client side operator (kubectl) or a server side one ?

[Liujingfang1]

This is kubectl. There is not server side actions.

[easeway]

ConfigMap is namespace scoped, does that mean --prune can only be used for deleting namespace scoped objects?

[Liujingfang1]

No, --prune will be used for all objects. ConfigMap is chosen since it has data field which can hold arbitrary data.

[pwittrock]

I think the question is, what namespace with the configmap live in for non-namespaced objects?

[easeway]

As ConfigMap is namespace scoped, is namespace is still required in each entry? From the example, is it allowed to specify objects in a different namespace from the ConfigMap?

If we allow Root object to specify resources across multiple namespaces, or even cluster scoped resources, it's better to use a cluster scoped CRD, instead of namespace scoped ConfigMap. But using a cluster scoped CRD requires additional care of setting up the correct RBAC to allow user tools to work properly (limiting the access on cluster level is more complicated and needs to be more careful than inside a namespace).

[Liujingfang1]

The namespace of the root object itself can be different from the namespace listed in data. So namespace is required in each entry.

Using a CRD requirest that the CRD is installed in the cluster. In addition, CRD based approach doesn't work with old version of clusters.

[easeway]

Does this imply the existing behavior of --prune when root object is missing? Will it be safer if root object is always required?

[Liujingfang1]

Does this imply the existing behavior of --prune when root object is missing?

When the root object is missing, kubectl will skip pruning part and no object is deleted. Since the proposed solution is quite different from existing behavior. I don't think it's good to keep both of them.

Will it be safer if root object is always required?

It could be. If the root object is not present, no object is deleted at each apply time. Eventually the cluster will need a manual clean up. If the user has a good strategy of cleaning up, it is safe for them to do so. Otherwise, using a root object every time is better.

[easeway]

The new objects and old objects are confusing to me (sorry, if I missed something). My understanding is the actual objects are used to construct one set, and those defined in root object data field are used to construct the other set. Which one is treated as new set and which one is the old set?

[Liujingfang1]

The new set is the set of objects to be applied. The old set is the set of objects running on cluster, which were applied previously.

The new objects are the child objects only in the new set. The old objects are the child objects only in the old set.

[crimsonfaith91]

I see the point why name of resource is needed for combining the new and old sets. If we want to integrate with App CRD, will the extra listing operation (based on GK details, assuming all underlying resources are within same namespace as App CR) expensive?

As synced by person, users may not know name of all underlying resources, but we can leverage controllers to generate App CRs, so supporting name for App CRD is a viable option too (i.e., I agree with you).

[easeway]

So every object must have this label to be associated with the root object and thus the tool is able to construct the new set and old set? I guess I understand how the new set and old set are constructed.

But I still feel it will be safer that the root object must be consistent with the actual objects in the system before --prune is allowed to proceed. The tools creating child objects are responsible for updating the root object to make it consistent. Otherwise it's still possible for making mistakes.

[Liujingfang1]

But I still feel it will be safer that the root object must be consistent with the actual objects in the system before --prune is allowed to proceed. The tools creating child objects are responsible for updating the root object to make it consistent. Otherwise it's still possible for making mistakes.

By this proposal, Kubectl will assume the root object is consistent with the actual objects. I agree with you that the tools are responsible to make sure this is true.

[smarterclayton]

Won’t work - config map names can be longer than allowed label length.

You’ll need to use an annotation if you want to do data like this.

[kow3ns]

iiuc the plan would be to auto-generate a fixed length name for the "root object" based on the (I'm guessing) the has of its contents and deal with collisions the same way we do with replica-sets. We could mandate that implementations ensure that the name of the ConfigMap for a "root object" is no greater than the allowable 63 chars.

[Liujingfang1]

Thank you for pointing out. I'll update it to use annotation.

[smarterclayton]

There are plenty of other extension types now like configmap and secret. How do they get blacklisted?

[kow3ns]

iiuc the objects that will participate in the prune must also be white listed by having them select the root object and by including them in the root object by GVKN? Does this proposal allow users to create objects (e.g. ConfigMaps and Secrets) that will not participate in pruning? If so are you really saying that you will (in addition to requiring white listing) allow for the implementation of explicit black listing in the future?

[Liujingfang1]

We can also add the blacklist types into data to allow customizing the blacklist.

data:
  object1: ...
  object2:...
  object3:...
  blacklist1: namespace1:group1:version1:kind1
  blacklist2: namespace2:group2:version2:kind2

[pwittrock]

Instead of blacklisting, would it work to only perform deletion after rollout has been completed if we could figure out how to detect that? I can imagine other resources being dependencies that shouldn't be deleted until a rollout is complete.

@smarterclayton

There are plenty of other extension types now like configmap and secret. How do they get blacklisted?

I believe the issue with configmaps and secrets that is being addressed is due to 2 factors - 1) some tools generate them based on the contents of another file - so they are likely to be replaced by a new instance without the user explicitly deleting them and 2) the old instances will be referenced by Pods that may not have been rolled out during a rolling update. Pruning them would then delete them before they stopped being used by these Pods.

Are you thinking that this issue or other issues might apply to extension or other types?

[pwittrock]

nm, I think we could do this without whitelisting.

[mattfarina]

If someone has multiple applications (think of it as a term for grouping) in a namespace, will a prune of application A cause the assets for application B to be deleted? Unless they know about each other and have them blacklisted from pruning?

[mattfarina]

Ah, nevermind... I see the diff discussed elsewhere. Ignore the prev comment.

[kow3ns]

iiuc the plan would be to auto-generate a fixed length name for the "root object" based on the (I'm guessing) the has of its contents and deal with collisions the same way we do with replica-sets. We could mandate that implementations ensure that the name of the ConfigMap for a "root object" is no greater than the allowable 63 chars.

[kow3ns]

I think this operation is idempotent. If it is you should call that out. If it isn't you should explain where multiple applications may lead to a different result. I can't, off the top of my head, think of any circumstance where an intermittent failure can't be corrected by subsequent invocations of the command on the same input. This seems to be the desired intent of the design. If so, you should call this out specifically. If not, it deserves careful thought.

[Liujingfang1]

I was considering a scenario when an object, belonging to some root object, was updated/applied by other means(either manually, or name conflicting with an object belonging to another root object). In that case, the label/annotation will be different. This gives us a signal that this object has been touched by others, or owned by others. It may not be safe to delete it.

[crimsonfaith91]

Will UUID a solution here?

[kow3ns]

iiuc the objects that will participate in the prune must also be white listed by having them select the root object and by including them in the root object by GVKN? Does this proposal allow users to create objects (e.g. ConfigMaps and Secrets) that will not participate in pruning? If so are you really saying that you will (in addition to requiring white listing) allow for the implementation of explicit black listing in the future?

[kow3ns]

You don't need V. GKN is sufficient. The version of the resource can be selected by the client and available versions can be discovered dynamically.

[Liujingfang1]

Thank you for pointing out. I'll update it.

[barney-s]

Could you give an example of this scenario ?

[Liujingfang1]

One example is a ConfigMap object being referred in a Deployment object. When rolling update happens, new pods refer to new ConfigMap and old pods are still running and referring to old ConfigMap. The old ConfigMap can't be deleted until the rolling up successfully finishes.

[enisoc]

This seems to require higher-level tools to generate a unique string to use as the ConfigMap name. If you assume that such a string has already been generated and given to kubectl, couldn't you then add that unique string in a label/annotation on every object created during kubectl apply, and then do label-based pruning like in alpha, except that the user does not manually provide a label selector? That is, we instead generate a selector based on this unique string we've assumed to exist, and which identifies a particular "apply group" as defined by the user or user's higher-level tool.

What are the additional benefits to maintaining an explicit, denormalized list of live objects in a ConfigMap? Is it just concern over selecting matching objects across all resources and namespaces? That may be enough justification, but it would be good to make that trade-off clear if so.

[Liujingfang1]

What are the additional benefits to maintaining an explicit, denormalized list of live objects in a ConfigMap? Is it just concern over selecting matching objects across all resources and namespaces? That may be enough justification, but it would be good to make that trade-off clear if so.

If we use a unique label to find the objects as in alpha --prune, we have to query all namespaces, all types to find the matched objects. With a list of objects, the query step is not necessary any more.

[enisoc]

For people who currently use apply without --prune, I imagine it might be common to do something like:

kubectl apply -f dir/
# later ...
cd dir
edit fileA.yaml
kubectl apply -f fileA.yaml

If the user wants to start using the new prune feature, is this workflow still supported? Or is it a non-goal to support this, and the user will need to always kubectl apply -f dir (the whole directory)?

[Liujingfang1]

It will be supported.
By kubectl apply -f fileA.yaml, I guess the user only wants to update the resources in fileA.yaml.
Let's consider apply with pruning.

kubectl apply --prune -f dir/ # if there is a root object, it prunes obsolete objects  
# later ...
cd dir
edit fileA.yaml
kubectl apply --prune -f fileA.yaml # if there is a root object, it triggers pruning actions. Assume the root object doesn't change, no object is deleted.

[Liujingfang1]

Thanks for all your comments. I'd like to address a common one here: why not using an Application object as the root object.

There are some overlaps between the purpose of a root ConfigMap object and an Application object:

• Both are used as a root object and tend to link to a set of child objects
• Both are aimed to support better clean up
• Both are supposed to be work with other tools

Regardless the difference of core type v.s. a CRD or existence of other spec fields or status, the major difference between a root ConfigMap object and an Application object is how they are connected to the child objects.

• a root ConfigMap object lists all the child objects explicitly

  data:
   object1:  namespace1:group1:kind1:somename
   object2:  namespace1:group2:kind2:secondname
   object3:  namespace3:group3:kind3:thirdname
  

• an application object selects child objects by matching labels for specified types

  spec:
    selector:
      matchLabels:
       app.kubernetes.io/name: "wordpress-01"
    componentKinds:
    - group: core
      kind: Service
    - group: apps
      kind: Deployment
    - group: apps
      kind: StatefulSet
  

In this design, we avoid selecting objects by labels when dealing with prune. It relies on users/tools to get the explicit list of child objects and write it to a root ConfigMap object. So using Application object with label selector doesn't fit with this purpose. However, if application objects can list the child objects explicitly, it can act the same as a ConfigMap root object. The API need to be updated to include the child objects names, such as

spec:
  selector:
    matchLabels:
     app.kubernetes.io/name: "wordpress-01"
  componentKinds:
  - group: core
    kind: Service
    name: app-service
  - group: apps
    kind: Deployment
    name: app-deployment
  - group: apps
    kind: StatefulSet
    name: app-statefulset

By looking at the application object itself, kubectl can get the set of child objects. I noticed that Application CRD may need assigning owner reference to make sure garbage collection can clean up child objects. But that part need to be handled by controller. If the application CRD can include child object names, Kubectl apply --prune will clean up child objects from the client side by this design.

@barney-s, @kow3ns Let me know what you think about this. Thank you.

[k8s-ci-robot]

@Liujingfang1: Cannot trigger testing until a trusted user reviews the PR and leaves an /ok-to-test message.

In response to this:

Thanks for all your comments. I'd like to address a common one here: why not using an Application object as the root object.

There are some overlaps between the purpose of a root ConfigMap object and an Application object:

• Both are used as a root object and tend to link to a set of child objects
• Both are aimed to support better clean up
• Both are supposed to be work with other tools

Regardless the difference of core type v.s. a CRD or existence of other spec fields or status, the major difference between a root ConfigMap object and an Application object is how they are connected to the child objects.

• a root ConfigMap object lists all the child objects explicitly

data:
 object1:  namespace1:group1:kind1:somename
 object2:  namespace1:group2:kind2:secondname
 object3:  namespace3:group3:kind3:thirdname

• an application object selects child objects by matching labels for specified types

spec:
  selector:
    matchLabels:
     app.kubernetes.io/name: "wordpress-01"
  componentKinds:
  - group: core
    kind: Service
  - group: apps
    kind: Deployment
  - group: apps
    kind: StatefulSet

In this design, we avoid selecting objects by labels when dealing with prune. It relies on users/tools to get the explicit list of child objects and write it to a root ConfigMap object. So using Application object with label selector doesn't fit with this purpose. However, if application objects can list the child objects explicitly, it can act the same as a ConfigMap root object. The API need to be updated to include the child objects names, such as

spec:
  selector:
    matchLabels:
     app.kubernetes.io/name: "wordpress-01"
  componentKinds:
  - group: core
    kind: Service
    name: app-service
  - group: apps
    kind: Deployment
    name: app-deployment
  - group: apps
    kind: StatefulSet
    name: app-statefulset

By looking at the application object itself, kubectl can get the set of child objects. I noticed that Application CRD may need assigning owner reference to make sure garbage collection can clean up child objects. But that part need to be handled by controller. If the application CRD can include child object names, Kubectl apply --prune will clean up child objects from the client side by this design.

@barney-s, @kow3as Let me know what you think about this. Thank you.

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.

[crimsonfaith91]

Should we prioritize end-to-end tests rather than integration tests? The integration tests are a must-have, but I think they does not test the real-world situation.

[Liujingfang1]

Yes, you're right. I'll update it.

[crimsonfaith91]

Do you mean: "When kubectl apply is given name of a root object"?

For example: kubectl apply -f --prune -r <some-root-object-name> (adding -r flag for inputting the root object name)

[Liujingfang1]

We don't introduce new flags for this. It will be kubectl apply --prune -f. As long as a ConfigMap object is present in the resources passed by -f with a proper lable, Kubectl will treat it as a root object.

[crimsonfaith91]

Got it, thanks!

[crimsonfaith91]

nit: uage to usage

[crimsonfaith91]

IMO, requiring creation of a root object may potentially discourage most users. Furthermore, the users may set up the root object wrongly (e.g., missing some intended objects).

Does Kustomize and/or Helm support old clusters? If so, we may direct the users to them when the users want to perform --prune.

[Liujingfang1]

Kustomize doesn't talk with server. It only expands the resources and passes the output YAMLs to kubectl apply. helm has its own cli, and I believe it support old version clusters.

[crimsonfaith91]

Noted. Should we also consider integrating --prune with helm? If not, is there any reason why not? Thanks!

[Liujingfang1]

Not direct integration. We should provide a pruning solution in Kubectl that can be easily leveraged by other tools such as helm.

[pwittrock]

@Liujingfang1

We may need to restrict this to cases where all the resources are in the same namespace, or create 1 configmap per namespace.

An alternative to using a configmap is store the same data but in an annotation on one of the objects. This object would need to be the "root object", and couldn't be deleted without changing the root object first.

[Liujingfang1]

We may need to restrict this to cases where all the resources are in the same namespace, or create 1 configmap per namespace.

@pwittrock Creating one ConfigMap per namespace seems not necessary. Is it to keep different namespaces isolated? Considering a kubectl apply, resources could be in the same namespace, or different namespaces, or even with non-namespaced resources. Which cases are more likely? If we restrict to one cases, the pruning won't support other cases.
What about putting all configmap root objects in a different namespace kube-apply?

[Liujingfang1]

An alternative to using a configmap is store the same data but in an annotation on one of the objects. This object would need to be the "root object", and couldn't be deleted without changing the root object first.

@pwittrock Storing data in annotations looks good, but I don't think we can drop the root configmap object completely. It helps us to find live root object. This alternation works for Application, since application cr servers as the root object and it's not likely to be deleted when there are still other resources. For manifests without Application, we still need an extra object, such as a configmap object, to act as a root object. If user randomly pick one object from the resources as the root, it's easy to forget about reassigning the root object label.

[ekimekim]

We're currently using prune in combination with some simple internal tooling for generating resources. Our tool automatically labels the resources with a unique label associated with that group of generated resources (similar to a helm "release name"), then uses kubectl apply --prune to update existing resources and delete any with the same release label that don't exist in the updated set of resources.
We also use this label for other tasks like finding the existing set of objects and displaying a diff.

Under this proposal, if I understand correctly, we would need to additionally create a root object Configmap listing all our resources, and for getting the set of existing objects we would have to parse the server's current copy of that same configmap then look up each object explicitly.

This isn't unreasonable, but it's a much more complex implementation than the previous "add a label", and is harder for operators to inspect by hand using kubectl commands (as they can no longer simply search on a label).
It's also much harder to reason about.

For example, suppose I have two root configmaps A and B, and they both list the same object. Perhaps due to user error, or perhaps because the object was previously generated in release A and we wish to migrate it to release B (this is a scenario that is admittedly rare, but we have faced in practice). If I first apply B, so the object's root-object label now points to configmap B, what happens if I subsequently apply A with that object removed? Under the old system, the answer is easy: The labels on the object have changed, so it's no longer part of the A release, so it's unaffected. But under the new system, I have no idea. It depends on the implementation.

You could get back the same behaviour as the old system by always checking that the object points at the root configmap you're applying, but my point is that now we're dealing with duplicated state that can get out of sync and you need to start adding caveats and explanations to what should be a simple operation: Delete any matching things that I don't explicitly list.

I realise that there's a desire to make pruning more useful when kubectl applying whole directories, but I don't understand the benefit to the configmap approach when we could backwards-compatibly tie it into the existing system simply by generating a unique label (which we would need to do anyway to produce a unique root configmap object name) and matching on it, as suggested in one of the comments above. The cost of doing a label query over all namespaces and types is mentioned, but I don't think the drastically increased complexity and number of edge cases is worth the minor performance benefit.

[hazcod]

/remove-lifecycle stale

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[hazcod]

/remove-lifecycle rotten

[stefanprodan]

In case someone needs an alternative to the current punning implementation, I made a kubectl plugin that prunes builtin resources as well as custom resources: https://github.com/stefanprodan/kustomizer

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[tonybenchsci]

What's the latest on kubectl --prune? We'd like to use this in combination with kustomize

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

[eestewart]

/remove-lifecycle rotten

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale

[fejta-bot]

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle rotten

[fejta-bot]

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

[k8s-ci-robot]

@fejta-bot: Closed this PR.

In response to this:

Rotten issues close after 30d of inactivity.
Reopen the issue with /reopen.
Mark the issue as fresh with /remove-lifecycle rotten.

Send feedback to sig-contributor-experience at kubernetes/community.
/close

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.