Skip to content

Commit

Permalink
TAS: introduce tas label
Browse files Browse the repository at this point in the history
  • Loading branch information
mimowo committed Oct 21, 2024
1 parent 20c83fb commit 41f961f
Show file tree
Hide file tree
Showing 4 changed files with 50 additions and 0 deletions.
5 changes: 5 additions & 0 deletions apis/kueue/v1alpha1/tas_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,11 @@ const (
// of the PodSet of the admitted Workload corresponding to the PodTemplate.
// The label is set when starting the Job, and removed on stopping the Job.
PodSetLabel = "kueue.x-k8s.io/podset"

// TASLabel is a label set on the Job's PodTemplate to indicate that the
// PodSet is admitted using TopologyAwareScheduling, and all Pods created
// from the Job's PodTemplate also have the label.
TASLabel = "kueue.x-k8s.io/tas"
)

// TopologySpec defines the desired state of Topology
Expand Down
22 changes: 22 additions & 0 deletions keps/2724-topology-aware-schedling/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,7 @@
- [Workload API alternatives](#workload-api-alternatives)
- [Drop the topologyAssignment.levels field](#drop-the-topologyassignmentlevels-field)
- [Rename the topologyAssignment.domains.values field as levelValues](#rename-the-topologyassignmentdomainsvalues-field-as-levelvalues)
- [Drop dedicated TAS label](#drop-dedicated-tas-label)
<!-- /toc -->

## Summary
Expand Down Expand Up @@ -564,6 +565,11 @@ const (
// of the PodSet of the admitted Workload corresponding to the PodTemplate.
// The label is set when starting the Job, and removed on stopping the Job.
PodSetLabel = "kueue.x-k8s.io/podset"

// TASLabel is a label set on the Job's PodTemplate to indicate that the
// PodSet is admitted using TopologyAwareScheduling, and all Pods created
// from the Job's PodTemplate also have the label.
TASLabel = "kueue.x-k8s.io/tas"
)
```

Expand Down Expand Up @@ -686,6 +692,7 @@ The new validations which are for MVP, but likely will be relaxed in the future:
ReplicatedJob level (see [Story 2](#story-2))
- re-evaluate the need to support for "preferred/required" preferences at the
Workload level (see [Story 3](#story-3))
- re-evaluate the need for the `kueue.x-k8s.io/tas`

#### Stable

Expand Down Expand Up @@ -864,3 +871,18 @@ it specifies values for the keys in the `levels` field.
* the field is on Workload API, which is generally not a user-facing API in
Kueue. Workload objects are meant for the internal mechanics of Kueue.
* the intention of the field can be expressed in a comment.

### Drop dedicated TAS label

We could reduce the API surface by dropping the TAS label. The label is going
to be used in two places:
1. in TopologyUngater to quickly skip non-TAS pods in the events handler
2. when accounting for usage from Pods to quickly list only non-TAS pods

However, it was pointed out in discussions that the use case could probably be
fulfilled with a dedicated indexed virtual field.

**Reasons for discarding/deferring**

Increased code complexity which could defer the 0.9 release for the Alpha
version. We will re-evaluate the need for the label before the Beta release.
1 change: 1 addition & 0 deletions pkg/podset/podset.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,7 @@ func FromAssignment(ctx context.Context, client client.Client, assignment *kueue
Annotations: make(map[string]string),
}
if features.Enabled(features.TopologyAwareScheduling) && assignment.TopologyAssignment != nil {
info.Labels[kueuealpha.TASLabel] = "true"
info.SchedulingGates = append(info.SchedulingGates, corev1.PodSchedulingGate{
Name: kueuealpha.TopologySchedulingGate,
})
Expand Down
22 changes: 22 additions & 0 deletions pkg/podset/podset_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -188,6 +188,9 @@ func TestFromAssignment(t *testing.T) {
wantInfo: PodSetInfo{
Name: "name",
Count: 4,
Labels: map[string]string{
kueuealpha.TASLabel: "true",
},
NodeSelector: map[string]string{
"f1l1": "f1v1",
"f1l2": "f1v2",
Expand Down Expand Up @@ -428,6 +431,25 @@ func TestMergeRestore(t *testing.T) {
Obj(),
wantRestoreChanges: true,
},
"podset with tas label; empty info": {
podSet: utiltesting.MakePodSet("", 1).
Labels(map[string]string{kueuealpha.TASLabel: "true"}).
Obj(),
wantPodSet: utiltesting.MakePodSet("", 1).
Labels(map[string]string{kueuealpha.TASLabel: "true"}).
Obj(),
},
"podset with tas label; info re-adds the same": {
podSet: utiltesting.MakePodSet("", 1).
Labels(map[string]string{kueuealpha.TASLabel: "true"}).
Obj(),
info: PodSetInfo{
Labels: map[string]string{kueuealpha.TASLabel: "true"},
},
wantPodSet: utiltesting.MakePodSet("", 1).
Labels(map[string]string{kueuealpha.TASLabel: "true"}).
Obj(),
},
}

for name, tc := range cases {
Expand Down

0 comments on commit 41f961f

Please sign in to comment.