Re: [PR] Allocate new ROOT volume (on restore virtual machine operation) only when resource count increment succeeds [cloudstack]
rohityadavcloud merged PR #8555: URL: https://github.com/apache/cloudstack/pull/8555 -- 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-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Allocate new ROOT volume (on restore virtual machine operation) only when resource count increment succeeds [cloudstack]
DaanHoogland commented on code in PR #8555:
URL: https://github.com/apache/cloudstack/pull/8555#discussion_r1470894257
##
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##
@@ -7765,49 +7766,68 @@ public UserVm restoreVirtualMachine(final Account
caller, final long vmId, final
List newVols = new ArrayList<>();
for (VolumeVO root : rootVols) {
-if ( !Volume.State.Allocated.equals(root.getState()) ||
newTemplateId != null ){
-Long templateId = root.getTemplateId();
-boolean isISO = false;
-if (templateId == null) {
-// Assuming that for a vm deployed using ISO, template ID
is set to NULL
-isISO = true;
-templateId = vm.getIsoId();
-}
-
-/* If new template/ISO is provided allocate a new volume from
new template/ISO otherwise allocate new volume from original template/ISO */
-Volume newVol = null;
-if (newTemplateId != null) {
-if (isISO) {
-newVol = volumeMgr.allocateDuplicateVolume(root, null);
-vm.setIsoId(newTemplateId);
-vm.setGuestOSId(template.getGuestOSId());
-vm.setTemplateId(newTemplateId);
-} else {
-newVol = volumeMgr.allocateDuplicateVolume(root,
newTemplateId);
-vm.setGuestOSId(template.getGuestOSId());
-vm.setTemplateId(newTemplateId);
-}
-// check and update VM if it can be dynamically scalable
with the new template
-updateVMDynamicallyScalabilityUsingTemplate(vm,
newTemplateId);
-} else {
-newVol = volumeMgr.allocateDuplicateVolume(root, null);
-}
-newVols.add(newVol);
+if ( !Volume.State.Allocated.equals(root.getState()) ||
newTemplateId != null ) {
Review Comment:
this if block, the transaction and possibibly even the branches within it
should be new methods. A lot of comments in it could be method names. These can
than be tested to improve the coverage of this bit of code.
--
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-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Allocate new ROOT volume (on restore virtual machine operation) only when resource count increment succeeds [cloudstack]
DaanHoogland commented on code in PR #8555:
URL: https://github.com/apache/cloudstack/pull/8555#discussion_r1470894257
##
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##
@@ -7765,49 +7766,68 @@ public UserVm restoreVirtualMachine(final Account
caller, final long vmId, final
List newVols = new ArrayList<>();
for (VolumeVO root : rootVols) {
-if ( !Volume.State.Allocated.equals(root.getState()) ||
newTemplateId != null ){
-Long templateId = root.getTemplateId();
-boolean isISO = false;
-if (templateId == null) {
-// Assuming that for a vm deployed using ISO, template ID
is set to NULL
-isISO = true;
-templateId = vm.getIsoId();
-}
-
-/* If new template/ISO is provided allocate a new volume from
new template/ISO otherwise allocate new volume from original template/ISO */
-Volume newVol = null;
-if (newTemplateId != null) {
-if (isISO) {
-newVol = volumeMgr.allocateDuplicateVolume(root, null);
-vm.setIsoId(newTemplateId);
-vm.setGuestOSId(template.getGuestOSId());
-vm.setTemplateId(newTemplateId);
-} else {
-newVol = volumeMgr.allocateDuplicateVolume(root,
newTemplateId);
-vm.setGuestOSId(template.getGuestOSId());
-vm.setTemplateId(newTemplateId);
-}
-// check and update VM if it can be dynamically scalable
with the new template
-updateVMDynamicallyScalabilityUsingTemplate(vm,
newTemplateId);
-} else {
-newVol = volumeMgr.allocateDuplicateVolume(root, null);
-}
-newVols.add(newVol);
+if ( !Volume.State.Allocated.equals(root.getState()) ||
newTemplateId != null ) {
Review Comment:
this if block, the transaction and possibibly even the branches within it
should be new methods. A lot of comments in ti could be method names.
--
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-unsubscr...@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] Allocate new ROOT volume (on restore virtual machine operation) only when resource count increment succeeds [cloudstack]
blueorangutan commented on PR #8555: URL: https://github.com/apache/cloudstack/pull/8555#issuecomment-1915689486 [SF] Trillian test result (tid-8973) Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7 Total time taken: 46211 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8555-t8973-kvm-centos7.zip Smoke tests completed. 121 look OK, 0 have errors, 0 did not run Only failed and skipped tests results shown below: Test | Result | Time (s) | Test File --- | --- | --- | --- -- 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-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Allocate new ROOT volume (on restore virtual machine operation) only when resource count increment succeeds [cloudstack]
blueorangutan commented on PR #8555: URL: https://github.com/apache/cloudstack/pull/8555#issuecomment-1914270745 @rohityadavcloud a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke 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-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Allocate new ROOT volume (on restore virtual machine operation) only when resource count increment succeeds [cloudstack]
rohityadavcloud commented on PR #8555: URL: https://github.com/apache/cloudstack/pull/8555#issuecomment-1914268294 @blueorangutan test -- 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-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Allocate new ROOT volume (on restore virtual machine operation) only when resource count increment succeeds [cloudstack]
sureshanaparti commented on code in PR #8555:
URL: https://github.com/apache/cloudstack/pull/8555#discussion_r1465980926
##
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##
@@ -7765,49 +7766,68 @@ public UserVm restoreVirtualMachine(final Account
caller, final long vmId, final
List newVols = new ArrayList<>();
for (VolumeVO root : rootVols) {
-if ( !Volume.State.Allocated.equals(root.getState()) ||
newTemplateId != null ){
-Long templateId = root.getTemplateId();
-boolean isISO = false;
-if (templateId == null) {
-// Assuming that for a vm deployed using ISO, template ID
is set to NULL
-isISO = true;
-templateId = vm.getIsoId();
-}
-
-/* If new template/ISO is provided allocate a new volume from
new template/ISO otherwise allocate new volume from original template/ISO */
-Volume newVol = null;
-if (newTemplateId != null) {
-if (isISO) {
-newVol = volumeMgr.allocateDuplicateVolume(root, null);
-vm.setIsoId(newTemplateId);
-vm.setGuestOSId(template.getGuestOSId());
-vm.setTemplateId(newTemplateId);
-} else {
-newVol = volumeMgr.allocateDuplicateVolume(root,
newTemplateId);
-vm.setGuestOSId(template.getGuestOSId());
-vm.setTemplateId(newTemplateId);
-}
-// check and update VM if it can be dynamically scalable
with the new template
-updateVMDynamicallyScalabilityUsingTemplate(vm,
newTemplateId);
-} else {
-newVol = volumeMgr.allocateDuplicateVolume(root, null);
-}
-newVols.add(newVol);
+if ( !Volume.State.Allocated.equals(root.getState()) ||
newTemplateId != null ) {
+final UserVmVO userVm = vm;
+Pair vmAndNewVol = Transaction.execute(new
TransactionCallbackWithException,
CloudRuntimeException>() {
+@Override
+public Pair doInTransaction(final
TransactionStatus status) throws CloudRuntimeException {
+Long templateId = root.getTemplateId();
+boolean isISO = false;
+if (templateId == null) {
+// Assuming that for a vm deployed using ISO,
template ID is set to NULL
+isISO = true;
+templateId = userVm.getIsoId();
+}
+
+/* If new template/ISO is provided allocate a new
volume from new template/ISO otherwise allocate new volume from original
template/ISO */
+Volume newVol = null;
+if (newTemplateId != null) {
+if (isISO) {
+newVol =
volumeMgr.allocateDuplicateVolume(root, null);
+userVm.setIsoId(newTemplateId);
+userVm.setGuestOSId(template.getGuestOSId());
+userVm.setTemplateId(newTemplateId);
+} else {
+newVol =
volumeMgr.allocateDuplicateVolume(root, newTemplateId);
+userVm.setGuestOSId(template.getGuestOSId());
+userVm.setTemplateId(newTemplateId);
+}
+// check and update VM if it can be dynamically
scalable with the new template
+
updateVMDynamicallyScalabilityUsingTemplate(userVm, newTemplateId);
+} else {
+newVol = volumeMgr.allocateDuplicateVolume(root,
null);
+}
+newVols.add(newVol);
+
+if (userVmDetailsDao.findDetail(userVm.getId(),
VmDetailConstants.ROOT_DISK_SIZE) == null &&
!newVol.getSize().equals(template.getSize())) {
+VolumeVO resizedVolume = (VolumeVO) newVol;
+if (template.getSize() != null) {
+resizedVolume.setSize(template.getSize());
+_volsDao.update(resizedVolume.getId(),
resizedVolume);
+}
+}
+
+// 1. Save usage event and update resource count for
user vm volumes
+try {
+
_resourceLimitMgr.incrementResourceCount(newVol.getAccountId(),
ResourceType.volume, newVol.isDisplay());
+
Re: [PR] Allocate new ROOT volume (on restore virtual machine operation) only when resource count increment succeeds [cloudstack]
blueorangutan commented on PR #8555: URL: https://github.com/apache/cloudstack/pull/8555#issuecomment-1908749801 [SF] Trillian test result (tid-8926) Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7 Total time taken: 47292 seconds Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr8555-t8926-kvm-centos7.zip Smoke tests completed. 120 look OK, 1 have errors, 0 did not run Only failed and skipped tests results shown below: Test | Result | Time (s) | Test File --- | --- | --- | --- ContextSuite context=TestSharedNetwork>:setup | `Error` | 53.62 | test_network.py -- 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-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Allocate new ROOT volume (on restore virtual machine operation) only when resource count increment succeeds [cloudstack]
blueorangutan commented on PR #8555: URL: https://github.com/apache/cloudstack/pull/8555#issuecomment-1907406495 @sureshanaparti a [SL] Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke 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-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Allocate new ROOT volume (on restore virtual machine operation) only when resource count increment succeeds [cloudstack]
sureshanaparti commented on PR #8555: URL: https://github.com/apache/cloudstack/pull/8555#issuecomment-1907402387 @blueorangutan test -- 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-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Allocate new ROOT volume (on restore virtual machine operation) only when resource count increment succeeds [cloudstack]
BryanMLima commented on code in PR #8555:
URL: https://github.com/apache/cloudstack/pull/8555#discussion_r1463918347
##
server/src/main/java/com/cloud/vm/UserVmManagerImpl.java:
##
@@ -7765,49 +7766,68 @@ public UserVm restoreVirtualMachine(final Account
caller, final long vmId, final
List newVols = new ArrayList<>();
for (VolumeVO root : rootVols) {
-if ( !Volume.State.Allocated.equals(root.getState()) ||
newTemplateId != null ){
-Long templateId = root.getTemplateId();
-boolean isISO = false;
-if (templateId == null) {
-// Assuming that for a vm deployed using ISO, template ID
is set to NULL
-isISO = true;
-templateId = vm.getIsoId();
-}
-
-/* If new template/ISO is provided allocate a new volume from
new template/ISO otherwise allocate new volume from original template/ISO */
-Volume newVol = null;
-if (newTemplateId != null) {
-if (isISO) {
-newVol = volumeMgr.allocateDuplicateVolume(root, null);
-vm.setIsoId(newTemplateId);
-vm.setGuestOSId(template.getGuestOSId());
-vm.setTemplateId(newTemplateId);
-} else {
-newVol = volumeMgr.allocateDuplicateVolume(root,
newTemplateId);
-vm.setGuestOSId(template.getGuestOSId());
-vm.setTemplateId(newTemplateId);
-}
-// check and update VM if it can be dynamically scalable
with the new template
-updateVMDynamicallyScalabilityUsingTemplate(vm,
newTemplateId);
-} else {
-newVol = volumeMgr.allocateDuplicateVolume(root, null);
-}
-newVols.add(newVol);
+if ( !Volume.State.Allocated.equals(root.getState()) ||
newTemplateId != null ) {
+final UserVmVO userVm = vm;
+Pair vmAndNewVol = Transaction.execute(new
TransactionCallbackWithException,
CloudRuntimeException>() {
+@Override
+public Pair doInTransaction(final
TransactionStatus status) throws CloudRuntimeException {
+Long templateId = root.getTemplateId();
+boolean isISO = false;
+if (templateId == null) {
+// Assuming that for a vm deployed using ISO,
template ID is set to NULL
+isISO = true;
+templateId = userVm.getIsoId();
+}
+
+/* If new template/ISO is provided allocate a new
volume from new template/ISO otherwise allocate new volume from original
template/ISO */
+Volume newVol = null;
+if (newTemplateId != null) {
+if (isISO) {
+newVol =
volumeMgr.allocateDuplicateVolume(root, null);
+userVm.setIsoId(newTemplateId);
+userVm.setGuestOSId(template.getGuestOSId());
+userVm.setTemplateId(newTemplateId);
+} else {
+newVol =
volumeMgr.allocateDuplicateVolume(root, newTemplateId);
+userVm.setGuestOSId(template.getGuestOSId());
+userVm.setTemplateId(newTemplateId);
+}
+// check and update VM if it can be dynamically
scalable with the new template
+
updateVMDynamicallyScalabilityUsingTemplate(userVm, newTemplateId);
+} else {
+newVol = volumeMgr.allocateDuplicateVolume(root,
null);
+}
+newVols.add(newVol);
+
+if (userVmDetailsDao.findDetail(userVm.getId(),
VmDetailConstants.ROOT_DISK_SIZE) == null &&
!newVol.getSize().equals(template.getSize())) {
+VolumeVO resizedVolume = (VolumeVO) newVol;
+if (template.getSize() != null) {
+resizedVolume.setSize(template.getSize());
+_volsDao.update(resizedVolume.getId(),
resizedVolume);
+}
+}
+
+// 1. Save usage event and update resource count for
user vm volumes
+try {
+
_resourceLimitMgr.incrementResourceCount(newVol.getAccountId(),
ResourceType.volume, newVol.isDisplay());
+
Re: [PR] Allocate new ROOT volume (on restore virtual machine operation) only when resource count increment succeeds [cloudstack]
blueorangutan commented on PR #8555: URL: https://github.com/apache/cloudstack/pull/8555#issuecomment-1906668362 Packaging result [SF]: ✔️ el7 ✔️ el8 ✔️ el9 ✔️ debian ✔️ suse15. SL-JID 8424 -- 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-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Allocate new ROOT volume (on restore virtual machine operation) only when resource count increment succeeds [cloudstack]
blueorangutan commented on PR #8555: URL: https://github.com/apache/cloudstack/pull/8555#issuecomment-1906557174 @sureshanaparti 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-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Allocate new ROOT volume (on restore virtual machine operation) only when resource count increment succeeds [cloudstack]
sureshanaparti commented on PR #8555: URL: https://github.com/apache/cloudstack/pull/8555#issuecomment-1906538892 @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-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Allocate new ROOT volume (on restore virtual machine operation) only when resource count increment succeeds [cloudstack]
codecov[bot] commented on PR #8555: URL: https://github.com/apache/cloudstack/pull/8555#issuecomment-1906210250 ## [Codecov](https://app.codecov.io/gh/apache/cloudstack/pull/8555?src=pr=h1_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) Report Attention: `39 lines` in your changes are missing coverage. Please review. > Comparison is base [(`6d916ca`)](https://app.codecov.io/gh/apache/cloudstack/commit/6d916cad348f5833a567c17f5c9dbccaf2135448?el=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) 30.85% compared to head [(`f1494cf`)](https://app.codecov.io/gh/apache/cloudstack/pull/8555?src=pr=desc_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) 18.35%. > Report is 37 commits behind head on main. | [Files](https://app.codecov.io/gh/apache/cloudstack/pull/8555?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Patch % | Lines | |---|---|---| | [.../src/main/java/com/cloud/vm/UserVmManagerImpl.java](https://app.codecov.io/gh/apache/cloudstack/pull/8555?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#diff-c2VydmVyL3NyYy9tYWluL2phdmEvY29tL2Nsb3VkL3ZtL1VzZXJWbU1hbmFnZXJJbXBsLmphdmE=) | 0.00% | [39 Missing :warning: ](https://app.codecov.io/gh/apache/cloudstack/pull/8555?src=pr=tree_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Additional details and impacted files ```diff @@ Coverage Diff @@ ## main#8555 +/- ## = - Coverage 30.85% 18.35% -12.50% + Complexity3404817148-16900 = Files 5341 4848 -493 Lines374861 324364-50497 Branches 5451845569 -8949 = - Hits 11565959540-56119 - Misses 243973 256335+12362 + Partials 15229 8489 -6740 ``` | [Flag](https://app.codecov.io/gh/apache/cloudstack/pull/8555/flags?src=pr=flags_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | Coverage Δ | | |---|---|---| | [simulator-marvin-tests](https://app.codecov.io/gh/apache/cloudstack/pull/8555/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `18.35% <0.00%> (-6.41%)` | :arrow_down: | | [uitests](https://app.codecov.io/gh/apache/cloudstack/pull/8555/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `?` | | | [unit-tests](https://app.codecov.io/gh/apache/cloudstack/pull/8555/flags?src=pr=flag_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache#carryforward-flags-in-the-pull-request-comment) to find out more. [:umbrella: View full report in Codecov by Sentry](https://app.codecov.io/gh/apache/cloudstack/pull/8555?src=pr=continue_medium=referral_source=github_content=comment_campaign=pr+comments_term=apache). :loudspeaker: Have feedback on the report? [Share it here](https://about.codecov.io/codecov-pr-comment-feedback/?utm_medium=referral_source=github_content=comment_campaign=pr+comments_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-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Allocate new ROOT volume (on restore virtual machine operation) only when resource count increment succeeds [cloudstack]
blueorangutan commented on PR #8555: URL: https://github.com/apache/cloudstack/pull/8555#issuecomment-1906100528 @sureshanaparti 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-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] Allocate new ROOT volume (on restore virtual machine operation) only when resource count increment succeeds [cloudstack]
sureshanaparti commented on PR #8555: URL: https://github.com/apache/cloudstack/pull/8555#issuecomment-1906096801 @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-unsubscr...@cloudstack.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
