Repository navigation
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
|
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
thanks @EduFrazao , clgtm.
|
also @EduFrazao , I marked it for 24 as you based the PR off main. please rebase if you need it on 22.2 or somewhere else. |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## 4.22 #13603 +/- ##
============================================
+ Coverage 17.69% 18.30% +0.60%
- Complexity 15835 16630 +795
============================================
Files 5925 5939 +14
Lines 533539 537820 +4281
Branches 65274 65939 +665
============================================
+ Hits 94427 98462 +4035
+ Misses 428435 428152 -283
- Partials 10677 11206 +529
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Thank you very mutch. |
- By explicit definition of the management ip address on agent.properties - By using OS routing table to determine the source address used to connect to any of avaliable management servers. - Fallback to current methods.
0a2c926 to
9e1f3be
Compare
boring-cyborg
Bot
added
component:api
component:backup
component:build
component:compute
component:console-proxy
component:database
component:dhcp
component:hyperv
component:integration-test
component:integration
component:ipv6
component:iscsi
component:kubernetes
component:LDAP
component:management-server
component:marvin
component:mysql
component:networking
labels
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 18691 |
|
[SF] Trillian test result (tid-16648)
|
There was a problem hiding this comment.
Pull request overview
This PR aims to make KVM agent “private/management” IP selection deterministic when the configured management NIC has no IP (e.g., IP is on a bridge member/veth), preventing the agent from reporting an incorrect private IP to management servers.
Changes:
- Pass additional agent.properties values into KVM resource configuration (management server host list and an optional explicit private IP).
- Add two new private-NIC discovery strategies in
ServerResourceBase: explicit private IP mapping to a NIC, and route-based source-IP discovery via UDP socket. - Introduce a new agent property
private.network.addressto optionally pin the management/private IP.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java | Adds agent.properties-derived config values into the resource params map used by base configuration. |
| core/src/main/java/com/cloud/resource/ServerResourceBase.java | Adds explicit-IP and route-lookup based private NIC discovery before the existing iteration fallback. |
| agent/src/main/java/com/cloud/agent/properties/AgentProperties.java | Adds a new agent property definition for explicitly setting the private NIC IP address. |
Suppressed comments (2)
core/src/main/java/com/cloud/resource/ServerResourceBase.java:182
- Avoid catching Throwable here; it can hide serious JVM errors and drops the stack trace. Catch Exception and pass the throwable to the logger so failures in route lookup can be diagnosed.
} catch (Throwable e) {
// Logging only, if this method was unable to find a valid interface, iteration will be tested
logger.debug(String.format("Unable to use routing table to determine private management interface: [%s]", e.getMessage()));
}
core/src/main/java/com/cloud/resource/ServerResourceBase.java:87
- New private-NIC resolution behaviors (private.network.address override and route-lookup discovery) are introduced here but there are no accompanying unit tests (ServerResourceBaseTest currently covers only defineResourceNetworkInterfaces/isValidNic/iteration-based discovery). Adding tests for: (1) selecting NIC by configured IP; (2) falling back to route lookup when the configured NIC has no usable address; and (3) falling back to simple iteration when route lookup fails, would help prevent regressions.
if (privateNic == null) {
checkForPrivateInterfaceDefinedByIp(params);
}
if (privateNic == null) {
tryToAutoDiscoverResourcePrivateNetworkInterfaceByRouteLookup(params);
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@DaanHoogland there is something that I need to change in this PR? I mean, because of the IA warnings, etc? |
Not necessarily, but please assess them and then either address or close with a comment. |
DaanHoogland
left a comment
DaanHoogland
left a comment
There was a problem hiding this comment.
clgtm, one remark but that is not functional but maintainability/readability. And not necessarily in scope for this PR
Co-authored-by: dahn <daan.hoogland@gmail.com>
|
Hi @DaanHoogland . Thanks!! |
It needs a second review with (3rd party) testing, @EduFrazao. |
| defineResourceNetworkInterfaces(params); | ||
|
|
||
| if (privateNic == null) { | ||
| checkForPrivateInterfaceDefinedByIp(params); |
There was a problem hiding this comment.
does this run when the bridge in private.network.device exists but has no ip? looks like it gets skipped, so the case in the description still fails
There was a problem hiding this comment.
Hi @Damans227
In my tests, private.network.device is never collected by params.get("private.network.device") even if its present on agent.properties.
On the hypervisor side, this setting works, because system vms are correctly placed on this device (that is a bridge linked to a VNI).
But as it is never resolved as private nic on the ServerResourceBase in this step, the original behavior is to jump into tryToAutoDiscoverResourcePrivateNetworkInterface() that picks the first local ip address regardless of the owner interface.
There was a problem hiding this comment.
@EduFrazao on kvm the bridge name comes straight from agent.properties, so a bridge that exists gets picked even with no ip. then the new lookups never run and the agent fails to start. can we also run them when the picked bridge has no usable ip?
There was a problem hiding this comment.
@Damans227 I've done another commit, checking for IP Address if private.network.device is found.
Is something like that you have in mind?
|
@EduFrazao , can you look at @Damans227 's questions/remarks? |
Hi @DaanHoogland ! |
|
@blueorangutan package |
|
@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with KVM, XenServer and VMware SystemVM templates. I'll keep you posted as I make progress. |
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 19778 |
|
@blueorangutan test |
|
@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests |
|
[SF] Trillian test result (tid-17298)
|
Description
This PR solves a situation when the management network interface does not have a defined IP address.
When this interface is a bridge, the management ip can be placed directly in the bridge (more common) and sometimes in virtual interfaces linked to it.
On this scenarios, the cloudstack agent iterates over all interfaces and select the first one with a valid ip address.
This brings unpredictable behavior, like choosing wrong interface address, and publishing this address to management servers, causing problems with migrations, guest consoles for example.
I've added two new ways to search for the correct management address:
If the above methods don't work, the actual behavior is used without changes.
Fixes: #13519
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
I have two hipervisors in production with this changes. Whitout it, I can't make my cluster to work well, cause I can't do migrations and open guest instances terminal
To test it I used both the explicit configuration and route based detection for several days, marking servers for maintenance, rebooting hypervisors and management servers.
How did you try to break this feature and the system with this change?
Configuring an invalid Ip address as management address. The setting was ignored as expected, because no interface was found with it. System fallback to another methods.