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 2 commits 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✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #6064 +/- ##
=======================================
Coverage 64.77% 64.78%
=======================================
Files 484 484
Lines 33892 33898 +6
=======================================
+ Hits 21954 21960 +6
Misses 10215 10215
Partials 1723 1723 ☔ 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.
Replace metav1.LabelSelectorAsSelector (which requires error handling) with labels.SelectorFromSet from k8s.io/apimachinery/pkg/labels. SelectorFromSet cannot fail for a static map of valid labels, so the error branch is eliminated and the implementation becomes a one-liner. Also add an exact selector string assertion to pin the expected output so any future change to label keys, ordering, or escaping is caught. Addresses Gemini and cheyang review comments on fluid-cloudnative#6064 Signed-off-by: Aditya Upasani <adityaupasani29@gmail.com>
|
Addressed both review points:
On cheyang's note about the scale subresource — understood, that's a separate follow-up. This PR just populates /assign @cheyang |
|
|
/copilot review |
|
@gemini-code-assist /review |
|
Warning Gemini encountered an error creating the review. You can try again by commenting |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request implements the getWorkerSelectors method for the CacheEngine to populate the Status.Selector field of the CacheRuntime, enabling external tooling like HPA to discover worker pods. Unit tests have also been added to verify the correct generation of the selector string. There are no review comments, and I have no feedback to provide.
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.
|
Thanks for the iteration — the unit test for Specifically, please add a test that:
You can look at the existing kind-e2e tests under Let me know if you have questions about the test scaffolding or which runtime type to target. |
| // This is used to populate Status.Selector so that HPA and other tooling can | ||
| // discover worker pods, matching the pattern used by other runtimes (JindoCache, | ||
| // JuiceFS, Vineyard, EFC). | ||
| func (e *CacheEngine) getWorkerSelectors() string { |
There was a problem hiding this comment.
This PR still ships without an e2e test for the new behavior, and that was called out as the merge blocker last round. getWorkerSelectors is a controller-level change that flows into the CacheRuntime status subresource — unit tests cover the string-building side, but they do not exercise the actual reconcile path, the Status.Selector field on the live CR, or worker pod discovery through the scale endpoint.
Please add a focused e2e case (patterned after test/gha-e2e/curvine/, which already brings up a CacheRuntime end-to-end) that:
- Creates a CacheRuntime and waits for it to become ready.
- Reads the runtime CR and asserts
.status.selectoris non-empty and matchescacheruntime.fluid.io/component-name=<name>-worker,cacheruntime.fluid.io/runtime-name=<name>. - Uses
kubectl get --rawon the scale subresource (orkubectl get pods -l <selector>) to prove the selector actually resolves to the worker pods.
Note: the earlier review pointed at test/e2e/, but this repo uses test/gha-e2e/ for GitHub Actions kind-e2e and test/prow/ for the Python-based prow suite — either location is fine, whichever matches the runtime you target.
| } | ||
|
|
||
| // TODO(cache runtime): set the CacheRuntime Status left fields: Selector | ||
| runtimeToUpdate.Status.Selector = e.getWorkerSelectors() |
There was a problem hiding this comment.
Status.Selector = e.getWorkerSelectors() is now called both in setupMasterInternal (master.go:87) and in CheckAndUpdateRuntimeStatus (status.go:152). That is fine for correctness and matches the pattern in other engines, but it is worth a mental note: if a third status-write path ever appears, please refactor rather than adding a third copy. Not a blocker for this PR.
| Expect(engine.getWorkerSelectors()).To(Equal(expected)) | ||
| }) | ||
| }) | ||
| }) |
There was a problem hiding this comment.
The new unit tests are tight — the exact-string pin in particular is a good addition. What's still missing is an e2e case exercising the field on a real CR. See the blocking issue for the concrete asks. The unit suite alone cannot detect, for example, a controller-side bug where Status.Selector gets overwritten by a later status-writer, or where the selector is set on master.go's path but not on the reconcile path.
|
Thanks for the detailed direction on the e2e test. I want to make sure I understand the expected scope correctly before investing time writing it. Looking at test/gha-e2e/curvine/ as you suggested this spins up a full CacheRuntime end-to-end. For asserting
A few questions before I start:
Happy to write this once the scope is clear. |
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.