feature(fluid-webhook): support to update check-mount.sh configmap on demand#5697
Conversation
|
Skipping CI for Draft Pull Request. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request enhances the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
Syspretor
changed the title
![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 introduces a mechanism to update the check-mount.sh ConfigMap on-demand by comparing a SHA256 hash of the script content. This is a valuable improvement. The overall logic is sound, but there are a couple of areas for improvement. First, the SHA256 hash is truncated to fit into a Kubernetes label, which is not ideal; using an annotation would be more appropriate. Second, there's some code duplication in the hash calculation logic. Finally, a comment for the new label constant is inconsistent. I've provided specific comments with suggestions to address these points.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #5697 +/- ##
==========================================
+ Coverage 61.22% 61.40% +0.17%
==========================================
Files 444 444
Lines 30557 30657 +100
==========================================
+ Hits 18710 18825 +115
+ Misses 10307 10285 -22
- Partials 1540 1547 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds “update-on-demand” behavior for the check-mount script ConfigMaps created/used by the Fluid FUSE webhook injection path, by tracking script content via a SHA256 label and updating existing ConfigMaps when the script changes.
Changes:
- Introduces a new label key to store a truncated SHA256 of the check-mount script content.
- Computes and attaches the script SHA256 label when generating the sidecar and app check-mount ConfigMaps.
- Updates existing ConfigMaps during injection when the stored SHA256 label is missing or differs from the current script.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 8 comments.
Show a summary per file
| File | Description |
|---|---|
| pkg/common/label.go | Adds a new label constant for storing the script SHA256 on ConfigMaps. |
| pkg/application/inject/fuse/poststart/script_gen_helper.go | Adds SHA256 computation helper and labels generated ConfigMaps with the script hash. |
| pkg/application/inject/fuse/poststart/check_fuse_default.go | Computes and stores the default sidecar script SHA256. |
| pkg/application/inject/fuse/poststart/check_fuse_app.go | Computes and stores the app script SHA256 and labels the app ConfigMap. |
| pkg/application/inject/fuse/mutator/mutator_default.go | Changes logic to update the sidecar ConfigMap when script hash differs. |
| pkg/application/inject/fuse/mount_point_script.go | Changes logic to update the app ConfigMap when script hash differs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // ConfigMap exists, check if the script SHA256 label matches | ||
| currentSHA256 := appScriptGen.GetScriptSHA256() | ||
| if existingCM.Labels != nil { | ||
| if labelSHA256, ok := existingCM.Labels[common.LabelCheckMountScriptSHA256]; ok && labelSHA256 == currentSHA256 { | ||
| s.log.V(1).Info("configmap script is up-to-date, skip update", "configMap", cmKey) | ||
| return appScriptGen, nil | ||
| } | ||
| } | ||
|
|
||
| // SHA256 mismatch or label missing: update the ConfigMap with latest script and SHA256 | ||
| s.log.Info("configmap script SHA256 mismatch or label missing, updating", "configMap", cmKey, "expectedSHA256", currentSHA256) | ||
| existingCM.Data = cm.Data | ||
| if existingCM.Labels == nil { | ||
| existingCM.Labels = map[string]string{} | ||
| } | ||
| existingCM.Labels[common.LabelCheckMountScriptSHA256] = currentSHA256 | ||
| if err = s.client.Update(context.TODO(), existingCM); err != nil { |
| } else { | ||
| // ConfigMap exists, check if the script SHA256 label matches | ||
| currentSHA256 := gen.GetScriptSHA256() | ||
| needUpdate := true | ||
| if existingCM.Labels != nil { | ||
| if labelSHA256, ok := existingCM.Labels[common.LabelCheckMountScriptSHA256]; ok && labelSHA256 == currentSHA256 { | ||
| needUpdate = false | ||
| } | ||
| } | ||
|
|
||
| if needUpdate { | ||
| // SHA256 mismatch or label missing: update the ConfigMap with latest script and SHA256 | ||
| newCM := gen.BuildConfigMap(dataset, cmKey) | ||
| existingCM.Data = newCM.Data | ||
| if existingCM.Labels == nil { | ||
| existingCM.Labels = map[string]string{} | ||
| } | ||
| existingCM.Labels[common.LabelCheckMountScriptSHA256] = currentSHA256 | ||
| // Preserve the dataset-id label if already set | ||
| if _, ok := existingCM.Labels[common.LabelAnnotationDatasetId]; !ok { | ||
| existingCM.Labels[common.LabelAnnotationDatasetId] = newCM.Labels[common.LabelAnnotationDatasetId] | ||
| } | ||
| if err = helper.client.Update(context.TODO(), existingCM); err != nil { | ||
| return err | ||
| } | ||
| } |
| // appScriptContentSHA256 stores the SHA256 hex of the app script content (first 63 chars for K8s label compatibility), | ||
| // computed once at package initialization. | ||
| var appScriptContentSHA256 string | ||
|
|
||
| func init() { | ||
| content := replacer.Replace(contentCheckMountReadyScript) | ||
| sum := sha256.Sum256([]byte(content)) | ||
| // K8s label values must be <= 63 characters; SHA256 hex is 64 chars, so truncate to 63. | ||
| appScriptContentSHA256 = fmt.Sprintf("%x", sum)[:63] | ||
| } |
| if needUpdate { | ||
| // SHA256 mismatch or label missing: update the ConfigMap with latest script and SHA256 | ||
| newCM := gen.BuildConfigMap(dataset, cmKey) | ||
| existingCM.Data = newCM.Data | ||
| if existingCM.Labels == nil { | ||
| existingCM.Labels = map[string]string{} | ||
| } | ||
| existingCM.Labels[common.LabelCheckMountScriptSHA256] = currentSHA256 | ||
| // Preserve the dataset-id label if already set | ||
| if _, ok := existingCM.Labels[common.LabelAnnotationDatasetId]; !ok { | ||
| existingCM.Labels[common.LabelAnnotationDatasetId] = newCM.Labels[common.LabelAnnotationDatasetId] |
| existingCM.Labels[common.LabelAnnotationDatasetId] = newCM.Labels[common.LabelAnnotationDatasetId] | ||
| } | ||
| if err = helper.client.Update(context.TODO(), existingCM); err != nil { | ||
| return err | ||
| } |
Syspretor
force-pushed
the
feature/support-update-check-mount-script-on-demand
branch
from
daa3a38 to
dabc4af
Compare
Syspretor
force-pushed
the
feature/support-update-check-mount-script-on-demand
branch
from
dabc4af to
e981521
Compare
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
| @@ -34,6 +36,14 @@ type scriptGeneratorHelper struct { | |||
| scriptContent string | |||
| scriptFileName string | |||
There was a problem hiding this comment.
Truncating SHA256 to 63 chars isn't really necessary for annotations - only labels have that limit. Keeping the full 64 chars removes any collision risk, however small.
| Data: data, | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
If scriptContent is empty, computeScriptSHA256 will panic on the [:63] slice. Worth a guard or at least a comment.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: cheyang The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Syspretor
force-pushed
the
feature/support-update-check-mount-script-on-demand
branch
from
e981521 to
7f06483
Compare
|
(fluid-webhook): support to update check-mount.sh configmap on demand
[WIP]
Ⅰ. Describe what this PR does
The check-mount script ConfigMap was only created when it did not exist. If the ConfigMap already existed, updates to the check-mount script would never be applied — there was no mechanism to detect or reconcile script changes at runtime.
This PR introduces a SHA256-based change detection mechanism for the check-mount script ConfigMap:
init()) and store it in thefluid.io/check-mount-script-sha256annotation on the ConfigMap.retry.RetryOnConflict— each retry re-fetches the latest ConfigMap to get a freshresourceVersion, avoiding409 Conflicterrors when multiple webhook requests are handled concurrently. Areflect.DeepEqualcheck guards the finalUpdatecall to avoid unnecessary writes.RefreshConfigMapContents()on the script generator to encapsulate the update logic, so callers don't need to know which fields (Data,Labels,Annotations) are managed by the generator. Future changes toBuildConfigMapautomatically apply to the update path as well.Ⅱ. Does this pull request fix one issue?
fixes #XXXX
Ⅲ. List the added test cases (unit test/integration test) if any, please explain if no tests are needed.
Ⅳ. Describe how to verify it
Ⅴ. Special notes for reviews