Remove storage as a task resource option (#4658)

* init Signed-off-by: Future Outlier <[email protected]> * better test for ephermeral storage Signed-off-by: Future Outlier <[email protected]> * remove resource field suggested by katrina Signed-off-by: Future-Outlier <[email protected]> --------- Signed-off-by: Future Outlier <[email protected]> Signed-off-by: Future-Outlier <[email protected]> Co-authored-by: Future Outlier <[email protected]>
flyteorg · Jan 30, 2024 · 00d0a81 · 00d0a81
1 parent 460f0f1
commit 00d0a81
Show file tree

Hide file tree

Showing 19 changed files with 80 additions and 150 deletions.
diff --git a/flyteadmin/pkg/manager/impl/execution_manager.go b/flyteadmin/pkg/manager/impl/execution_manager.go
@@ -249,23 +249,6 @@ func (m *ExecutionManager) setCompiledTaskDefaults(ctx context.Context, task *co
 		})
 	}
 
-	// Only assign storage when it is either requested or limited in the task definition, or a platform
-	// default exists.
-	if !taskResourceRequirements.Defaults.Storage.IsZero() ||
-		!taskResourceRequirements.Limits.Storage.IsZero() ||
-		!platformTaskResources.Defaults.Storage.IsZero() {
-		storageResource := flytek8s.AdjustOrDefaultResource(taskResourceRequirements.Defaults.Storage, taskResourceRequirements.Limits.Storage,
-			platformTaskResources.Defaults.Storage, platformTaskResources.Limits.Storage)
-		finalizedResourceRequests = append(finalizedResourceRequests, &core.Resources_ResourceEntry{
-			Name:  core.Resources_STORAGE,
-			Value: storageResource.Request.String(),
-		})
-		finalizedResourceLimits = append(finalizedResourceLimits, &core.Resources_ResourceEntry{
-			Name:  core.Resources_STORAGE,
-			Value: storageResource.Limit.String(),
-		})
-	}
-
 	// Only assign gpu when it is either requested or limited in the task definition, or a platform default exists.
 	if !taskResourceRequirements.Defaults.GPU.IsZero() ||
 		!taskResourceRequirements.Limits.GPU.IsZero() ||

diff --git a/flyteadmin/pkg/manager/impl/util/resources.go b/flyteadmin/pkg/manager/impl/util/resources.go
@@ -66,10 +66,6 @@ func fromAdminProtoTaskResourceSpec(ctx context.Context, spec *admin.TaskResourc
 		result.Memory = parseQuantityNoError(ctx, "project", "memory", spec.Memory)
 	}
 
-	if len(spec.Storage) > 0 {
-		result.Storage = parseQuantityNoError(ctx, "project", "storage", spec.Storage)
-	}
-
 	if len(spec.EphemeralStorage) > 0 {
 		result.EphemeralStorage = parseQuantityNoError(ctx, "project", "ephemeral storage", spec.EphemeralStorage)
 	}

diff --git a/flyteadmin/pkg/manager/impl/util/resources_test.go b/flyteadmin/pkg/manager/impl/util/resources_test.go
@@ -31,14 +31,12 @@ func TestGetTaskResources(t *testing.T) {
 		GPU:              resource.MustParse("8"),
 		Memory:           resource.MustParse("200Gi"),
 		EphemeralStorage: resource.MustParse("500Mi"),
-		Storage:          resource.MustParse("400Mi"),
 	}
 	taskConfig.Limits = runtimeInterfaces.TaskResourceSet{
 		CPU:              resource.MustParse("300m"),
 		GPU:              resource.MustParse("8"),
 		Memory:           resource.MustParse("500Gi"),
 		EphemeralStorage: resource.MustParse("501Mi"),
-		Storage:          resource.MustParse("450Mi"),
 	}
 
 	t.Run("use runtime application values", func(t *testing.T) {
@@ -61,14 +59,12 @@ func TestGetTaskResources(t *testing.T) {
 				GPU:              resource.MustParse("8"),
 				Memory:           resource.MustParse("200Gi"),
 				EphemeralStorage: resource.MustParse("500Mi"),
-				Storage:          resource.MustParse("400Mi"),
 			},
 			Limits: runtimeInterfaces.TaskResourceSet{
 				CPU:              resource.MustParse("300m"),
 				GPU:              resource.MustParse("8"),
 				Memory:           resource.MustParse("500Gi"),
 				EphemeralStorage: resource.MustParse("501Mi"),
-				Storage:          resource.MustParse("450Mi"),
 			},
 		})
 	})
@@ -91,14 +87,12 @@ func TestGetTaskResources(t *testing.T) {
 								Gpu:              "18",
 								Memory:           "1200Gi",
 								EphemeralStorage: "1500Mi",
-								Storage:          "1400Mi",
 							},
 							Limits: &admin.TaskResourceSpec{
 								Cpu:              "300m",
 								Gpu:              "8",
 								Memory:           "500Gi",
 								EphemeralStorage: "501Mi",
-								Storage:          "450Mi",
 							},
 						},
 					},
@@ -112,14 +106,12 @@ func TestGetTaskResources(t *testing.T) {
 				GPU:              resource.MustParse("18"),
 				Memory:           resource.MustParse("1200Gi"),
 				EphemeralStorage: resource.MustParse("1500Mi"),
-				Storage:          resource.MustParse("1400Mi"),
 			},
 			Limits: runtimeInterfaces.TaskResourceSet{
 				CPU:              resource.MustParse("300m"),
 				GPU:              resource.MustParse("8"),
 				Memory:           resource.MustParse("500Gi"),
 				EphemeralStorage: resource.MustParse("501Mi"),
-				Storage:          resource.MustParse("450Mi"),
 			},
 		})
 	})
@@ -129,14 +121,12 @@ func TestFromAdminProtoTaskResourceSpec(t *testing.T) {
 	taskResourceSet := fromAdminProtoTaskResourceSpec(context.TODO(), &admin.TaskResourceSpec{
 		Cpu:              "1",
 		Memory:           "100",
-		Storage:          "200",
 		EphemeralStorage: "300",
 		Gpu:              "2",
 	})
 	assert.EqualValues(t, runtimeInterfaces.TaskResourceSet{
 		CPU:              resource.MustParse("1"),
 		Memory:           resource.MustParse("100"),
-		Storage:          resource.MustParse("200"),
 		EphemeralStorage: resource.MustParse("300"),
 		GPU:              resource.MustParse("2"),
 	}, taskResourceSet)

diff --git a/flyteadmin/pkg/runtime/interfaces/task_resource_configuration.go b/flyteadmin/pkg/runtime/interfaces/task_resource_configuration.go
@@ -6,7 +6,6 @@ type TaskResourceSet struct {
 	CPU              resource.Quantity `json:"cpu"`
 	GPU              resource.Quantity `json:"gpu"`
 	Memory           resource.Quantity `json:"memory"`
-	Storage          resource.Quantity `json:"storage"`
 	EphemeralStorage resource.Quantity `json:"ephemeralStorage"`
 }
 

diff --git a/flyteadmin/pkg/workflowengine/impl/prepare_execution.go b/flyteadmin/pkg/workflowengine/impl/prepare_execution.go
@@ -85,9 +85,6 @@ func addExecutionOverrides(taskPluginOverrides []*admin.PluginOverride,
 		if !taskResources.Defaults.EphemeralStorage.IsZero() {
 			requests.EphemeralStorage = taskResources.Defaults.EphemeralStorage
 		}
-		if !taskResources.Defaults.Storage.IsZero() {
-			requests.Storage = taskResources.Defaults.Storage
-		}
 		if !taskResources.Defaults.GPU.IsZero() {
 			requests.GPU = taskResources.Defaults.GPU
 		}
@@ -102,9 +99,6 @@ func addExecutionOverrides(taskPluginOverrides []*admin.PluginOverride,
 		if !taskResources.Limits.EphemeralStorage.IsZero() {
 			limits.EphemeralStorage = taskResources.Limits.EphemeralStorage
 		}
-		if !taskResources.Limits.Storage.IsZero() {
-			limits.Storage = taskResources.Limits.Storage
-		}
 		if !taskResources.Limits.GPU.IsZero() {
 			limits.GPU = taskResources.Limits.GPU
 		}

diff --git a/flyteadmin/pkg/workflowengine/impl/prepare_execution_test.go b/flyteadmin/pkg/workflowengine/impl/prepare_execution_test.go
@@ -131,7 +131,6 @@ func TestAddExecutionOverrides(t *testing.T) {
 			Limits: runtimeInterfaces.TaskResourceSet{
 				CPU:              resource.MustParse("2"),
 				Memory:           resource.MustParse("200Gi"),
-				Storage:          resource.MustParse("5Gi"),
 				EphemeralStorage: resource.MustParse("1Gi"),
 				GPU:              resource.MustParse("1"),
 			},
@@ -144,7 +143,6 @@ func TestAddExecutionOverrides(t *testing.T) {
 		assert.EqualValues(t, v1alpha1.TaskResourceSpec{
 			CPU:              resource.MustParse("2"),
 			Memory:           resource.MustParse("200Gi"),
-			Storage:          resource.MustParse("5Gi"),
 			EphemeralStorage: resource.MustParse("1Gi"),
 			GPU:              resource.MustParse("1"),
 		}, workflow.ExecutionConfig.TaskResources.Limits)

diff --git a/flyteplugins/go/tasks/config_load_test.go b/flyteplugins/go/tasks/config_load_test.go
@@ -44,23 +44,14 @@ func TestLoadConfig(t *testing.T) {
 		}, k8sConfig.DefaultEnvVars)
 		assert.NotNil(t, k8sConfig.ResourceTolerations)
 		assert.Contains(t, k8sConfig.ResourceTolerations, v1.ResourceName("nvidia.com/gpu"))
-		assert.Contains(t, k8sConfig.ResourceTolerations, v1.ResourceStorage)
 		tolGPU := v1.Toleration{
 			Key:      "flyte/gpu",
 			Value:    "dedicated",
 			Operator: v1.TolerationOpEqual,
 			Effect:   v1.TaintEffectNoSchedule,
 		}
 
-		tolStorage := v1.Toleration{
-			Key:      "storage",
-			Value:    "special",
-			Operator: v1.TolerationOpEqual,
-			Effect:   v1.TaintEffectPreferNoSchedule,
-		}
-
 		assert.Equal(t, []v1.Toleration{tolGPU}, k8sConfig.ResourceTolerations[v1.ResourceName("nvidia.com/gpu")])
-		assert.Equal(t, []v1.Toleration{tolStorage}, k8sConfig.ResourceTolerations[v1.ResourceStorage])
 		expectedCPU := resource.MustParse("1000m")
 		assert.True(t, expectedCPU.Equal(k8sConfig.DefaultCPURequest))
 		expectedMemory := resource.MustParse("1024Mi")

diff --git a/flyteplugins/go/tasks/pluginmachinery/flytek8s/container_helper.go b/flyteplugins/go/tasks/pluginmachinery/flytek8s/container_helper.go
@@ -160,8 +160,6 @@ func ApplyResourceOverrides(resources, platformResources v1.ResourceRequirements
 
 	// TODO: Make configurable. 1/15/2019 Flyte Cluster doesn't support setting storage requests/limits.
 	// https://github.com/kubernetes/enhancements/issues/362
-	delete(resources.Requests, v1.ResourceStorage)
-	delete(resources.Limits, v1.ResourceStorage)
 
 	gpuResourceName := config.GetK8sPluginConfig().GpuResourceName
 	shouldAdjustGPU := false

diff --git a/flyteplugins/go/tasks/pluginmachinery/flytek8s/container_helper_test.go b/flyteplugins/go/tasks/pluginmachinery/flytek8s/container_helper_test.go
@@ -211,13 +211,11 @@ func TestApplyResourceOverrides_RemoveStorage(t *testing.T) {
 	requestedResourceQuantity := resource.MustParse("1")
 	overrides := ApplyResourceOverrides(v1.ResourceRequirements{
 		Requests: v1.ResourceList{
-			v1.ResourceStorage:          requestedResourceQuantity,
 			v1.ResourceMemory:           requestedResourceQuantity,
 			v1.ResourceCPU:              requestedResourceQuantity,
 			v1.ResourceEphemeralStorage: requestedResourceQuantity,
 		},
 		Limits: v1.ResourceList{
-			v1.ResourceStorage:          requestedResourceQuantity,
 			v1.ResourceMemory:           requestedResourceQuantity,
 			v1.ResourceEphemeralStorage: requestedResourceQuantity,
 		},
@@ -261,7 +259,6 @@ func TestMergeResources_EmptyIn(t *testing.T) {
 			v1.ResourceEphemeralStorage: requestedResourceQuantity,
 		},
 		Limits: v1.ResourceList{
-			v1.ResourceStorage:          requestedResourceQuantity,
 			v1.ResourceMemory:           requestedResourceQuantity,
 			v1.ResourceEphemeralStorage: requestedResourceQuantity,
 		},
@@ -280,7 +277,6 @@ func TestMergeResources_EmptyOut(t *testing.T) {
 			v1.ResourceEphemeralStorage: requestedResourceQuantity,
 		},
 		Limits: v1.ResourceList{
-			v1.ResourceStorage:          requestedResourceQuantity,
 			v1.ResourceMemory:           requestedResourceQuantity,
 			v1.ResourceEphemeralStorage: requestedResourceQuantity,
 		},

diff --git a/flyteplugins/go/tasks/pluginmachinery/flytek8s/k8s_resource_adds_test.go b/flyteplugins/go/tasks/pluginmachinery/flytek8s/k8s_resource_adds_test.go
@@ -33,8 +33,8 @@ func TestGetTolerationsForResources(t *testing.T) {
 		Effect:   v12.TaintEffectNoSchedule,
 	}
 
-	tolStorage := v12.Toleration{
-		Key:      "storage",
+	tolEphemeralStorage := v12.Toleration{
+		Key:      "ephemeral-storage",
 		Value:    "dedicated",
 		Operator: v12.TolerationOpExists,
 		Effect:   v12.TaintEffectNoSchedule,
@@ -55,8 +55,8 @@ func TestGetTolerationsForResources(t *testing.T) {
 			args{
 				v12.ResourceRequirements{
 					Limits: v12.ResourceList{
-						v12.ResourceCPU:     resource.MustParse("1024m"),
-						v12.ResourceStorage: resource.MustParse("100M"),
+						v12.ResourceCPU:              resource.MustParse("1024m"),
+						v12.ResourceEphemeralStorage: resource.MustParse("100M"),
 					},
 				},
 			},
@@ -69,8 +69,8 @@ func TestGetTolerationsForResources(t *testing.T) {
 			args{
 				v12.ResourceRequirements{
 					Requests: v12.ResourceList{
-						v12.ResourceCPU:     resource.MustParse("1024m"),
-						v12.ResourceStorage: resource.MustParse("100M"),
+						v12.ResourceCPU:              resource.MustParse("1024m"),
+						v12.ResourceEphemeralStorage: resource.MustParse("100M"),
 					},
 				},
 			},
@@ -83,12 +83,12 @@ func TestGetTolerationsForResources(t *testing.T) {
 			args{
 				v12.ResourceRequirements{
 					Limits: v12.ResourceList{
-						v12.ResourceCPU:     resource.MustParse("1024m"),
-						v12.ResourceStorage: resource.MustParse("100M"),
+						v12.ResourceCPU:              resource.MustParse("1024m"),
+						v12.ResourceEphemeralStorage: resource.MustParse("100M"),
 					},
 					Requests: v12.ResourceList{
-						v12.ResourceCPU:     resource.MustParse("1024m"),
-						v12.ResourceStorage: resource.MustParse("100M"),
+						v12.ResourceCPU:              resource.MustParse("1024m"),
+						v12.ResourceEphemeralStorage: resource.MustParse("100M"),
 					},
 				},
 			},
@@ -101,84 +101,84 @@ func TestGetTolerationsForResources(t *testing.T) {
 			args{
 				v12.ResourceRequirements{
 					Limits: v12.ResourceList{
-						v12.ResourceCPU:     resource.MustParse("1024m"),
-						v12.ResourceStorage: resource.MustParse("100M"),
+						v12.ResourceCPU:              resource.MustParse("1024m"),
+						v12.ResourceEphemeralStorage: resource.MustParse("100M"),
 					},
 				},
 			},
 			map[v12.ResourceName][]v12.Toleration{
-				v12.ResourceStorage: {tolStorage},
-				ResourceNvidiaGPU:   {tolGPU},
+				v12.ResourceEphemeralStorage: {tolEphemeralStorage},
+				ResourceNvidiaGPU:            {tolGPU},
 			},
 			nil,
-			[]v12.Toleration{tolStorage},
+			[]v12.Toleration{tolEphemeralStorage},
 		},
 		{
 			"tolerations-req",
 			args{
 				v12.ResourceRequirements{
 					Requests: v12.ResourceList{
-						v12.ResourceCPU:     resource.MustParse("1024m"),
-						v12.ResourceStorage: resource.MustParse("100M"),
+						v12.ResourceCPU:              resource.MustParse("1024m"),
+						v12.ResourceEphemeralStorage: resource.MustParse("100M"),
 					},
 				},
 			},
 			map[v12.ResourceName][]v12.Toleration{
-				v12.ResourceStorage: {tolStorage},
-				ResourceNvidiaGPU:   {tolGPU},
+				v12.ResourceEphemeralStorage: {tolEphemeralStorage},
+				ResourceNvidiaGPU:            {tolGPU},
 			},
 			nil,
-			[]v12.Toleration{tolStorage},
+			[]v12.Toleration{tolEphemeralStorage},
 		},
 		{
 			"tolerations-both",
 			args{
 				v12.ResourceRequirements{
 					Limits: v12.ResourceList{
-						v12.ResourceCPU:     resource.MustParse("1024m"),
-						v12.ResourceStorage: resource.MustParse("100M"),
+						v12.ResourceCPU:              resource.MustParse("1024m"),
+						v12.ResourceEphemeralStorage: resource.MustParse("100M"),
 					},
 					Requests: v12.ResourceList{
-						v12.ResourceCPU:     resource.MustParse("1024m"),
-						v12.ResourceStorage: resource.MustParse("100M"),
+						v12.ResourceCPU:              resource.MustParse("1024m"),
+						v12.ResourceEphemeralStorage: resource.MustParse("100M"),
 					},
 				},
 			},
 			map[v12.ResourceName][]v12.Toleration{
-				v12.ResourceStorage: {tolStorage},
-				ResourceNvidiaGPU:   {tolGPU},
+				v12.ResourceEphemeralStorage: {tolEphemeralStorage},
+				ResourceNvidiaGPU:            {tolGPU},
 			},
 			nil,
-			[]v12.Toleration{tolStorage},
+			[]v12.Toleration{tolEphemeralStorage},
 		},
 		{
 			"no-tolerations-both",
 			args{
 				v12.ResourceRequirements{
 					Limits: v12.ResourceList{
-						v12.ResourceCPU:     resource.MustParse("1024m"),
-						v12.ResourceStorage: resource.MustParse("100M"),
-						ResourceNvidiaGPU:   resource.MustParse("1"),
+						v12.ResourceCPU:              resource.MustParse("1024m"),
+						v12.ResourceEphemeralStorage: resource.MustParse("100M"),
+						ResourceNvidiaGPU:            resource.MustParse("1"),
 					},
 					Requests: v12.ResourceList{
-						v12.ResourceCPU:     resource.MustParse("1024m"),
-						v12.ResourceStorage: resource.MustParse("100M"),
+						v12.ResourceCPU:              resource.MustParse("1024m"),
+						v12.ResourceEphemeralStorage: resource.MustParse("100M"),
 					},
 				},
 			},
 			map[v12.ResourceName][]v12.Toleration{
-				v12.ResourceStorage: {tolStorage},
-				ResourceNvidiaGPU:   {tolGPU},
+				v12.ResourceEphemeralStorage: {tolEphemeralStorage},
+				ResourceNvidiaGPU:            {tolGPU},
 			},
 			nil,
-			[]v12.Toleration{tolStorage, tolGPU},
+			[]v12.Toleration{tolEphemeralStorage, tolGPU},
 		},
 		{
 			"default-tolerations",
 			args{},
 			nil,
-			[]v12.Toleration{tolStorage},
-			[]v12.Toleration{tolStorage},
+			[]v12.Toleration{tolEphemeralStorage},
+			[]v12.Toleration{tolEphemeralStorage},
 		},
 	}
 	for _, tt := range tests {