You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/11/26 06:34:35 UTC
[GitHub] [cloudstack] harikrishna-patnala opened a new pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
harikrishna-patnala opened a new pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721
### Description
This PR fixes issue #5719 NPE during scale VM operation on a VM for which the corresponding template is already deleted.
<!--- Describe your changes in DETAIL - And how has behaviour functionally changed. -->
<!-- For new features, provide link to FS, dev ML discussion etc. -->
<!-- In case of bug fix, the expected and actual behaviours, steps to reproduce. -->
<!-- When "Fixes: #<id>" is specified, the issue/PR will automatically be closed when this PR gets merged -->
<!-- For addressing multiple issues/PRs, use multiple "Fixes: #<id>" -->
<!-- Fixes: # -->
<!--- ********************************************************************************* -->
<!--- NOTE: AUTOMATATION 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)
- [ ] 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)
### Feature/Enhancement Scale or Bug Severity
#### Feature/Enhancement Scale
- [ ] Major
- [ ] Minor
#### Bug Severity
- [ ] BLOCKER
- [ ] Critical
- [ ] Major
- [ ] Minor
- [ ] Trivial
### Screenshots (if appropriate):
### How Has This Been Tested?
<!-- Please describe in detail how you tested your changes. -->
<!-- Include details of your testing environment, and the tests you ran to -->
<!-- see how your change affects other areas of the code, etc. -->
1. Created a VM with a template
2. Force deleted the template
3. Dynamically scaled VM created from that template -> successful
<!-- Please read the [CONTRIBUTING](https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md) document -->
--
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
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721#discussion_r757448189
##########
File path: engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -3808,7 +3808,7 @@ public boolean upgradeVmDb(final long vmId, final ServiceOffering newServiceOffe
if (currentServiceOffering.isDynamic() && !newServiceOffering.isDynamic()) {
removeCustomOfferingDetails(vmId);
}
- VMTemplateVO template = _templateDao.findById(vmForUpdate.getTemplateId());
+ VMTemplateVO template = _templateDao.findByIdIncludingRemoved(vmForUpdate.getTemplateId());
Review comment:
-1, this may not be the right fix, it checks to update a VM whose template is deleted.
--
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
[GitHub] [cloudstack] harikrishna-patnala commented on a change in pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on a change in pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721#discussion_r758073819
##########
File path: engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -3808,7 +3808,7 @@ public boolean upgradeVmDb(final long vmId, final ServiceOffering newServiceOffe
if (currentServiceOffering.isDynamic() && !newServiceOffering.isDynamic()) {
removeCustomOfferingDetails(vmId);
}
- VMTemplateVO template = _templateDao.findById(vmForUpdate.getTemplateId());
+ VMTemplateVO template = _templateDao.findByIdIncludingRemoved(vmForUpdate.getTemplateId());
Review comment:
yes, agree with @weizhouapache
--
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
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721#discussion_r757452504
##########
File path: engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -3808,7 +3808,7 @@ public boolean upgradeVmDb(final long vmId, final ServiceOffering newServiceOffe
if (currentServiceOffering.isDynamic() && !newServiceOffering.isDynamic()) {
removeCustomOfferingDetails(vmId);
}
- VMTemplateVO template = _templateDao.findById(vmForUpdate.getTemplateId());
+ VMTemplateVO template = _templateDao.findByIdIncludingRemoved(vmForUpdate.getTemplateId());
Review comment:
got it @weizhouapache , but is it right thing to consider the dynamic scaling setting from the deleted template?
--
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
[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721#discussion_r757502180
##########
File path: engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -3808,7 +3808,7 @@ public boolean upgradeVmDb(final long vmId, final ServiceOffering newServiceOffe
if (currentServiceOffering.isDynamic() && !newServiceOffering.isDynamic()) {
removeCustomOfferingDetails(vmId);
}
- VMTemplateVO template = _templateDao.findById(vmForUpdate.getTemplateId());
+ VMTemplateVO template = _templateDao.findByIdIncludingRemoved(vmForUpdate.getTemplateId());
Review comment:
@sureshanaparti
I think it should be ok. If template is configured to support dynamic scaling, the VM image should support it as well because the vm image is created from the template.
--
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
[GitHub] [cloudstack] harikrishna-patnala commented on a change in pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on a change in pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721#discussion_r758074338
##########
File path: engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -3808,7 +3808,7 @@ public boolean upgradeVmDb(final long vmId, final ServiceOffering newServiceOffe
if (currentServiceOffering.isDynamic() && !newServiceOffering.isDynamic()) {
removeCustomOfferingDetails(vmId);
}
- VMTemplateVO template = _templateDao.findById(vmForUpdate.getTemplateId());
+ VMTemplateVO template = _templateDao.findByIdIncludingRemoved(vmForUpdate.getTemplateId());
Review comment:
@sureshanaparti would you like to review this again
--
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
[GitHub] [cloudstack] harikrishna-patnala commented on a change in pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
Posted by GitBox <gi...@apache.org>.
harikrishna-patnala commented on a change in pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721#discussion_r758953514
##########
File path: engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -3808,7 +3808,7 @@ public boolean upgradeVmDb(final long vmId, final ServiceOffering newServiceOffe
if (currentServiceOffering.isDynamic() && !newServiceOffering.isDynamic()) {
removeCustomOfferingDetails(vmId);
}
- VMTemplateVO template = _templateDao.findById(vmForUpdate.getTemplateId());
+ VMTemplateVO template = _templateDao.findByIdIncludingRemoved(vmForUpdate.getTemplateId());
Review comment:
@sureshanaparti, as @shwstppr mentioned we use deleted template properties in other places also. So this procedure looks good to me and we cannot consider true because it can happen that the deleted template was not dynamically scalable. So better use the template properties as it is
--
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
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721#discussion_r758956573
##########
File path: engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -3808,7 +3808,7 @@ public boolean upgradeVmDb(final long vmId, final ServiceOffering newServiceOffe
if (currentServiceOffering.isDynamic() && !newServiceOffering.isDynamic()) {
removeCustomOfferingDetails(vmId);
}
- VMTemplateVO template = _templateDao.findById(vmForUpdate.getTemplateId());
+ VMTemplateVO template = _templateDao.findByIdIncludingRemoved(vmForUpdate.getTemplateId());
Review comment:
lgtm, discussed offline with @harikrishna-patnala
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721#issuecomment-979735166
@shwstppr a Jenkins job has been kicked to build packages. 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
[GitHub] [cloudstack] shwstppr commented on pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721#issuecomment-979734897
@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
[GitHub] [cloudstack] sureshanaparti commented on pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721#issuecomment-983301635
Minor change, merged based on the approvals and manual tests.
--
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
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721#discussion_r757446199
##########
File path: engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -3808,7 +3808,7 @@ public boolean upgradeVmDb(final long vmId, final ServiceOffering newServiceOffe
if (currentServiceOffering.isDynamic() && !newServiceOffering.isDynamic()) {
removeCustomOfferingDetails(vmId);
}
- VMTemplateVO template = _templateDao.findById(vmForUpdate.getTemplateId());
+ VMTemplateVO template = _templateDao.findByIdIncludingRemoved(vmForUpdate.getTemplateId());
Review comment:
does a VM exists after its template got deleted?
--
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
[GitHub] [cloudstack] shwstppr commented on pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
Posted by GitBox <gi...@apache.org>.
shwstppr commented on pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721#issuecomment-982276265
@rhtyd looks good to me. We use the same method at several other places 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
[GitHub] [cloudstack] rhtyd commented on pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
Posted by GitBox <gi...@apache.org>.
rhtyd commented on pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721#issuecomment-981442003
@sureshanaparti @harikrishna-patnala @weizhouapache @shwstppr - what's the consensus here, are we looking for more changes, or is this PR acceptable/reviewable?
@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
[GitHub] [cloudstack] weizhouapache commented on a change in pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
Posted by GitBox <gi...@apache.org>.
weizhouapache commented on a change in pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721#discussion_r757450726
##########
File path: engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -3808,7 +3808,7 @@ public boolean upgradeVmDb(final long vmId, final ServiceOffering newServiceOffe
if (currentServiceOffering.isDynamic() && !newServiceOffering.isDynamic()) {
removeCustomOfferingDetails(vmId);
}
- VMTemplateVO template = _templateDao.findById(vmForUpdate.getTemplateId());
+ VMTemplateVO template = _templateDao.findByIdIncludingRemoved(vmForUpdate.getTemplateId());
Review comment:
> does a VM exists after its template got deleted?
@sureshanaparti
yes, template can be forced deleted (from secondary storage), but the base image of template still exist on primary storage, vms are not impacted (reinstall might not work)
--
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
[GitHub] [cloudstack] blueorangutan commented on pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
Posted by GitBox <gi...@apache.org>.
blueorangutan commented on pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721#issuecomment-979754154
Packaging result: :heavy_check_mark: el7 :heavy_check_mark: el8 :heavy_check_mark: debian :heavy_check_mark: suse15. SL-JID 1791
--
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
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721#discussion_r758084256
##########
File path: engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -3808,7 +3808,7 @@ public boolean upgradeVmDb(final long vmId, final ServiceOffering newServiceOffe
if (currentServiceOffering.isDynamic() && !newServiceOffering.isDynamic()) {
removeCustomOfferingDetails(vmId);
}
- VMTemplateVO template = _templateDao.findById(vmForUpdate.getTemplateId());
+ VMTemplateVO template = _templateDao.findByIdIncludingRemoved(vmForUpdate.getTemplateId());
Review comment:
@weizhouapache @harikrishna-patnala If VM existence (and it's scaling) is allowed after template deletion, better to skip dynamically scalable flag in that template (and consider as 'true'). If one wants to scale the VM when the flag is false, it is not possible to update the flag on the deleted template (this blocks to retry the scaling operation when all the relevant flags are enabled). What's your thoughts?
--
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
[GitHub] [cloudstack] sureshanaparti commented on a change in pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
Posted by GitBox <gi...@apache.org>.
sureshanaparti commented on a change in pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721#discussion_r758084256
##########
File path: engine/orchestration/src/main/java/com/cloud/vm/VirtualMachineManagerImpl.java
##########
@@ -3808,7 +3808,7 @@ public boolean upgradeVmDb(final long vmId, final ServiceOffering newServiceOffe
if (currentServiceOffering.isDynamic() && !newServiceOffering.isDynamic()) {
removeCustomOfferingDetails(vmId);
}
- VMTemplateVO template = _templateDao.findById(vmForUpdate.getTemplateId());
+ VMTemplateVO template = _templateDao.findByIdIncludingRemoved(vmForUpdate.getTemplateId());
Review comment:
@weizhouapache @harikrishna-patnala If VM existence (and it's scaling) is allowed after template deletion, better to skip dynamically scalable flag in that template (and consider as 'true'). If one wants to scale the VM when the flag is false, it is not possible to update the flag on the deleted template. What's your thoughts?
--
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
[GitHub] [cloudstack] sureshanaparti merged pull request #5721: Fix NPE on scale VM operation after the corresponding template is del…
Posted by GitBox <gi...@apache.org>.
sureshanaparti merged pull request #5721:
URL: https://github.com/apache/cloudstack/pull/5721
--
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