Change in vdsm[master]: virt: Move VM status check from vm.cont() to API level
Nir Soffer has posted comments on this change. Change subject: virt: Move VM status check from vm.cont() to API level .. Patch Set 4: (1 comment) https://gerrit.ovirt.org/#/c/47527/4//COMMIT_MSG Commit Message: Line 10: occured while hibernating a VM, in SourceThread._recover() vm.cont() Line 11: will fail and the VM will be left paused. Line 12: Line 13: To prevent client from calling vm.cont() in VM states that don't allow Line 14: this operation, check the state of the VM an API level. > That was my first solution that I posted in the first version of this patch Why internal usage does not need this check? This kind of checks is certainly needed in the object performing the operation, and the place for this code is the object holding the state. Line 15: Line 16: Change-Id: I5b1c7b4eecacf87ece48dc563fd2da294af0510b Line 17: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1238536 -- To view, visit https://gerrit.ovirt.org/47527 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5b1c7b4eecacf87ece48dc563fd2da294af0510b Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shmuel Leib Melamud Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Shmuel Leib Melamud Gerrit-Reviewer: Shmuel Melamud Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Move VM status check from vm.cont() to API level
Shmuel Leib Melamud has posted comments on this change. Change subject: virt: Move VM status check from vm.cont() to API level .. Patch Set 4: (1 comment) https://gerrit.ovirt.org/#/c/47527/4//COMMIT_MSG Commit Message: Line 10: occured while hibernating a VM, in SourceThread._recover() vm.cont() Line 11: will fail and the VM will be left paused. Line 12: Line 13: To prevent client from calling vm.cont() in VM states that don't allow Line 14: this operation, check the state of the VM an API level. > I don't understand why we need to do this in API.py. That was my first solution that I posted in the first version of this patch. But reviewers noted correctly, that adding a parameter for one specific case to override the check is not a beautiful solution. The question is: why we need to check VM status in vm.cont()? What does it prevent? The only possible cause is to prevent client from using cont() on a VM that is not down/paused/hibernated. If so, we need to check the VM state only when cont() is called by client, the internal usages don't need this check. Thus, it is logical to move this check to API layer. Line 15: Line 16: Change-Id: I5b1c7b4eecacf87ece48dc563fd2da294af0510b Line 17: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1238536 -- To view, visit https://gerrit.ovirt.org/47527 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5b1c7b4eecacf87ece48dc563fd2da294af0510b Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shmuel Leib Melamud Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Shmuel Leib Melamud Gerrit-Reviewer: Shmuel Melamud Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Move VM status check from vm.cont() to API level
Nir Soffer has posted comments on this change. Change subject: virt: Move VM status check from vm.cont() to API level .. Patch Set 4: Code-Review-1 (2 comments) https://gerrit.ovirt.org/#/c/47527/4//COMMIT_MSG Commit Message: Line 7: virt: Move VM status check from vm.cont() to API level Line 8: Line 9: Allow vm.cont() to resume the VM in any state. Otherwise, if error Line 10: occured while hibernating a VM, in SourceThread._recover() vm.cont() Line 11: will fail and the VM will be left paused. Please provide a method or an option in the Vm.cont, so SourceThread._recover can call to unpause the vm in all states. For example vm.cont(force=True) Line 12: Line 13: To prevent client from calling vm.cont() in VM states that don't allow Line 14: this operation, check the state of the VM an API level. Line 15: Line 10: occured while hibernating a VM, in SourceThread._recover() vm.cont() Line 11: will fail and the VM will be left paused. Line 12: Line 13: To prevent client from calling vm.cont() in VM states that don't allow Line 14: this operation, check the state of the VM an API level. I don't understand why we need to do this in API.py. Vm state is available to the vm in Vm.cont, why not use it there? We want to kill the API layer, not add more funcionallity. Line 15: Line 16: Change-Id: I5b1c7b4eecacf87ece48dc563fd2da294af0510b Line 17: Bug-Url: https://bugzilla.redhat.com/show_bug.cgi?id=1238536 -- To view, visit https://gerrit.ovirt.org/47527 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5b1c7b4eecacf87ece48dc563fd2da294af0510b Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shmuel Leib Melamud Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Shmuel Leib Melamud Gerrit-Reviewer: Shmuel Melamud Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: Yes ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Move VM status check from vm.cont() to API level
Martin Polednik has posted comments on this change. Change subject: virt: Move VM status check from vm.cont() to API level .. Patch Set 4: Code-Review+1 -- To view, visit https://gerrit.ovirt.org/47527 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5b1c7b4eecacf87ece48dc563fd2da294af0510b Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shmuel Leib Melamud Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Shmuel Leib Melamud Gerrit-Reviewer: Shmuel Melamud Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Move VM status check from vm.cont() to API level
Francesco Romani has posted comments on this change. Change subject: virt: Move VM status check from vm.cont() to API level .. Patch Set 4: Rerun-Hooks: all -- To view, visit https://gerrit.ovirt.org/47527 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5b1c7b4eecacf87ece48dc563fd2da294af0510b Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shmuel Leib Melamud Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Shmuel Leib Melamud Gerrit-Reviewer: Shmuel Melamud Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Move VM status check from vm.cont() to API level
Francesco Romani has posted comments on this change. Change subject: virt: Move VM status check from vm.cont() to API level .. Patch Set 4: Code-Review+2 -- To view, visit https://gerrit.ovirt.org/47527 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5b1c7b4eecacf87ece48dc563fd2da294af0510b Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shmuel Leib Melamud Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Shmuel Leib Melamud Gerrit-Reviewer: Shmuel Melamud Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Move VM status check from vm.cont() to API level
Shmuel Leib Melamud has posted comments on this change. Change subject: virt: Move VM status check from vm.cont() to API level .. Patch Set 4: Verified+1 As discussed, moved VM status check from vm.cont() to the API level. -- To view, visit https://gerrit.ovirt.org/47527 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5b1c7b4eecacf87ece48dc563fd2da294af0510b Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shmuel Leib Melamud Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Shmuel Leib Melamud Gerrit-Reviewer: Shmuel Melamud Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
Change in vdsm[master]: virt: Move VM status check from vm.cont() to API level
gerrit-hooks has posted comments on this change. Change subject: virt: Move VM status check from vm.cont() to API level .. Patch Set 4: * #1238536::Update tracker: OK * Check Bug-Url::OK * Check Public Bug::#1238536::OK, public bug * Check Product::#1238536::OK, Correct classification oVirt * Check TM::SKIP, not in a monitored branch (ovirt-3.6) * Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.6']) -- To view, visit https://gerrit.ovirt.org/47527 To unsubscribe, visit https://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I5b1c7b4eecacf87ece48dc563fd2da294af0510b Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Shmuel Leib Melamud Gerrit-Reviewer: Dan Kenigsberg Gerrit-Reviewer: Francesco Romani Gerrit-Reviewer: Jenkins CI Gerrit-Reviewer: Martin Polednik Gerrit-Reviewer: Milan Zamazal Gerrit-Reviewer: Nir Soffer Gerrit-Reviewer: Shahar Havivi Gerrit-Reviewer: Shmuel Leib Melamud Gerrit-Reviewer: Shmuel Melamud Gerrit-Reviewer: gerrit-hooks Gerrit-HasComments: No ___ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches