fix: propagate context through configmap follow-up#5808
fix: propagate context through configmap follow-up#5808CAICAIIs wants to merge 2 commits intofluid-cloudnative:masterfrom
Conversation
Signed-off-by: CAICAIIs <3360776475@qq.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 @CAICAIIs. 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. |
There was a problem hiding this comment.
Code Review
This pull request refactors several internal methods and utility functions to support context propagation, ensuring better lifecycle management and cancellation handling during ConfigMap and DaemonSet operations. Key changes include updating the CacheEngine and ReferenceDatasetEngine to pass contexts through their setup and synchronization flows, and introducing context-aware helper functions in the kubeclient package. Comprehensive unit tests were added to verify context cancellation behavior and error handling. I have no feedback to provide as the implementation is consistent and well-tested.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5808 +/- ##
==========================================
+ Coverage 58.17% 58.49% +0.32%
==========================================
Files 478 478
Lines 32485 32479 -6
==========================================
+ Hits 18899 19000 +101
+ Misses 12042 11916 -126
- Partials 1544 1563 +19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Follow-up to #5705/#5747 to finish propagating reconcile context.Context through remaining ConfigMap/DaemonSet helper paths so cancellation/timeouts flow down to controller-runtime client calls.
Changes:
- Add
CreateConfigMapWithOwnerWithContext(...)to kubeclient ConfigMap helpers and keepCreateConfigMapWithOwner(...)as a compatibility wrapper. - Wire reconcile context through cache engine ConfigMap create/sync paths and thin referencedataset DaemonSet/ConfigMap copy paths.
- Add/extend focused unit tests to validate context cancellation propagation and existing “skip/AlreadyExists” semantics.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/utils/kubeclient/configmap.go | Introduces ctx-aware CreateConfigMapWithOwnerWithContext and wraps legacy helper. |
| pkg/utils/kubeclient/configmap_test.go | Adds a cancellation-propagation test for the new ctx-aware owner create helper. |
| pkg/ddc/cache/engine/cm.go | Threads context into cache runtime ConfigMap creation paths (extra-resources + runtime value CM). |
| pkg/ddc/cache/engine/setup.go | Passes reconcile context into cache ConfigMap setup chain. |
| pkg/ddc/cache/engine/sync.go | Threads context into cache runtime value ConfigMap sync path (Get/Create/Update). |
| pkg/ddc/cache/engine/cm_test.go | Adds unit tests for cache ConfigMap create/sync behavior and cancellation propagation. |
| pkg/ddc/thin/referencedataset/engine.go | Passes reconcile context down into referencedataset DaemonSet/ConfigMap helpers. |
| pkg/ddc/thin/referencedataset/cm.go | Converts referencedataset DaemonSet/ConfigMap operations to ctx-aware kubeclient/client calls. |
| pkg/ddc/thin/referencedataset/cm_test.go | Adds tests ensuring cancellation propagation and preserves existing skip/error semantics. |
Comments suppressed due to low confidence (1)
pkg/ddc/cache/engine/sync.go:64
syncRuntimeValueConfigMapuses ctx-aware ConfigMap helpers, butgenerateRuntimeConfigData(runtime)still reads the Dataset viautils.GetDataset(...)which usescontext.TODO()internally. That means a canceled reconcile context may still allow the datasetGetcall to proceed. To fully propagate cancellation/timeouts, passctxintogenerateRuntimeConfigDataand useutils.GetDatasetWithContext(ctx, ...)(or passctx.Contextexplicitly).
func (e *CacheEngine) syncRuntimeValueConfigMap(ctx cruntime.ReconcileRequestContext, runtime *datav1alpha1.CacheRuntime) error {
configMap, err := kubeclient.GetConfigmapByNameWithContext(ctx, e.Client, e.getRuntimeConfigConfigMapName(), e.namespace)
if err != nil {
return err
}
data, err := e.generateRuntimeConfigData(runtime)
if err != nil {
return err
}
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: CAICAIIs <3360776475@qq.com>
|



Ⅰ. Describe what this PR does
This PR is a small follow-up for issue #5705 to finish wiring reconcile context into the remaining short ConfigMap/DaemonSet helper paths after #5747.
It keeps the change narrowly scoped to:
Main changes:
CreateConfigMapWithOwnerWithContext(...)inpkg/utils/kubeclient/configmap.goCreateConfigMapWithOwner(...)as a compatibility wrapperctxfrom cacheSetup(ctx)/Sync(ctx)down to:createRuntimeConfigMapscreateConfigMapInRuntimeClasscreateRuntimeValueConfigMapsyncRuntimeValueConfigMapGetConfigmapByNameWithContextCreateConfigMapWithOwnerWithContextUpdateConfigMapWithContextctxfrom thin referencedatasetSetup(ctx)down to:copyFuseDaemonSetForRefDatasetcreateConfigMapForRefDatasetGetDaemonsetWithContextCopyConfigMapWithContextclient.Create(ctx, ...)This PR does not expand into repo-wide context cleanup or unrelated refactors.
Ⅱ. Does this pull request fix one issue?
fixes #5705
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Added/updated unit tests:
pkg/utils/kubeclient/configmap_test.goCreateConfigMapWithOwnerWithContextreally passes canceled context to the underlying client viacontextAwareClientpkg/ddc/cache/engine/cm_test.gocontext.Canceledon cache ConfigMap pathspkg/ddc/thin/referencedataset/cm_test.goAlreadyExistssemantics remain unchangedⅣ. Describe how to verify it
Run the focused verification set: