bugfix: Allow for optional TimeSlicing configuration

[danielkleinstein]

This fixes a vestigial bug from the introduction of MPS. Now that two timeslicing configurations are available, it's possible for MPS to be configured instead of timeslicing.

But since the TimeSlicing field was made non-optional - its existence is forced when unmarshalling, and then the parsing fails because no resources are specified under the empty TimeSlicing field.

[copy-pr-bot]

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

[elezar]

Thanks @danielkleinstein.

I have made some suggestions for minor cleanups. I understand that the way this was done probably mirrors the same checks done for MPS, so I'm also ok to merge this as-is and then drop the redundant == nil checks in a follow-up (or as a second commit on top of the one here).

[danielkleinstein]

@elezar Thanks for the quick review 🙂 I removed the unnecessary nil checks you commented on in a separate commit.

[elezar]

@danielkleinstein the email address on your second commit is different to the first. Please update.

[danielkleinstein]

@elezar Fixed

[elezar]

Please run make check and / or make test locally if possible:

diff --git a/internal/lm/mig-strategy_test.go b/internal/lm/mig-strategy_test.go
index 005fc961..5aa7b9f6 100644
--- a/internal/lm/mig-strategy_test.go
+++ b/internal/lm/mig-strategy_test.go
@@ -30,7 +30,7 @@ func TestMigStrategyNoneLabels(t *testing.T) {
 	testCases := []struct {
 		description    string
 		devices        []resource.Device
-		timeSlicing    spec.ReplicatedResources
+		timeSlicing    *spec.ReplicatedResources
 		expectedError  bool
 		expectedLabels Labels
 	}{
diff --git a/internal/lm/resource_test.go b/internal/lm/resource_test.go
index 3f180188..3f44763c 100644
--- a/internal/lm/resource_test.go
+++ b/internal/lm/resource_test.go
@@ -271,7 +271,7 @@ func TestMigResourceLabeler(t *testing.T) {
 		description    string
 		resourceName   spec.ResourceName
 		count          int
-		timeSlicing    spec.ReplicatedResources
+		timeSlicing    *spec.ReplicatedResources
 		expectedLabels Labels
 	}{
 		{
@@ -301,7 +301,7 @@ func TestMigResourceLabeler(t *testing.T) {
 			description:  "shared appends suffix and doubles count",
 			resourceName: "nvidia.com/gpu",
 			count:        1,
-			timeSlicing: spec.ReplicatedResources{
+			timeSlicing: &spec.ReplicatedResources{
 				Resources: []spec.ReplicatedResource{
 					{
 						Name:     "nvidia.com/gpu",
@@ -329,7 +329,7 @@ func TestMigResourceLabeler(t *testing.T) {
 			description:  "renamed does not append suffix and doubles count",
 			resourceName: "nvidia.com/gpu",
 			count:        1,
-			timeSlicing: spec.ReplicatedResources{
+			timeSlicing: &spec.ReplicatedResources{
 				Resources: []spec.ReplicatedResource{
 					{
 						Name:     "nvidia.com/gpu",
@@ -358,7 +358,7 @@ func TestMigResourceLabeler(t *testing.T) {
 			description:  "mig mixed appends shared",
 			resourceName: "nvidia.com/mig-1g.1gb",
 			count:        1,
-			timeSlicing: spec.ReplicatedResources{
+			timeSlicing: &spec.ReplicatedResources{
 				Resources: []spec.ReplicatedResource{
 					{
 						Name:     "nvidia.com/gpu",
@@ -391,7 +391,7 @@ func TestMigResourceLabeler(t *testing.T) {
 			description:  "mig mixed rename does not append",
 			resourceName: "nvidia.com/mig-1g.1gb",
 			count:        1,
-			timeSlicing: spec.ReplicatedResources{
+			timeSlicing: &spec.ReplicatedResources{
 				Resources: []spec.ReplicatedResource{
 					{
 						Name:     "nvidia.com/mig-1g.1gb",

Changes applied.

[danielkleinstein]

@elezar Sorry, was kind of hoping to use the project's CI to run the tests 👻 I was working from a temporary Github Codespaces and make test didn't run so well. I'll clone locally and make sure tests pass before I push again.

[elezar]

@elezar Sorry, was kind of hoping to use the project's CI to run the tests 👻 I was working from a temporary Github Codespaces and make test didn't run so well. I'll clone locally and make sure tests pass before I push again.

We require an opt-in to run tests. (Still working on improving the configurations here).

diff --git a/internal/lm/resource_test.go b/internal/lm/resource_test.go
index 3f180188..6a3d7ae7 100644
--- a/internal/lm/resource_test.go
+++ b/internal/lm/resource_test.go
@@ -422,7 +422,7 @@ func TestMigResourceLabeler(t *testing.T) {
 		t.Run(tc.description, func(t *testing.T) {
 			config := &spec.Config{
 				Sharing: spec.Sharing{
-					TimeSlicing: tc.timeSlicing,
+					TimeSlicing: &tc.timeSlicing,
 				},
 			}
 			l, err := NewMIGResourceLabeler(tc.resourceName, config, device, tc.count)

[danielkleinstein]

@elezar I need some guidance - in a separate commit, I changed sharingDisabled from:

func (rl resourceLabeler) sharingDisabled() bool {
	return rl.sharing == nil
}

To:

func (rl resourceLabeler) sharingDisabled() bool {
    return rl.sharing == nil || (rl.sharing.SharingStrategy() == spec.SharingStrategyNone)
}

Which was necessary to get TestRunOneshot to pass. But this causes TestMigResourceLabeler to fail, because in the first test case it expects nvidia.com/gpu.replicas: 1, but it gets nvidia.com/gpu.replicas: 0, because of this code:

func (rl resourceLabeler) getReplicas() int {
	if rl.sharingDisabled() {
		return 0
	} else if r := rl.replicationInfo(); r != nil && r.Replicas > 0 {
		return r.Replicas
	}
	return 1
}

What's strange to me is that in the first test case, sharing seems to indeed be disabled (because nvidia.com/gpu.sharing-strategy: none) - so it seems like the number of replicas really should be 0 - but the expected label is 1?

---

An alternative fix for TestRunOneshot - which wouldn't trigger a failure in TestMigResourceLabeler, is to change replicationInfo from:

func (rl resourceLabeler) replicationInfo() *spec.ReplicatedResource {
	if rl.sharingDisabled() {
		return nil
	}
	for _, r := range rl.sharing.ReplicatedResources().Resources {
		if r.Name == rl.resourceName {
			return &r
		}
	}
	return nil
}

To:

func (rl resourceLabeler) replicationInfo() *spec.ReplicatedResource {
	if rl.sharingDisabled() {
		return nil
	}

	rr := rl.sharing.ReplicatedResources()
	if rr == nil {
		return nil
	}

	for _, r := range rr.Resources {
		if r.Name == rl.resourceName {
			return &r
		}
	}
	return nil
}

But I'm not sure if this is a desirable change.

[elezar]

In the existing implementation, the sharing == nil code path should only be triggered when calling NewGPUResourceLabelerWithoutSharing. This explicitly sets the replicas label to 0 and does not add other sharing labels. It has been on my TODO list for a while to rework this to be clearer, but we have not had time to focus on it.

What about removing the last commit that attempts to address this and making the following change:

diff --git a/api/config/v1/sharing.go b/api/config/v1/sharing.go
index ab768112..0c35cf30 100644
--- a/api/config/v1/sharing.go
+++ b/api/config/v1/sharing.go
@@ -34,10 +34,9 @@ const (
 
 // SharingStrategy returns the active sharing strategy.
 func (s *Sharing) SharingStrategy() SharingStrategy {
-	if s.MPS != nil && s.MPS.isReplicated() {
+	if s.MPS.isReplicated() {
 		return SharingStrategyMPS
 	}
-
 	if s.TimeSlicing.isReplicated() {
 		return SharingStrategyTimeSlicing
 	}
@@ -46,8 +45,16 @@ func (s *Sharing) SharingStrategy() SharingStrategy {
 
 // ReplicatedResources returns the resources associated with the active sharing strategy.
 func (s *Sharing) ReplicatedResources() *ReplicatedResources {
-	if s.MPS != nil {
+	none := &ReplicatedResources{}
+	if s == nil {
+		return none
+	}
+	switch s.SharingStrategy() {
+	case SharingStrategyMPS:
 		return s.MPS
+	case SharingStrategyTimeSlicing:
+		return s.TimeSlicing
+	default:
+		return none
 	}
-	return s.TimeSlicing
 }

This ensures that Sharing.ReplicatedResources() always returns a valid pointer and that an empty set of resources are returned when required.