support inplace update for runtime workers #4864
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.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
cheyang
changed the title
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4864 +/- ##
==========================================
- Coverage 55.77% 55.46% -0.31%
==========================================
Files 413 414 +1
Lines 28087 28261 +174
==========================================
+ Hits 15666 15676 +10
- Misses 11006 11167 +161
- Partials 1415 1418 +3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Signed-off-by: xliuqq <[email protected]> webhook cache the specific configmap and only inplace update worker pod has annotation Signed-off-by: xliuqq <[email protected]> fix test Signed-off-by: xliuqq <[email protected]> use crd instead of cm Signed-off-by: xliuqq <[email protected]> run make gen-openapi Signed-off-by: xliuqq <[email protected]> fix e2e test error Signed-off-by: xliuqq <[email protected]>
Signed-off-by: xliuqq <[email protected]>
|
There was a problem hiding this comment.
Pull Request Overview
This PR implements in-place update support for JuiceFS runtime worker pods to maintain pod-to-node affinity during updates. The feature introduces three update strategies: ReCreate (default), InPlace, and InPlaceIfPossible.
Key changes:
- Adds a new
PersistentPodStateCRD to track the mapping between worker pod names and their assigned nodes - Implements webhook mutation logic to inject node affinity/preference based on the update strategy
- Updates the JuiceFS runtime spec to include an
UpdateStrategyfield
Reviewed Changes
Copilot reviewed 29 out of 29 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
api/v1alpha1/persistentpodstate_types.go |
Defines new PersistentPodState CRD to store pod-to-node mappings |
api/v1alpha1/juicefsruntime_types.go |
Adds UpdateStrategy field to JuiceFSRuntimeSpec |
pkg/webhook/handler/mutating/mutating_handler.go |
Implements MutateRuntimeWorkerPod to inject node affinity based on update strategy |
pkg/utils/dataset/lifecycle/node.go |
Syncs worker pod states to PersistentPodState during reconciliation |
pkg/ddc/juicefs/transform.go |
Sets runtime pod type labels and annotations for worker pods requiring in-place updates |
charts/juicefs/templates/worker/persistent_podstate.yaml |
Template for creating PersistentPodState resource |
charts/fluid/fluid/templates/webhook/webhookconfiguration.yaml |
Adds webhook configuration for mutating worker pods |
| Multiple RBAC files | Grants permissions to access persistentpodstates resources |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| return admission.Errored(http.StatusInternalServerError, err) | ||
| // mutating runtime worker pod | ||
| if common.CheckExpectValue(pod.Labels, common.RuntimePodType, common.RuntimeWorkerPod) { | ||
| // mutating app pod, check whether should inject |
There was a problem hiding this comment.
The comment on line 93 is incorrect. It says 'mutating app pod' but this code block is actually for mutating runtime worker pods. The comment should say 'mutating runtime worker pod, check whether should inject' to match the actual context.
| // mutating app pod, check whether should inject | |
| // mutating runtime worker pod, check whether should inject |
| // states are not sync yet. | ||
| podState, ok := persistentPodState.Status.PodStates[pod.Name] | ||
| if !ok { | ||
| return |
There was a problem hiding this comment.
Missing explicit nil return. Line 185 returns without a value when podState is not found, but the function signature requires returning an error. While Go allows bare return for named return values (defaulting to the zero value), it's inconsistent with other return statements in this function (lines 153, 160, 169, 179, 205, 224, 227) which explicitly return nil. This should be return nil for consistency and clarity.
| return | |
| return nil |
| }, | ||
| } | ||
| utils.InjectNodeSelectorTerms(terms, pod) | ||
| return |
There was a problem hiding this comment.
Missing explicit nil return. Line 205 returns without a value after injecting node selector terms, but the function signature requires returning an error. This should be return nil for consistency with other return statements in the function.
| return | |
| return nil |
| }, | ||
| } | ||
| utils.InjectPreferredSchedulingTerms(terms, pod) | ||
| return |
There was a problem hiding this comment.
Missing explicit nil return. Line 224 returns without a value after injecting preferred scheduling terms, but the function signature requires returning an error. This should be return nil for consistency with other return statements in the function.
| return | |
| return nil |
|
|
||
| if !reflect.DeepEqual(persistentPodState.Status, updatedPodState.Status) { | ||
| updatedPodState.Status.LastUpdateTime = &metav1.Time{Time: time.Now()} | ||
| rootLog.Info("update podpersistentstate status", "status", updatedPodState.Status) |
There was a problem hiding this comment.
The log message contains 'podpersistentstate' which should be separated as 'pod persistent state' or 'PersistentPodState' for clarity and consistency with the resource name.
| rootLog.Info("update podpersistentstate status", "status", updatedPodState.Status) | |
| rootLog.Info("update PersistentPodState status", "status", updatedPodState.Status) |
2 participants
Ⅰ. Describe what this PR does
support inplace update for juicefs runtime workers pods .
Ⅱ. 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
manual test
Ⅴ. Special notes for reviews