fix: populate Status.Selector in CacheEngine for worker pod discovery#6064
fix: populate Status.Selector in CacheEngine for worker pod discovery#6064adityaupasani2 wants to merge 1 commit into
Conversation
CacheEngine never set Status.Selector on the CacheRuntime resource, unlike every other runtime (JindoCache, JuiceFS, Vineyard, EFC) which all call getWorkerSelectors() during master setup. Without it, HPA and any tooling that reads CacheRuntime.Status.Selector to discover worker pods sees an empty string. - Add getWorkerSelectors() to pkg/ddc/cache/engine/util.go, building the selector from LabelCacheRuntimeName and LabelCacheRuntimeComponentName labels — matching the convention used by other runtimes - Call it in setupMasterInternal() (master.go) to set Status.Selector when the master is first initialized, removing the TODO comment - Also set it in CheckAndUpdateRuntimeStatus() (status.go) on every reconcile so it stays consistent, removing the second TODO comment - Add tests for getWorkerSelectors() covering non-empty output, correct label content, and selector uniqueness per runtime name Fixes fluid-cloudnative#6063 Signed-off-by: Aditya Upasani <adityaupasani29@gmail.com>
|
[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 @adityaupasani2. 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. |
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements the getWorkerSelectors helper method to populate the Status.Selector field for CacheRuntime worker pods, enabling discovery by HPA and other tooling. Unit tests have also been added to verify this functionality. The review feedback suggests simplifying the implementation of getWorkerSelectors by using labels.SelectorFromSet from k8s.io/apimachinery/pkg/labels instead of metav1.LabelSelectorAsSelector, which eliminates unnecessary error handling and simplifies the imports.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
|
|
||
| "github.com/fluid-cloudnative/fluid/pkg/common" | ||
| "github.com/fluid-cloudnative/fluid/pkg/utils" | ||
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" |
There was a problem hiding this comment.
Since we can construct the label selector directly using labels.SelectorFromSet, we don't need to import metav1 here. We can import k8s.io/apimachinery/pkg/labels instead.
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | |
| "k8s.io/apimachinery/pkg/labels" |
| labels := map[string]string{ | ||
| common.LabelCacheRuntimeName: e.name, | ||
| common.LabelCacheRuntimeComponentName: workerName, | ||
| } | ||
| labelSelector := &metav1.LabelSelector{ | ||
| MatchLabels: labels, | ||
| } | ||
| selectorValue := "" | ||
| selector, err := metav1.LabelSelectorAsSelector(labelSelector) | ||
| if err != nil { | ||
| e.Log.Error(err, "Failed to parse the labelSelector of the runtime", "labels", labels) | ||
| } else { | ||
| selectorValue = selector.String() | ||
| } | ||
| return selectorValue |
There was a problem hiding this comment.
Instead of manually constructing a metav1.LabelSelector and converting it via metav1.LabelSelectorAsSelector (which requires error handling), you can directly use labels.SelectorFromSet to build the selector string. This is cleaner, more idiomatic, and avoids unnecessary error handling since SelectorFromSet is guaranteed to succeed for a static map of valid labels.
| labels := map[string]string{ | |
| common.LabelCacheRuntimeName: e.name, | |
| common.LabelCacheRuntimeComponentName: workerName, | |
| } | |
| labelSelector := &metav1.LabelSelector{ | |
| MatchLabels: labels, | |
| } | |
| selectorValue := "" | |
| selector, err := metav1.LabelSelectorAsSelector(labelSelector) | |
| if err != nil { | |
| e.Log.Error(err, "Failed to parse the labelSelector of the runtime", "labels", labels) | |
| } else { | |
| selectorValue = selector.String() | |
| } | |
| return selectorValue | |
| return labels.SelectorFromSet(labels.Set{ | |
| common.LabelCacheRuntimeName: e.name, | |
| common.LabelCacheRuntimeComponentName: workerName, | |
| }).String() |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6064 +/- ##
=======================================
Coverage 64.77% 64.78%
=======================================
Files 484 484
Lines 33892 33909 +17
=======================================
+ Hits 21954 21968 +14
- Misses 10215 10216 +1
- Partials 1723 1725 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
|
||
| // TODO(cache runtime): figure out how to use this selector | ||
| // runtimeToUpdate.Status.Selector = e.getWorkerSelectors() | ||
| runtimeToUpdate.Status.Selector = e.getWorkerSelectors() |
There was a problem hiding this comment.
Heads-up, out of scope for this PR but worth flagging in #6063: populating Status.Selector here is correct and useful for any tooling that reads it directly, but HPA targeting CacheRuntime/scale will still not work until the scale subresource is declared. Unlike AlluxioRuntime, JuiceFSRuntime, and VineyardRuntime, the CacheRuntime type currently only declares +kubebuilder:subresource:status — there is no +kubebuilder:subresource:scale:...selectorpath=.status.selector annotation, and the generated CRD (config/crd/bases/data.fluid.io_cacheruntimes.yaml) reflects that (only subresources: status: {}). A follow-up PR adding the scale subresource will likely be needed to fully close the linked issue.
| e.Log.Error(err, "Failed to parse the labelSelector of the runtime", "labels", labels) | ||
| } else { | ||
| selectorValue = selector.String() | ||
| } |
There was a problem hiding this comment.
Minor: the gemini-code-assist bot suggested using labels.SelectorFromSet(labels) from k8s.io/apimachinery/pkg/labels here, which would drop the metav1.LabelSelectorAsSelector error branch entirely (SelectorFromSet cannot fail for plain MatchLabels). Not a blocker — the current form is consistent with the existing pattern in pkg/ddc/jindocache/worker.go, so keeping symmetry across runtimes is also a valid choice. Up to you.
| engine1 := &CacheEngine{name: "runtime-a", namespace: "default", Log: fake.NullLogger()} | ||
| engine2 := &CacheEngine{name: "runtime-b", namespace: "default", Log: fake.NullLogger()} | ||
| Expect(engine1.getWorkerSelectors()).NotTo(Equal(engine2.getWorkerSelectors())) | ||
| }) |
There was a problem hiding this comment.
Consider adding one assertion that pins the exact selector string (e.g. Expect(selector).To(Equal(...)) with the expected sorted form) so any future change to selector composition — label keys, ordering, escaping — is caught by tests. Right now the assertions only check substrings, so an unrelated label silently added in the future would not fail this test.
2 participants
Ⅰ. Describe what this PR does
CacheEnginenever populatedStatus.Selectoron theCacheRuntimeresource. Every other runtime (JindoCache, JuiceFS, Vineyard, EFC) callsgetWorkerSelectors()during master setup to set this field. Without it,CacheRuntime.Status.Selectoris always empty, breaking HPA and any tooling that reads this field to discover worker pods.Two TODOs marked this gap:
pkg/ddc/cache/engine/master.go:// TODO(cache runtime): figure out how to use this selectorpkg/ddc/cache/engine/status.go:// TODO(cache runtime): set the CacheRuntime Status left fields: SelectorThis PR resolves both.
Ⅱ. Does this pull request fix one issue?
Fixes #6063
Ⅲ. List the added test cases
Added 4 tests for
getWorkerSelectors()inutil_test.go:Ⅳ. Describe how to verify it
After creating a
CacheRuntime, check thatstatus.selectoris populated with a valid label selector string matching the worker pods.Ⅴ. Special notes for reviews
getWorkerSelectors()usescommon.LabelCacheRuntimeNameandcommon.LabelCacheRuntimeComponentName— the same labels set bygetCommonLabelsFromComponent()in the component manager, which are used as the StatefulSet selector. This matches the pattern used by JindoCache, JuiceFS, Vineyard and EFC.