fix: add node affinity for node where PVC is mounted to workspace's backup Jobs #1677
fix: add node affinity for node where PVC is mounted to workspace's backup Jobs #1677rohanKanojia wants to merge 1 commit into
Conversation
|
Skipping CI for Draft Pull Request. |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: rohanKanojia 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 |
📝 WalkthroughWalkthroughChangesPVC node scheduling
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant BackupCronJobController
participant FindNodeForPVC
participant KubernetesAPI
participant BackupJob
BackupCronJobController->>FindNodeForPVC: Resolve PVC node
FindNodeForPVC->>KubernetesAPI: List labeled Pods
KubernetesAPI-->>FindNodeForPVC: Running Pod node
FindNodeForPVC-->>BackupCronJobController: Target node
BackupCronJobController->>BackupJob: Set required hostname affinity
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Hi! I'm che-ai-assistant — I help with your pull requests. Available commands:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@controllers/backupcronjob/backupcronjob_controller.go`:
- Around line 462-465: The FindNodeForPVC lookups must use fresh, non-cached Pod
scheduling data. In controllers/backupcronjob/backupcronjob_controller.go lines
462-465, update the FindNodeForPVC call to use r.NonCachingClient instead of
r.Client; in pkg/provision/storage/cleanup.go lines 122-127, use clusterAPI’s
equivalent non-caching client if available, otherwise leave the site unchanged
and document that no equivalent exists.
- Around line 466-484: Add an ActiveDeadlineSeconds value to the Job spec when
targetNode is set, alongside the hostname-pinning affinity in the backup Job
construction. Use the existing backup timeout/deadline configuration if
available, ensuring Pods that remain Pending on the pinned node terminate after
the configured limit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: e50314d0-655e-4718-9714-7e09ee17ac34
📒 Files selected for processing (5)
controllers/backupcronjob/backupcronjob_controller.gocontrollers/backupcronjob/backupcronjob_controller_test.gopkg/provision/storage/cleanup.gopkg/provision/storage/shared.gopkg/provision/storage/shared_test.go
| targetNode, err := provstorage.FindNodeForPVC(ctx, r.Client, workspace.Namespace, pvc.Name) | ||
| if err != nil { | ||
| log.Error(err, "Failed to find node with PVC, backup Job will not have node affinity", "pvc", pvc.Name) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Use a non-caching read for FindNodeForPVC's Pod list. Both call sites pass a (likely) cached client to look up live Pod scheduling state, which is exactly the kind of externally-mutated data the NonCachingClient guideline is meant to protect against — a stale cache read here can recreate the Multi-Attach race this PR is fixing.
controllers/backupcronjob/backupcronjob_controller.go#L462-L465: passr.NonCachingClientinstead ofr.Clienttoprovstorage.FindNodeForPVC(the reconciler already has this field per its test setup).pkg/provision/storage/cleanup.go#L122-L127: verify whetherclusterAPIexposes an equivalent non-caching client forFindNodeForPVC, and use it if available.
Based on path instructions: "Use the cached Client for normal reads in controller code and NonCachingClient only when freshness is required" and "Use NonCachingClient whenever fresh, non-cached resource data is required, especially immediately after create/update or when an external system may have changed the object."
📍 Affects 2 files
controllers/backupcronjob/backupcronjob_controller.go#L462-L465(this comment)pkg/provision/storage/cleanup.go#L122-L127
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@controllers/backupcronjob/backupcronjob_controller.go` around lines 462 -
465, The FindNodeForPVC lookups must use fresh, non-cached Pod scheduling data.
In controllers/backupcronjob/backupcronjob_controller.go lines 462-465, update
the FindNodeForPVC call to use r.NonCachingClient instead of r.Client; in
pkg/provision/storage/cleanup.go lines 122-127, use clusterAPI’s equivalent
non-caching client if available, otherwise leave the site unchanged and document
that no equivalent exists.
Source: Path instructions
There was a problem hiding this comment.
The cleanup job (cleanup.go) already uses the cached clusterAPI.Client for the same purpose before this PR.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
ast-grep outline pkg/provision/storage/cleanup.go --items all --type function --view expanded
rg -n -C 5 '\bFindNodeForPVC\s*\(' --glob '*.go'
rg -n -C 4 'type\s+ClusterAPI\b|struct\s*\{.*Client|NonCachingClient' --glob '*.go'Length of output: 4360
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- cleanup FindNodeForPVC call ---'
sed -n '111,145p' pkg/provision/storage/cleanup.go
printf '%s\n' '--- backup reconciler initialization ---'
rg -n -C 6 'BackupCronJobReconciler\s*\{' --glob '*.go' || true
printf '%s\n' '--- ClusterAPI declaration ---'
rg -n -C 10 'type\s+ClusterAPI\s+struct' --glob '*.go' || true
printf '%s\n' '--- all FindNodeForPVC callers ---'
rg -n -C 3 'FindNodeForPVC\s*\(' --glob '*.go' || trueLength of output: 2068
@rohanKanojia, confirmed: the cleanup path already uses clusterAPI.Client, and clusterAPI.NonCachingClient is available.
That pre-existing cached lookup does not remove the race, though: FindNodeForPVC lists Pods to select mandatory hostname affinity for a Job mounting an RWO PVC. Pod placement and volume attachment can change outside the controller cache, so this decision requires fresh state. The finding remains applicable to both paths:
- backup: pass
r.NonCachingClient - cleanup: pass
clusterAPI.NonCachingClient
🧬 Code Graph Analysis Results
controllers/workspace/devworkspace_controller.go (line 110-269)
clusterAPI := sync.ClusterAPI{
Client: r.Client,
NonCachingClient: r.NonCachingClient,
Scheme: r.Scheme,
Logger: reqLogger,
Ctx: ctx,
}Summary (from this snippet): Initializes a sync.ClusterAPI value with a cached Client (r.Client) plus a NonCachingClient (r.NonCachingClient), and stores Scheme, Logger, and Ctx. No return values; used later by other reconciliation logic to access cluster resources via the chosen client.
pkg/library/storage/storage.go (line 37-69)
func GetWorkspacePVCInfo(
ctx context.Context,
workspace *dw.DevWorkspace,
config *controllerv1alpha1.OperatorConfiguration,
k8sClient client.Client,
log logr.Logger,
) (pvcName string, workspacePath string, err error) {
workspaceWithConfig := &common.DevWorkspaceWithConfig{}
workspaceWithConfig.DevWorkspace = workspace
workspaceWithConfig.Config = config
storageProvisioner, err := storage.GetProvisioner(workspaceWithConfig)
if err != nil {
return "", "", err
}
if _, ok := storageProvisioner.(*storage.PerWorkspaceStorageProvisioner); ok {
pvcName := common.PerWorkspacePVCName(workspace.Status.DevWorkspaceId)
return pvcName, constants.DefaultProjectsSourcesRoot, nil
} else if _, ok := storageProvisioner.(*storage.CommonStorageProvisioner); ok {
if !storageProvisioner.NeedsStorage(&workspace.Spec.Template) {
// No storage provisioned for this workspace
return "", "", nil
}
pvcName := constants.DefaultWorkspacePVCName
if config.Workspace != nil && config.Workspace.PVCName != "" {
pvcName = config.Workspace.PVCName
}
return pvcName, workspace.Status.DevWorkspaceId + constants.DefaultProjectsSourcesRoot, nil
}
return "", "", nil
}Summary: Chooses PVC name/path based on the workspace’s storage provisioner; accepts k8sClient client.Client plus ctx and log, and returns (pvcName, workspacePath, err).
pkg/provision/storage/shared.go (line 274-287)
func FindNodeForPVC(ctx context.Context, k8sClient client.Client, namespace, pvcName string) (string, error) {
labelSelector, err := labels.Parse(constants.DevWorkspaceIDLabel)
if err != nil {
return "", err
}
podList := &corev1.PodList{}
if err := k8sClient.List(ctx, podList, &client.ListOptions{
Namespace: namespace,
LabelSelector: labelSelector,
}); err != nil {
return "", err
}
return getNodeNameWithPVC(podList, pvcName), nil
}Summary: Lists Pods using k8sClient.List(...) filtered by constants.DevWorkspaceIDLabel and returns the node name for the given pvcName, or an error.
pkg/provision/storage/shared.go (line 292-310)
func NodeAffinityForHostname(nodeName string) *corev1.Affinity {
return &corev1.Affinity{
NodeAffinity: &corev1.NodeAffinity{
RequiredDuringSchedulingIgnoredDuringExecution: &corev1.NodeSelector{
NodeSelectorTerms: []corev1.NodeSelectorTerm{
{
MatchExpressions: []corev1.NodeSelectorRequirement{
{
Key: corev1.LabelHostname,
Operator: corev1.NodeSelectorOpIn,
Values: []string{nodeName},
},
},
},
},
},
},
}
}Summary: Builds a NodeAffinity targeting corev1.LabelHostname=nodeName. Returns *corev1.Affinity.
…evfile#1675) On multi-node clusters, backup Jobs for workspaces using ReadWriteOnce PVCs can fail with Multi-Attach errors when scheduled on a different node than the one where the PVC is currently mounted. Pin backup Jobs to the correct node using NodeAffinity, matching the existing pattern in cleanup Jobs. Consolidate duplicated node-lookup logic from cleanup.go and the backup controller into a shared FindNodeForPVC function in pkg/provision/storage. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: Rohan Kumar <rohaan@redhat.com>
d227890 to
6f2e86a
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
controllers/backupcronjob/backupcronjob_controller_test.go (1)
429-499: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd
By(...)steps to the new Ginkgo tests.These
Itblocks contain several setup, execution, and assertion phases but noBy(...)markers, making failures harder to localize and violating the repository test guideline.As per coding guidelines, Ginkgo tests should use
By("...")to document test steps.Also applies to: 501-538
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@controllers/backupcronjob/backupcronjob_controller_test.go` around lines 429 - 499, Add descriptive By(...) markers throughout the new Ginkgo It blocks, including the setup of configuration and resources, execution of executeBackupSync, retrieval of the Job, and affinity assertions. Apply the same step documentation to the additional test covering lines 501-538, using markers that clearly identify each phase and preserve the existing test behavior.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/provision/storage/shared.go`:
- Around line 274-283: The callers of FindNodeForPVC in the backup cronjob
reconciliation and workspace cleanup flows must use their respective
NonCachingClient instead of cached clients. Replace r.Client and
clusterAPI.Client at those call sites while leaving FindNodeForPVC’s lookup
logic unchanged.
- Around line 280-286: Update both FindNodeForPVC callers to distinguish lookup
failures from a valid empty node result: propagate or return non-NotFound errors
so reconciliation retries and does not create a Job without NodeAffinity, while
preserving the existing behavior for NotFound and genuinely empty node lookups.
- Around line 289-310: The NodeAffinityForHostname function currently uses the
Node name directly as the kubernetes.io/hostname label value, which can fail on
cloud nodes where they differ. Resolve the target Node’s kubernetes.io/hostname
label before constructing the affinity, or use a Node-name field selector if
that is the intended contract, and ensure the resulting scheduling constraint
matches the node returned by FindNodeForPVC.
---
Nitpick comments:
In `@controllers/backupcronjob/backupcronjob_controller_test.go`:
- Around line 429-499: Add descriptive By(...) markers throughout the new Ginkgo
It blocks, including the setup of configuration and resources, execution of
executeBackupSync, retrieval of the Job, and affinity assertions. Apply the same
step documentation to the additional test covering lines 501-538, using markers
that clearly identify each phase and preserve the existing test behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 36927a32-e92c-47e0-b7ba-008a9b8c9874
📒 Files selected for processing (6)
controllers/backupcronjob/backupcronjob_controller.gocontrollers/backupcronjob/backupcronjob_controller_test.gopkg/provision/storage/cleanup.gopkg/provision/storage/cleanup_test.gopkg/provision/storage/shared.gopkg/provision/storage/shared_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- controllers/backupcronjob/backupcronjob_controller.go
- pkg/provision/storage/shared_test.go
- pkg/provision/storage/cleanup.go
| func FindNodeForPVC(ctx context.Context, k8sClient client.Client, namespace, pvcName string) (string, error) { | ||
| labelSelector, err := labels.Parse(constants.DevWorkspaceIDLabel) | ||
| if err != nil { | ||
| return "", err | ||
| } | ||
| podList := &corev1.PodList{} | ||
| if err := k8sClient.List(ctx, podList, &client.ListOptions{ | ||
| Namespace: namespace, | ||
| LabelSelector: labelSelector, | ||
| }); err != nil { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | grep -E '(^|/)(shared\.go|backupcronjob_controller\.go|cleanup\.go)$' || true
echo
echo "Relevant occurrences:"
rg -n "FindNodeForPVC|NonCachingClient|backupcronjob_controller\.go|cleanup\.go" -S .
echo
echo "Context shared.go around FindNodeForPVC:"
file=$(git ls-files | grep 'pkg/provision/storage/shared.go' | head -n1)
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '240,310p' "$file" | cat -n
fi
echo
echo "Context backup cron job around call:"
file=$(git ls-files | grep 'controllers/backupcronjob/backupcronjob_controller.go' | head -n1)
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '430,485p' "$file" | cat -n
fi
echo
echo "Context cleanup around call:"
file=$(git ls-files | grep 'pkg/provision/storage/cleanup.go' |头 n1 | sed 's/ / /;s/ */ /')
if [ -n "${file:-}" ]; then
wc -l "$file"
sed -n '90,145p' "$file" | cat -n
fiRepository: devfile/devworkspace-operator
Length of output: 8245
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Backup caller and type fields:"
sed -n '48,70p' controllers/backupcronjob/backupcronjob_controller.go | cat -n
sed -n '445,468p' controllers/backupcronjob/backupcronjob_controller.go | cat -n
echo
echo "Cleanup callers:"
rg -n "FindNodeForPVC\\(" controllers pkg/provision/storage -S
sed -n '110,130p' controllers/workspace/cleanup.go | cat -n
sed -n '90,130p' pkg/provision/storage/cleanup.go | cat -n
echo
echo "ClusterAPI fields:"
rg -n "type ClusterAPI|NonCachingClient|Client.*ClusterAPI|Ctx.*ClusterAPI" -S pkg controllers -g '*.go' | head -n 80
sed -n '1,110p' pkg/provision/sync/cluster_api.go | cat -nRepository: devfile/devworkspace-operator
Length of output: 8337
Use NonCachingClient for PVC placement lookups.
FindNodeForPVC determines where the PVC is currently mounted, but both callers pass cached clients:
controllers/backupcronjob/backupcronjob_controller.go:462usesr.Clientcontrollers/workspace/cleanup.go:122usesclusterAPI.Client
Fresh pod data is needed to avoid pinning cleanup or backup Jobs to a stale node and causing ReadWriteOnce PV Multi-Attach failures. Pass each caller’s NonCachingClient.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/provision/storage/shared.go` around lines 274 - 283, The callers of
FindNodeForPVC in the backup cronjob reconciliation and workspace cleanup flows
must use their respective NonCachingClient instead of cached clients. Replace
r.Client and clusterAPI.Client at those call sites while leaving
FindNodeForPVC’s lookup logic unchanged.
Source: Coding guidelines
|
/che-ai-assistant ok-pr-readiness Task completed. |
PR Readiness Assessment: PR #1677 — fix: add node affinity for node where PVC is mounted to workspace's backup JobsRepository: devfile/devworkspace-operator
Overall: ✅ READY Missing Information
What's Good
|
|
@rohanKanojia: The following tests failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
2 participants
What does this PR do?
I noticed this issue while testing pre-release-testing claude workflow where Backup restore test scripts failed on clusterbot clusters.
Multi-Attach errorwhen the Job is scheduled on a different node than the one where the ReadWriteOnce PVC is mountedNodeAffinitywithkubernetes.io/hostname, matching the existing pattern used by cleanup Jobscleanup.goand the backup controller into a sharedFindNodeForPVCfunction inpkg/provision/storage/shared.goWhat issues does this PR fix or reference?
Fix #1675
By default,
claim-devworkspacePVC usesReadWriteOnceaccess mode, meaning it can only be attached to one node at a time. When another workspace is running and has the PVC mounted, the backup Job must be scheduled on the same node. Without node affinity, Kubernetes may schedule the Job on a different node, causing:The cleanup Job in
cleanup.goalready solved this by looking up which node has the PVC mounted and settingNodeAffinity. This PR:FindNodeForPVC(ctx, client, namespace, pvcName)inpkg/provision/storage/shared.gocleanup.goto use the shared function (removing ~37 lines of duplicated code)BackupCronJobReconcilerIs it tested? How?
Managed to reproduce the Multi-Attach error in a cluster bot cluster while running the backup/restore workflow.
Test case 1: NodeAffinity is added when another workspace is running (2 DevWorkspaces: 1 running, 1 stopped)
This verifies that when a running workspace has the PVC mounted, the backup Job for the stopped workspace gets NodeAffinity pinning it to the same node.
Deploy DWO from the
fix-backup-job-node-affinitybranch to the clusterEnable backup in the
DevWorkspaceOperatorConfig:oc patch devworkspaceoperatorconfig devworkspace-operator-config \ -n openshift-operators --type=merge \ -p '{"config":{"workspace":{"backupCronJob":{"enable":true,"schedule":"* * * * *","registry":{"path":"image-registry.openshift-image-registry.svc:5000"},"oras":{"extraArgs":"--insecure"}}}}}'Create a test namespace:
Create two DevWorkspaces that share the common PVC (
claim-devworkspace):Both workspaces use the default
commonstorage type, so they share theclaim-devworkspacePVC.Wait for both workspaces to reach
Runningstate:Stop one of the workspaces (this triggers a backup Job on the next cron tick):
oc patch dw test-workspace-a -n dwo-backup-test --type=merge \ -p '{"spec":{"started":false}}'Wait for the backup Job to be created (within 1 minute based on cron schedule):
oc get jobs -n dwo-backup-test -wVerify the backup Job has NodeAffinity pinning it to the node where
test-workspace-bis running:Expected output:
The backup Job pod should start successfully without Multi-Attach errors
Test case 2: NodeAffinity is not added when no other workspace is running (1 DevWorkspace, stopped)
This verifies that when no running workspace has the PVC mounted, the backup Job is created without NodeAffinity (since there's no contention risk).
Deploy DWO from the
fix-backup-job-node-affinitybranch to the clusterEnable backup in the
DevWorkspaceOperatorConfig:oc patch devworkspaceoperatorconfig devworkspace-operator-config \ -n openshift-operators --type=merge \ -p '{"config":{"workspace":{"backupCronJob":{"enable":true,"schedule":"* * * * *","registry":{"path":"image-registry.openshift-image-registry.svc:5000"},"oras":{"extraArgs":"--insecure"}}}}}'Create a test namespace:
Create a single DevWorkspace:
Wait for the workspace to reach
Runningstate:Stop the workspace:
oc patch dw test-workspace-a -n dwo-backup-test --type=merge \ -p '{"spec":{"started":false}}'Wait for the backup Job to be created (within 1 minute based on cron schedule):
oc get jobs -n dwo-backup-test -wVerify the backup Job does not have NodeAffinity (since no running pod mounts the PVC):
Expected output:
0The backup Job pod should complete successfully (no contention for the PVC)
PR Checklist
/test v8-devworkspace-operator-e2e, v8-che-happy-pathto trigger)v8-devworkspace-operator-e2e: DevWorkspace e2e testv8-che-happy-path: Happy path for verification integration with CheSummary by CodeRabbit
Bug Fixes
Tests