Gluster Service check and Remove device node selection #735
Conversation
|
Can one of the admins verify this patch? |
| } | ||
| _, err := s.RemoteExecutor.RemoteCommandExecute(host, commands, 10) | ||
| if err != nil { | ||
| logger.Err(err) |
MohamedAshiqrh
force-pushed
the
n4
branch
3 times, most recently
from
cbc67f3
to
7bdc07a
Compare
|
@raghavendra-talur Thanks for helping to test the PR and Review. :) |
There was a problem hiding this comment.
aproach looks good in general, but please address the comments. And maybe it should be mentioned in the commit message that due to choosing a node with glusterd running, the actual replacement code is largely simplified by removing the brick killing code.
Please generally improve the commit message: the first line does not really tell me what this patch is doing. a short highlevel description of the achievement should be the first line.
apps/glusterfs/node_entry.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| return nil |
There was a problem hiding this comment.
if you don't take any special action upon err != nil, you can just return err here?
apps/glusterfs/node_entry.go
Outdated
| if err != nil { | ||
| return err | ||
| } | ||
| return nil |
apps/glusterfs/node_entry.go
Outdated
| } | ||
| return nil | ||
| }) | ||
|
|
There was a problem hiding this comment.
what's happening with err here? no checking?
apps/glusterfs/node_entry.go
Outdated
| } | ||
| return nil | ||
| }) | ||
| // Ignore if the node is not online |
apps/glusterfs/node_entry.go
Outdated
| @@ -68,6 +69,49 @@ func (n *NodeEntry) registerStorageKey(host string) string { | |||
| return "STORAGE" + host | |||
| } | |||
|
|
|||
| // Verify gluster process in the node and return the manage hostname of a node in the cluster | |||
| func GetNode(db *bolt.DB, e executors.Executor, clusterId string) (string, error) { | |||
There was a problem hiding this comment.
maybe a slightly longer and more explanatory function name would be good?
|
@MohamedAshiqrh additionally, the new GetNodes code would need tests. |
MohamedAshiqrh
force-pushed
the
n4
branch
2 times, most recently
from
5aed141
to
9e7ae98
Compare
|
@obnoxxx added tests and changed the name of the function. |
GetVerifiedManageHostname will return the node's manage hostname which is verifed for the glusterd service to be running. We need not kill the old gluster brick process, hence cleaning up. This also removed the requirement of using the same node of device to be removed. This enables device remove to verify the node is up or not and then choose a different node for replace brick command. Signed-off-by: Mohamed Ashiq Liyazudeen <mliyazud@redhat.com>
|
@centos-ci test |
|
@centos-ci ok to test and your pickyness about the words is really really irritating! simply "test" should be sufficient imho... |
|
LGTM |
GetNode will return the node's manage hostname when called for a
clusterID. This hostname is verifed for the glusterd service to be
running.
This enables device remove to verify the node is up or not and then
choose a different node for replace brick command.
Signed-off-by: Mohamed Ashiq Liyazudeen mliyazud@redhat.com