Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fence_compute: Fix disabling force_down on node when action is on #44

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

vuntz
Copy link
Contributor

@vuntz vuntz commented Feb 3, 2016

When the action is on, the goal is to disable force_down for the
nova-compute service on the node.

However, we were only doing that if the nova-compute service was up;
which is impossible if it's forced to be down... So just always disable
force_down, and then, if it's up, do more things.

@vuntz
Copy link
Contributor Author

vuntz commented Feb 3, 2016

Hrm, it's probably not correct as that would re-enable the nova-compute too early. We probably need to do that from the NovaCompute OCF agent, and add a condition here (like evacuate attribute unset or set to no).

Oh, actually it's probably correct because we use fabric_fencing. This still means the "on" action should be made from the NovaCompute OCF agent, though.

@beekhof
Copy link
Member

beekhof commented Jun 29, 2016

seems reasonable. @aspiers ?

When the action is on, the goal is to disable force_down for the
nova-compute service on the node.

However, we were only doing that if the nova-compute service was up;
which is impossible if it's forced to be down... So just always disable
force_down, and then, if it's up, do more things.
@vuntz
Copy link
Contributor Author

vuntz commented Dec 1, 2016

Rebased, cc'ing @krcmarik since he did 5964f95 not too long ago, but I feel it's not fully correct.

Regression introduced in 0f170a9
@aspiers
Copy link

aspiers commented Jan 18, 2018

@vuntz I don't follow the logic here, but this code has changed loads since then anyway, so I'm not sure if your concern is already addressed. Please can you check?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants