fix#4497: hypernode update bug #4498

cyf-2002 · 2025-07-29T12:15:08Z

What type of PR is this?

fix

What this PR does / why we need it:

Hypernode controller will ignore updating spec, as it only calls UpdateStatus() to update hypernode. When I test the function of autoupdate, hypernode spec will never be updated since it's created. So we should add Update() call.

Which issue(s) this PR fixes:

#4497

Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?

volcano-sh-bot · 2025-07-29T12:15:12Z

Welcome @cyf-2002!

It looks like this is your first PR to volcano-sh/volcano.

Thank you, and welcome to Volcano. 😃

gemini-code-assist

Summary of Changes

Hello @cyf-2002, I'm Gemini Code Assist¹! 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 provides a critical fix for the HyperNode controller's update mechanism. It corrects an issue where only the status subresource of a HyperNode was being updated, preventing full resource modifications from taking effect. By switching to the appropriate client method, the PR ensures that all intended changes to HyperNode objects are properly persisted.

Highlights

Bug Fix: HyperNode Update Logic: This PR addresses a bug where updates to HyperNode resources were not being fully applied. Previously, the UpdateStatus method was incorrectly used, which only modifies the status subresource of the HyperNode object. This meant that changes to other fields, such as annotations (as seen in the preceding code), were not being persisted.
API Client Method Correction: The core change involves replacing the call to vcClient.TopologyV1alpha1().HyperNodes().UpdateStatus() with vcClient.TopologyV1alpha1().HyperNodes().Update() in pkg/controllers/hypernode/utils/utils.go. This ensures that the entire HyperNode resource, including its metadata and spec, is correctly updated by the Kubernetes API server.

Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature	Command	Description
Code Review	`/gemini review`	Performs a code review for the current pull request in its current state.
Pull Request Summary	`/gemini summary`	Provides a summary of the current pull request in its current state.
Comment	@gemini-code-assist	Responds in comments when explicitly tagged, both in issue comments and review comments.
Help	`/gemini help`	Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

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 or fill out our survey to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.

Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩

Copilot

Pull Request Overview

This PR fixes a bug in the hypernode update mechanism by changing from status-only updates to full object updates. The change addresses issue #4497 where the hypernode controller was incorrectly using UpdateStatus() instead of Update().

Changes the update method from UpdateStatus() to Update() in the hypernode controller
Ensures that annotation changes are properly persisted to the hypernode object

Copilot · 2025-07-29T12:15:30Z

pkg/controllers/hypernode/utils/utils.go

 		}

-		_, err = vcClient.TopologyV1alpha1().HyperNodes().UpdateStatus(context.Background(), current, metav1.UpdateOptions{})
+		_, err = vcClient.TopologyV1alpha1().HyperNodes().Update(context.Background(), current, metav1.UpdateOptions{})


The change from UpdateStatus() to Update() is correct for persisting annotation changes, but consider whether the full object update is necessary. If only annotations are being modified, you might want to use a strategic merge patch to avoid potential conflicts with concurrent updates to other fields.

gemini-code-assist

Code Review

This pull request fixes a bug where HyperNode spec and metadata updates were not being persisted. The change from UpdateStatus to Update is correct for this purpose. However, the review identifies a remaining high-severity issue in the UpdateHyperNode function: it attempts to update the resource's status, but this will be silently ignored by the Update call due to the status subresource being enabled on the CRD. This creates a latent bug. I've provided a detailed explanation and recommendation in the line comment.

gemini-code-assist · 2025-07-29T12:17:15Z

pkg/controllers/hypernode/utils/utils.go

+		_, err = vcClient.TopologyV1alpha1().HyperNodes().Update(context.Background(), current, metav1.UpdateOptions{})
 		return err


While this change to use Update is correct for persisting changes to the spec and metadata, a significant issue remains. The HyperNode CRD has the status subresource enabled. When a CRD has a status subresource, the .status field can only be modified via a dedicated UpdateStatus call. The standard Update call will silently ignore any changes to the .status field. The UpdateHyperNode function modifies the .status field of the current object before calling Update. This is misleading as the status change will not be persisted, creating a latent bug for any caller that expects the status to be updated.

Recommendations:

The function should either be refactored to perform a separate UpdateStatus call, or it should not be responsible for status updates at all, leaving that to dedicated functions like syncHyperNodeStatus.

As a general best practice, objects retrieved from a lister cache should be deep-copied before modification to avoid mutating a shared object in the cache.

JesseStutler · 2025-07-30T02:26:10Z

/priority high
Thanks, but please complete your description of the PR, right now it's too simple. @cyf-2002

Monokaix · 2025-07-30T02:33:07Z

pkg/controllers/hypernode/utils/utils.go

 		}

-		_, err = vcClient.TopologyV1alpha1().HyperNodes().UpdateStatus(context.Background(), current, metav1.UpdateOptions{})
+		_, err = vcClient.TopologyV1alpha1().HyperNodes().Update(context.Background(), current, metav1.UpdateOptions{})


We need update both spec and status as the HyperNode status may also include Network condition in status and this also need to be updated.

Can update func update status？

No, I mean we should update both spec and status, any other way is ok.

Thanks，I got it. So if I want to fix this bug, I can call both update and updatestatus func.

cyf-2002 · 2025-07-31T08:37:41Z

/priority high Thanks, but please complete your description of the PR, right now it's too simple. @cyf-2002

I added some details, and I described the question on #4497.

JesseStutler · 2025-07-31T11:15:55Z

pkg/controllers/hypernode/utils/utils.go

 			current.Annotations[k] = v
 		}

+		_, err = vcClient.TopologyV1alpha1().HyperNodes().Update(context.Background(), current, metav1.UpdateOptions{})


We still need to judge err here to avoid override

if err != nil{ return err }

Monokaix · 2025-08-01T10:18:22Z

Is there a way to just call API once and update both spec and status?

Monokaix · 2025-08-04T03:46:15Z

/ok-to-test

Monokaix · 2025-08-04T03:46:35Z

Please also merge to one commit.

Signed-off-by: cyf-2002 <865636335@qq.com>

cyf-2002 · 2025-08-04T07:12:14Z

Please also merge to one commit.

Done. I can't find a way to just call API once temporarily.

Monokaix · 2025-08-04T08:13:17Z

/approve

volcano-sh-bot · 2025-08-04T08:13:26Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Monokaix

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

~~pkg/controllers/OWNERS~~ [Monokaix]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

JesseStutler · 2025-08-05T06:17:20Z

/lgtm

…node fix#4497: hypernode update bug Signed-off-by: Monokaix <changxuzheng@huawei.com>

Copilot AI review requested due to automatic review settings July 29, 2025 12:15

volcano-sh-bot requested review from hwdef and wangyang0616 July 29, 2025 12:15

volcano-sh-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 29, 2025

gemini-code-assist bot reviewed Jul 29, 2025

View reviewed changes

Copilot AI reviewed Jul 29, 2025

View reviewed changes

gemini-code-assist bot reviewed Jul 29, 2025

View reviewed changes

cyf-2002 force-pushed the fix/fix-update-hypernode branch from 9d76b95 to 5389552 Compare July 29, 2025 12:30

cyf-2002 changed the title ~~fix hypernode update bug~~ fix#4497: hypernode update bug Jul 30, 2025

volcano-sh-bot added the priority/high label Jul 30, 2025

Monokaix reviewed Jul 30, 2025

View reviewed changes

cyf-2002 force-pushed the fix/fix-update-hypernode branch from ab3b22e to ccad709 Compare July 30, 2025 09:29

cyf-2002 closed this Jul 31, 2025

cyf-2002 reopened this Jul 31, 2025

JesseStutler reviewed Jul 31, 2025

View reviewed changes

volcano-sh-bot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Aug 4, 2025

cyf-2002 force-pushed the fix/fix-update-hypernode branch from 02e692f to 5c5352f Compare August 4, 2025 07:03

fix: update both spec and status of hypernode

823460f

Signed-off-by: cyf-2002 <865636335@qq.com>

cyf-2002 force-pushed the fix/fix-update-hypernode branch from 5c5352f to 823460f Compare August 4, 2025 07:06

volcano-sh-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 4, 2025

cyf-2002 requested a review from JesseStutler August 5, 2025 01:48

volcano-sh-bot assigned JesseStutler Aug 5, 2025

volcano-sh-bot added the lgtm Indicates that a PR is ready to be merged. label Aug 5, 2025

volcano-sh-bot merged commit 5682afb into volcano-sh:master Aug 5, 2025
19 checks passed

Monokaix pushed a commit to Monokaix/volcano that referenced this pull request Aug 7, 2025

Merge pull request volcano-sh#4498 from cyf-2002/fix/fix-update-hyper…

c9e359d

…node fix#4497: hypernode update bug Signed-off-by: Monokaix <changxuzheng@huawei.com>

		_, err = vcClient.TopologyV1alpha1().HyperNodes().Update(context.Background(), current, metav1.UpdateOptions{})
		return err

fix#4497: hypernode update bug #4498

fix#4497: hypernode update bug #4498

Uh oh!

Conversation

cyf-2002 commented Jul 29, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What type of PR is this?

What this PR does / why we need it:

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Uh oh!

volcano-sh-bot commented Jul 29, 2025

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Summary of Changes

Highlights

Footnotes

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

gemini-code-assist bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

gemini-code-assist bot Jul 29, 2025

Choose a reason for hiding this comment

Uh oh!

JesseStutler commented Jul 30, 2025

Uh oh!

Monokaix Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

cyf-2002 Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

Monokaix Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

cyf-2002 Jul 30, 2025

Choose a reason for hiding this comment

Uh oh!

cyf-2002 commented Jul 31, 2025

Uh oh!

JesseStutler Jul 31, 2025

Choose a reason for hiding this comment

Uh oh!

cyf-2002 Aug 1, 2025

Choose a reason for hiding this comment

Uh oh!

Monokaix commented Aug 1, 2025

Uh oh!

Monokaix commented Aug 4, 2025

Uh oh!

Monokaix commented Aug 4, 2025

Uh oh!

cyf-2002 commented Aug 4, 2025

Uh oh!

Monokaix commented Aug 4, 2025

Uh oh!

volcano-sh-bot commented Aug 4, 2025

Uh oh!

JesseStutler commented Aug 5, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cyf-2002 commented Jul 29, 2025 •

edited

Loading