Fixes #39067 - rebuild only if host powered off#10860
Fixes #39067 - rebuild only if host powered off#10860sbernhard wants to merge 1 commit intotheforeman:developfrom
Conversation
Use theforeman/foreman#10860 and refactor the vsphere/proxmox methods as it is no longer needed to start/stop/reboot
sbernhard
force-pushed
the
rebuild_requires_poweroff
branch
3 times, most recently
from
0387d0e to
bef0a46
Compare
sbernhard
force-pushed
the
rebuild_requires_poweroff
branch
from
bef0a46 to
85686f8
Compare
stejskalleos
left a comment
stejskalleos
left a comment
There was a problem hiding this comment.
Question: What if Foreman stopped the machine before/during the rebuild action?
What's the point in rebuilding the VM host without rebooting?
Can we break something if we introduce such a behavior?
| isDisabled={!canBuild} | ||
| description={ | ||
| canBuild && !isBuild && buildRequiresPowerOff && isHostActive | ||
| ? __('Host must be powered off to initiate "Build".') |
There was a problem hiding this comment.
What if the buildRequiresPowerOff is true but canBuild is false?
The description will be misleading.
IMHO we should show the real reason, aka for each check have description
There was a problem hiding this comment.
Understand. There was no description before. So, if a user did not have the permission and canBuild was false, the button was simply disabled.
In case, the user has the permission to build the host, but a) the used provision method required that the host is powered off AND b) the host is powered on currently, the description is shown.
Actually, I would be OK to show a description in both cases:
!canBuild = "You don't have the permission to build the host"
canBuild && buildRequiredPowerOff && isHostActiv = "Host must be powereed off to initiate 'Build'"
your choice
| Foreman::Plugin.all.map(&:provision_methods).inject(:merge) || {} | ||
| end | ||
|
|
||
| def rebuild_requires_poweroff |
There was a problem hiding this comment.
I'm not sure that this is the right place for the method.
Should the host model be the one who decides if it needs to be powered off, when it's the compute resource who defines this provisioning method, and therefore should keep this information as well.
Looking at can_be_built?
def can_be_built?
managed? && !image_build? && !build?
endmanaged & build are attrs, and image_build? is a method in managed.rb:
def image_build?
provision_method == 'image'
endIt feels weird to me, I need to think about it.
There was a problem hiding this comment.
this method is used to determine, if the "re-build" method (= press the 'Build' button) requires that the host is powered off.
It will try to ask the used provision_method. So,in case of foreman_bootdisk, https://github.com/theforeman/foreman_bootdisk/pull/180/changes#diff-64bce33ed5907be7299b1be146db9be2a5c5a4d8b5e726187911c3c0ff38ec32R34 this will answer with "true"
So, the behavior can be adjusted for each provision_method in general. IF the host needs to be turned of is then related on the actual state of the host with the UI / react.
|
how can we continue with this @stejskalleos ? |
2 participants
Rebuild a host is sometimes not possibe if the host is powered on.
e.g. there are 2 issues for bootdisk:
Requires theforeman/foreman_bootdisk#180
This is how it currently look like:
