rpower support utilizing the python framework for OpenBMC #4587

xuweibj · 2017-12-27T07:36:06Z

UT:

# rpower f6u17 state
f6u17: on

UT with "xcatdebugmode" is 1:

# rpower f6u17 bmcreboot
Wed Dec 27 02:08:47 2017 f6u17: [openbmc_debug] login curl -k -c cjar -b cjar -X POST -H "Content-Type: application/json" https://10.6.17.100/login -d '{"data": ["root", "xxxxxx"]}'
Wed Dec 27 02:08:47 2017 f6u17: [openbmc_debug] login 200 OK
Wed Dec 27 02:08:47 2017 f6u17: [openbmc_debug] rpower_bmcreboot curl -k -c cjar -b cjar -X PUT -H "Content-Type: application/json" https://10.6.17.100/xyz/openbmc_project/state/bmc0/attr/RequestedBMCTransition -d '{"data": "xyz.openbmc_project.State.BMC.Transition.Reboot"}'
Wed Dec 27 02:08:47 2017 f6u17: [openbmc_debug] rpower_bmcreboot 200 OK
f6u17: BMC reboot

UT when password is wrong:

# rpower openbmctest state
openbmctest: Error: [401] Invalid username or password

500 all timeout 47.763s (Timeout is 30s)
500 all ok 22.039s
500 all 503 21.692s

xcatbot · 2017-12-27T07:48:15Z

CI CHECK RESULT : > PR FORMAT CORRECT> BUILD SUCCESSFUL > INSTALL XCAT SUCCESSFUL> CODE SYNTAX CORRECT> FAST REGRESSION TEST Successful: Totalcase 215 Pass 215 failed 0

chenglch · 2017-12-27T07:56:54Z

xCAT-server/lib/python/agent/xcatagent/openbmc.py

@@ -1,26 +1,223 @@
 from xcatagent import base
 import gevent
+import gevent.monkey
+gevent.monkey.patch_all()


You'd better put this in the __init__.py, not sure if this would affect the keep_peer_alive method.

chenglch · 2017-12-27T07:58:35Z

xCAT-server/lib/python/agent/client.py

               'cwd': os.getcwd(),
               'nodes': nodes,
-               'nodeinfo': nodeinfo}
+               'nodeinfo': nodeinfo,
+               'debugmode': '1'}


If multiple levels , integer should be used, otherwise boolean value would be better.

chenglch · 2017-12-27T07:59:53Z

xCAT-server/lib/python/agent/xcatagent/openbmc.py

+import openbmc_rest
+
+http_protocol="https://";
+openbmc_project_url = "/xyz/openbmc_project";


In python, if it is a constant value, upper case should be used.

chenglch · 2017-12-27T08:04:55Z

xCAT-server/lib/python/agent/xcatagent/openbmc.py

+}
+
+subcommand_tup_dict = {
+    "rpower_set_onoff" : ['on', 'off', 'bmcreboot', 'softoff'],


upper case

tuple is better than list here.

No need to use subcommand_tup_dict, you are writing openbmc module, a class to provide service, not command handler, I prefer the following:

POWER_STATE = ('on', 'off', 'bmcreboot', 'softoff')

chenglch · 2017-12-27T08:09:17Z

xCAT-server/lib/python/agent/xcatagent/openbmc.py

+http_protocol="https://";
+openbmc_project_url = "/xyz/openbmc_project";
+
+command_dict = {


Only method, no command here.
I think it's better to put the url and the data field in the method.

def _power_on(): url = PROJECT_URL + "/state/host0/attr/RequestedHostTransition" field = "xyz.openbmc_project.State.Host.Transition.On"

You could also rename this dict like RPOWER_URLS = { on: {...}, off:...}, init_url is not a proper name here.

chenglch · 2017-12-27T08:10:38Z

xCAT-server/lib/python/agent/xcatagent/openbmc_rest.py

@@ -0,0 +1,85 @@
+#!/usr/bin/env python


I think the file name should be rest.py, make it as a common module. Add get, post, put, delete, patch method may be more friendly.

chenglch · 2017-12-27T08:24:11Z

xCAT-server/lib/python/agent/xcatagent/openbmc.py

-        gevent.joinall(lst)
-
+    def rpower(self, nodeinfo, args, debugmode):
+        glist = [ gevent.spawn(self.handler, node, nodeinfo[node], 'rpower', args, debugmode) for node in self.nodes ]


Actually your OpenBMC class is worked like the original OpenBMC driver.
You should create a list of driver(OpenBMC) objects, then create gevent objects to call the driver method(rpower). Like below:

objs = [OpenBMC(node, nodeinfo[node]) for node in self.nodes] glist = [ gevent.spawn(obj.rpower, args, debugmode) for obj in objs]

chenglch · 2017-12-27T08:27:13Z

xCAT-server/lib/python/agent/xcatagent/openbmc.py

-        super(OpenBMCDriver, self).__init__(messager)
+    def handler(self, node, node_info, command, args, debugmode): 
+        openbmc = OpenBMC(node, node_info, self.messager, debugmode)
+        getattr(openbmc,command)(args)


Call the rpower method directly, handler is not a meaningful method name compare with the behavior like rpower

chenglch · 2017-12-27T08:43:55Z

xCAT-server/lib/python/agent/xcatagent/openbmc.py

+
+        if subcommand == 'bmcstate' :
+            if bmc_current_state == 'Ready' :
+                return 'BMC Ready'


Put the message in a constant dict, the return the message based on that dict.

chenglch · 2017-12-27T08:45:57Z

xCAT-server/lib/python/agent/xcatagent/openbmc.py

+    '''
+    def rpower(self, args) :
+        subcommand = args[0]
+        self._login()


I think _login() method should return something or raise some exception, it could not be success all the time.

chenglch · 2017-12-27T09:14:59Z

xCAT-server/lib/python/agent/xcatagent/openbmc.py

+        login_data = { "data": [ self.username, self.password ] }
+        self.client.request('POST', login_url, OpenBMC.headers, login_data, self.node, 'login')
+
+    '''


Take the docstring from pyghmi and ironic as a example

def _got_session(self, response): """Private function to navigate SOL payload activation """ if 'error' in response: self._print_error(response['error']) return #Send activate sol payload directive #netfn= 6 (application) #command = 0x48 (activate payload) #data = (1, sol payload type # 1, first instance # 0b11000000, -encrypt, authenticate, # disable serial/modem alerts, CTS fine # 0, 0, 0 reserved response = self.ipmi_session.raw_command(netfn=0x6, command=0x48, data=(1, 1, 192, 0, 0, 0)) #given that these are specific to the command, #it's probably best if one can grep the error #here instead of in constants sol_activate_codes = { 0x81: 'SOL is disabled', 0x82: 'Maximum SOL session count reached', 0x83: 'Cannot activate payload with encryption', 0x84: 'Cannot activate payload without encryption', }

From ironic which list the parameters

def update_volume_target(self, context, target): """Update a volume target. :param context: request context :param target: a changed (but not saved) volume target object :returns: an updated volume target object :raises: InvalidParameterValue if the volume target's UUID is being changed :raises: NodeLocked if the node is already locked :raises: NodeNotFound if the node associated with the volume target does not exist :raises: VolumeTargetNotFound if the volume target cannot be found :raises: VolumeTargetBootIndexAlreadyExists if a volume target already exists with the same node ID and boot index values """ LOG.debug("RPC update_volume_target called for target %(target)s.", {'target': target.uuid})

chenglch · 2017-12-29T09:17:52Z

xCAT-server/lib/python/agent/xcatagent/xcat_exception.py

@@ -0,0 +1,7 @@
+#!/usr/bin/env python
+
+class SelfServerException(Exception) :


Define the exception like this is meaningless, you could refer the design of openstack nova https://github.com/openstack/nova/blob/master/nova/exception.py to make the error message more standard.
When the exception is captured, we could call messager.error(exception.msg) to send the error message back to the user directly.

But I do not want to show the message to the user directly. Whether show message should method decide.

chenglch · 2017-12-29T09:23:15Z

xCAT-server/lib/python/agent/xcatagent/openbmc.py

+        :returns: on/off/softoff/BMC reboot if success
+        :raise: error message if failed
+        """
+        request_url = HTTP_PROTOCOL + self.bmcip + RPOWER_URLS[subcommand]['url']


I prefer url, data directly, the prefix of request_ is meaningless here.

chenglch · 2017-12-29T09:31:33Z

xCAT-server/lib/python/agent/xcatagent/openbmc.py

-            gevent.sleep(3)
-        self.messager.info(
-            "%s: rpower called info=%s args=%s" % (node, info, args))
+        super(OpenBMCManager, self).process_nodes_worker('openbmc', 'OpenBMC', self.nodes, nodeinfo, 'rpower', args, self.debugmode)


You misunderstand again.
OpenBMCManager is to manage the collection of nodes. It call the method of OpenBMC inside the coroutine.
OpenBMC class is for one node, you should make it work without the OpenBMCManager.

In short, manager is a scheduler, OpenBMCDriver is a class to handle the business logic.

chenglch

Append my concern inline. When you think it's done, you could merge it.

chenglch · 2018-01-03T05:59:50Z

xCAT-server/lib/python/agent/xcatagent/openbmc.py

-            gevent.sleep(3)
-        self.messager.info(
-            "%s: rpower called info=%s args=%s" % (node, info, args))
+        super(OpenBMCManager, self).process_nodes_worker('openbmc', 'OpenBMC', self.nodes, nodeinfo, 'rpower', args)


My concern is that you use reflect again which is unnecessary. Why not to just create the openbmc objects, and trasfer the rpower func directly?

robin2008 · 2018-01-03T08:24:30Z

xCAT-server/lib/python/agent/client.py

-                    nodes}
+        nodes = ['node1', 'node2']
+        nodeinfo = {'node1':{'bmc':'9.3.185.161', 'bmcip':'9.3.185.161', 'username':'root', 'password': '0penBmc1'},
+                    'node2':{'bmc':'10.6.17.100', 'bmcip': '10.6.17.100', 'username':'root', 'password': '0penBmc'}}



Don't put the the real IP into public repository, even it is for UT.

robin2008 · 2018-01-03T08:26:15Z

xCAT-server/lib/python/agent/client.py

               'cwd': os.getcwd(),
               'nodes': nodes,
-               'nodeinfo': nodeinfo}
+               'nodeinfo': nodeinfo,
+               'debugmode': 1}


better to use envs to cover debugmode, as we may have more variables which could passed into python client.

robin2008 · 2018-01-03T09:21:46Z

xCAT-server/lib/python/agent/xcatagent/server.py

@@ -86,9 +86,9 @@ def _handle(self, sock, address):
                messager.error("Could not find manager for %s" % req['module'])
                return
            nodes = req.get("nodes", None)
-            manager = manager_func(messager, req['cwd'], nodes)
+            manager = manager_func(messager, req['cwd'], nodes, req['envs'])


@chenglch Maybe we can cache the whole req dict in manager, I wonder we may have more variables in req from perl.

@robin2008 , dict is too flexible, maybe we need a context class to help store the global attributes like this in the future.

whowutwut · 2018-01-03T20:12:55Z

xCAT-server/lib/python/agent/client.py

-                    in
-                    nodes}
+        nodes = ['node1', 'node2']
+        nodeinfo = {'node1':{'bmc':'10.0.0.1', 'bmcip':'10.0.0.1', 'username':'root', 'password': 'xxxxxx'},


What are this used for? nodes and nodeinfo ?

The client.py is used to debug python part code, "nodes" and "nodeinfo" are got from perl script and used in python script. We could modify here instead of running perl code to generate data for test.

whowutwut · 2018-01-03T20:28:40Z

Before we allow this to merge this have a few questions:

The error message is not the same with the Perl support:

[root@stratton01 ~]# rpower p9euh01 state
p9euh01: Error: [401] Invalid username or password

How do we toggle between currently mgt=openbmc and this new one?
How does this fit into RedFish support... I see a lot is defined in openbmc.py, could we create a base class that OpenBMC and RedFish could both be derived from...
Can you show me how we can call this utility outside of xCAT? It would make it dual purpose...
- This would lead us to package this python module in another rpm or standard python pkg... and the user would install it and get openbmc python support instead of having it combined into xCAT,
Then some more internal discussion questions based on the above...
- integrate option parsing using docopt? http://docopt.org/
- Consider using python package management or structure the layout so we could package with pip?
The initial idea I had was to have this as a "stand alone" python application that xCAT would call it's API with --raw to get JSON data returned and consume it as a JSON parser... Then users could code to it or we could use it stand alone outside xCAT. Let's discuss a little tonight.

xuweibj · 2018-01-04T05:22:56Z

Hi @whowutwut ,
For 1, I have modified the error msg. But now python will not return 503 or 504 error, so just print msg like "Error: Unable to connect to server", "Error: Timeout to connect to server".

For 2, For "rpower" command, before release, we will check mgt if "mgt=openbmc2", run python script. If release, will not run perl version again.

For 3, we can create "RedFish" class if we needed. In init.py will check the "module" to mapping class.

For 4, we just could run python script for debug.
Open 2 terminals:
one run ./agent.py --standalone --sock agent.sock
one run ./client.py --sock agent.sock
all under "/opt/xcat/lib/python/agent"

whowutwut · 2018-01-04T19:49:39Z

@robin2008 whomever merges, please set the current milestone at the time of the merge so that it accurately reflects the correct release that the code went into and leverage GitHub queries

robin2008 · 2018-01-05T04:49:17Z

Now it is still in the dev branch, and we will see the test result to decide if it is okay to in master branch. And for how to be delivered in master (just in xcat-server or separate package), it is still under discussion.

xuweibj added the component:openbmc label Dec 27, 2017

xuweibj requested a review from chenglch December 27, 2017 07:36

xuweibj force-pushed the rpower_python branch from 80fa3b6 to 043e27e Compare December 27, 2017 07:37

chenglch suggested changes Dec 27, 2017

View reviewed changes

chenglch reviewed Dec 27, 2017

View reviewed changes

whowutwut changed the title ~~rpower work on python for OpenBMC~~ rpower support utilizing the python framework for OpenBMC Dec 28, 2017

xuweibj force-pushed the rpower_python branch 2 times, most recently from 4dba9d4 to 65ffc75 Compare December 29, 2017 08:57

chenglch suggested changes Dec 29, 2017

View reviewed changes

xuweibj force-pushed the rpower_python branch from 65ffc75 to 9538a13 Compare January 2, 2018 02:18

rpower work on python for OpenBMC

9028e31

xuweibj force-pushed the rpower_python branch from 9538a13 to 9028e31 Compare January 2, 2018 07:02

chenglch reviewed Jan 3, 2018

View reviewed changes

chenglch approved these changes Jan 3, 2018

View reviewed changes

robin2008 reviewed Jan 3, 2018

View reviewed changes

modified depending on comments

6065cc3

robin2008 reviewed Jan 3, 2018

View reviewed changes

whowutwut reviewed Jan 3, 2018

View reviewed changes

modify error output

6183ec2

xuweibj force-pushed the rpower_python branch from 4f162db to 6183ec2 Compare January 4, 2018 05:20

robin2008 merged commit 744500b into xcat2:openbmc_python Jan 4, 2018

xuweibj mentioned this pull request Jan 4, 2018

[OpenBMC] rpower support with Python and coroutine #4554

Closed

3 tasks

whowutwut added this to the 2.13.10 milestone Jan 4, 2018

xuweibj deleted the rpower_python branch January 5, 2018 00:44

chenglch mentioned this pull request Jan 19, 2018

rflash for openbmc on python, including options a|c|l|delete|u #4645

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rpower support utilizing the python framework for OpenBMC #4587

rpower support utilizing the python framework for OpenBMC #4587

xuweibj commented Dec 27, 2017 •

edited

xcatbot commented Dec 27, 2017 •

edited

chenglch Dec 27, 2017

chenglch Dec 27, 2017

chenglch Dec 27, 2017

chenglch Dec 27, 2017

chenglch Dec 27, 2017

chenglch Dec 27, 2017

chenglch Dec 27, 2017

chenglch Dec 27, 2017

chenglch Dec 27, 2017

chenglch Dec 27, 2017 •

edited

chenglch Dec 27, 2017 •

edited

chenglch Dec 29, 2017

xuweibj Jan 2, 2018

chenglch Dec 29, 2017

chenglch Dec 29, 2017 •

edited

chenglch left a comment

chenglch Jan 3, 2018

robin2008 Jan 3, 2018

robin2008 Jan 3, 2018

robin2008 Jan 3, 2018

chenglch Jan 3, 2018

whowutwut Jan 3, 2018

xuweibj Jan 4, 2018

whowutwut commented Jan 3, 2018 •

edited

xuweibj commented Jan 4, 2018

whowutwut commented Jan 4, 2018

robin2008 commented Jan 5, 2018

		@@ -0,0 +1,7 @@
		#!/usr/bin/env python

		class SelfServerException(Exception) :

rpower support utilizing the python framework for OpenBMC #4587

rpower support utilizing the python framework for OpenBMC #4587

Conversation

xuweibj commented Dec 27, 2017 • edited

xcatbot commented Dec 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenglch Dec 27, 2017 • edited

Choose a reason for hiding this comment

chenglch Dec 27, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chenglch Dec 29, 2017 • edited

Choose a reason for hiding this comment

chenglch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

whowutwut commented Jan 3, 2018 • edited

xuweibj commented Jan 4, 2018

whowutwut commented Jan 4, 2018

robin2008 commented Jan 5, 2018

xuweibj commented Dec 27, 2017 •

edited

xcatbot commented Dec 27, 2017 •

edited

chenglch Dec 27, 2017 •

edited

chenglch Dec 27, 2017 •

edited

chenglch Dec 29, 2017 •

edited

whowutwut commented Jan 3, 2018 •

edited