You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by "rg9975 (via GitHub)" <gi...@apache.org> on 2024/12/21 00:17:13 UTC
[PR] Primera pure patches & various small fixes [cloudstack]
rg9975 opened a new pull request, #10132:
URL: https://github.com/apache/cloudstack/pull/10132
### Description
This PR...
This PR fixes several small issues with Fiberchannel implementation and locally discovered issues:
1. volume disconnect timing issues
2. rescan cleanup script not working correctly
3. template copy issues
4. masked/hidden errors and exceptions
<!--- ******************************************************************************* -->
<!--- NOTE: AUTOMATION USES THE DESCRIPTIONS TO SET LABELS AND PRODUCE DOCUMENTATION. -->
<!--- PLEASE PUT AN 'X' in only **ONE** box -->
<!--- ******************************************************************************* -->
### Types of changes
- [ ] Breaking change (fix or feature that would cause existing functionality to change)
- [ ] New feature (non-breaking change which adds functionality)
- [X] 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
- [X] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [X] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
Tested locally with real world workloads
#### How did you try to break this feature and the system with this change?
Tested locally with real world workloads
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
3.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "rg9975 (via GitHub)" <gi...@apache.org>.
rg9975 commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2631125728
I've reverted commit #f066100
> > > AFAIS the script already gets the sizes and writes them correctly into the `template.properties` file, so all it would need to do is also update the database entries too?
> >
> >
> > sounds good, does that mean we can merge this, @rp- ?
>
> well, we can revert/drop then that last commit: [f066100]
Reverted.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2631326931
> Just to have it noted here, I'll probably create an issue out of it. It isn't the `cloud-install-sys-tmplt` script (maybe its usage), but the actual problem is in:
>
> https://github.com/apache/cloudstack/blob/cd81398cb7176698196f72002df5d6088befbaed/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java#L1242
>
> For pre-seeded templates, the `template_store_ref` entries get pre filled with 0 and never updated, which is bad I guess for most primary storages. Using preseeded templates and this commit will break using those templates, until the DB entries are fixed.
ok, that to me sounds like we shouldn't merge this untill we fix that as well.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2628992745
@rp- can you review the last changes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "rp- (via GitHub)" <gi...@apache.org>.
rp- commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2630759239
> @rp- can you review the last changes.
@DaanHoogland @rg9975
So I dug a bit deeper and found the actual problem to be the `cloud-install-sys-tmplt` script.
I use it in my automated CloudStack installations and this scripts sets `size` and `physical_size` `0` in the `template_store_ref` table, which than Linstor can't use to create a template.
If I don't use the script at all, and let CloudStack fetch the template on its own, the entries are correct and the Linstor driver already creates the volume on management server and everything is fine.
So my question is, is this script still recommended for installation? And if so can we fix it?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2596021277
<b>[SF] Trillian Build Failed (tid-12125)<b/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "rg9975 (via GitHub)" <gi...@apache.org>.
rg9975 commented on code in PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#discussion_r1930693707
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java:
##########
@@ -1035,7 +1035,9 @@ private void copyTemplateToManagedTemplateVolume(TemplateInfo srcTemplateInfo, T
try {
grantAccess(templateOnPrimary, destHost, destPrimaryDataStore);
} catch (Exception e) {
- throw new StorageAccessException("Unable to grant access to template: " + templateOnPrimary.getId() + " on host: " + destHost.getId());
+ StorageAccessException e2 = new StorageAccessException("Unable to grant access to template: " + templateOnPrimary.getId() + " on host: " + destHost.getId());
+ e2.initCause(e);
+ throw e;
Review Comment:
updated this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2634126067
> I have worked out a possible fix #10317
will regression test #10317 and merge it together with this.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "rp- (via GitHub)" <gi...@apache.org>.
rp- commented on code in PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#discussion_r1931917770
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -274,6 +274,7 @@ public Answer copyTemplateToPrimaryStorage(final CopyCommand cmd) {
if (!storagePoolMgr.connectPhysicalDisk(primaryStore.getPoolType(), primaryStore.getUuid(), path, details)) {
s_logger.warn("Failed to connect physical disk at path: " + path + ", in storage pool id: " + primaryStore.getUuid());
+ return new PrimaryStorageDownloadAnswer("Failed to spool template disk at path: " + path + ", in storage pool id: " + primaryStore.getUuid());
Review Comment:
Spun up a fresh Linstor cluster, and this here makes problems (I think again, we had this change already discussed).
I don't know if this is only for the special first systemvm template case, but what happens is that I get this exception:
```
com.cloud.exception.StorageUnavailableException: Resource [StoragePool:1] is unreachable: Unable to create volume [{"name":"ROOT-2","uuid":"1185cf31-cf58-44c9-b998-80ca3485ef6e"}] due to [Failed to spool template disk at path: null, in storage pool id: 7a4756ae-c7b6-45fe-b8a2-04d34db421cf].
at org.apache.cloudstack.engine.orchestration.VolumeOrchestrator.recreateVolume(VolumeOrchestrator.java:1817)
at org.apache.cloudstack.engine.orchestration.VolumeOrchestrator.prepare(VolumeOrchestrator.java:1930)
...
```
I don't know why CS is trying to connect a disk with path `null`, but it is and the Linstor driver ignores/return false in this case.
And usually in the second connect I get the correct path and everything works out.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#discussion_r1931924242
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -274,6 +274,7 @@ public Answer copyTemplateToPrimaryStorage(final CopyCommand cmd) {
if (!storagePoolMgr.connectPhysicalDisk(primaryStore.getPoolType(), primaryStore.getUuid(), path, details)) {
s_logger.warn("Failed to connect physical disk at path: " + path + ", in storage pool id: " + primaryStore.getUuid());
+ return new PrimaryStorageDownloadAnswer("Failed to spool template disk at path: " + path + ", in storage pool id: " + primaryStore.getUuid());
Review Comment:
are sugesting a change gere @rp- ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "rg9975 (via GitHub)" <gi...@apache.org>.
rg9975 commented on code in PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#discussion_r1932302874
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -923,6 +924,7 @@ private HostVO getHostOnWhichToExecuteMigrationCommand(VolumeInfo srcVolumeInfo,
HostVO hostVO;
// if either source or destination is a HOST-scoped storage pool, the migration MUST be performed on that host
+
Review Comment:
in upcoming commit
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -1568,6 +1580,8 @@ else if (volumeInfo.getFormat() == ImageFormat.OVA) {
}
}
+
+
Review Comment:
in upcoming commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "rg9975 (via GitHub)" <gi...@apache.org>.
rg9975 commented on code in PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#discussion_r1937900293
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -274,6 +274,7 @@ public Answer copyTemplateToPrimaryStorage(final CopyCommand cmd) {
if (!storagePoolMgr.connectPhysicalDisk(primaryStore.getPoolType(), primaryStore.getUuid(), path, details)) {
s_logger.warn("Failed to connect physical disk at path: " + path + ", in storage pool id: " + primaryStore.getUuid());
+ return new PrimaryStorageDownloadAnswer("Failed to spool template disk at path: " + path + ", in storage pool id: " + primaryStore.getUuid());
Review Comment:
This must be because of a semantic difference in the storage drivers. In our drivers, the volume is created from the management server on the storage array before asking the template to be copied to it. This is why the connect is necessary - the volume has to be made visible on the host where the template copy is being performed. In Linstor, I presume, the volume is 'born' when the copyPhysicalDisk is first called correct?
For now, I coded a "compromise" to only fail WHEN its type FiberChannel; otherwise, it will maintain the previous behavior to log a warning and continue.
More ideal would be some sort of attribute on the storage pool or adapter class to indicate to the storage processor whether that particular storage pool type requires a connect before disk copy (in all or certain situations like template copy), but that would be a more extensive change, and I believe not necessary to complete the other changes that are part of this PR.
Please confirm if this is workable and you are able to retest.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2631256542
@rp- , can you approve??
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "rp- (via GitHub)" <gi...@apache.org>.
rp- commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2631534111
> > Just to have it noted here, I'll probably create an issue out of it. It isn't the `cloud-install-sys-tmplt` script (maybe its usage), but the actual problem is in:
> > https://github.com/apache/cloudstack/blob/cd81398cb7176698196f72002df5d6088befbaed/engine/storage/image/src/main/java/org/apache/cloudstack/storage/image/TemplateServiceImpl.java#L1242
> >
> > For pre-seeded templates, the `template_store_ref` entries get pre filled with 0 and never updated, which is bad I guess for most primary storages. Using preseeded templates and this commit will break using those templates, until the DB entries are fixed.
>
> ok, that to me sounds like we shouldn't merge this untill we fix that as well.
I have worked out a possible fix #10317
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "rg9975 (via GitHub)" <gi...@apache.org>.
rg9975 commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2624918265
@DaanHoogland I'd like to review @rp- comments today, just could not get to it yesterday. Will provide comments/updates later today.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "slavkap (via GitHub)" <gi...@apache.org>.
slavkap commented on code in PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#discussion_r1931681107
##########
engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/PrimaryDataStoreDriver.java:
##########
@@ -183,4 +183,20 @@ default boolean volumesRequireGrantAccessWhenUsed() {
default boolean zoneWideVolumesAvailableWithoutClusterMotion() {
return false;
}
+
+ /**
+ * This method returns the actual size required on the pool for a volume.
+ *
+ * @param volumeSize
+ * Size of volume to be created on the store
+ * @param templateSize
+ * Size of template, if any, which will be used to create the volume
+ * @param isEncryptionRequired
+ * true if volume is encrypted
+ *
+ * @return the size required on the pool for the volume
+ */
+ default long getVolumeSizeRequiredOnPool(long volumeSize, Long templateSize, boolean isEncryptionRequired) {
Review Comment:
@rg9975, I'm sorry but I couldn't find where this method is used
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "rp- (via GitHub)" <gi...@apache.org>.
rp- commented on code in PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#discussion_r1938950971
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -272,8 +272,17 @@ public Answer copyTemplateToPrimaryStorage(final CopyCommand cmd) {
String path = derivePath(primaryStore, destData, details);
- if (!storagePoolMgr.connectPhysicalDisk(primaryStore.getPoolType(), primaryStore.getUuid(), path, details)) {
- s_logger.warn("Failed to connect physical disk at path: " + path + ", in storage pool id: " + primaryStore.getUuid());
+ if (path == null) {
+ path = destTempl.getUuid();
+ }
Review Comment:
The problem is now rather this.
Linstor ignored path == `null` requests before and kept going, but as this is now setting a UUID that doesn't exist yet, so the connect fails and the Linstor adapter will throw an exception not the code below.
So if we want to keep it ignoring it, we will have to keep the `null` path here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "shwstppr (via GitHub)" <gi...@apache.org>.
shwstppr commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2594978836
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2595123994
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12096
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "rp- (via GitHub)" <gi...@apache.org>.
rp- commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2630991827
> > AFAIS the script already gets the sizes and writes them correctly into the `template.properties` file, so all it would need to do is also update the database entries too?
>
> sounds good, does that mean we can merge this, @rp- ?
well, we can revert/drop then that last commit: f066100de9e14a909be95091a5b5b0b2b8176ef1
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2618169708
Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 12232
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland merged PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#discussion_r1930708611
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java:
##########
@@ -1035,9 +1035,7 @@ private void copyTemplateToManagedTemplateVolume(TemplateInfo srcTemplateInfo, T
try {
grantAccess(templateOnPrimary, destHost, destPrimaryDataStore);
} catch (Exception e) {
- StorageAccessException e2 = new StorageAccessException("Unable to grant access to template: " + templateOnPrimary.getId() + " on host: " + destHost.getId());
- e2.initCause(e);
- throw e;
+ throw new StorageAccessException("Unable to grant access to template: " + templateOnPrimary.getId() + " on host: " + destHost.getId(), e);
Review Comment:
did you also add the constructor? I think it didn't exist yet.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2618075240
@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "slavkap (via GitHub)" <gi...@apache.org>.
slavkap commented on code in PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#discussion_r1931672228
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -1568,6 +1580,8 @@ else if (volumeInfo.getFormat() == ImageFormat.OVA) {
}
}
+
+
Review Comment:
@rg9975, can you remove both new lines ^^
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "rg9975 (via GitHub)" <gi...@apache.org>.
rg9975 commented on code in PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#discussion_r1932476650
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -274,6 +274,7 @@ public Answer copyTemplateToPrimaryStorage(final CopyCommand cmd) {
if (!storagePoolMgr.connectPhysicalDisk(primaryStore.getPoolType(), primaryStore.getUuid(), path, details)) {
s_logger.warn("Failed to connect physical disk at path: " + path + ", in storage pool id: " + primaryStore.getUuid());
+ return new PrimaryStorageDownloadAnswer("Failed to spool template disk at path: " + path + ", in storage pool id: " + primaryStore.getUuid());
Review Comment:
Ok, analyizing the source of all the storage adapters, it appears they all expect either a path or the uuid of the destination object or simply return true. This code appears to have been setting leaving this value null going back 8 years. I updated the code to grab the uuid if path is null before calling the connect method. Can you retest with this change and see if it now works correctly with Linstor?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2594983784
@shwstppr 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2559288354
## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/10132?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
All modified and coverable lines are covered by tests :white_check_mark:
> Project coverage is 4.30%. Comparing base [(`ed1b145`)](https://app.codecov.io/gh/apache/cloudstack/commit/ed1b145a3a06ab58488a01614bc1ea3c614036d0?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) to head [(`3449689`)](https://app.codecov.io/gh/apache/cloudstack/commit/3449689f60ae860fd848cd5bb04a10bf50b89ae3?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
> :exclamation: There is a different number of reports uploaded between BASE (ed1b145) and HEAD (3449689). Click for more details.
>
> <details><summary>HEAD has 1 upload less than BASE</summary>
>
>| Flag | BASE (ed1b145) | HEAD (3449689) |
>|------|------|------|
>|unittests|1|0|
></details>
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## 4.19 #10132 +/- ##
=============================================
- Coverage 15.13% 4.30% -10.83%
=============================================
Files 5408 366 -5042
Lines 473900 29571 -444329
Branches 57791 5180 -52611
=============================================
- Hits 71705 1272 -70433
+ Misses 394192 28155 -366037
+ Partials 8003 144 -7859
```
| [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/10132/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/10132/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `4.30% <ø> (ø)` | |
| [unittests](https://app.codecov.io/gh/apache/cloudstack/pull/10132/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `?` | |
Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more.
</details>
[:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/10132?dropdown=coverage&src=pr&el=continue&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
:loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "blueorangutan (via GitHub)" <gi...@apache.org>.
blueorangutan commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2596079176
<b>[LL] Trillian Build Failed (tid-7056)<b/>
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "rp- (via GitHub)" <gi...@apache.org>.
rp- commented on code in PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#discussion_r1931948031
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -274,6 +274,7 @@ public Answer copyTemplateToPrimaryStorage(final CopyCommand cmd) {
if (!storagePoolMgr.connectPhysicalDisk(primaryStore.getPoolType(), primaryStore.getUuid(), path, details)) {
s_logger.warn("Failed to connect physical disk at path: " + path + ", in storage pool id: " + primaryStore.getUuid());
+ return new PrimaryStorageDownloadAnswer("Failed to spool template disk at path: " + path + ", in storage pool id: " + primaryStore.getUuid());
Review Comment:
Either we fix the `null` path attempt or can explain why it happens, but until than we can't return a exception here otherwise this will break Linstor primary storage only clusters and maybe other too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "rg9975 (via GitHub)" <gi...@apache.org>.
rg9975 commented on code in PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#discussion_r1932300469
##########
engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/PrimaryDataStoreDriver.java:
##########
@@ -183,4 +183,20 @@ default boolean volumesRequireGrantAccessWhenUsed() {
default boolean zoneWideVolumesAvailableWithoutClusterMotion() {
return false;
}
+
+ /**
+ * This method returns the actual size required on the pool for a volume.
+ *
+ * @param volumeSize
+ * Size of volume to be created on the store
+ * @param templateSize
+ * Size of template, if any, which will be used to create the volume
+ * @param isEncryptionRequired
+ * true if volume is encrypted
+ *
+ * @return the size required on the pool for the volume
+ */
+ default long getVolumeSizeRequiredOnPool(long volumeSize, Long templateSize, boolean isEncryptionRequired) {
Review Comment:
removed in upcoming code commit
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2624769898
is there consensus growing on this or shall I move this to 4.19.3 and up?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "rp- (via GitHub)" <gi...@apache.org>.
rp- commented on code in PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#discussion_r1931654204
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -923,6 +924,7 @@ private HostVO getHostOnWhichToExecuteMigrationCommand(VolumeInfo srcVolumeInfo,
HostVO hostVO;
// if either source or destination is a HOST-scoped storage pool, the migration MUST be performed on that host
+
Review Comment:
I would cleanup all those new line introductions, if they don't have any purpose,
as they just make the diff/review larger and leave useless traces in `git blame`
##########
engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/StorageSystemDataMotionStrategy.java:
##########
@@ -1568,6 +1580,8 @@ else if (volumeInfo.getFormat() == ImageFormat.OVA) {
}
}
+
+
Review Comment:
I would cleanup all those new line introductions, if they don't have any purpose,
as they just make the diff/review larger and leave useless traces in `git blame`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "rp- (via GitHub)" <gi...@apache.org>.
rp- commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2630904666
> > > @rp- can you review the last changes.
> >
> >
> > @DaanHoogland @rg9975
> > So I dug a bit deeper and found the actual problem to be the `cloud-install-sys-tmplt` script. I use it in my automated CloudStack installations and this scripts sets `size` and `physical_size` `0` in the `template_store_ref` table, which than Linstor can't use to create a template.
> > If I don't use the script at all, and let CloudStack fetch the template on its own, the entries are correct and the Linstor driver already creates the volume on management server and everything is fine.
> > So my question is, is this script still recommended for installation? And if so can we fix it?
>
> because ACS packages no (can) include the template the script has somewhat gotten out of use. It should still work, but I am not sure how to fix it.
AFAIS the script already gets the sizes and writes them correctly into the `template.properties` file, so all it would need to do is also update the database entries too?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2618073749
@slavkap , @rp- can you please review?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2618074114
@blueorangutan package
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "rp- (via GitHub)" <gi...@apache.org>.
rp- commented on code in PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#discussion_r1938905993
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -274,6 +274,7 @@ public Answer copyTemplateToPrimaryStorage(final CopyCommand cmd) {
if (!storagePoolMgr.connectPhysicalDisk(primaryStore.getPoolType(), primaryStore.getUuid(), path, details)) {
s_logger.warn("Failed to connect physical disk at path: " + path + ", in storage pool id: " + primaryStore.getUuid());
+ return new PrimaryStorageDownloadAnswer("Failed to spool template disk at path: " + path + ", in storage pool id: " + primaryStore.getUuid());
Review Comment:
Linstor does it the same way, BUT as I said the very first system template, doesn't provide a size on the management server, so we just can't create it there.
Did you ever try the FiberChannel driver as only primary storage and from start?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2630844638
> > @rp- can you review the last changes.
>
> @DaanHoogland @rg9975
>
> So I dug a bit deeper and found the actual problem to be the `cloud-install-sys-tmplt` script. I use it in my automated CloudStack installations and this scripts sets `size` and `physical_size` `0` in the `template_store_ref` table, which than Linstor can't use to create a template.
>
> If I don't use the script at all, and let CloudStack fetch the template on its own, the entries are correct and the Linstor driver already creates the volume on management server and everything is fine.
>
> So my question is, is this script still recommended for installation? And if so can we fix it?
because ACS packages no (can) include the template the script has somewhat gotten out of use. It should still work, but I am not sure how to fix it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#issuecomment-2630950156
> AFAIS the script already gets the sizes and writes them correctly into the `template.properties` file, so all it would need to do is also update the database entries too?
sounds good, does that mean we can merge this, @rp- ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "rp- (via GitHub)" <gi...@apache.org>.
rp- commented on code in PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#discussion_r1933416600
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -274,6 +274,7 @@ public Answer copyTemplateToPrimaryStorage(final CopyCommand cmd) {
if (!storagePoolMgr.connectPhysicalDisk(primaryStore.getPoolType(), primaryStore.getUuid(), path, details)) {
s_logger.warn("Failed to connect physical disk at path: " + path + ", in storage pool id: " + primaryStore.getUuid());
+ return new PrimaryStorageDownloadAnswer("Failed to spool template disk at path: " + path + ", in storage pool id: " + primaryStore.getUuid());
Review Comment:
Sorry no, that still doesn't work.
Because now Linstor tries to attach a resource on the agent node, that doesn't exist yet.
Usually this would probably not happen, but the first systemvm template is special,
because on the management server we don't know the size of the template yet, so Linstor can't create it
the volumes there yet and I have to get to the `copyPhysicalDisk` on the storageAdaptor code and this
happens after this `null` path connect (if returned `false`) .
But now I throw a `CloudRuntimeException` because we don't know the volume we are asked for to connect (because it isn't there yet).
So I think to ultimately fix this, we would need to figure out why we don't know the size on the management server, when doing the TEMPLATE -> TEMPLATE `copyAsync`, because then I could already create the volumes there.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "rg9975 (via GitHub)" <gi...@apache.org>.
rg9975 commented on code in PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#discussion_r1932476650
##########
plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/storage/KVMStorageProcessor.java:
##########
@@ -274,6 +274,7 @@ public Answer copyTemplateToPrimaryStorage(final CopyCommand cmd) {
if (!storagePoolMgr.connectPhysicalDisk(primaryStore.getPoolType(), primaryStore.getUuid(), path, details)) {
s_logger.warn("Failed to connect physical disk at path: " + path + ", in storage pool id: " + primaryStore.getUuid());
+ return new PrimaryStorageDownloadAnswer("Failed to spool template disk at path: " + path + ", in storage pool id: " + primaryStore.getUuid());
Review Comment:
Ok, analyzing the source of all the storage adapters, it appears they all expect either a path or the uuid of the destination object or simply return true. This code appears to have been setting this value to null going back 8 years. I updated the code to grab the uuid if path is null before calling the connect method. Can you retest with this change and see if it now works correctly with Linstor?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Primera pure patches & various small fixes [cloudstack]
Posted by "DaanHoogland (via GitHub)" <gi...@apache.org>.
DaanHoogland commented on code in PR #10132:
URL: https://github.com/apache/cloudstack/pull/10132#discussion_r1918766565
##########
engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java:
##########
@@ -1035,7 +1035,9 @@ private void copyTemplateToManagedTemplateVolume(TemplateInfo srcTemplateInfo, T
try {
grantAccess(templateOnPrimary, destHost, destPrimaryDataStore);
} catch (Exception e) {
- throw new StorageAccessException("Unable to grant access to template: " + templateOnPrimary.getId() + " on host: " + destHost.getId());
+ StorageAccessException e2 = new StorageAccessException("Unable to grant access to template: " + templateOnPrimary.getId() + " on host: " + destHost.getId());
+ e2.initCause(e);
+ throw e;
Review Comment:
how about adding the msg+cause contructor to StorageAccessException?
i.e. `StorageAccessException.StorageAccessException(String msg, Exception cause)`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org