kubernetes
KEP-2872 Add keystone containers KEP
Signed-off-by: Aditi Sharma [email protected]
- One-line PR description: Add KEP for keystone containers
- Issue link: https://github.com/kubernetes/enhancements/issues/2872
- Other comments: Discussion on sig-node slack https://kubernetes.slack.com/archives/C0BP8PW9G/p1620304727344300
@thockin We would like to have your early input on this idea since you looked into the sidecar problems in depth and also authored one sidecar proposal
/cc @thockin
Why is this annotation based instead of a proper (alpha feature gated) field on the pod? What value does starting off as an annotation provide?
The idea here is to not introduce a new api change before we move to beta...
The idea here is to not introduce a new api change before we move to beta...
The annotation is a new API, just a harder to discover and reason about one because it lacks proper typing through a field. You can also do proper conversions from an alpha field to a beta field.
One nice thing about an annotation is as a webhook-injecting-sidecar implementor, I can just put it on all pods and if the cluster supports it - great. With a flag guarded field there is no reliable way to know if it's supported or not (?)
One nice thing about an annotation is as a webhook-injecting-sidecar implementor, I can just put it on all pods and if the cluster supports it - great. With a flag guarded field there is no reliable way to know if it's supported or not (?)
I do not understand what you mean. You can simply set the field in the same way you set the annotation. The server will ignore it if it is unsupported.
I do not understand what you mean. You can simply set the field in the same way you set the annotation. The server will ignore it if it is unsupported.
I thought for an alpha feature a flag had to be enable for the field to be set? If that is not the case maybe its not too bad. At the very least we would need a api-server version check and conditional set it though, although that is not a big deal
@howardjohn there would be feature gate to enable weather we set sidecars using annotation or using api field @enj We are not against api fields, seeing the issues sig-node faces when a core code is changed we decided to use annotation for now and later if all goes well promote it to api field. However if majority agrees on having api field I'll update the KEP
We are not against api fields, seeing the issues sig-node faces when a core code is changed we decided to use annotation
This sounds like an unrelated systemic problem? Do you have specific examples of issues encountered?
Thanks for attacking this again.
Do we really need more than one keystone container?
Probably not for jobs, but when the main functionality is achieved in conjuction (like a git repo and a web server serving those files), it might be desired to have more than one.
I think both existing pre-proposals allow for more than one "keystone" container in some way or another, so allowing that here seems to better fit with that. I'm fine if for jobs, that is unlikely that we will need more than one, we allow only for one and whenever some pre-proposal is implemented, then we can allow a more generic case with more than one keystone container.
@thockin what do you think?
Why an annotation? Once we put this out for people, it becomes an API we support forever. Annotations are terrible APIs.
I agree. The idea of annotations come several times, sometimes as an easy way to experiment with this while a more in depth approach (like the pre-proposals you and I wrote) is chosen. The downside of using a field for this, is that it might not be very useful in the future if we implement one of the pre-proposals. That's why here we are using an annotation, besides the reasons that were mentioned here AFAIK.
Do we really need more than one keystone container?
Probably not for jobs, but when the main functionality is achieved in conjuction (like a git repo and a web server serving those files), it might be desired to have more than one.
The "sync from git" pattern is also a sidecar and should want to not block turndown of a pod, right? I am not saying there's not a use-case for two keystone containers, but maybe we could start by limiting it to one and then waiting for the further use-case to emerge? Of course, we would want to leave room in the API for more than one.
I think both existing pre-proposals allow for more than one "keystone" container in some way or another, so allowing that here seems to better fit with that. I'm fine if for jobs, that is unlikely that we will need more than one, we allow only for one and whenever some pre-proposal is implemented, then we can allow a more generic case with more than one keystone container.
@thockin what do you think?
Precisely where my head was. We can "artificially" limit it to one for now (and avoid defining semantics for > 1) and then lift that limit later. I think.
Why an annotation? Once we put this out for people, it becomes an API we support forever. Annotations are terrible APIs.
I agree. The idea of annotations come several times, sometimes as an easy way to experiment with this while a more in depth approach (like the pre-proposals you and I wrote) is chosen. The downside of using a field for this, is that it might not be very useful in the future if we implement one of the pre-proposals. That's why here we are using an annotation, besides the reasons that were mentioned here AFAIK.
So, to be clear, this annotation would NEVER move beyond Alpha? It would be a data-gathering exercise at best. Users would have to go out of their way to turn it on and it would come with no warranty at all. I could be OK with that, and it certainly reduces the risk of being stuck with it forever.
I'm still left with a question of whether sidecars indicate "I am not a keystone" or users indicate "I am a keystone". In the limit, the less we ask users to do, the better - right? So we'd want to position this as "I am a sidecar" - which feels like the previous sidecar proposal but more limited (in a good way). Yes or no?
Precisely where my head was. We can "artificially" limit it to one for now (and avoid defining semantics for > 1) and then lift that limit later. I think.
LGTM
So, to be clear, this annotation would NEVER move beyond Alpha? It would be a data-gathering exercise at best. Users would have to go out of their way to turn it on and it would come with no warranty at all. I could be OK with that, and it certainly reduces the risk of being stuck with it forever.
I guess not moving out of alpha makes sense to me. We can change our minds and move to a pod.spec field if we feel this is better than any pre-proposal or something, but seems quite unlikely.
I'm still left with a question of whether sidecars indicate "I am not a keystone" or users indicate "I am a keystone". In the limit, the less we ask users to do, the better - right? So we'd want to position this as "I am a sidecar" - which feels like the previous sidecar proposal but more limited (in a good way). Yes or no?
Yes, it is like the previous but more limited as this only tackles jobs. The previous one tackles jobs and regular pod startups/shutdown.
I think "I'm keystone" makes more sense than "I'm sidecar" if we want to force only one keystone for now (with room to grow that later). We can limit the annotation key too if go go down that route, and clearly express that there is one for now. Also, given that we plan to gather data out of this, having it be "I'm keystone" will gather us more data than "I'm sidecar", as we can know of use cases with more than one keystone is needed.
If we make it "I'm sidecar", then it will be messy to make sure that exactly only one container is not marked as sidecar.
I agree that when we move to the pod.spec, this really smells like having it be "I'm sidecar" is the way to go. But to limit the annotation cardinality, limit this to 1 for now and gather more data, it seems having it be "I'm keystone" is more helpful.
@thockin What do you think?
As a long time follower of this one I’d just reiterate that many people are using kubexit to achieve what we need. In the context of Jobs, this mainly looks like one or more support containers that do not naturally shut down and need to be terminated when the keystone exits. Many of these containers will not respond to sigterm, or any signal, with a 0 exit code, so it’s also desirable that they not affect the usability of the Job sucesss/failure status.
All that to say: kubexit and it’s features/feature requests should be considered the v0 IMO.
@iamnoah thank you very much for your input too! I'll have a closer look at kubexit (I was not aware of it, it seems the first commit was in April 2020, so probably not quite popular when most of the sidecar KEP discussion in 2020 happened)
@ehashman could you please review the KEP when possible, since you seem to be the official reviewer. Thanks!
@ehashman thanks for your review. I have addressed most of your comments, I'm just waiting for @adisky to give me write access to her branch to push.
@ehashman thanks for your review. I have addressed most of your comments, I'm just waiting for @adisky to give me write access to her branch to push.
@ehashman done
The Kubernetes project currently lacks enough contributors to adequately respond to all issues and PRs.
This bot triages issues and PRs according to the following rules:
- After 90d of inactivity,
lifecycle/staleis applied - After 30d of inactivity since
lifecycle/stalewas applied,lifecycle/rottenis applied - After 30d of inactivity since
lifecycle/rottenwas applied, the issue is closed
You can:
- Mark this issue or PR as fresh with
/remove-lifecycle stale - Mark this issue or PR as rotten with
/lifecycle rotten - Close this issue or PR with
/close - Offer to help out with Issue Triage
Please send feedback to sig-contributor-experience at kubernetes/community.
/lifecycle stale
