Add KEP #257: Leader Only SubGroup

[Edwinhr716]

What type of PR is this?

/kind documentation

What this PR does / why we need it

Which issue(s) this PR fixes

Part of #257

Special notes for your reviewer

Does this PR introduce a user-facing change?

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Edwinhr716

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:

• OWNERS [Edwinhr716]

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

[kerthcet]

I may review this tomorrow because of limited bandwidth, sorry for that.
/assign

[kerthcet]

reviewing now.

[kerthcet]

several comments.

[kerthcet]

I think LeaderOnly is a bit hard to understand, what about LeaderAlone vs All.

[Edwinhr716]

I'm fine with LeaderAlone, but All sounds a bit ambiguous to me. LeaderAlone vs LeaderWorker maybe? Or something that specifies that the leader and a worker pod will be part of the same subgroup

[kerthcet]

Sorry, is this only work for TPU? I mean the exclusive annotation, I forgot it.

[Edwinhr716]

Both TPU and GPU are supported for exclusive placement. I'll rephrase to be accelerator agnostic

[kerthcet]

I think even we add the annotation for both types is still backward compatibility. No behavior change.

[Edwinhr716]

The scenario that I see is that there is an existing deployment using SubGroupPolicy which then upgrades the LWS controller. After the upgrade, it will inject the annotation on the PodSpec, which will then trigger an update at the leader Sts level since the Pod has changed.

[kerthcet]

Ah, yes, this is disgusting, usually we only reconcile when spec changes.

[kerthcet]

One argue here is should we mark the leader a sub group? What's the Cons if we don't do this.

[Edwinhr716]

That's a good point, hadn't considered not making the leader part of a subgroup at all. Doing it this way simplifies the rest of the changes (though the TPU environment injection will still need some adjustment). Will test it out and add to the Alternatives which ever version we don't implement

[Edwinhr716]

Changed implementation to the leader not actually being part of any subgroup since it is simpler, added original implementation to Alternatives. Also changed LeaderOnly to LeaderAlone. Please take another look @kerthcet

[ahg-g]

The indexing within the group will be different now. Currently we don't define an index within the group, we do it only for tpuWorkerId, and this should be calculated differently now to exclude the leader: https://github.com/kubernetes-sigs/lws/blob/main/pkg/utils/accelerators/tpu.go#L113, right?

[Edwinhr716]

We already do this when the leader doesn't request TPU resources https://github.com/kubernetes-sigs/lws/blob/main/pkg/utils/accelerators/tpu.go#L115-L117

[kerthcet]

I think we can leave more implementation details to the PR.

[kerthcet]

I think the calculation of worker subGroup index still needs to change, for example, we set the subgroupSize=2,

• before with group size = 6, it looks like (0, 0), (1, 0), (2, 1), (3, 1), (4, 2), (5 ,2)
• after with group size = 7, it looks like (0, x), (1, 0), (2, 0), (3, 1), (4, 1), (5, 2), (6, 2)

(x, y) represents the (workerIndex, subGroupIndex), for the workerIndex=2, before the subgroupIndex is 1, after is 0. Not familiar with the TPU implementation, maybe the same? But this is just code implementation, we can leave the detailed review to the PR, just mention this fact in the proposal briefly.

[Edwinhr716]

I think the calculation of worker subGroup index still needs to change, for example, we set the subgroupSize=2,

• before with group size = 6, it looks like (0, 0), (1, 0), (2, 1), (3, 1), (4, 2), (5 ,2)
• after with group size = 7, it looks like (0, x), (1, 0), (2, 0), (3, 1), (4, 1), (5, 2), (6, 2)

(x, y) represents the (workerIndex, subGroupIndex), for the workerIndex=2, before the subgroupIndex is 1, after is 0. Not familiar with the TPU implementation, maybe the same? But this is just code implementation, we can leave the detailed review to the PR, just mention this fact in the proposal briefly.

Is this not what we want? With this calculation, we ensure that there are two pods per subgroup, which should be the case if subGroupSize=2. If we want the subGroupIndex to match, then the first subgroup would only have 1 pod, same with the last subgroup.

[kerthcet]

Yes, the result is as we expected, I just mean the method may need to change:

lws/pkg/webhooks/pod_webhook.go

Lines 245 to 251 in ab40b62

	func getSubGroupIndex(podCount int, subGroupSize int, workerIndex int) string {
	if (podCount-1)%subGroupSize == 0 {
	// Leader is considered as extra pod, it is part of the first group
	return fmt.Sprint((workerIndex - 1) / subGroupSize)
	}
	return fmt.Sprint(workerIndex / subGroupSize)
	}

For workIndex=2, before it's (2, 1), now it's (2, 0), the subGroupIndex changes. Do I miss anything? I think we'll call the method, right?

[Edwinhr716]

Yeah we call that method. I guess I'm a little bit confused on what scenario you are referring to. The current behavior when using this feature has workerIndex=2 at subgroup 0. If using the default configuration, but still having an odd number for size, it will still be workerIndex=2 at subgroup 0. The only case where workerIndex=2 gives subgroup 1, is if using default behavior and size is even

[kerthcet]

If using the default configuration, but still having an odd number for size, it will still be workerIndex=2 at subgroup 0

Doesn't this should be workerIndex/subGroupSize = 2/2 = 1, rather than 0.

Let's move the discussion to PR then.
/lgtm
/label tide/merge-method-squash