I had a few problems when testing this patchset. Below is the sequence of commands I tried, their outputs and my opinions. I'm running Fedora 21.

First, I tried to query the current VGs on my system:

GET /host/vgs
[
  {
    "path":"/dev/fedora",
    "name":"fedora",
    "pool":"",
    "size":"19,51g"
  }
]

That is the default VG created by the Fedora installer. The field "size" contains "19,51g" and I think that's not a desirable output due to these two reasons:

1) The value contains a unit. No other API on Kimchi returns a size value with a unit, the return unit is always documented. For example, in docs/API.md there is the following description for /storagepools/<pool>: "capacity: The total space which can be used to store volumes. The unit is Bytes". Notice how the unit is specified on the documentation. This makes it easy for the user to parse the output value because they always know which unit the value will be in. On Kimchi, we have the function "kimchi.utils.convert_data_size" which converts values from any unit to any other unit. I suggest we use bytes as the VG size unit as it is also the unit used elsewhere.

2) The value contains a comma instead of a dot. Probably that's because my system is using a Brazilian Portuguese locale (for context: in Brazilian Portuguese we use commas instead of dots to split decimal values, e.g. "1,5" = one and a half, unlike in English, where they use "1.5"). We shouldn't output different values based on the system's locale, otherwise, the user will never know how to parse the server's output. If we use bytes as the unit as suggested above, we wouldn't even need to use a decimal separator here, because we don't deal with values less than one byte anyway.

Both of the problems above are probably related to the output of "vgs". We should normalize that output so the user always know what to expect.

Another problem I'm having here is that I can't filter the VGs based on the field 'pool':

GET /host/vgs?pool='x'
[
  {
    "path":"/dev/fedora",
    "name":"fedora",
    "pool":"",
    "size":"19,51g"
  }
]

If I understand correctly, I should get an empty list here because I filtered for "pool=x", and the result doesn't have "pool=x".

Then I created a new VG using "vgcreate" and I queried Kimchi again:

GET /host/vgs
[
  {
    "path":"/dev/fedora",
    "name":"fedora",
    "pool":"",
    "size":"19,51g"
  },
  {
    "path":"/dev/kimchivg",
    "name":"kimchivg",
    "pool":"",
    "size":"5,00g"
  }
]

And then I created a new storage pool with that VG:

POST /storagepools/ '{"path": "kimchivg", "name": "kimchi-logical" "type": "logical"}'
{
  "available":0,
  "nr_volumes":0,
  "path":"/dev/kimchi-logical",
  "allocated":0,
  "capacity":0,
  "name":"kimchi-logical",
  "persistent":true,
  "source":{},
  "state":"inactive",
  "autostart":false,
  "type":"logical"
}

However, if I query the VGs again, "kimchivg" still doesn't report that it now belongs to "kimchi-logical":

GET /host/vgs?name='kimchivg'
[
  {
    "path":"/dev/fedora",
    "name":"fedora",
    "pool":"",
    "size":"19,51g"
  },
  {
    "path":"/dev/kimchivg",
    "name":"kimchivg",
    "pool":"",
    "size":"5,00g"
  }
]

Notice how I couldn't filter by the VG name as well. And "pool" in "kimchivg" is empty. As far as I understand, it should contain "kimchi-logical" now.

And lastly, when I tried to activate that new storage pool, I got an error:

POST /storagepools/kimchi-logical/activate
{
  "reason":"KCHPOOL0009E: Unable to activate storage pool kimchi-logical. Details: internal error: Child process (/usr/sbin/vgchange -aly kimchi-logical) unexpected exit status 5:   Volume group \"kimchi-logical\" not found\n  Cannot process volume group kimchi-logical\n",
  "code":"500 Internal Server Error",
  "call_stack":"Traceback (most recent call last):\n  File \"/usr/lib/python2.7/site-packages/cherrypy/_cprequest.py\", line 656, in respond\n    response.body = self.handler()\n  File \"/usr/lib/python2.7/site-packages/cherrypy/lib/encoding.py\", line 188, in __call__\n    self.body = self.oldhandler(*args, **kwargs)\n  File \"/usr/lib/python2.7/site-packages/cherrypy/_cpdispatch.py\", line 34, in __call__\n    return self.callable(*self.args, **self.kwargs)\n  File \"/home/kimchi/kimchi/src/kimchi/control/base.py\", line 129, in wrapper\n    raise cherrypy.HTTPError(500, e.message)\nHTTPError: (500, u'KCHPOOL0009E: Unable to activate storage pool kimchi-logical. Details: internal error: Child process (/usr/sbin/vgchange -aly kimchi-logical) unexpected exit status 5:   Volume group \"kimchi-logical\" not found\\n  Cannot process volume group kimchi-logical\\n')\n"

By looking at that error message, I guess Kimchi is assuming that "kimchi-logical" is the VG name. Currently, when Kimchi creates the VGs before creating the logical storage pools, we use the VG name the same as the pool name. With this patchset, that may not be the case anymore; I have a VG named "kimchivg" and a storage pool named "kimchi-logical", and I believe that's the root of this error here.

Please correct me if I misunderstood something about this new patchset. As this is still a new feature, I may have assumed something wrong.

On 12-04-2015 11:13, lvroyce@linux.vnet.ibm.com wrote:
From: Royce Lv <lvroyce@linux.vnet.ibm.com>


Royce Lv (6):
  Storagepool on existent VG: Update document
  Add helper function to retrieve vg information
  Update controller
  Update model for existent VG
  Update mockmodel tests
  Fix fh leak in run_command subprocess

 docs/API.md                            | 20 ++++++++++++++++
 src/kimchi/control/host.py             |  6 +++++
 src/kimchi/i18n.py                     |  2 ++
 src/kimchi/mockmodel.py                |  9 ++++++++
 src/kimchi/model/libvirtstoragepool.py | 11 +++++----
 src/kimchi/model/storagepools.py       | 28 +++++++++++++----------
 src/kimchi/model/vgs.py                | 42 ++++++++++++++++++++++++++++++++++
 src/kimchi/utils.py                    | 22 ++++++++++++++++--
 tests/test_mock_storagepool.py         |  9 +++++++-
 9 files changed, 130 insertions(+), 19 deletions(-)
 create mode 100644 src/kimchi/model/vgs.py