review: build the storage params sparse at one site

cmd/lambda/run.go

@@ -120,16 +120,9 @@ func generateLambdaOptions(cmd *cli.Command) (*corepb.RunAndWaitOptions, error)
 		return nil, fmt.Errorf("parse storage: %w", err)
 	}
 
-	storage := resourcetypes.RawParams{
-		"storage-request": storageRequest,
-		"storage-limit":   storageLimit,
-		"volumes-request": cmd.StringSlice("volume-request"),
-		"volumes-limit":   cmd.StringSlice("volume"),
-	}
-
 	resources, err := utils.EncodeResources(cmd, resourcetypes.Resources{
 		utils.ResourceCPUMem:  cpumem,
-		utils.ResourceStorage: utils.CompactParams(storage),
+		utils.ResourceStorage: utils.StorageParams(storageRequest, storageLimit, cmd.StringSlice("volume-request"), cmd.StringSlice("volume")),
 	})
 	if err != nil {
 		return nil, err

cmd/lambda/run_test.go

@@ -58,6 +58,7 @@ func TestGenerateLambdaOptions(t *testing.T) {
 	tests := []struct {
 		name            string
 		args            []string
+		wantStorage     bool
 		wantStorageReq  int64
 		wantStorageLim  int64
 		wantVolumesReq  []string
@@ -72,7 +73,8 @@ func TestGenerateLambdaOptions(t *testing.T) {
 			wantMemoryLimit: 512 * 1024 * 1024,
 		},
 		{
-			name: "storage and volumes",
+			name:        "storage and volumes",
+			wantStorage: true,
 			args: []string{
 				"lambda",
 				"--storage-request", "1G",
@@ -95,9 +97,8 @@ func TestGenerateLambdaOptions(t *testing.T) {
 			opts := runLambdaCommand(t, tt.args)
 
 			raw, ok := opts.DeployOptions.Resources[utils.ResourceStorage]
-			wantStorage := tt.wantStorageReq != 0 || tt.wantStorageLim != 0 || tt.wantVolumesReq != nil || tt.wantVolumesLim != nil
-			if ok != wantStorage {
-				t.Fatalf("storage entry present=%v, want %v", ok, wantStorage)
+			if ok != tt.wantStorage {
+				t.Fatalf("storage entry present=%v, want %v", ok, tt.wantStorage)
 			}
 			if ok {
 				storage := decodeParams(t, raw)

cmd/pod/capacity.go

@@ -92,10 +92,7 @@ func capacityResources(cmd *cli.Command) (map[string][]byte, error) {
 	}
 
 	return utils.EncodeResources(cmd, resourcetypes.Resources{
-		utils.ResourceCPUMem: cpumem,
-		utils.ResourceStorage: utils.CompactParams(resourcetypes.RawParams{
-			"storage-request": storage,
-			"storage-limit":   storage,
-		}),
+		utils.ResourceCPUMem:  cpumem,
+		utils.ResourceStorage: utils.StorageParams(storage, storage, nil, nil),
 	})
 }

cmd/utils/resource.go

@@ -15,23 +15,22 @@ const (
 	FlagExtraResources = "extra-resources"
 )
 
-// CompactParams drops zero values so an all-default plugin entry defers to --extra-resources.
-func CompactParams(params resourcetypes.RawParams) resourcetypes.RawParams {
-	compact := make(resourcetypes.RawParams, len(params))
-	for key, value := range params {
-		switch v := value.(type) {
-		case int64:
-			if v == 0 {
-				continue
-			}
-		case []string:
-			if len(v) == 0 {
-				continue
-			}
-		}
-		compact[key] = value
+// StorageParams builds the storage plugin request; zero values stay out, so an untouched entry defers to --extra-resources.
+func StorageParams(storageRequest, storageLimit int64, volumesRequest, volumesLimit []string) resourcetypes.RawParams {
+	params := resourcetypes.RawParams{}
+	if storageRequest != 0 {
+		params["storage-request"] = storageRequest
+	}
+	if storageLimit != 0 {
+		params["storage-limit"] = storageLimit
+	}
+	if len(volumesRequest) != 0 {
+		params["volumes-request"] = volumesRequest
+	}
+	if len(volumesLimit) != 0 {
+		params["volumes-limit"] = volumesLimit
 	}
-	return compact
+	return params
 }
 
 // EncodeResources encodes plugin params for the core rpc; --extra-resources fills only the plugins not already present.

cmd/utils/resource_test.go

@@ -85,27 +85,16 @@ func TestEncodeResources(t *testing.T) {
 	}
 }
 
-func TestCompactParamsLetsExtraResourcesFillStorage(t *testing.T) {
-	storage := resourcetypes.RawParams{
-		"storage-request": int64(0),
-		"storage-limit":   int64(0),
-		"volumes-request": []string(nil),
-		"volumes-limit":   []string{},
-	}
-	if got := CompactParams(storage); len(got) != 0 {
-		t.Errorf("got %v, want every zero value dropped", got)
+func TestStorageParamsStaysSparse(t *testing.T) {
+	if got := StorageParams(0, 0, nil, []string{}); len(got) != 0 {
+		t.Errorf("got %v, want an untouched request empty so --extra-resources can fill it", got)
 	}
 
-	kept := resourcetypes.RawParams{
-		"storage-request": int64(0),
-		"storage-limit":   int64(1073741824),
-		"volumes-limit":   []string{"/data0:1G"},
-	}
 	want := resourcetypes.RawParams{
 		"storage-limit": int64(1073741824),
 		"volumes-limit": []string{"/data0:1G"},
 	}
-	if got := CompactParams(kept); !maps.EqualFunc(got, want, func(a, b any) bool {
+	if got := StorageParams(0, 1073741824, nil, []string{"/data0:1G"}); !maps.EqualFunc(got, want, func(a, b any) bool {
 		aj, _ := json.Marshal(a)
 		bj, _ := json.Marshal(b)
 		return string(aj) == string(bj)
@@ -114,7 +103,7 @@ func TestCompactParamsLetsExtraResourcesFillStorage(t *testing.T) {
 	}
 
 	cmd := commandWithExtraResources(t, `{"resource-storage":{"storage":2147483648}}`)
-	encoded, err := EncodeResources(cmd, resourcetypes.Resources{"resource-storage": CompactParams(storage)})
+	encoded, err := EncodeResources(cmd, resourcetypes.Resources{"resource-storage": StorageParams(0, 0, nil, nil)})
 	if err != nil {
 		t.Fatal(err)
 	}

cmd/workload/deploy.go

@@ -145,16 +145,9 @@ func generateDeployOptions(ctx context.Context, cmd *cli.Command) (*corepb.Deplo
 	if cmd.Bool("cpu-bind") {
 		cpumem["cpu-bind"] = true
 	}
-	storage := resourcetypes.RawParams{
-		flagStorageRequest: storageRequest,
-		flagStorageLimit:   storageLimit,
-		flagVolumesRequest: specs.VolumesRequest,
-		flagVolumesLimit:   specs.Volumes,
-	}
-
 	resources, err := utils.EncodeResources(cmd, resourcetypes.Resources{
 		utils.ResourceCPUMem:  cpumem,
-		utils.ResourceStorage: utils.CompactParams(storage),
+		utils.ResourceStorage: utils.StorageParams(storageRequest, storageLimit, specs.VolumesRequest, specs.Volumes),
 	})
 	if err != nil {
 		return nil, err

cmd/workload/realloc.go

@@ -86,13 +86,6 @@ func generateReallocOptions(cmd *cli.Command) (*corepb.ReallocOptions, error) {
 		flagMemoryRequest: memoryRequest,
 		flagMemoryLimit:   memoryLimit,
 	}
-	storage := resourcetypes.RawParams{
-		flagStorageRequest: storageRequest,
-		flagStorageLimit:   storageLimit,
-		flagVolumesRequest: volumesRequest,
-		flagVolumesLimit:   volumesLimit,
-	}
-
 	switch {
 	case bindCPU:
 		cpumem["cpu-bind"] = true
@@ -102,7 +95,7 @@ func generateReallocOptions(cmd *cli.Command) (*corepb.ReallocOptions, error) {
 
 	resources, err := utils.EncodeResources(cmd, resourcetypes.Resources{
 		utils.ResourceCPUMem:  cpumem,
-		utils.ResourceStorage: utils.CompactParams(storage),
+		utils.ResourceStorage: utils.StorageParams(storageRequest, storageLimit, volumesRequest, volumesLimit),
 	})
 	if err != nil {
 		return nil, err