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

Fixes #37602 - Don't validate medium for unmanaged hosts #10226

Closed
wants to merge 1 commit into from

Conversation

sbernhard
Copy link
Contributor

@sbernhard sbernhard commented Jun 27, 2024

See https://projects.theforeman.org/issues/37602 for full issue description.

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.

I don't think this is the way we want to go, creating OS is part of the registration and disabling it is against user experience IMO.

This will create an OS inForemann, but this new OS doesn’t have the installation media assigned (because it is a new OS)

What kind of OS is that? Can we create a default installation medium for it inForemann, the same way we do for other operating systems?

@sbernhard
Copy link
Contributor Author

To create default installation media doesn't help @stejskalleos. As described in https://projects.theforeman.org/issues/37602 for every OS a major.minor is created. Liked Alma Linux 8.10. So, if you want to maintain AlmaLinux, OracleLinux and RockyLinux it would create 10 different minor versions (Alma 8.1, 8.2 .... 8.10) for each OS which would be somehow a mess.

Is this the same for RHEL or are the RHEL Major.Minor version somehow auto-generated?

Therefore the idea is, to re-use the operating system which is configured for the Hostgroup. Of course it would be possible to have this feature as a option.

@sbernhard sbernhard changed the title Fixes #37602 - Ignore OS creation during host registartion with sub-man Fixes #37602 - Don't validate for medium for unmanaged hosts Jul 4, 2024
@sbernhard
Copy link
Contributor Author

sbernhard commented Jul 4, 2024

Important

I have implemented a different approach which should solve the issue. During host registration the hosts are created as "unmanaged" hosts. In the UI the Operatingsystem tab in the Hosts Edit View is hidden. Therefore, don't run the medium validations.

@sbernhard sbernhard changed the title Fixes #37602 - Don't validate for medium for unmanaged hosts Fixes #37602 - Don't validate medium for unmanaged hosts Jul 4, 2024
@jeremylenz
Copy link
Contributor

@ianballou Any concerns here in light of Katello/katello#11049 ?

@@ -320,10 +320,10 @@ class Jail < ::Safemode::Jail
:if => proc { |host| host.managed && host.disk.empty? && !Foreman.in_rake? && !host.image_build? && host.build? }
validates :provision_method, :inclusion => {:in => proc { provision_methods }, :message => N_('is unknown')}, :if => proc { |host| host.managed? }
validates :medium_id, :presence => true,
:if => proc { |host| host.validate_media? }
:if => proc { |host| host.managed? && host.validate_media? }
Copy link
Member

Choose a reason for hiding this comment

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

Isn't this redundant? validate_media? already checks for managed:

def validate_media?
managed && !image_build? && build?
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @ekohl Good catch. I was actually mainly testing https://github.com/theforeman/foreman/pull/10226/files#diff-b7f82bd4fe3d5a5a8b246f502ab1e894197164cf7bf48fbfd5c2f3d76da2362eL326 and thought its also a good idea to have the managed? check for this part, too.

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.

See https://projects.theforeman.org/issues/37602 for full issue description.

Please also include a description of why you make a change in the actual commit message. I still consider https://cbea.ms/git-commit/ as a must read.

Note that Redmine is useful, but a bug report is often different than the change itself. That describes to future developers why a code change was made.

I have implemented a different approach which should solve the issue. During host registration the hosts are created as "unmanaged" hosts. In the UI the Operatingsystem tab in the Hosts Edit View is hidden. Therefore, don't run the medium validations.

It's correct that unmanaged hosts don't have certain fields. You can see we hide those in the host form:

<% if @host.managed %>
<% if authorized_for(:controller => "Compute::Resources::Vms", :action => :create) %>
<li id="compute_resource_tab" <%= display? !(@host.compute_resource_id || params[:host] && params[:host][:compute_resource_id].present?)%>><a href="#compute_resource" data-toggle="tab"><%= _('Virtual Machine') %></a></li>
<% end %>
<li><a href="#os" data-toggle="tab"><%= _('Operating System') %></a></li>
<% end %>

<%= render('hosts/unattended', :f => f) if @host.managed %>

That latter is:

<div class="tab-pane" id="compute_resource">
<%= render('hosts/compute', host: @host, vm: @vm, compute_resource: @host.compute_resource) if @host.compute_resource %>
</div>
<div class="tab-pane" id="os">
<%= render 'hosts/operating_system', :host => @host, :f => f %>
</div>

And that refers to https://github.com/theforeman/foreman/blob/develop/app/views/hosts/_operating_system.html.erb

And I think the medium is only used when the provision method is build.

So really, it's even more nuanced.

validates :medium_id, :inclusion => {:in => proc { |host| host.operatingsystem.medium_ids },
:message => N_('must belong to host\'s operating system')},
:if => proc { |host| host.operatingsystem && host.medium }
:if => proc { |host| host.managed? && host.operatingsystem && host.medium }
Copy link
Member

Choose a reason for hiding this comment

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

Is this really correct? If it's unmanaged, why does it even have a medium in the first place? And if it doesn't have a medium, the check isn't blocking.

IMHO that's really the root cause: somehow the host is getting a medium assigned that's invalid for the operating system. We should not do that.

Perhaps this is a bug in the inheritance from a hostgroup?

Copy link
Contributor Author

@sbernhard sbernhard Jul 12, 2024

Choose a reason for hiding this comment

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

A host was registered using HostRegistartion. During this process, a Hostgroup was used and assigned to the host.
For this Hostgroup the Installation Media was configured.

The host is using the subscription-manager to register. This will create a new OS in foreman if the OS doesn't currently exist. This new OS doesn’t have the installation media assigned (because its a new OS).

Summary:

  • The new created Operatingsystem like AlmaLinux9 doesn't have a Installation Media.
  • The used Hostgroup has different Operatingsystems and is using a InstallationMedia which is valid for the Operatingsystem used in this Hostgroup.

The POST request during the HostRegistration process which assign Hostgroup to the Host and should render the host_init_config Template will fail with:

The system has been registered with ID: 21312a8f-1b6e-4302-ae9e-1a64b643c6f8
The registered system name is: myra-bergh.master.dev.atix
ERROR: Validation failed: Medium must belong to host's operating system

Notice:

  • if the Hostgroup doesn't have a Operatingsystem OR Installation Media > everything works
  • If the Operatingsystem of the Host, is already configured on foreman, including valid Installatin Media -> everything works

@ianballou
Copy link
Contributor

@ianballou Any concerns here in light of Katello/katello#11049 ?

No concerns from the kickstart repo side, this change would only make it less likely for hosts to hit the medium validation issue.

@sbernhard
Copy link
Contributor Author

Obsoleted by #10246

@sbernhard sbernhard closed this Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants