Repository navigation
ssvm: prevent sql constraint violation on SVM removal during net outage - #14438
Open
DaanHoogland wants to merge 1 commit into
Open
DaanHoogland wants to merge 1 commit into
DaanHoogland wants to merge 1 commit into
Conversation
DaanHoogland
requested review from
vishesh92
and
a balanced review from Copilot
October 19, 2026 13:41
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 4.20 #14438 +/- ##
=========================================
Coverage 16.41% 16.41%
- Complexity 13657 13662 +5
=========================================
Files 5669 5669
Lines 501628 501647 +19
Branches 60942 60944 +2
=========================================
+ Hits 82336 82365 +29
+ Misses 410064 410053 -11
- Partials 9228 9229 +1
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:
|
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
4 open issues
removedHostin-memory but does not persist the change, so the host may remain… · New
What changed in this PR
Prevents SQL unique-constraint violations when a previously removed System VM/host reconnects by reviving the removed host row (instead of attempting to create a duplicate) and adds coverage for the new behavior.
Changes:
- Revive an existing removed
HostVOwith the same GUID during host discovery. - Add DAO support to find a host by GUID including removed records.
- Add unit tests covering the “revive removed host on GUID collision” path.
| File | Description |
|---|---|
| server/src/main/java/com/cloud/resource/ResourceManagerImpl.java | Revives removed host records by GUID during getNewHost to avoid duplicate insert/constraint violations. |
| engine/schema/src/main/java/com/cloud/host/dao/HostDao.java | Adds a new DAO API to find a host by GUID including removed rows. |
| engine/schema/src/main/java/com/cloud/host/dao/HostDaoImpl.java | Implements findByGuidIncludingRemoved. |
| engine/schema/src/main/java/com/cloud/host/HostVO.java | Adds a setter for the removed field to support “revival.” |
| server/src/test/java/com/cloud/resource/ResourceManagerImplTest.java | Adds unit tests (via reflection) for the revival/null-return behavior. |
Review effort: Lite
| } | ||
| logger.warn(String.format("Host with guid %s reconnected but was previously removed (id: %s); reviving the removed host record instead of creating a duplicate", | ||
| guid, removedHost.getId())); | ||
| removedHost.setRemoved(null); |
Comment on lines
+2293
to
+2298
| /** | ||
| * A host can be marked removed (e.g. a system VM force-stopped and destroyed from the UI) while the | ||
| * underlying resource is still alive and later reconnects with the same guid. The `guid` column is | ||
| * uniquely constrained regardless of removal, so blindly inserting a new row for it fails with a | ||
| * SQLIntegrityConstraintViolationException. Revive the removed row instead of creating a duplicate. | ||
| */ |
Comment on lines
+2300
to
+2307
| HostVO removedHost = _hostDao.findByGuidIncludingRemoved(guid); | ||
| if (removedHost == null || removedHost.getRemoved() == null) { | ||
| return null; | ||
| } | ||
| logger.warn(String.format("Host with guid %s reconnected but was previously removed (id: %s); reviving the removed host record instead of creating a duplicate", | ||
| guid, removedHost.getId())); | ||
| removedHost.setRemoved(null); | ||
| return removedHost; |
Comment on lines
+688
to
+732
|
|
||
| private HostVO invokeGetNewHost(StartupCommand[] cmds) throws Exception { | ||
| Method method = ResourceManagerImpl.class.getDeclaredMethod("getNewHost", StartupCommand[].class); | ||
| method.setAccessible(true); | ||
| return (HostVO) method.invoke(resourceManager, (Object) cmds); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetNewHostRevivesRemovedHostOnGuidCollision() throws Exception { | ||
| String guid = "s-14-VM-PremiumSecondaryStorageResource"; | ||
| String guidPrefix = "s-14-VM"; | ||
| StartupCommand cmd = Mockito.mock(StartupCommand.class); | ||
| when(cmd.getGuid()).thenReturn(guid); | ||
| when(cmd.getGuidWithoutResource()).thenReturn(guidPrefix); | ||
|
|
||
| Mockito.doReturn(null).when(resourceManager).findHostByGuid(guid); | ||
| Mockito.doReturn(null).when(resourceManager).findHostByGuidPrefix(guidPrefix); | ||
|
|
||
| HostVO removedHost = Mockito.mock(HostVO.class); | ||
| when(removedHost.getId()).thenReturn(99L); | ||
| when(removedHost.getRemoved()).thenReturn(new Date()); | ||
| when(hostDao.findByGuidIncludingRemoved(guid)).thenReturn(removedHost); | ||
|
|
||
| HostVO result = invokeGetNewHost(new StartupCommand[]{cmd}); | ||
|
|
||
| Assert.assertSame(removedHost, result); | ||
| Mockito.verify(removedHost).setRemoved(null); | ||
| } | ||
|
|
||
| @Test | ||
| public void testGetNewHostReturnsNullWhenNoHostAndNoRemovedMatch() throws Exception { | ||
| String guid = "unknown-guid"; | ||
| String guidPrefix = "unknown"; | ||
| StartupCommand cmd = Mockito.mock(StartupCommand.class); | ||
| when(cmd.getGuid()).thenReturn(guid); | ||
| when(cmd.getGuidWithoutResource()).thenReturn(guidPrefix); | ||
|
|
||
| Mockito.doReturn(null).when(resourceManager).findHostByGuid(guid); | ||
| Mockito.doReturn(null).when(resourceManager).findHostByGuidPrefix(guidPrefix); | ||
| when(hostDao.findByGuidIncludingRemoved(guid)).thenReturn(null); | ||
|
|
||
| HostVO result = invokeGetNewHost(new StartupCommand[]{cmd}); | ||
|
|
||
| Assert.assertNull(result); | ||
| } |
|
Copilot stopped reviewing on behalf of
DaanHoogland due to an error
October 19, 2026 15:15
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
This PR...
Fixes: #14391
Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
How did you try to break this feature and the system with this change?