Remove storage as a task resource option

[Future-Outlier]

Tracking issue

Fixes #3955

Why are the changes needed?

Make the storage unused in the task resource option, so that users will not be confused about why they can define the storage but it didn't work.

What changes were proposed in this pull request?

1. Delete storage variable
   search strings here

• Resources_STORAGE
• v1.ResourceStorage
• TaskResourceSpec
• tolStorage
  Search variable like this.
  

2. Left flytek8s/copilot.go as the same, I am not sure about how it works, so I left it the same.

How was this patch tested?

1. run flytekit task with specified resource
2. use kubectl describe nodeId to check if the Resources as expected

Success Scenario

from flytekit import task, Resources

@task(
        requests=Resources(cpu="2", ephemeral_storage="200Mi", mem="200Mi"),
)
def test_storage() -> None:
    import time
    print("@@@ I am sleeping for 10 seconds")
    time.sleep(10)
    return

pyflyte run --remote remove_resource_storage.py test_storage

kubectl describe pod ff436fac5f4d74583b75-f4wb3a3y-0 -n flytesnacks-development

The output is below.

    Limits:
      cpu:                2
      ephemeral-storage:  200Mi
      memory:             200Mi
    Requests:
      cpu:                2
      ephemeral-storage:  200Mi
      memory:             200Mi

Failure Scenario

from flytekit import task, Resources

@task(
        requests=Resources(cpu="2", storage="200Mi"),
)
def test_storage() -> None:
    import time
    print("@@@ I am sleeping for 10 seconds")
    time.sleep(10)
    return

pyflyte run --remote remove_resource_storage.py test_storage

Related PRs

flyteorg/flytekit#2078

Docs link

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (abef3f5) 58.54% compared to head (c92cbc2) 58.57%.
Report is 2 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #4658      +/-   ##
==========================================
+ Coverage   58.54%   58.57%   +0.02%     
==========================================
  Files         625      624       -1     
  Lines       53669    53595      -74     
==========================================
- Hits        31423    31393      -30     
+ Misses      19731    19688      -43     
+ Partials     2515     2514       -1     

Flag	Coverage Δ
unittests	58.57% <ø> (+0.02%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[Future-Outlier]

@katrogan Please take a look, thanks a lot!

[katrogan]

this is fantastic! cc @eapolinario for flytekit owners review

[Future-Outlier]

Hi, @katrogan can we merge it?
thank you!

[katrogan]

LGTM can we remove storage from here

flyte/flyteadmin/pkg/manager/impl/util/resources.go

Lines 69 to 71 in a23b266

	if len(spec.Storage) > 0 {
	result.Storage = parseQuantityNoError(ctx, "project", "storage", spec.Storage)
	}

, and

flyte/flyteadmin/pkg/workflowengine/impl/prepare_execution.go

Lines 88 to 90 in a23b266

	if !taskResources.Defaults.Storage.IsZero() {
	requests.Storage = taskResources.Defaults.Storage
	}

and here too https://github.com/flyteorg/flyte/blob/master/flyteadmin/pkg/runtime/interfaces/task_resource_configuration.go#L9 to avoid any confusion?

cc @eapolinario

[Future-Outlier]

LGTM can we remove storage from here

flyte/flyteadmin/pkg/manager/impl/util/resources.go

Lines 69 to 71 in a23b266

	if len(spec.Storage) > 0 {
	result.Storage = parseQuantityNoError(ctx, "project", "storage", spec.Storage)
	}

, and

flyte/flyteadmin/pkg/workflowengine/impl/prepare_execution.go

Lines 88 to 90 in a23b266

	if !taskResources.Defaults.Storage.IsZero() {
	requests.Storage = taskResources.Defaults.Storage
	}

and here too https://github.com/flyteorg/flyte/blob/master/flyteadmin/pkg/runtime/interfaces/task_resource_configuration.go#L9 to avoid any confusion?
cc @eapolinario

I think the copilot will not be affected by it right?

[katrogan]

hey @Future-Outlier what do you mean by "I think the copilot will not be affected by it right?" Sorry not entirely following!

[Future-Outlier]

hey @Future-Outlier what do you mean by "I think the copilot will not be affected by it right?" Sorry not entirely following!

I was initially concerned that removing the Resource field in these three places might impact Copilot's service creation due to its use of Resource.
(However, after checking, Copilot is not affected!)

[Future-Outlier]

LGTM can we remove storage from here

flyte/flyteadmin/pkg/manager/impl/util/resources.go

Lines 69 to 71 in a23b266

	if len(spec.Storage) > 0 {
	result.Storage = parseQuantityNoError(ctx, "project", "storage", spec.Storage)
	}

, and

flyte/flyteadmin/pkg/workflowengine/impl/prepare_execution.go

Lines 88 to 90 in a23b266

	if !taskResources.Defaults.Storage.IsZero() {
	requests.Storage = taskResources.Defaults.Storage
	}

and here too https://github.com/flyteorg/flyte/blob/master/flyteadmin/pkg/runtime/interfaces/task_resource_configuration.go#L9 to avoid any confusion?
cc @eapolinario

Just remove all of them, thanks a lot!
8d5c0db

[katrogan]

thank you so much for verifying and refactoring all the tests!