Repository navigation
Keep management server IDs same across restarts - #14551
MashrafeeAryan wants to merge 6 commits into
Conversation
Save the initial ID and reuse it when network interfaces change. Add JVM overrides and regression tests for issue apache#14253.
Explain the ID file, JVM settings, and upgrade steps for issue apache#14253.
Flush the initial identity to a temporary sibling file and atomically publish it while holding the existing process lock. Clean up failed writes and verify that initialization can be retried without an invalid identity file. Related to apache#14253.
Route the remaining management identity consumers through ManagementServerNode instead of reading the current MAC address. Cover the management service ID and disk statistics ownership when the saved identity differs from the MAC. Related to apache#14253.
Build the avoid list from one snapshot of nonremoved management server records. An Up record takes precedence over stale non-Up identities sharing its IP; unavailable addresses remain avoided without duplicates. Retain database records and document the recovery behavior. Related to apache#14253.
Explain the persistent ID flow, identity consumers, and regression tests with short comments without ending periods. Keep installation instructions concise and document preserving the ID file, configuration overrides, and recovery for existing installations. Update release notes for stable IDs and active IP handling. Restore CloudZonesStartupProcessor to its pre-fix version because its local node ID is unused. Keep its existing configure() behavior and exclude the file from the overall PR diff.
|
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)
|
baf944d to
c22a289
Compare
MashrafeeAryan
changed the title
|
Hi @DaanHoogland, would you please be able to review this PR when you have a chance? I understand you have other commitments, and I would appreciate any feedback or suggestions whenever you have time. Thank you so much! |
| } | ||
|
|
||
| // First startup: obtain the initial MAC-derived ID and validate it | ||
| long id = parse(Long.toString(initialId.getAsLong())); |
There was a problem hiding this comment.
@MashrafeeAryan on upgrade, first start has no id file so it still takes id from mac. if the upgrade changes java and nic order flips, wont we save the wrong id forever? maybe reuse the old id from mshost table if one exists for this host?
|
|
||
| private static final long s_nodeId = MacAddress.getMacAddress().toLong(); | ||
| // Load the ID once and keep the same value for the life of this process | ||
| private static final long s_nodeId = ManagementServerId.get(); |
There was a problem hiding this comment.
@MashrafeeAryan id is now loaded when the class loads. if reading the file fails, wont it break every class using ManagementServerNode instead of giving a clean error? can we load it in start() instead?
| StandardOpenOption.CREATE, StandardOpenOption.WRITE); | ||
| FileLock lock = lockChannel.lock()) { | ||
| // Reuse the saved ID without checking network interfaces again | ||
| if (Files.exists(path)) { |
There was a problem hiding this comment.
@MashrafeeAryan if someone clones the ms vm, the clone gets the same id file and same id as the original. today the new mac would give it a new one. should we catch this somehow?
| // This method handles duplicate records that already exist | ||
| // Old Down records must not make agents skip an address that also has an Up record | ||
| @Override | ||
| public List<String> listNonUpStateMsIPs() { |
There was a problem hiding this comment.
@MashrafeeAryan instead of skipping old down rows in the avoid list, can we just mark old rows with the same ip as removed when the ms registers? fixes the leftover rows too, like in #14448
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #14551 +/- ##
============================================
+ Coverage 18.05% 20.77% +2.72%
- Complexity 16320 21924 +5604
============================================
Files 5937 6497 +560
Lines 536919 591647 +54728
Branches 65810 72590 +6780
============================================
+ Hits 96932 122931 +25999
- Misses 428915 455380 +26465
- Partials 11072 13336 +2264
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:
|
Description
Fixes: #14253
CloudStack could give a management server a new ID after a restart if it chose a different network card. This could prevent agents from reconnecting.
This PR saves the server ID in a file and reuses it after restarts. It also makes the other affected parts of CloudStack use the same ID.
I also updated the avoid list so an old Down record does not block an address that has an Up record. This helps installations already affected by the bug. (I am not sure if this is necessary. I am open to removing this part from the PR)
The main changes I did:
Types of changes
Bug Severity
How Has This Been Tested?
Built the management server and ran 17 automated tests on Windows using OpenJDK 11 and Maven 3.9.9.
All 17 tests passed:
ManagementServerIdTest: 11 testsManagementServerHostDaoImplTest: 4 testsManagementServerIdentityTest: 2 testsWhitespace checks also passed.
How did you try to break this feature and the system with this change?
Reproduction
I have not reproduced this on a live CloudStack installation.