support soft-only memory oversubscription (#488)

Implement driver-side support in the `podman` drivers for `resource.memory_max =
-1`, which allows a reserve-only memory request for clusters with oversubscription
enabled. This was already allowed by the server, but undocumented and unevenly
supported by drivers.

Ref: hashicorp/nomad#27354
Ref: https://hashicorp.atlassian.net/browse/NMD-911
Ref: hashicorp/web-unified-docs#1629

Author: tgross

driver.go

@@ -680,11 +680,11 @@ func (d *Driver) StartTask(cfg *drivers.TaskConfig) (*drivers.TaskHandle, *drive
 		return nil, nil, err
 	}
 
-	hard, soft, err := memoryLimits(cfg.Resources.NomadResources.Memory, podmanTaskConfig.MemoryReservation)
+	hard, reserved, err := memoryLimits(cfg.Resources.NomadResources.Memory, podmanTaskConfig.MemoryReservation)
 	if err != nil {
 		return nil, nil, err
 	}
-	createOpts.ContainerResourceConfig.ResourceLimits.Memory.Reservation = soft
+	createOpts.ContainerResourceConfig.ResourceLimits.Memory.Reservation = reserved
 	createOpts.ContainerResourceConfig.ResourceLimits.Memory.Limit = hard
 	// set PidsLimit only if configured.
 	if podmanTaskConfig.PidsLimit > 0 {
@@ -1034,31 +1034,59 @@ func (d *Driver) StartTask(cfg *drivers.TaskConfig) (*drivers.TaskHandle, *drive
 	return handle, driverNet, nil
 }
 
+const (
+	// memoryNoLimit is a sentinel value for memory_max that indicates the
+	// driver should not enforce a maximum memory limit
+	memoryNoLimit = -1
+)
+
+// memoryLimits computes the memory and memory_reservation values passed along to
+// the podman host config. These fields represent hard and soft/reserved memory
+// limits from podman's perspective, respectively.
+//
+// The resources.memory field in the jobspec is normally interpreted as a hard
+// limit.
+//
+// If task.config.memory_reservation is set, it is treated as the reserve and
+// resources.memory is the hard limit. This entirely bypasses the scheduler
+// oversubscription setting.
+//
+// If oversubscription is enabled and resources.memory_max is set,
+// resources.memory_max is treated as the hard limit and either resources.memory
+// or task.config.memory_reservation (whichever is less) is the soft limit. If
+// resources.memory_max = -1, there is no hard limit.
+//
+// Returns (memory (hard), memory_reservation (soft)) values in bytes.
 func memoryLimits(r drivers.MemoryResources, reservation string) (hard, soft *int64, err error) {
-	memoryMax := r.MemoryMaxMB * 1024 * 1024
+	memoryMax := r.MemoryMaxMB
 	memory := r.MemoryMB * 1024 * 1024
-
 	var reserved *int64
+
 	if reservation != "" {
 		reservation, err := memoryInBytes(reservation)
 		if err != nil {
 			return nil, nil, err
 		}
 		reserved = &reservation
 	}
-
+	if memoryMax == memoryNoLimit {
+		if reserved != nil && *reserved < memory {
+			return nil, reserved, nil
+		}
+		return nil, &memory, nil
+	}
 	if memoryMax > 0 {
+		memoryMax = memoryMax * 1024 * 1024
 		if reserved != nil && *reserved < memory {
 			memory = *reserved
 		}
 		return &memoryMax, &memory, nil
 	}
-
 	if memory > 0 {
 		return &memory, reserved, nil
 	}
 
-	// We may never actually be here
+	// should be unreachable b/c a default should always be set by the server
 	return nil, reserved, nil
 }
 

driver_test.go

@@ -2522,77 +2522,87 @@ func Test_memoryLimits(t *testing.T) {
 	ci.Parallel(t)
 
 	cases := []struct {
-		name         string
-		memResources drivers.MemoryResources
-		reservation  string
-		expectedHard int64
-		expectedSoft int64
+		name            string
+		memResources    drivers.MemoryResources
+		reservation     string
+		expectedHard    int64
+		expectedReserve int64
 	}{
 		{
 			name: "plain",
 			memResources: drivers.MemoryResources{
 				MemoryMB: 20,
 			},
-			expectedHard: 20 * 1024 * 1024,
-			expectedSoft: 0,
+			expectedHard:    20 * 1024 * 1024,
+			expectedReserve: 0,
 		},
 		{
 			name: "memory oversubscription",
 			memResources: drivers.MemoryResources{
 				MemoryMB:    20,
 				MemoryMaxMB: 30,
 			},
-			expectedHard: 30 * 1024 * 1024,
-			expectedSoft: 20 * 1024 * 1024,
+			expectedHard:    30 * 1024 * 1024,
+			expectedReserve: 20 * 1024 * 1024,
 		},
 		{
 			name: "plain but using memory reservations",
 			memResources: drivers.MemoryResources{
 				MemoryMB: 20,
 			},
-			reservation:  "10m",
-			expectedHard: 20 * 1024 * 1024,
-			expectedSoft: 10 * 1024 * 1024,
+			reservation:     "10m",
+			expectedHard:    20 * 1024 * 1024,
+			expectedReserve: 10 * 1024 * 1024,
 		},
 		{
 			name: "oversubscription but with specifying memory reservation",
 			memResources: drivers.MemoryResources{
 				MemoryMB:    20,
 				MemoryMaxMB: 30,
 			},
-			reservation:  "10m",
-			expectedHard: 30 * 1024 * 1024,
-			expectedSoft: 10 * 1024 * 1024,
+			reservation:     "10m",
+			expectedHard:    30 * 1024 * 1024,
+			expectedReserve: 10 * 1024 * 1024,
 		},
 		{
 			name: "oversubscription but with specifying high memory reservation",
 			memResources: drivers.MemoryResources{
 				MemoryMB:    20,
 				MemoryMaxMB: 30,
 			},
-			reservation:  "25m",
-			expectedHard: 30 * 1024 * 1024,
-			expectedSoft: 20 * 1024 * 1024,
+			reservation:     "25m",
+			expectedHard:    30 * 1024 * 1024,
+			expectedReserve: 20 * 1024 * 1024,
+		},
+		{
+			name: "oversubscription without hard limit",
+			memResources: drivers.MemoryResources{
+				MemoryMB:    20,
+				MemoryMaxMB: -1,
+			},
+			reservation:     "25m",
+			expectedHard:    0,
+			expectedReserve: 20 * 1024 * 1024,
 		},
 	}
 
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
-			hard, soft, err := memoryLimits(c.memResources, c.reservation)
+			hard, reserve, err := memoryLimits(c.memResources, c.reservation)
 			must.NoError(t, err)
 
 			if c.expectedHard > 0 {
-				must.NotNil(t, hard)
+				must.NotNil(t, hard, must.Sprintf("hard limit was nil"))
 				must.Eq(t, c.expectedHard, *hard)
 			} else {
-				must.Nil(t, hard)
+				must.Nil(t, hard, must.Sprintf("hard limit was not nil: %d", hard))
 			}
 
-			if c.expectedSoft > 0 {
-				must.NotNil(t, soft)
-				must.Eq(t, c.expectedSoft, *soft)
+			if c.expectedReserve > 0 {
+				must.NotNil(t, reserve, must.Sprint("reserve was nil"))
+				must.Eq(t, c.expectedReserve, *reserve)
 			} else {
-				must.Nil(t, soft)
+				must.Nil(t, reserve, must.Sprintf("reserve was not nil: %d", reserve))
 			}
 		})
 	}