[WIP] Add support to update secondary and primary storage URLs#13193
Draft
Pearl1594 wants to merge 3 commits into
Draft
[WIP] Add support to update secondary and primary storage URLs#13193Pearl1594 wants to merge 3 commits into
Pearl1594 wants to merge 3 commits into
Conversation
boring-cyborg
Bot
added
component:api
component:management-server
component:storage
labels
Contributor
Author
|
@DaanHoogland do you think we need to expose updating this via UI or leave this as a API only field? I feel it could be a bit of a dangerous operation should it be used incorrectly. |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #13193 +/- ##
============================================
+ Coverage 18.08% 18.12% +0.04%
- Complexity 16718 16790 +72
============================================
Files 6037 6037
Lines 542580 543893 +1313
Branches 66427 66757 +330
============================================
+ Hits 98136 98597 +461
- Misses 433417 434227 +810
- Partials 11027 11069 +42
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
I agree, let limit to API |
…he hosts only for nfs and gluster
Comment on lines
+4222
to
+4227
| if (url != null) { | ||
| if (!imageStoreVO.isReadonly()) { | ||
| throw new InvalidParameterValueException("Image store must be set to read-only (maintenance) state before its URL can be changed. " + | ||
| "Please set readOnly=true on the image store first."); | ||
| } | ||
| imageStoreVO.setUrl(url); |
Comment on lines
+4222
to
+4228
| if (url != null) { | ||
| if (!imageStoreVO.isReadonly()) { | ||
| throw new InvalidParameterValueException("Image store must be set to read-only (maintenance) state before its URL can be changed. " + | ||
| "Please set readOnly=true on the image store first."); | ||
| } | ||
| imageStoreVO.setUrl(url); | ||
| } |
Comment on lines
+76
to
+81
| description = "the URL of the storage pool. Supported only for NFS and Gluster pool type." + | ||
| "The pool must be in Maintenance mode before changing the URL. WARNING: use this parameter" + | ||
| "with caution. It is intended for failover scenarios where the storage content is already " + | ||
| "fully mirrored at the new location. Pointing to a new location without ensuring complete " + | ||
| "data parity will result in data loss or corruption. After the URL is updated, the new mount" + | ||
| "is applied to all connected hosts or restart the Management server", |
Comment on lines
1306
to
+1336
| if (cmd.getUrl() != null) { | ||
| details.put("url", cmd.getUrl()); | ||
|
|
||
| // Updating host/path/port and propagating the remount to agents is only | ||
| // supported for NFS and Gluster pools. For other types of storage pools, the URL is just informational and won't be used for actual connection, so we don't need to parse and propagate it. | ||
| StoragePoolType poolType = storagePool.getPoolType(); | ||
| if (poolType == StoragePoolType.NetworkFilesystem || poolType == StoragePoolType.Gluster) { | ||
| if (!storagePool.isInMaintenance()) { | ||
| throw new InvalidParameterValueException("Storage pool must be in Maintenance state before its URL can be changed. " + | ||
| "Please put the pool into maintenance first."); | ||
| } | ||
| URI newUri; | ||
| try { | ||
| newUri = new URI(cmd.getUrl()); | ||
| } catch (URISyntaxException e) { | ||
| throw new InvalidParameterValueException("Invalid URL format: " + cmd.getUrl()); | ||
| } | ||
| storagePool.setHostAddress(newUri.getHost()); | ||
| storagePool.setPath(newUri.getPath()); | ||
| if (newUri.getPort() != -1) { | ||
| storagePool.setPort(newUri.getPort()); | ||
| } | ||
| } | ||
| } | ||
| _storagePoolDao.update(id, storagePool); | ||
| _storagePoolDao.updateDetails(id, details); | ||
| ((PrimaryDataStoreLifeCycle) dataStoreLifeCycle).updateStoragePool(storagePool, details); | ||
| StoragePoolType poolType = storagePool.getPoolType(); | ||
| if (cmd.getUrl() != null && | ||
| (poolType == StoragePoolType.NetworkFilesystem || poolType == StoragePoolType.Gluster)) { | ||
| propagateStoragePoolUrlChangeToHosts(storagePool); |
Comment on lines
+4222
to
+4226
| if (url != null) { | ||
| if (!imageStoreVO.isReadonly()) { | ||
| throw new InvalidParameterValueException("Image store must be set to read-only (maintenance) state before its URL can be changed. " + | ||
| "Please set readOnly=true on the image store first."); | ||
| } |
Contributor
Author
|
@blueorangutan package |
|
@Pearl1594 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. |
|
|
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17965 |
Contributor
Author
|
@sureshanaparti I'd like some advice, as part of addressing this improvement request, I attempted to allow changing primary store url and let it propagate to the hosts, and remount the stores on the host, but it's only taken care of for kvm. Now Im wondering :
For secondary store it's alright as we can restart the SSVM. |
Pearl1594
changed the title
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: #11742
Adds support to update the URL of primary and secondary stores. However, the support is limited to API and not extended to the UI as this is an operation that if performed without caution can lead to issues - data loss.
Secondary store:
The store needs to be in read-only mode before attempting to change the url.
Secondary storage VM needs to be recreated to remount the new store
Primary store:
The primary store needs to be in Maintenance mode to proceed with updating the URL
The update of URL was previously purely cosmetic / informational ie updated in the database - details table. This continues to be true for storage pools other than nfs and gluster
For NFS and Gluster, if the URL is changed, it propagates it to the connected hosts and attempts to remove the old store and add the new one
In case , no hosts are found, it only changes the URL in the DB and this would require a MS restart to propagate to the host.
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?