fix(cache): propagate runtime-level podMetadata and imagePullSecrets to component pods - #6188
Conversation
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
|
Hi @btxu-db. Thanks for your PR. I'm waiting for a fluid-cloudnative member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
`CacheRuntimeSpec.PodMetadata` and `CacheRuntimeSpec.ImagePullSecrets` are in the CRD and the API server takes them, but nothing on the transform path reads them. `transformComponentPodTemplate` composes a component's pod template from the CacheRuntimeClass template and the component-level spec only, so the runtime-level layer never enters and neither field reaches the pod. Pass `runtime.Spec` into `transformComponentPodTemplate` and layer `podMetadata` the way `spec.options` already is: template < runtime level < component level. `imagePullSecrets` has no component-level counterpart, so the runtime-level list is merged onto whatever the template declares, comparing by name -- the CRD marks the field with a merge patch strategy keyed on name, so a secret named at both layers should be carried once. Both fields are read on the transform path, which runs at setup, so this does not make them updatable on a live CacheRuntime; `syncRuntimeSpec` still handles only runtimeVersion, resources and replicas. Fixes fluid-cloudnative#6184 Signed-off-by: btxu-db <btxu-db@outlook.com>
c6526ed to
ce7ef73
Compare
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6188 +/- ##
==========================================
+ Coverage 65.24% 65.29% +0.04%
==========================================
Files 486 486
Lines 34194 34217 +23
==========================================
+ Hits 22309 22341 +32
+ Misses 10135 10129 -6
+ Partials 1750 1747 -3 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|



Ⅰ. Describe what this PR does
CacheRuntimeSpechas two runtime-level fields whose doc comments say they apply to everycomponent:
They are in the CRD and the API server takes them, but the transform path does not read
either.
transformComponentPodTemplatecomposes a component's pod template from theCacheRuntimeClass template and the component-level
RuntimeComponentCommonSpec; theruntime-level layer is not one of its arguments, so it never enters. Setting both fields on a
CacheRuntime is accepted, and then nothing lands on the pod:
The component-level control landing is what narrows it: template rendering and the manifest
are fine, only the middle layer goes missing.
imagePullSecretsis the one likely to hurt —the kubelet gets no credentials for a component image behind a private registry, and the pull
failure names the image and the registry rather than the field that was dropped, so the runtime
spec is not an obvious place to look.
Approach.
podMetadatahas the same three-layer shape asspec.options, which alreadycomposes all three layers (
cm.go:146), so it is given the same order here: CacheRuntimeClasstemplate < runtime level < component level.
UnionMapsWithOverridecopies both operands andtolerates nil, so the old
!= nilguards around the component layer are no longer needed.imagePullSecretshas no component-level counterpart, so there is no precedence to settle.The runtime-level list is appended onto whatever the template declares, skipping names that
are already there. The CRD marks the field
+patchStrategy=merge +patchMergeKey=name, whichI read as a name declared at both layers being one entry rather than two. Replacing the
template's list outright would be defensible too, but that seemed more likely to surprise an
owner whose class template already pins a registry secret — happy to switch if maintainers
prefer the simpler rule.
Master, worker and client all go through
transformComponentPodTemplate, so all three callsites now pass
runtime.Spec.One thing this does not do: both fields are read on the transform path, which runs at setup,
so editing either on a live CacheRuntime still has no effect —
syncRuntimeSpechandlesruntimeVersion,resourcesandreplicasonly. That looks like a separate change, and itis part of what #6185 is about.
Ⅱ. Does this pull request fix one issue?
fixes #6184
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
transform_common_test.gocoverstransformComponentPodTemplatedirectly:the runtime level alone reaches the pod; the component level alone reaches the pod; all
three layers union, and a key declared at more than one layer takes the value from the
latest one.
runtime-level list alone reaches the pod; both layers merge, and a name declared at both
appears once.
composition_matrix_test.gois the executable form of the composition table in #6185. Eachrow states what the three layers declare and what the component should end up with, and every
row is replayed through both paths that compose them — the transform path on create and the
sync path on update — since the two are written independently and are expected to agree.
Rows tagged
knownBugpin current behaviour rather than intended behaviour, so that achange to any of them shows up in review; each names the issue that will change it and what
it should say afterwards (#6173 for resources, #6178 for
runtimeVersion, and theenv/nodeSelectormismatches under #6185). If #6177 merges first, the#6173row is theone that will need updating.
Rows for fields the sync path does not patch are restricted to the create path, which is
stated per row rather than left implicit.
Existing specs in the package are unchanged. The 12 failures in
ufs_test.goandsync_test.goon this package are present onmasteras well and are unrelated to thesechanges.