linstor: Use template's uuid if pool's downloadPath is null as resour… #11053
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)
|
ghernadi
force-pushed
the
gh/fix-linstor-tmpl-download-path
branch
from
2037fe4 to
935ed8a
Compare
| @@ -668,8 +671,15 @@ private String cloneResource(long csCloneId, VolumeInfo volumeInfo, StoragePoolV | |||
| storagePoolVO.getId(), csCloneId, null); | |||
|
|
|||
| if (tmplPoolRef != null) { | |||
| final String templateRscName = LinstorUtil.RSC_PREFIX + tmplPoolRef.getLocalDownloadPath(); | |||
| final String templateRscName; | |||
There was a problem hiding this comment.
I can change that if some coding guidelines of Cloudstack wants me to, but the thought-process here was to keep the variable effectively final after it was initialized (which happens in the next few lines. Do you still want me to remove it?
There was a problem hiding this comment.
ok, if it works it works but a buit strange to have a final be assigned later (guess it is alright during initialisation)
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #11053 +/- ##
============================================
+ Coverage 15.18% 16.57% +1.39%
- Complexity 11365 13969 +2604
============================================
Files 5416 5743 +327
Lines 475890 510499 +34609
Branches 58093 62076 +3983
============================================
+ Hits 72254 84623 +12369
- Misses 395550 416413 +20863
- Partials 8086 9463 +1377
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:
|
“recent” menaing unreleased versions, or also 4.19.3 and/or 4.20.1, @ghernadi ? in the later case you may want to base your fix off of an older release branch. |
To my understanding the old version (i.e. the version before my PR) was working as expected with versions 4.19.1.3 and 4.20.0.0, but 4.19.2.0 or 4.19.3.0 already require my PR to work again. What branch do you suggest to rebase this PR? |
|
in that case 4.19 @ghernadi |
|
I'm back and will also update this PR and add the scenario to the integration tests |
rp-
force-pushed
the
gh/fix-linstor-tmpl-download-path
branch
from
935ed8a to
f1f6cc4
Compare
boring-cyborg
bot
added
component:integration-test
Python
| @@ -668,8 +671,15 @@ private String cloneResource(long csCloneId, VolumeInfo volumeInfo, StoragePoolV | |||
| storagePoolVO.getId(), csCloneId, null); | |||
|
|
|||
| if (tmplPoolRef != null) { | |||
| final String templateRscName = LinstorUtil.RSC_PREFIX + tmplPoolRef.getLocalDownloadPath(); | |||
| final String templateRscName; | |||
There was a problem hiding this comment.
ok, if it works it works but a buit strange to have a final be assigned later (guess it is alright during initialisation)
…ce-name Also added an integration test for templates from snapshots
rp-
force-pushed
the
gh/fix-linstor-tmpl-download-path
branch
from
f1f6cc4 to
2e1de6f
Compare
Description
My colleague, @rp- is on vacation right now and we found a bug where the previously non-null
VMTemplateStoragePoolVO#getLocalDownloadPathfor some reason returns null in the recent versions of Cloudstack. This is my attempt to patch the issue, which worked in our test-setup.Types of changes
Feature/Enhancement Scale or Bug Severity
Feature/Enhancement Scale
Bug Severity
Screenshots (if appropriate):
How Has This Been Tested?
The following scenario no longer works without this patch:
The last step causes the cloud-plugin-storage-volume-linstor to try to clone from
cs-nullinstead ofcs-${template_uuid}.cs-nullobviously does not exist within LINSTOR itself, which causes an error.How did you try to break this feature and the system with this change?