Skip to content

Add API to enable/disable NICs for KVM#12819

Open
hsato03 wants to merge 1 commit intoapache:mainfrom
scclouds:api-to-enable-disable-nics
Open

Add API to enable/disable NICs for KVM#12819
hsato03 wants to merge 1 commit intoapache:mainfrom
scclouds:api-to-enable-disable-nics

Conversation

@hsato03
Copy link
Member

@hsato03 hsato03 commented Mar 16, 2026 •
edited
Loading

Description

This PR introduces the updateVmNic API, allowing users to enable or disable their VMs network interface cards (NICs).

Currently, this API supports only KVM with the following parameters:

Parameter Description Required
nicid The ID of the NIC that will be updated Yes
state Defines the NICs state. Values can be Enabled or Disabled No

Future updates will extend this API to support additional NIC properties and hypervisors.

Types of changes

  • Breaking change (fix or feature that would cause existing functionality to change)
  • New feature (non-breaking change which adds functionality)
  • Bug fix (non-breaking change which fixes an issue)
  • Enhancement (improves an existing feature and functionality)
  • Cleanup (Code refactoring and cleanup, that may add test cases)
  • Build/CI
  • Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

  • Major
  • Minor

Bug Severity

  • BLOCKER
  • Critical
  • Major
  • Minor
  • Trivial

Screenshots (if appropriate):

How Has This Been Tested?

All tests were performed using only one NIC in the VM. Furthermore, the results of all tests were obtained using the ip a command executed inside the VM.

N Test Result
1 With the VM running, I changed the state of its NIC from Enabled to Disabled The NIC was DOWN
2 With the VM running, I changed the state of its NIC from Disabled to Enabled The NIC was UP
3 With the VM stopped, I started it with its NIC in the Enabled state The NIC was UP
4 With the VM stopped, I started it with its NIC in the Disabled state The NIC was DOWN

How did you try to break this feature and the system with this change?

@hsato03
Copy link
Member Author

hsato03 commented Mar 16, 2026

@blueorangutan package

@blueorangutan
Copy link

blueorangutan commented Mar 16, 2026

@hsato03 a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

@blueorangutan
Copy link

blueorangutan commented Mar 16, 2026

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17163

@codecov
Copy link

codecov bot commented Mar 16, 2026 •
edited
Loading

Codecov Report

❌ Patch coverage is 13.61702% with 203 lines in your changes missing coverage. Please review.
✅ Project coverage is 17.94%. Comparing base (93239e0) to head (9c6d9a0).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
...n/java/com/cloud/vm/VirtualMachineManagerImpl.java 0.00% 52 Missing ⚠️
...cloudstack/api/command/user/vm/UpdateVmNicCmd.java 0.00% 37 Missing ⚠️
...urce/wrapper/LibvirtUpdateVmNicCommandWrapper.java 4.00% 24 Missing ⚠️
...n/java/com/cloud/agent/api/UpdateVmNicCommand.java 0.00% 20 Missing ⚠️
...on/src/main/java/com/cloud/vm/VmWorkUpdateNic.java 0.00% 11 Missing ⚠️
.../src/main/java/com/cloud/vm/UserVmManagerImpl.java 67.74% 7 Missing and 3 partials ⚠️
...pi/src/main/java/com/cloud/agent/api/to/NicTO.java 0.00% 6 Missing ⚠️
api/src/main/java/com/cloud/vm/NicProfile.java 14.28% 6 Missing ⚠️
...rg/apache/cloudstack/api/response/NicResponse.java 0.00% 6 Missing ⚠️
...in/java/com/cloud/agent/api/UpdateVmNicAnswer.java 0.00% 5 Missing ⚠️
... and 10 more
Additional details and impacted files 
@@             Coverage Diff              @@
##               main   #12819      +/-   ##
============================================
- Coverage     17.95%   17.94%   -0.01%     
- Complexity    16259    16264       +5     
============================================
  Files          5954     5959       +5     
  Lines        534838   535094     +256     
  Branches      65423    65440      +17     
============================================
+ Hits          96010    96043      +33     
- Misses       428053   428273     +220     
- Partials      10775    10778       +3
Flag Coverage Δ
uitests 3.64% <ø> (-0.01%) ⬇️
unittests 19.06% <13.61%> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

JoaoJandre
JoaoJandre approved these changes Mar 17, 2026
View reviewed changes
Copy link
Contributor

@JoaoJandre JoaoJandre left a comment

Choose a reason for hiding this comment

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

Minor nit

api/src/main/java/org/apache/cloudstack/api/command/user/vm/UpdateVmNicCmd.java
private Long nicId;

@Parameter(name = ApiConstants.STATE, type = CommandType.STRING, description = "Whether the NIC link state is enabled or disabled")
private String linkState;
Copy link
Contributor

@JoaoJandre JoaoJandre Mar 17, 2026

Choose a reason for hiding this comment

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

Maybe using a boolean instead of a String would be a better choice. There are only two possible options anyway right? Or do you se a new link state being added in the future?

Copy link
Contributor

@GaOrtiga GaOrtiga Mar 17, 2026

Choose a reason for hiding this comment

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

+1 on this. If there are only two options, I believe it should be a boolean and be called Enabled instead of State. Disregard this if there is a possibility of having more states in the future.

@winterhazel winterhazel added this to the 4.23.0 milestone Mar 17, 2026
GaOrtiga
GaOrtiga approved these changes Mar 17, 2026
View reviewed changes
Copy link
Contributor

@GaOrtiga GaOrtiga left a comment •
edited
Loading

Choose a reason for hiding this comment

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

@hsato03
Code lgtm (apart from @JoaoJandre's review)

I also tested locally by enabling/disabling the NIC and using ip a inside the VM, I also tested:

  • Using the API through the UI and CMK.
  • Editing the default NIC when the VM has multiple NICs.
  • Editing the NIC with the VM stopped and then starting it.

Everything worked as expected.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@JoaoJandre JoaoJandre JoaoJandre approved these changes

+1 more reviewer

@GaOrtiga GaOrtiga GaOrtiga approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

component:api component:compute component:kvm component:management-server component:orchestration status:needs-testing status:waiting-for-author

Projects

None yet

Milestone

4.23.0

Development

Successfully merging this pull request may close these issues.

5 participants

@hsato03 @blueorangutan @JoaoJandre @GaOrtiga @winterhazel