Introducing the annotation shouldn't impact existing workloads unless they explicitly use it. I don't expect that users would create an indexed job that is tolerant to not having the index. So I don't really see the need for addressing this scenario.

This is more about the controller itself. It needs to do something with the existing Pods in terms of completion index tracking.

Please avoid using "scale" here, as Jobs don't follow the normal semantics for scaling. Ref kubernetes/kubernetes#60139

Fixed

Why? What's the problem with having the value referenced by two separate env vars?

I think it makes sense to give the user control over the names of the variables. A binary could always have some behavior defined for JOB_COMPLETION_INDEX which is different from the one provided by k8s.

I guess I wasn't clear. I wasn't asking to touch customer-defined variable.

I was just asking - why we don't want to provide JOB_COMPLETION_INDEX unconditionally (either with user-propagated-value if set by user or as above if not).

I don't see a problem if you have two env vars, say FOO and JOB_COMPLETION_INDEX with the same value set to the val of that annotation.

I understood you correctly :)

What I'm saying is: what if a JOB_COMPLETION_INDEX variable causes an undesirable effect on the user binary?

I guess in this case they could override that environment variable with another value.

Make the env name unique, KUBERNETES_JOB_INDEX, for example. User can always map that annotation to other pre-defined name, but we should always inject it.

I don't have a strong opinion about the name. But I'm still not convinced we should always inject the value if the user is already adding it.

We should always inject, unless user explicitly overwrites that name.

The behavior should be consistent - if you job is indexed, var X (I don't have strong opinion about name) will always be set (in your behavior you have "the var may or may not be set" - that's super confusing).

Sounds good. Removed second condition.

Can you explicitly describe how JobStatus is updated then (for completeness)?

Another question is: with the bitmask field and the proposal above, will we end-up with two separate fields for that?

Ohh - or you're saying that "we don't solve it for Alpha at all" - just rely on existence of all pods for now?

Correct. This would be a beta graduation criteria, pending on #2307

OK - if you're not touching JobStatus for now at all, that's fine.

I agree that tracking completed pods is orthogonal to this proposal.

I'm assuming that the reasoning behind this 10^6 is the underlying bitmask, right?
i.e. we're saying that tracking 10^6 items in bitmask takes O(125kB) and this is fine, right?

That sounds fine to me, but I would like to ensure that somewhere in the design section we will add a requirement that such big objects won't be updated frequently (e.g. not more often that once per couple seconds or something like that).

Yes, this is about being able to maintain the bitmask. Although, if we are going to use it to track completion, we actually need to cap .spec.completions too. The idea is that in the future we don't depend on lingering pods. @erictune thoughts?

e.g. not more often that once per couple seconds or something like that

Should we be doing some kind of client side throttling? In the current design, Job status updates would come after removing finalizers from finishing Pods, which already throttles Job updates in a way. But if one Pod finishes every 100ms (for example), we might still do more Job updates than what you are suggesting.

I changed the limit to be on completions, instead of parallelism

I don't understand how this is done (and how you distinguish downgrade from disabling exactly). Can you be more specific?

This is standard logic in API strategies for disabled feature gates. Reworded a bit.

I don't exactly distinguish a downgrade, as I cannot control the code of version N-1. So the paragraphs above describe how the code behaves in that scenario.

But what I can control is a feature being disabled, in which case I prefer to ignore Indexed Jobs that were already created.

If we're going to have something fancy for feature disablement, than we should have (at least) unit test for disablement.

You can see an example of doing that here (under review, but the idea is there):
https://github.com/kubernetes/kubernetes/pull/97058/files#diff-7826f7adbc1996a05ab52e3f5f02429e94b68ce6bce0dc534d1be636154fded3R246

That was the intention of my text here. Reworded.

I guess I wasn't clear. I wasn't asking to touch customer-defined variable.

I was just asking - why we don't want to provide JOB_COMPLETION_INDEX unconditionally (either with user-propagated-value if set by user or as above if not).

I don't see a problem if you have two env vars, say FOO and JOB_COMPLETION_INDEX with the same value set to the val of that annotation.

OK - if you're not touching JobStatus for now at all, that's fine.

Yes, this is about being able to maintain the bitmask. Although, if we are going to use it to track completion, we actually need to cap .spec.completions too. The idea is that in the future we don't depend on lingering pods. @erictune thoughts?

I don't think you need to limit completions. The whole points that Eric made in one of other comments proved that it's enough to limit parallelism [because if you won't be storing bitmask, but rather [a-b,c-d,e-f,...] format, there will be at most "parallelism` of such pairs, so the length of that is actually bounded by parallelism.
Bounding completions is much more limiting that limiting parallelism to something as high as 10^6.

Should we be doing some kind of client side throttling? In the current design, Job status updates would come after removing finalizers from finishing Pods, which already throttles Job updates in a way. But if one Pod finishes every 100ms (for example), we might still do more Job updates than what you are suggesting.

Yes - we should batch those, and send 1 update per X second. We can tune X later.

I don't think you need to limit completions. The whole points that Eric made in one of other comments proved that it's enough to limit parallelism [because if you won't be storing bitmask, but rather [a-b,c-d,e-f,...] format, there will be at most "parallelism` of such pairs, so the length of that is actually bounded by parallelism.

So it depends on the implementation. If we use a bitmask, then we have to limit completions. If we limit parallelism, we would have to use the compressed list format, but then the limit might have to be lower than 10^6, as we require more characters. We don't have to be concerned about display space, as we can just limit kubectl describe to print less characters.

@soltysh any thoughts?

Running 10^6 of pods at the same time is far from supported at this point.
Having more than 10^6 of pods within a single job sounds like a valid usecase to me.

So I would be much more comfortable saying that "parallelism <= 10^5" (so decreasing that by order of magnitude even), than by setting a limit on the completions

Moved back to limit parallelism and use a compressed list format instead of bitmask.

How exactly those are ignored? Are we killing the existing pods? Are we just not starting new ones? Are we stacking pods being finished?

Added specificity.

I'd propose creating a new CompletionModeType with pre-defined constants, like we do in other places such as this.

Maybe some more generic like JobType which we could re-use in the future if we decide to implement different behaviour of the job controller. I'm worried that CompletionMode might be limiting. Other ideas: WorkMode, JobMode

I think CompletionMode has a good balance of specificity and generality. We already have one in mind for the future (see Alternative): IndexedAndUnique.

The point is that all refer to how completion is handled. If there is a mode that refers to something other than completion, it should have its own field.

I think I agree with @soltysh on making a dedicated type, but I think this will get figured out during API review.

I already gave it a type :)

Make the env name unique, KUBERNETES_JOB_INDEX, for example. User can always map that annotation to other pre-defined name, but we should always inject it.

I agree that tracking completed pods is orthogonal to this proposal.

200 seems a lot, I don't think we have a hard limit in kubectl, but I'm positive this is too much. Also it's not relevant to this proposal, I'd drop it from here. It's implementation detail.

Removed specificity.

Warn and run as non-indexed? Warn and ignore? Can you be more specific?

Added specificity.

Will there be validation in kube-apiserver not to allow setting indexed jobs when this feature is disabled?

the value is overriden. Reworded for clarity.