gluster / gluster-block

A framework for gluster block storage
GNU General Public License v2.0
74 stars 32 forks source link

info-cmd: add list of resize failed nodes #271

Closed pkalever closed 4 years ago

pkalever commented 4 years ago

What does this PR achieve? Why do we need it?

$ gluster-block modify blockhostingvol/blockvol size 4GiB --json-pretty
{
  "IQN":"iqn.2016-12.org.gluster-block:7a4938b1-f153-4de7-b91a-04c34fb57f36",
  "FAILED ON":[
    "192.168.121.55",
    "192.168.121.206"
  ],
  "SUCCESSFUL ON":[
    "192.168.121.253"
  ],
  "RESULT":"FAIL"
}

$ gluster-block info blockhostingvol/blockvol
NAME: blockvol
VOLUME: blockhostingvol
GBID: 7a4938b1-f153-4de7-b91a-04c34fb57f36
SIZE: 4.0 GiB
HA: 3
IOTIMEOUT: 0 Seconds
PASSWORD:
EXPORTED ON: 192.168.121.253 192.168.121.55 192.168.121.206
RESIZE FAILED ON: 192.168.121.55:[2.0 GiB] 192.168.121.206:[1.0 GiB]

$ gluster-block info blockhostingvol/blockvol --json-pretty
{
  "NAME":"blockvol",
  "VOLUME":"blockhostingvol",
  "GBID":"7a4938b1-f153-4de7-b91a-04c34fb57f36",
  "SIZE":"4.0 GiB",
  "HA":3,
  "IOTIMEOUT":"0 Seconds",
  "PASSWORD":"",
  "EXPORTED ON":[
    "192.168.121.253",
    "192.168.121.55",
    "192.168.121.206"
  ],
  "RESIZE FAILED ON":{
    "192.168.121.55":"2.0 GiB",
    "192.168.121.206":"1.0 GiB"
  }
}

Notes for the reviewer

This is needed by heketi for expand future integration.

Signed-off-by: Prasanna Kumar Kalever \prasanna.kalever@redhat.com\

pkalever commented 4 years ago

@phlogistonjohn @raghavendra-talur FYI. Just in case if you want to see any additional details.

lxbsz commented 4 years ago

@pkalever BTW, does add the old size for failing resize nodes make sense here ?

pkalever commented 4 years ago

@pkalever BTW, does add the old size for failing resize nodes make sense here ?

@lxbsz make sense, done now.

Thanks!

pkalever commented 4 years ago

@lxbsz Thanks for testing this, let's wait for a day on this, I would like to hear back from @phlogistonjohn @raghavendra-talur

raghavendra-talur commented 4 years ago

I have not looked at the code.

The data in the output looks good and sufficient. However it does not seem like "RESIZE FAILED ON" fits in the info structure. Taking inspiration from the kuberentes PV info yaml, I think if you keep a status field in the output then you can have a value something along the lines of needs resizing and then a per node message.

"status": {
       "message": {
           "10.72.36.245": "14.0 GiB"
        }
        "phase": "Needs Resize"
}

Only take that suggestion if it is doable and if it also applies well to other workflows in gluster-block, like create, delete etc.

pkalever commented 4 years ago

Only take that suggestion if it is doable and if it also applies well to other workflows in gluster-block, like create, delete etc.

@raghavendra-talur Thanks! I went back and checked the code, and this is not only the place using such kind of naming, its in gluster-block genes :-)

rpc/block_create.c:                                    "ROLLBACK FAILED ON");
rpc/block_create.c:      if (GB_ASPRINTF(&tmp, "ROLLBACK FAILED ON: %s\n",
rpc/block_info.c:      json_object_object_add(json_obj, "RESIZE FAILED ON", json_obj2);
rpc/block_info.c:      if (GB_ASPRINTF(&tmp, "%s\nRESIZE FAILED ON:%s", tmp4, rsf_nodes) == -1) {
rpc/block_modify.c:                                   "ROLLBACK FAILED ON");
rpc/block_modify.c:        GB_ASPRINTF(&tmp, "ROLLBACK FAILED ON: %s\n", savereply->rb_attempt);
rpc/block_replace.c:                                     "REPLACE PORTAL FAILED ON");
rpc/block_replace.c:          if (GB_ASPRINTF(&entry, "%sREPLACE PORTAL FAILED ON: %s\n", tmp?tmp:"",

Hence for consistency purpose many be we should continue using it :-)

pkalever commented 4 years ago

@lxbsz refreshed with the changes discussed, please take a look. Thanks!

pkalever commented 4 years ago

Thanks @lxbsz, merging now!