[PATCH v4] shutdown and reboot actions on linux vs linux with KVM
From: chandrureddy <chandra@linux.vnet.ibm.com> Review Comments taken care --- src/wok/plugins/gingerbase/i18n.py | 1 + src/wok/plugins/gingerbase/model/host.py | 47 +++++++++++++++------- .../gingerbase/ui/js/src/gingerbase.host.js | 25 ++++++------ src/wok/plugins/gingerbase/ui/pages/i18n.json.tmpl | 3 ++ 4 files changed, 49 insertions(+), 27 deletions(-) diff --git a/src/wok/plugins/gingerbase/i18n.py b/src/wok/plugins/gingerbase/i18n.py index 8596f17..ef97266 100644 --- a/src/wok/plugins/gingerbase/i18n.py +++ b/src/wok/plugins/gingerbase/i18n.py @@ -39,6 +39,7 @@ messages = { "GGBHOST0001E": _("Unable to shutdown host machine as there are running virtual machines"), "GGBHOST0002E": _("Unable to reboot host machine as there are running virtual machines"), + "GGBHOST0003E": _("There may be vms running on the host"), "GGBHOST0005E": _("When specifying CPU topology, each element must be an integer greater than zero."), "GGBPKGUPD0001E": _("No packages marked for update"), diff --git a/src/wok/plugins/gingerbase/model/host.py b/src/wok/plugins/gingerbase/model/host.py index 670fec5..766d6c7 100644 --- a/src/wok/plugins/gingerbase/model/host.py +++ b/src/wok/plugins/gingerbase/model/host.py @@ -39,6 +39,14 @@ from wok.plugins.gingerbase.repositories import Repositories from wok.plugins.gingerbase.swupdate import SoftwareUpdate HOST_STATS_INTERVAL = 1 +DOM_STATE_MAP = {0: 'nostate', + 1: 'running', + 2: 'blocked', + 3: 'paused', + 4: 'shutdown', + 5: 'shutoff', + 6: 'crashed', + 7: 'pmsuspended'} class HostModel(object): @@ -142,29 +150,40 @@ class HostModel(object): def shutdown(self, args=None): # Check for running vms before shutdown - # FIXME : Find alternative way to figure out if any vms running - # running_vms = self._get_vms_list_by_state('running') - # if len(running_vms) > 0: - # raise OperationFailed("GGBHOST0001E") + running_vms = self.get_vmlist_bystate('running') + if len(running_vms) > 0: + raise OperationFailed("GGBHOST0001E") wok_log.info('Host is going to shutdown.') os.system('shutdown -h now') def reboot(self, args=None): - # Find running VMs - # FIXME : Find alternative way to figure out if any vms running - # running_vms = self._get_vms_list_by_state('running') - # if len(running_vms) > 0: - # raise OperationFailed("GGBHOST0002E") + # Check for running vms before reboot + running_vms = self.get_vmlist_bystate('running') + if len(running_vms) > 0: + raise OperationFailed("GGBHOST0002E") wok_log.info('Host is going to reboot.') os.system('reboot') - # def _get_vms_list_by_state(self, state): - # conn = self.conn.get() - # return [dom.name().decode('utf-8') - # for dom in conn.listAllDomains(0) - # if (DOM_STATE_MAP[dom.info()[0]]) == state] + def get_vmlist_bystate(self, state='running'): + try: + libvirt_mod = __import__('libvirt') + except Exception, e: + wok_log.info("Unable to import libvirt module. Details:", + e.message) + # Ignore any error and assume there is no vm running in the host + return [] + + try: + conn = libvirt_mod.open(None) + return [dom.name().decode('utf-8') + for dom in conn.listAllDomains(0) + if (DOM_STATE_MAP[dom.info()[0]] == state)] + except Exception, e: + wok_log.info("Unable to get virtual machines information. " + "Details:", e.message) + raise OperationFailed("GGBHOST0003E") class SoftwareUpdateProgressModel(object): diff --git a/src/wok/plugins/gingerbase/ui/js/src/gingerbase.host.js b/src/wok/plugins/gingerbase/ui/js/src/gingerbase.host.js index 0d52b92..77ee736 100644 --- a/src/wok/plugins/gingerbase/ui/js/src/gingerbase.host.js +++ b/src/wok/plugins/gingerbase/ui/js/src/gingerbase.host.js @@ -496,22 +496,21 @@ kimchi.host_main = function() { }; wok.confirm(settings, function() { - kimchi.shutdown(params); $(shutdownButtonID).prop('disabled', true); $(restartButtonID).prop('disabled', true); // Check if there is any VM is running. - // FIXME : Find alternative way to figure out if any vms running - // kimchi.listVMs(function(vms) { - // for(var i = 0; i < vms.length; i++) { - // if(vms[i]['state'] === 'running') { - // wok.message.error.code('GGBHOST6001E'); - // $(shutdownButtonID).prop('disabled', false); - // $(restartButtonID).prop('disabled', false); - // return; - // } - // } - // - // }); + // Based on the success will shutdown/reboot + kimchi.shutdown(params, function(success) { + wok.message.success(i18n['GGBHOST6009M']) + $(shutdownButtonID).prop('disabled', false); + $(restartButtonID).prop('disabled', false); + return; + }, function(error) { + // Looks like VMs are running. + wok.message.error.code('GGBHOST6001E'); + $(shutdownButtonID).prop('disabled', false); + $(restartButtonID).prop('disabled', false); + }); }, function() { }); }; diff --git a/src/wok/plugins/gingerbase/ui/pages/i18n.json.tmpl b/src/wok/plugins/gingerbase/ui/pages/i18n.json.tmpl index f6228ab..3f3b662 100644 --- a/src/wok/plugins/gingerbase/ui/pages/i18n.json.tmpl +++ b/src/wok/plugins/gingerbase/ui/pages/i18n.json.tmpl @@ -69,6 +69,7 @@ "GGBHOST6006M": "$_("Received")", "GGBHOST6007M": "$_("Sent")", "GGBHOST6008M": "$_("Shutting down or restarting host will cause unsaved work lost. Continue to shut down/restarting?")", + "GGBHOST6009M": "$_("The system is going down")", "GGBREPO6001M": "$_("Confirm")", @@ -113,6 +114,8 @@ "GGBDR6012M": "$_("Pending...")", "GGBDR6013M": "$_("Report name is the same as the original one.")", + "GGBHOST6001E": "$_("Unable to shut down system as there are some virtual machines running!")", + "GGBVM6001M": "$_("This will delete the virtual machine and its virtual disks. This operation cannot be undone. Would you like to continue?")", "GGBVM6002M": "$_("Power off Confirmation")", "GGBVM6003M": "$_("This action may produce undesirable results, " -- 2.1.0
On Fri, 2015-11-06 at 12:05 +0530, chandra@linux.vnet.ibm.com wrote:
From: chandrureddy <chandra@linux.vnet.ibm.com>
Review Comments taken care
Sorry, but what this mean? Again, send a commit message explaining what your patch is fixing and/or adding.
--- src/wok/plugins/gingerbase/i18n.py | 1 + src/wok/plugins/gingerbase/model/host.py | 47 +++++++++++++++------- .../gingerbase/ui/js/src/gingerbase.host.js | 25 ++++++------ src/wok/plugins/gingerbase/ui/pages/i18n.json.tmpl | 3 ++ 4 files changed, 49 insertions(+), 27 deletions(-)
diff --git a/src/wok/plugins/gingerbase/i18n.py b/src/wok/plugins/gingerbase/i18n.py index 8596f17..ef97266 100644 --- a/src/wok/plugins/gingerbase/i18n.py +++ b/src/wok/plugins/gingerbase/i18n.py @@ -39,6 +39,7 @@ messages = {
"GGBHOST0001E": _("Unable to shutdown host machine as there are running virtual machines"), "GGBHOST0002E": _("Unable to reboot host machine as there are running virtual machines"), + "GGBHOST0003E": _("There may be vms running on the host"),
All messages are using the term "virtual machine" instead of "vm".
"GGBHOST0005E": _("When specifying CPU topology, each element must be an integer greater than zero."),
"GGBPKGUPD0001E": _("No packages marked for update"), diff --git a/src/wok/plugins/gingerbase/model/host.py b/src/wok/plugins/gingerbase/model/host.py index 670fec5..766d6c7 100644 --- a/src/wok/plugins/gingerbase/model/host.py +++ b/src/wok/plugins/gingerbase/model/host.py @@ -39,6 +39,14 @@ from wok.plugins.gingerbase.repositories import Repositories from wok.plugins.gingerbase.swupdate import SoftwareUpdate
HOST_STATS_INTERVAL = 1 +DOM_STATE_MAP = {0: 'nostate', + 1: 'running', + 2: 'blocked', + 3: 'paused', + 4: 'shutdown', + 5: 'shutoff', + 6: 'crashed', + 7: 'pmsuspended'}
class HostModel(object): @@ -142,29 +150,40 @@ class HostModel(object):
def shutdown(self, args=None): # Check for running vms before shutdown - # FIXME : Find alternative way to figure out if any vms running - # running_vms = self._get_vms_list_by_state('running') - # if len(running_vms) > 0: - # raise OperationFailed("GGBHOST0001E") + running_vms = self.get_vmlist_bystate('running') + if len(running_vms) > 0: + raise OperationFailed("GGBHOST0001E")
wok_log.info('Host is going to shutdown.') os.system('shutdown -h now')
def reboot(self, args=None): - # Find running VMs - # FIXME : Find alternative way to figure out if any vms running - # running_vms = self._get_vms_list_by_state('running') - # if len(running_vms) > 0: - # raise OperationFailed("GGBHOST0002E") + # Check for running vms before reboot + running_vms = self.get_vmlist_bystate('running') + if len(running_vms) > 0: + raise OperationFailed("GGBHOST0002E")
wok_log.info('Host is going to reboot.') os.system('reboot')
- # def _get_vms_list_by_state(self, state): - # conn = self.conn.get() - # return [dom.name().decode('utf-8') - # for dom in conn.listAllDomains(0) - # if (DOM_STATE_MAP[dom.info()[0]]) == state] + def get_vmlist_bystate(self, state='running'): + try: + libvirt_mod = __import__('libvirt')
You are creating a new dependency here for GingerBase. Need update README.md, all contrib files, etc, as well do something similar to what Kimchi did here https://github.com/kimchi-project/kimchi/commit/cef3e5f dbac3443d3bb93e2ebd543ef38e07eda5 with service dependency.
+ except Exception, e: + wok_log.info("Unable to import libvirt module. Details:", + e.message)
Why wok_log.info()? Isn't it an error? I suggest to change to wok_log.error().
+ # Ignore any error and assume there is no vm running in the host + return [] + + try: + conn = libvirt_mod.open(None) + return [dom.name().decode('utf-8') + for dom in conn.listAllDomains(0) + if (DOM_STATE_MAP[dom.info()[0]] == state)] + except Exception, e: + wok_log.info("Unable to get virtual machines information. " + "Details:", e.message)
Same as above. Change to wok_log.error().
+ raise OperationFailed("GGBHOST0003E")
I got a little bit confuse here. First, you assume there is no vm running in the host and return an empty list in privous try-catch block. Then, here, if you can not get the list of VM's you raise an exception with a message totally different from the message you are logging in the instruction before. IMO, you should return here an empty list also. Let's try follow an use case: User request to shutdown/reboot the system, then you can not import libvirt module and return an empty list, that trigers the shutdown/reboot in shutdown() and reboot() methods, even if I have one, two or more VMs running. Let's assume now that libvirt could be imported, but for some reason, you could not check the VMs status. The user wants the machine off or reboot and could not execute this, because there are, may be, VMs running. Even if the user shutdown all VMs manually, and execute the action a second time, he/she doesn't be able to shutdown/reboot the machine, because the block can not check the VMs status. I see two options here: (1) you change the first try-catch block to raise the same error of this one; or (2) return here an empty list. My suggestion is for (2) - I'm the user and want to shutdown my machine. I don't care if there're VMs running. I want the machine off. :-D
class SoftwareUpdateProgressModel(object): diff --git a/src/wok/plugins/gingerbase/ui/js/src/gingerbase.host.js b/src/wok/plugins/gingerbase/ui/js/src/gingerbase.host.js index 0d52b92..77ee736 100644 --- a/src/wok/plugins/gingerbase/ui/js/src/gingerbase.host.js +++ b/src/wok/plugins/gingerbase/ui/js/src/gingerbase.host.js @@ -496,22 +496,21 @@ kimchi.host_main = function() { };
wok.confirm(settings, function() { - kimchi.shutdown(params); $(shutdownButtonID).prop('disabled', true); $(restartButtonID).prop('disabled', true); // Check if there is any VM is running. - // FIXME : Find alternative way to figure out if any vms running - // kimchi.listVMs(function(vms) { - // for(var i = 0; i < vms.length; i++) { - // if(vms[i]['state'] === 'running') { - // wok.message.error.code('GGBHOST6001E'); - // $(shutdownButtonID).prop('disabled', false); - // $(restartButtonID).prop('disabled', false); - // return; - // } - // } - // - // }); + // Based on the success will shutdown/reboot + kimchi.shutdown(params, function(success) { + wok.message.success(i18n['GGBHOST6009M']) + $(shutdownButtonID).prop('disabled', false); + $(restartButtonID).prop('disabled', false); + return; + }, function(error) { + // Looks like VMs are running. + wok.message.error.code('GGBHOST6001E'); + $(shutdownButtonID).prop('disabled', false); + $(restartButtonID).prop('disabled', false); + }); }, function() { }); }; diff --git a/src/wok/plugins/gingerbase/ui/pages/i18n.json.tmpl b/src/wok/plugins/gingerbase/ui/pages/i18n.json.tmpl index f6228ab..3f3b662 100644 --- a/src/wok/plugins/gingerbase/ui/pages/i18n.json.tmpl +++ b/src/wok/plugins/gingerbase/ui/pages/i18n.json.tmpl @@ -69,6 +69,7 @@ "GGBHOST6006M": "$_("Received")", "GGBHOST6007M": "$_("Sent")", "GGBHOST6008M": "$_("Shutting down or restarting host will cause unsaved work lost. Continue to shut down/restarting?")", + "GGBHOST6009M": "$_("The system is going down")",
"GGBREPO6001M": "$_("Confirm")", @@ -113,6 +114,8 @@ "GGBDR6012M": "$_("Pending...")", "GGBDR6013M": "$_("Report name is the same as the original one.")",
+ "GGBHOST6001E": "$_("Unable to shut down system as there are some virtual machines running!")", + "GGBVM6001M": "$_("This will delete the virtual machine and its virtual disks. This operation cannot be undone. Would you like to continue?")", "GGBVM6002M": "$_("Power off Confirmation")", "GGBVM6003M": "$_("This action may produce undesirable results, "
On 11/06/2015 04:37 PM, Paulo Ricardo Paz Vital wrote:
On Fri, 2015-11-06 at 12:05 +0530, chandra@linux.vnet.ibm.com wrote:
From: chandrureddy <chandra@linux.vnet.ibm.com>
Review Comments taken care Sorry, but what this mean? Again, send a commit message explaining what your patch is fixing and/or adding.
--- src/wok/plugins/gingerbase/i18n.py | 1 + src/wok/plugins/gingerbase/model/host.py | 47 +++++++++++++++------- .../gingerbase/ui/js/src/gingerbase.host.js | 25 ++++++------ src/wok/plugins/gingerbase/ui/pages/i18n.json.tmpl | 3 ++ 4 files changed, 49 insertions(+), 27 deletions(-)
diff --git a/src/wok/plugins/gingerbase/i18n.py b/src/wok/plugins/gingerbase/i18n.py index 8596f17..ef97266 100644 --- a/src/wok/plugins/gingerbase/i18n.py +++ b/src/wok/plugins/gingerbase/i18n.py @@ -39,6 +39,7 @@ messages = {
"GGBHOST0001E": _("Unable to shutdown host machine as there are running virtual machines"), "GGBHOST0002E": _("Unable to reboot host machine as there are running virtual machines"), + "GGBHOST0003E": _("There may be vms running on the host"), All messages are using the term "virtual machine" instead of "vm". Will take care.
"GGBHOST0005E": _("When specifying CPU topology, each element must be an integer greater than zero."),
"GGBPKGUPD0001E": _("No packages marked for update"), diff --git a/src/wok/plugins/gingerbase/model/host.py b/src/wok/plugins/gingerbase/model/host.py index 670fec5..766d6c7 100644 --- a/src/wok/plugins/gingerbase/model/host.py +++ b/src/wok/plugins/gingerbase/model/host.py @@ -39,6 +39,14 @@ from wok.plugins.gingerbase.repositories import Repositories from wok.plugins.gingerbase.swupdate import SoftwareUpdate
HOST_STATS_INTERVAL = 1 +DOM_STATE_MAP = {0: 'nostate', + 1: 'running', + 2: 'blocked', + 3: 'paused', + 4: 'shutdown', + 5: 'shutoff', + 6: 'crashed', + 7: 'pmsuspended'}
class HostModel(object): @@ -142,29 +150,40 @@ class HostModel(object):
def shutdown(self, args=None): # Check for running vms before shutdown - # FIXME : Find alternative way to figure out if any vms running - # running_vms = self._get_vms_list_by_state('running') - # if len(running_vms) > 0: - # raise OperationFailed("GGBHOST0001E") + running_vms = self.get_vmlist_bystate('running') + if len(running_vms) > 0: + raise OperationFailed("GGBHOST0001E")
wok_log.info('Host is going to shutdown.') os.system('shutdown -h now')
def reboot(self, args=None): - # Find running VMs - # FIXME : Find alternative way to figure out if any vms running - # running_vms = self._get_vms_list_by_state('running') - # if len(running_vms) > 0: - # raise OperationFailed("GGBHOST0002E") + # Check for running vms before reboot + running_vms = self.get_vmlist_bystate('running') + if len(running_vms) > 0: + raise OperationFailed("GGBHOST0002E")
wok_log.info('Host is going to reboot.') os.system('reboot')
- # def _get_vms_list_by_state(self, state): - # conn = self.conn.get() - # return [dom.name().decode('utf-8') - # for dom in conn.listAllDomains(0) - # if (DOM_STATE_MAP[dom.info()[0]]) == state] + def get_vmlist_bystate(self, state='running'): + try: + libvirt_mod = __import__('libvirt')
You are creating a new dependency here for GingerBase. Need update README.md, all contrib files, etc, as well do something similar to what Kimchi did here https://github.com/kimchi-project/kimchi/commit/cef3e5f dbac3443d3bb93e2ebd543ef38e07eda5 with service dependency. I guess I am not creating any dependency here. I am looking for some
I have sent a summary on my first patch sent. Any way will do it again. package and decide based on that. Core should work on a machine even with out libvirtd running. Scenario's considered: 1. Ginger Base can be installed on plain linux machine 2. Ginger Base can also be istalled on linux with KVM So my code will not break if libvirt is not available on the machine rather it indicates me that linux is running with out KVM capabilities. I do not think it required to change files as you mentioned above.
+ except Exception, e: + wok_log.info("Unable to import libvirt module. Details:", + e.message) Why wok_log.info()? Isn't it an error? I suggest to change to wok_log.error().
This is still for me not an error considering the case on linux with out KVM capabilities.
+ # Ignore any error and assume there is no vm running in the host + return [] + + try: + conn = libvirt_mod.open(None) + return [dom.name().decode('utf-8') + for dom in conn.listAllDomains(0) + if (DOM_STATE_MAP[dom.info()[0]] == state)] + except Exception, e: + wok_log.info("Unable to get virtual machines information. " + "Details:", e.message) Same as above. Change to wok_log.error(). This is still for me not an error considering the case on linux with out KVM capabilities.
+ raise OperationFailed("GGBHOST0003E") I got a little bit confuse here. First, you assume there is no vm running in the host and return an empty list in privous try-catch block. Then, here, if you can not get the list of VM's you raise an exception with a message totally different from the message you are logging in the instruction before.
IMO, you should return here an empty list also. Let's try follow an use case: User request to shutdown/reboot the system, then you can not import libvirt module and return an empty list, that trigers the shutdown/reboot in shutdown() and reboot() methods, even if I have one, two or more VMs running.
Let's assume now that libvirt could be imported, but for some reason, you could not check the VMs status. The user wants the machine off or reboot and could not execute this, because there are, may be, VMs running. Even if the user shutdown all VMs manually, and execute the action a second time, he/she doesn't be able to shutdown/reboot the machine, because the block can not check the VMs status.
I see two options here: (1) you change the first try-catch block to raise the same error of this one; or (2) return here an empty list. My suggestion is for (2) - I'm the user and want to shutdown my machine. I don't care if there're VMs running. I want the machine off. :-D
This was earlier implementation and I guess we are enforcing all the virtual machine's shut down before shutdown/reboot of the actual host. If libvirt is trouble for you here in the code then it is also trouble for you even shutdown of the vms the other areas libvirt way :-) Please let me know if we have a different opinion and we need to changes our code accordingly.
class SoftwareUpdateProgressModel(object): diff --git a/src/wok/plugins/gingerbase/ui/js/src/gingerbase.host.js b/src/wok/plugins/gingerbase/ui/js/src/gingerbase.host.js index 0d52b92..77ee736 100644 --- a/src/wok/plugins/gingerbase/ui/js/src/gingerbase.host.js +++ b/src/wok/plugins/gingerbase/ui/js/src/gingerbase.host.js @@ -496,22 +496,21 @@ kimchi.host_main = function() { };
wok.confirm(settings, function() { - kimchi.shutdown(params); $(shutdownButtonID).prop('disabled', true); $(restartButtonID).prop('disabled', true); // Check if there is any VM is running. - // FIXME : Find alternative way to figure out if any vms running - // kimchi.listVMs(function(vms) { - // for(var i = 0; i < vms.length; i++) { - // if(vms[i]['state'] === 'running') { - // wok.message.error.code('GGBHOST6001E'); - // $(shutdownButtonID).prop('disabled', false); - // $(restartButtonID).prop('disabled', false); - // return; - // } - // } - // - // }); + // Based on the success will shutdown/reboot + kimchi.shutdown(params, function(success) { + wok.message.success(i18n['GGBHOST6009M']) + $(shutdownButtonID).prop('disabled', false); + $(restartButtonID).prop('disabled', false); + return; + }, function(error) { + // Looks like VMs are running. + wok.message.error.code('GGBHOST6001E'); + $(shutdownButtonID).prop('disabled', false); + $(restartButtonID).prop('disabled', false); + }); }, function() { }); }; diff --git a/src/wok/plugins/gingerbase/ui/pages/i18n.json.tmpl b/src/wok/plugins/gingerbase/ui/pages/i18n.json.tmpl index f6228ab..3f3b662 100644 --- a/src/wok/plugins/gingerbase/ui/pages/i18n.json.tmpl +++ b/src/wok/plugins/gingerbase/ui/pages/i18n.json.tmpl @@ -69,6 +69,7 @@ "GGBHOST6006M": "$_("Received")", "GGBHOST6007M": "$_("Sent")", "GGBHOST6008M": "$_("Shutting down or restarting host will cause unsaved work lost. Continue to shut down/restarting?")", + "GGBHOST6009M": "$_("The system is going down")",
"GGBREPO6001M": "$_("Confirm")", @@ -113,6 +114,8 @@ "GGBDR6012M": "$_("Pending...")", "GGBDR6013M": "$_("Report name is the same as the original one.")",
+ "GGBHOST6001E": "$_("Unable to shut down system as there are some virtual machines running!")", + "GGBVM6001M": "$_("This will delete the virtual machine and its virtual disks. This operation cannot be undone. Would you like to continue?")", "GGBVM6002M": "$_("Power off Confirmation")", "GGBVM6003M": "$_("This action may produce undesirable results, "
participants (3)
-
Chandra Shehkhar Reddy Potula -
chandra＠linux.vnet.ibm.com
-
Paulo Ricardo Paz Vital