apps:glusterfs update volume hosts info during node deletion #1294
Conversation
|
Can one of the admins verify this patch? |
apps/glusterfs/app_node.go
Outdated
| @@ -320,6 +321,49 @@ func (a *App) NodeDelete(w http.ResponseWriter, r *http.Request) { | |||
| return err | |||
| } | |||
|
|
|||
| volumes, err := ListCompleteVolumes(tx) | |||
There was a problem hiding this comment.
This feels like it should be it's own function, it's fairly long and conceptually independent from the surrounding code. Something like refreshVolumeNodes(tx, clusterID)
apps/glusterfs/app_node.go
Outdated
| @@ -320,6 +321,49 @@ func (a *App) NodeDelete(w http.ResponseWriter, r *http.Request) { | |||
| return err | |||
| } | |||
|
|
|||
| volumes, err := ListCompleteVolumes(tx) | |||
There was a problem hiding this comment.
I think this should only iterate over the volumes in the cluster that was changed rather than all volumes.
apps/glusterfs/app_node.go
Outdated
| } | ||
| } | ||
| //update mountpoint info | ||
| if err = volEntry.updateMountInfo(txdb); err != nil { |
There was a problem hiding this comment.
I don't think this function will do what we want. Based on my reading of the code this may wipe out any other options in the map other than backup-volfile-servers.
Additionally, this function has the same problem as I mentioned above about repeatedly going to the db only to get the same results over and over again in a loop.
I suggest:
- Extracting (as you've already partially done) the inner parts of
updateMountInfointo new functions:- One that iterates over the nodes in the cluster and returns a list of the storage host names of those nodes. This function would be standalone and not attached to any struct.
- Make the latter half of updateMountInfo into a helper function that takes a list of hosts and sets/updates the hosts and Options.
- Call it from updateMountInfo and before the start of the loop over the volumes.
- Now the loop over the block volumes sets .Hosts to the new list
- The loop over the volumes uses the helper function to update (& not overwrite) the mountpoint and options.
apps/glusterfs/app_node.go
Outdated
| continue | ||
| } | ||
|
|
||
| if err = blockEntry.updateHosts(txdb); err != nil { |
There was a problem hiding this comment.
This strikes me as rather suboptimal. We're in a nested loop calling the same "db queries" over and over again in a transaction such that we're always going to get the same results. It strikes me as more correct to simply fetch the list of nodes for the cluster and re-use that list throughout the loop. See my comment on updateMountInfo for more details.
Madhu-1
force-pushed
the
update_host
branch
2 times, most recently
from
c4c303a
to
ec30060
Compare
during the node delete in heketi, we remove node from heketi management,volume info such as Hosts (which may contain the deleting node hostname). and we need to update the backup-volfile-servers and volume mountpoint (this also may contain the deleting node hostname) if it's a block hosting volume, we need to fetch all the block volumes belongs to it, and update the hosts in those volumes, so that next block volume creation fetches the new hosts from the heketi cluster. Signed-off-by: Madhu Rajanna <mrajanna@redhat.com>
|
@phlogistonjohn, addressed review comments, please check |
| } | ||
| return hosts, nil | ||
| } |
|
ok to test |
|
LGTM. |
What does this PR achieve? Why do we need it?
during the node delete in heketi, we remove the node from
heketi management, volume info such as Hosts (which may
contain the deleting node hostname). and we need to
update the backup-volfile-servers and volume mountpoint
(this also may contain the deleting node hostname)
if it's a block hosting volume, we need to
fetch all the block volumes belongs to it,
and update the hosts in those volumes, so that
next block volume creation fetches the new
hosts from the heketi cluster.
Signed-off-by: Madhu Rajanna mrajanna@redhat.com
Does this PR fix issues?
Fixes #1274
Fixes #1273
Notes for the reviewer