Skip to content

e2e: use PATCH instead of UPDATE for node labeling - #3020

Closed
tariq1890 wants to merge 1 commit into
mainfrom
use-patch-not-update
Closed

tariq1890 wants to merge 1 commit into
mainfrom
use-patch-not-update

Conversation

@tariq1890

@tariq1890 tariq1890 commented Oct 6, 2026 •

Copy link
Copy Markdown
Contributor

The gpu-operator controller uses PATCH to perform updates to labels on a node. This change aligns the e2e test behaviour with that of the controller.


Devin Review

@tariq1890
tariq1890 requested a review from a team as a code owner October 6, 2026 17:57
The gpu-operator controller uses PATCH to perform updates to labels on a
node. This change aligns the e2e test behaviour with that of the controller.

Signed-off-by: Tariq Ibrahim <tibrahim@nvidia.com>
@tariq1890
tariq1890 force-pushed the use-patch-not-update branch from 7ca8cef to 014e25b Compare October 6, 2026 17:57
@coderabbitai

coderabbitai Bot commented Oct 6, 2026 •

Copy link
Copy Markdown

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Repository: NVIDIA/gpu-operator/.coderabbit.yaml
  • Review profile: QUIET
  • Plan: Enterprise
  • Run ID: 0b0d8469-d8f4-4acc-92c7-cc21e0b892d6
📥 Commits

Reviewing files that changed from the base of the PR and between 3d141d8 and 014e25b.

📒 Files selected for processing (1)
  • tests/e2e/helpers/node.go

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 11 remain after this review.


📝 Walkthrough

Walkthrough

LabelNode and UnlabelNode now use a shared helper to patch node labels with JSON merge patches. Unlabeling sends a null value to delete the label. The helper returns wrapped errors for patch serialization and API failures.

Priority: ⬇️ Low

Merge Risk: ⚪ Minimal · up to 014e2

The node-label test helpers can proceed through normal checks; no merge-blocking issue was identified.

  • Autopilot · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@kvalliyurnatt kvalliyurnatt left a comment •

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@coveralls

Copy link
Copy Markdown

Coverage Status

coverage: 44.966%. remained the same — use-patch-not-update into main

@devin-ai-integration devin-ai-integration Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Devin Review found 1 potential issue.

Devin Review

Comment thread tests/e2e/helpers/node.go
Comment on lines +50 to +60
func (h *NodeClient) patchNodeLabels(ctx context.Context, nodeName string, nodeLabels map[string]any) error {
patch, err := json.Marshal(map[string]any{
"metadata": map[string]any{
"labels": nodeLabels,
},
})
if err != nil {
return fmt.Errorf("failed to build label patch: %w", err)
}

_, err = h.client.CoreV1().Nodes().Update(ctx, node, metav1.UpdateOptions{})
_, err = h.client.CoreV1().Nodes().Patch(ctx, nodeName, types.MergePatchType, patch, metav1.PatchOptions{})

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔍 Node label patches lack regression coverage

The new patch behavior has no test for adding or removing one label without changing others. AGENTS.md requires coverage for new behavior; add a focused helper test.

Devin Review


Was this helpful? React with 👍 or 👎 to provide feedback.

@tariq1890

Copy link
Copy Markdown
Contributor Author

After discussing this further with @kvalliyurnatt , we've decided that it may be a good idea to revisit our decision to use PATCH instead of UPDATE throughout the controller production code.

I'll close this PR now. Thanks to everyone for their reviews!

@tariq1890 tariq1890 closed this Oct 6, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants