Skip to content

Fixes #39067 - rebuild only if host powered off#10860

Merged
stejskalleos merged 1 commit intotheforeman:developfrom
ATIX-AG:rebuild_requires_poweroff
Mar 3, 2026
Merged

Fixes #39067 - rebuild only if host powered off#10860
stejskalleos merged 1 commit intotheforeman:developfrom
ATIX-AG:rebuild_requires_poweroff

Conversation

@sbernhard
Copy link
Contributor

@sbernhard sbernhard commented Feb 6, 2026

Rebuild a host is sometimes not possibe if the host is powered on.

e.g. there are 2 issues for bootdisk:

  1. on vsphere, if the host is running, the bootdisk can not be attached. To fix this in a user friendly manner, its not possible to fix this only in foreman_bootdisk
  2. the current mechanism for vsphere in foreman_bootdisk is, that it restarts the host during attaching the iso image - without a warning that this happens. this isn't user friendly as it is not done for image/network based deployment.

Requires theforeman/foreman_bootdisk#180

This is how it currently look like:
image

@github-actions github-actions bot added the UI label Feb 6, 2026
sbernhard added a commit to ATIX-AG/foreman_bootdisk that referenced this pull request Feb 6, 2026
Use theforeman/foreman#10860 and refactor
the vsphere/proxmox methods as it is no longer needed to
start/stop/reboot
@sbernhard sbernhard force-pushed the rebuild_requires_poweroff branch 4 times, most recently from bef0a46 to 85686f8 Compare February 6, 2026 22:09
@stejskalleos stejskalleos self-assigned this Feb 9, 2026
Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Foreman::Plugin.all.map(&:provision_methods).inject(:merge) || {}
end

def rebuild_requires_poweroff
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?
  end

managed & build are attrs, and image_build? is a method in managed.rb:

  def image_build?
    provision_method == 'image'
  end

It feels weird to me, I need to think about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sbernhard
Copy link
Contributor Author

how can we continue with this @stejskalleos ?

Copy link
Member

@ekohl ekohl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual changing of a host to build mode happens in PUT /api/v2/hosts/:id:

api :PUT, "/hosts/:id/", N_("Update a host")
param :id, :identifier, :required => true
param_group :host
def update
@parameters = true
@all_parameters = true
@host.attributes = host_attributes(host_params, @host)
process_response @host.save
rescue InterfaceTypeMapper::UnknownTypeException => e
render_error :custom_error, :status => :unprocessable_entity, :locals => { :message => e.to_s }
end

But perhaps it should be done at the model layer that prevents a save in the first place.

@sbernhard sbernhard force-pushed the rebuild_requires_poweroff branch from 5c39680 to 441f639 Compare February 25, 2026 22:05
@sbernhard
Copy link
Contributor Author

But perhaps it should be done at the model layer that prevents a save in the first place.
Implemented as validation and added some tests. please have a look again.

@sbernhard sbernhard force-pushed the rebuild_requires_poweroff branch 2 times, most recently from f834aac to f7b5a0e Compare February 26, 2026 08:57
@sbernhard sbernhard force-pushed the rebuild_requires_poweroff branch from f7b5a0e to 34c1af9 Compare February 26, 2026 16:28
@sbernhard sbernhard force-pushed the rebuild_requires_poweroff branch 2 times, most recently from b7404e1 to 4f0eb98 Compare February 27, 2026 21:10
@sbernhard sbernhard force-pushed the rebuild_requires_poweroff branch from 4f0eb98 to 4e87c24 Compare February 27, 2026 21:19
@sbernhard
Copy link
Contributor Author

@stejskalleos ready to merge.

@stejskalleos
Copy link
Contributor

@stejskalleos ready to merge.

Working on my test setup, it's on my board for this week. Code LGTM, so if there are no test issues, I think we can merge.

Copy link
Contributor

@stejskalleos stejskalleos left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🍏 LGTM

Tested with Packit packit/theforeman-foreman-10860 rhel-9-x86_64 and with rubygem-foreman_bootdisk-23.1.2-1.20260206220301374331.pr180.5.g2a5e190.el9.noarch, works as expected.

Screenshot_20260303_154423

@stejskalleos
Copy link
Contributor

Failed container job is not related to the PR changes, merging.

@stejskalleos stejskalleos merged commit 3642b09 into theforeman:develop Mar 3, 2026
39 of 42 checks passed
@stejskalleos
Copy link
Contributor

thanks @sbernhard

stejskalleos pushed a commit to theforeman/foreman_bootdisk that referenced this pull request Mar 3, 2026
Use theforeman/foreman#10860 and refactor
the vsphere/proxmox methods as it is no longer needed to
start/stop/reboot
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants