Enable pod template and Use copy to construct head/worker in ray plugin

[ByronHsu]

Type

• Bug Fix
• Feature
• Plugin

Are all requirements met?

• Code completed
• Smoke tested
• Unit tests added
• Code documentation added
• Any pending items have an associated Issue

Complete description

Why?

1. The current implementation is unable to parse podtemplate
2. The current implementation will lose container information (like Volume) when constructing worker/head pod because it created a new container from scratch

What?

1. Use flytek8s.ToK8sPodSpec to parse both container and pod type
2. Copy the container from the original primary container instead of creating a new one

[codecov]

Codecov Report

Merging #349 (5e7619a) into master (9a2bbba) will increase coverage by 1.40%.
The diff coverage is 84.09%.

❗ Current head 5e7619a differs from pull request most recent head d725c98. Consider uploading reports for the commit d725c98 to get more accurate results

@@            Coverage Diff             @@
##           master     #349      +/-   ##
==========================================
+ Coverage   62.77%   64.18%   +1.40%     
==========================================
  Files         148      148              
  Lines       12664    10283    -2381     
==========================================
- Hits         7950     6600    -1350     
+ Misses       4105     3073    -1032     
- Partials      609      610       +1     

Flag	Coverage Δ
unittests	?

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

Impacted Files	Coverage Δ
go/tasks/plugins/k8s/ray/ray.go	77.48% <84.09%> (-0.09%)	⬇️

... and 130 files with indirect coverage changes

[hamersaw]

So calling ToK8sPodSpec does a lot of things, like injecting Flyte-specific customizations. I think the original implementation decided to call ToK8sContainer directly to keep these out of the Pod definition. I just want to make sure that all of this additional Pod construction logic will not introduce regressions.

The alternative is to call MergeWithBasePodTemplate similar to here which just merges the PodSpec with the default PodTemplate (if exists). This was purposefully refactored to ensure the default PodTemplate could be applied to any PodSpec.

[hamersaw]

So calling ToK8sPodSpec does a lot of things, like injecting Flyte-specific customizations. I think the original implementation decided to call ToK8sContainer directly to keep these out of the Pod definition. I just want to make sure that all of this additional Pod construction logic will not introduce regressions.

The alternative is to call MergeWithBasePodTemplate similar to here which just merges the PodSpec with the default PodTemplate (if exists). This was purposefully refactored to ensure the default PodTemplate could be applied to any PodSpec.

cc @pingsutw thoughts?

[hamersaw]

Also @bstadlbauer, whatever we decide to do here (ie. applying default PodTemplate to PodSpec) we should mimic in the Dask plugin. These two plugins are the last to wrap up that work.

[bstadlbauer]

@hamersaw Sounds good to me 👍 Should I start right away or wait on this PR?

[hamersaw]

@hamersaw Sounds good to me +1 Should I start right away or wait on this PR?

@bstadlbauer it looks like the dask plugin will call the MergeWithBasePodTemplate function directly because we're building the PodSpec manually with specific options (ex. workerSpec), is this what you think too?

Just trying to keep some semblance of consistency across the Flyte maintained plugins. Very interested in hearing thoughts from all involved!

[pingsutw]

The reason we use ToK8sContainer is because flyte didn't support podtemplate while we were working on it.
I think it's fine to use ToK8sPodSpec. there are a lot of same logics between ToK8sContainer and ToK8sPodSpec

• Update cmd and resource. ToK8sContainer vs ToK8sPodSpec
• Copilot is only enabled in rawContainer task, so this code is fine
• Update security context. ToK8sContainer and ToK8sPodSpec
• Merge default pod template into podSpec. ray plugin needs it.

I think all the k8s plugins should use ToK8sPodSpec now because we need to merge flytekit podTemplate and default podTemplate into the podSpec. btw, kubeflow plugin already used ToK8sPodSpec. @hamersaw wdyt