KEP-4368: Job API managed-by label

[mimowo]

• One-line PR description: Proposes Job API label for Multi-Kueue

• Issue link: Job API managed-by mechanism #4368

• Other comments:

[kikisdeliveryservice]

Is there a reason or some background as to why this is going straight to beta?

[mimowo]

The base-functionality implementation is very simple, just short-circuit the syncJob invocation. Also, I think going via Alpha is a way to allow for version skew between kube-apiservers in HA setup. To ensure that all apiserver instances recognize the new field. However, since this is a label, there is no such need technically.

Finally, we would like to have the feature soak without unnecessary delay.

I have added a note along the lines. PTAL.

[liggitt]

I'm not sure about requiring the label on all jobs. I was expecting the normal default controller to keep working as-is for jobs without the label, and to switch to excluding jobs with the label, without needing to decorate or modify any existing jobs. That would mean that only exceptional jobs that wanted to be managed by a non-default controller would opt into the label.

We also default the label for all newly created jobs

Does this mean 1) we'd set a label on a job created by a user, or 2) our controllers that create jobs (e.g. cronjob controller) would populate a label? If 1, is there precedence for that? What component would default that and how would it know what job controller name the "default" controller was running as?

[mimowo]

I'm not sure about requiring the label on all jobs. I was expecting the normal default controller to keep working as-is for jobs without the label, and to switch to excluding jobs with the label, without needing to decorate or modify any existing jobs. That would mean that only exceptional jobs that wanted to be managed by a non-default controller would opt into the label.

Makes sense to me. This requirement complicates defaulting and would prolong graduation. Let's consider how it can be avoided.

One question is if the built-in controller should handle jobs with managed-by: job-controller? I assume yes, by analogy to prior work on managed-by label.

So, if we do with simple ifs to skip synchronization, then it is simple. If we want to use label selectors for jobs, then we can probably do it with two informers, one for no label, and one for job-controller value.

I will adjust the KEP assuming we don't require it.

[wojtek-t]

I don't think we want to change the fact that we're using shared-informers. If that is true, then simple "if skip" is the only path forward.

[liggitt]

I like the simplicity of the surface area here. Some benefits of the label approach over a field:

• it generalizes to custom resources as well
• it could consistently live in the same place in multiple objects (all objects have metadata.labels)

The main downside I see is that there's no signal in openapi whether the cluster is new enough to be aware of the controller subdivision. That's largely a problem time would cure.

I wonder if it would make sense to teach all the core workload controllers a generic version of this (kubernetes.io/managed-by) on their parent objects. That would enable use of multiple controllers on the same type, with the default (no managed-by label) meaning "use the default controller for this type".

[mimowo]

Thank you!

The main downside I see is that there's no signal in openapi whether the cluster is new enough to be aware of the controller subdivision. That's largely a problem time would cure.

Indeed, I added a note on the related risk in the Two controllers running at the same time on old version section

[soltysh]

I wonder if it would make sense to teach all the core workload controllers a generic version of this (kubernetes.io/managed-by) on their parent objects. That would enable use of multiple controllers on the same type, with the default (no managed-by label) meaning "use the default controller for this type".

@atiratree this ^ would probably be a good goal for sig-apps long term to address.

[atiratree]

It is an interesting idea. I suppose we could consider it, if there were clear benefits and a clearly defined process and contract for the ownership.

The downsides mentioned in this KEP are still quite significant to support this in all controllers. I think CRDs are still preferable for extending workloads like Deployments, StatefulSets, etc.

I know we have conformance tests, but that doesn't stop people from forking the controllers and implementing incompatible behavior. And API validation has its limits. This would then be problematic for the consumers of these workloads who would either be unprepared and unable to react in a safe and bug-free manner. And if they could, they would need extra logic and switch the behavior according to the current controller. They would also run into a similar problem if they found a controller they do not support.

As we see for example with StatefulSets and their need for migration. I expect the same need to arise for these controllers. E.g. I want to migrate from a controller A to a controller B for a new shiny functionality. So sooner or later we might make the managed label/field mutable. This would probably require some kind of locking, so that two controller would not operate on the same object. Even in mid change. And to consider rogue controllers.

The benefit for Jobs is that it is used as a building block in many projects, and we do not expect people to fork the controller. Not sure if this applies elsewhere.

@mimowo can we add the following to the GA graduation criteria?

Asses the fragmentation of the ecosystem. Look for other implementations of a job controller and asses their conformance with k8s.

[mimowo]

Makes sense, added the point.

[mimowo]

Regarding the generic kubernetes.io/managed-by label. I would like to defer it for this KEP.

There is currently no clear use case to support the label across the stack for APIs like StatefulSets, Deployments, or DaemonSets.

A generic name without support across all k8s APIs might be confusing to the users, and supporting it for all k8s APIs would be much bigger effort than
currently needed for the MultiKueue scenario use.

The "managed-by" label idea has significant risks, such as "ecosystem fragmentation due to forks".
It makes sense to start with limited scope as a "pilot" and assess the impact.

I put the proposal, along with the reasons to defer in the Alternatives section.

[soltysh]

Regarding the generic kubernetes.io/managed-by label. I would like to defer it for this KEP.

Generic is definitely outside the scope of this KEP.

[liggitt]

One question is what happens if the label is mutated on an existing job? From the point of view of a controller filtering on a label selector, that will result in a "delete" event if the label mutates to no longer match its selector.

[wojtek-t]

Given that all our controllers are using SharedInformers anyway, I don't think that setting a selector is even possible. So the only thing we can effectively handle differently events where the label is set and that means that we will effectively be handling update in those cases.

[mimowo]

I wasn't sure how label selectors would work so deferred as a potential thing to reconsider for second Beta.

However, since we use SharedInformers we would keep all job objects in memory anyway. This greatly undermines the complicated idea of label selectors. I will update the KEP to just stay with the skip if within the syncJob function.

[mimowo]

I give up on the idea of using label selectors, added them to the alternatives section, along with the reason why. The main point is that we cannot really use them to make memory gains due to used shared-informers. They are also much more complex conceptually.

[wojtek-t]

Given that all our controllers are using SharedInformers anyway, I don't think that setting a selector is even possible. So the only thing we can effectively handle differently events where the label is set and that means that we will effectively be handling update in those cases.

[soltysh]

/label tide/merge-method-squash

[soltysh]

Some minor nits, but overall
/lgtm
/approve

[soltysh]

Strong 👍, if we're planning to allow external controllers update the status, we need to ensure that job won't start flapping between states.

[mimowo]

/assign @wojtek-t
For PRR review

[soltysh]

/lgtm

[wojtek-t]

Just two very small comments - other than that LGTM.

[wojtek-t]

/approve PRR

[will lgtm after fixing the nit]

[mimowo]

[will lgtm after fixing the nit]

done, also updated the checkpoints, PTAL (09321d0)

[wojtek-t]

/lgtm

@soltysh - for SIG-level approval [you only lgtm-ed it]

[soltysh]

@soltysh - for SIG-level approval [you only lgtm-ed it]

That's weird, I did that in #4370 (review) 🤷‍♂️ oh well, let me do that again.

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mimowo, soltysh, wojtek-t

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:

• keps/prod-readiness/OWNERS [wojtek-t]
• keps/sig-apps/OWNERS [soltysh]

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