fence_pve: Replace `nodename` with `pmx-node` in fence_pve.py (matching original intent)

ClusterLabs / fence-agents

Fence agents

104 stars 160 forks source link

fence_pve: Replace `nodename` with `pmx-node` in fence_pve.py (matching original intent) #424

Closed broadstack-au closed 3 years ago

broadstack-au commented 3 years ago

refactor to suit original intent of internal nodename logic.

Change;

pmx-node option is used to specify the proxmox cluster node controlling the vm
nodename option indicates the "node to fence"

fence_pve relies on plug for node actions and never nodename - this is unchanged

Current behaviour; If nodename is not provided to the fence agent, autodiscovery will fail during fencing activities.

The "watch/list" behaviours would execute correctly, (due to no nodename provided for those actions).
Status and all node-focused actions would fail due to nodename being the cluster node, not the proxmox node.

New behaviour;

If pmx-node is provided to the fence agent, it will use that node
if pmx-node is not provided to the fence agent, it will discover and set it correctly

This will break existing implementations that specify the node name - I'm open to suggestions for how to deal with that

knet-ci-bot commented 3 years ago

Can one of the admins verify this patch?

oalbrigt commented 3 years ago

We have to keep nodename, with comment "Replaced by " to avoid breaking setups for current users: https://github.com/ClusterLabs/fence-agents/blob/master/agents/mpath/fence_mpath.py#L232-L239 https://github.com/ClusterLabs/fence-agents/blob/master/agents/mpath/fence_mpath.py#L287-L294

You can run make xml-upload to update the the metadata files.

Commits should be prefixed by the agent name.

broadstack-au commented 3 years ago

Okay, I'm as happy with this as I think I'll be,

nodename is getting injected by fenced as "the node to fence" in some actions (because it should - that's what nodename is for), but it's being (mis)used in fence_pve to specify the specific proxmox cluster node that the vm is running on. This means It can't be cleanly replaced without inheriting the old runtime behaviour.

Given cluster node discovery is broken currently (fencing will fail if the nodename parameter is not specified in the fencing config), I've introduced pve-node as a replacement input parameter for nodename and pve-node-auto to force autodiscovery.

Those using the old nodename param (incl shortform N) won't see a behaviour change. Updated/new implementations can choose to use pve-node or pve-node-auto.

Ready for re-review, thanks :)

oalbrigt commented 3 years ago

ok to test

oalbrigt commented 3 years ago

You need to run make xml-upload and attach the updated metadata to the commit, so it can pass the CI tests.

broadstack-au commented 3 years ago

You need to run make xml-upload and attach the updated metadata to the commit, so it can pass the CI tests.

sorry about that - forgot to rerun after changing ordering. fixed.

oalbrigt commented 3 years ago

No worries. Now we just need to fix the CI side of things :smiley:

oalbrigt commented 3 years ago

retest this please

oalbrigt commented 3 years ago

LGTM. Thanks.