Update workload form to work with pod manifest #10208
Conversation
This updates the pod workload spec to match the expected manifest described by `kubectl explain workloads.pod.spec`. Previously, pods were treated as a Workload Resource [^1], but workload resources are meant for dealing with Deployments, Jobs, ReplicaSets, etc... It's more accurate to treat Pods as a workload, as described in [^2] [^1]: https://kubernetes.io/docs/concepts/workloads/controllers/ [^2]: https://kubernetes.io/docs/concepts/workloads/pods/ Signed-off-by: Phillip Rak <[email protected]>
This creates a new method, `updateWorkloadManifest()`, that can properly parse a manifest for both Workloads (Pods) and Workload Resources (Jobs, Deployments, ReplicaSets, etc...). Signed-off-by: Phillip Rak <[email protected]>
| if ((this.mode === _EDIT || this.mode === _VIEW || this.realMode === _CLONE ) && this.value.type === 'pod') { | ||
| const podSpec = { ...this.value.spec }; | ||
| const metadata = { ...this.value.metadata }; | ||
|
|
||
| this.$set(this.value.spec, 'template', { spec: podSpec, metadata }); | ||
| } |
There was a problem hiding this comment.
I don't believe that this is required anymore as it appears to have existed so that we can translate a valid pod spec into an invalid one.
| const spec = this.value.spec; | ||
| let podTemplateSpec = type === WORKLOAD_TYPES.CRON_JOB ? spec.jobTemplate.spec.template.spec : spec?.template?.spec; | ||
| let podTemplateSpec = type === WORKLOAD_TYPES.CRON_JOB ? spec.jobTemplate.spec.template.spec : this.value.type === POD ? spec : spec?.template?.spec; |
There was a problem hiding this comment.
I might consider rewriting this so that it's much more clear that we have multiple conditionals to deal Pods, Cron job, and the rest.
There was a problem hiding this comment.
I don't find this terribly unreadable. It looks like it renders lines 202-204 redundant though
| this.type !== WORKLOAD_TYPES.JOB && | ||
| this.type !== WORKLOAD_TYPES.CRON_JOB && | ||
| (this.mode === _CREATE || this.realMode === _CLONE) |
There was a problem hiding this comment.
This might be another case where we want to add
this.type !== POD
rak-phillip
changed the title
|
Just a note, i don't think there'll be conflicts or changes, but there's some work in a pending pr around the area of saving workloads. I think this PR might make a line or two of that redundant |
| const spec = this.value.spec; | ||
| let podTemplateSpec = type === WORKLOAD_TYPES.CRON_JOB ? spec.jobTemplate.spec.template.spec : spec?.template?.spec; | ||
| let podTemplateSpec = type === WORKLOAD_TYPES.CRON_JOB ? spec.jobTemplate.spec.template.spec : this.value.type === POD ? spec : spec?.template?.spec; |
There was a problem hiding this comment.
I don't find this terribly unreadable. It looks like it renders lines 202-204 redundant though
| }, | ||
| set(neu) { | ||
| if (this.isCronJob) { | ||
| if (this.isPod) { | ||
| this.spec = { ...this.spec, ...neu }; |
There was a problem hiding this comment.
I think this would prevent deleting any fields from podTemplateSpec. Not sure how much it matters but it seems like an unintended effect?
| if (spec?.affinity && Object.keys(spec?.affinity).length === 0) { | ||
| delete spec.affinity; | ||
| } | ||
|
|
||
| // Removing `affinity` fixes the issue with setting the `imagePullSecrets` | ||
| // However, this field should not be set. Therefore this is explicitly removed. | ||
| if (template?.spec?.imagePullSecrets && template?.spec?.imagePullSecrets.length === 0) { | ||
| delete template.spec.imagePullSecrets; | ||
| if (spec?.imagePullSecrets && spec?.imagePullSecrets.length === 0) { | ||
| delete spec.imagePullSecrets; | ||
| } |
There was a problem hiding this comment.
When I create a pod in the UI then go to 'edit config' and save without making changes OR with only a valid change (eg changing image) I get an error:
I traced it back to this block of code. When a pod is created in the UI and affinity isn't configured it's still saved with affinity: {} - we remove that on edit to ensure pods created outside of the ui don't have this field erroneously added, which, to me, really sounds like it shouldn't work for pods created in the UI but it does in master. I'm guessing because the whole pod edit process is jacked up anyway...?
Summary
This updates the pod workload spec to match the expected manifest described by
kubectl explain workloads.pod.spec. Previously, pods were treated as a Workload Resource 1, but workload resources are meant for dealing with Deployments, Jobs, ReplicaSets, etc... It's more accurate to treat Pods as a workload, as described in 2We also create a new method,
updateWorkloadManifest(), that can properly parse a manifest for both Workloads (Pods) and Workload Resources (Jobs, Deployments, ReplicaSets, etc...).Fixes #10171
Occurred changes and/or fixed issues
kubectl explain workloads.pod.specsaveWorkload()method so that it can now account for pods in addition to existing workloadsTechnical notes summary
I noted some difficulties associated with this change in #10171 (comment). In summary, the manifest for a Pod differs from Deployments, ReplicaSet, StatefulSets, etc... The tightly coupled nature of CruResource and the Yaml Editor makes it a little difficult to simply write a parser to consume the YAML as we would expect it for a pod, so we have to take a more difficult approach of updating
specin all the instances where we might be dealing with a pod to make sure that it adheres to the correct shape.With the complexity of this form, there might be edge cases that have been missed.
Areas or cases that should be tested
Creating, Reading, Updating existing workload forms, this includes:
Areas which could experience regressions
Screenshot/Video
Before
After
Footnotes
https://kubernetes.io/docs/concepts/workloads/controllers/ ↩
https://kubernetes.io/docs/concepts/workloads/pods/ ↩