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 #35397 - Preseed Autoinstall incorporate host_params #9363

Merged

Conversation

bastian-src
Copy link
Contributor

Several host params exist to make the host creation more configurable ('keyboard' to set the layout, 'lang' to set the locale). The Ubuntu Autoinstall cloud-init template does not take any of these parameters into account.
This PR incorporates the following:

  • Param 'ubuntu_mirror_uri' to set default mirror during upgrade
  • Param 'keyboard' to set default layout
  • Param 'lang' to set default locale

@theforeman-bot
Copy link
Member

Issues: #35397

@bastian-src
Copy link
Contributor Author

Integration test looks unrelated IMHO.

@bastian-src bastian-src force-pushed the fix_35397_ubuntu_autoinstall_host_params branch 2 times, most recently from d830cb4 to 4ccfafb Compare September 12, 2022 07:28
@bastian-src bastian-src force-pushed the fix_35397_ubuntu_autoinstall_host_params branch 2 times, most recently from 57a726b to 90dae99 Compare October 4, 2022 09:54
@bastian-src
Copy link
Contributor Author

@sbernhard do you have feedback maybe?
I rebased to current develop and tests are fine - the changes aren't severe at all.

sbernhard
sbernhard previously approved these changes Oct 5, 2022
@bastian-src
Copy link
Contributor Author

Hey @MariaAga! Maybe you are busy at the moment - just wanted to ask to assign someone to this PR please :) evgeni reviewed PRs regarding Ubuntu Autoinstall before.

Copy link
Contributor Author

@bastian-src bastian-src left a comment

Choose a reason for hiding this comment

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

Thanks for your review @evgeni!

@bastian-src
Copy link
Contributor Author

@evgeni are there any more questions regarding the architecture? I just rebased to the latest master.

@bastian-src bastian-src force-pushed the fix_35397_ubuntu_autoinstall_host_params branch from d095c4c to 397c91b Compare October 28, 2022 07:33
@evgeni
Copy link
Member

evgeni commented Oct 28, 2022

@evgeni are there any more questions regarding the architecture? I just rebased to the latest master.

I'm still trying to wrap my head why you set disable_suites and disable_components

@bastian-src
Copy link
Contributor Author

bastian-src commented Oct 31, 2022

It relates to this post about disabling the automatic update: Stackoverflow

It's the configuration which worked with the official Ubuntu image as an installation media.

@evgeni
Copy link
Member

evgeni commented Oct 31, 2022

Okay, and then the user ends up with a system that has the updates/security repos disabled, and you need a config management tool to re-enable them?
Why do you want to disable the auto-updates in the first place? If we're providing a local mirror, all is cool?

@evgeni
Copy link
Member

evgeni commented Oct 31, 2022

And maybe more to the "why": https://projects.theforeman.org/issues/35397 speaks about missing parameters, like keyboard, cool, but I can't find a ubuntu_mirror_uri parameter anywhere.

Not saying this change as such is wrong, I just don't understand the motivation behind it (the mirror part)

@bastian-src
Copy link
Contributor Author

Okay, and then the user ends up with a system that has the updates/security repos disabled, and you need a config management tool to re-enable them?
Why do you want to disable the auto-updates in the first place? If we're providing a local mirror, all is cool?

The motivation behind this is actually related to a katello-based environment. When deploying a host with subscription-manager, updates are handled by the finish script. If we do the auto-updates during installation and again when subscription-manager runs, packages which are outdated in the auto-updates repo are updated twice. This takes usually more time.
Still, it can be discussed whether this should become default for non-katello environments as well.

And maybe more to the "why": https://projects.theforeman.org/issues/35397 speaks about missing parameters, like keyboard, cool, but I can't find a ubuntu_mirror_uri parameter anywhere.

In general, it is useful to have this variable especially in case of a smart proxy setup. Depending on the configuration, the local repo might be on the foreman itself, but in other cases the smart proxy could contain a copy of that repo. I thought that a host parameter would solve this issue quite elegantly, since we can now define the parameter per host group.
However, this is a different feature that is not mentioned in the issue - should I create another one for this change?

Thanks for your feedback on this! Highly appreciated 🙌

@evgeni
Copy link
Member

evgeni commented Nov 3, 2022

Okay, and then the user ends up with a system that has the updates/security repos disabled, and you need a config management tool to re-enable them?
Why do you want to disable the auto-updates in the first place? If we're providing a local mirror, all is cool?

The motivation behind this is actually related to a katello-based environment. When deploying a host with subscription-manager, updates are handled by the finish script. If we do the auto-updates during installation and again when subscription-manager runs, packages which are outdated in the auto-updates repo are updated twice. This takes usually more time. Still, it can be discussed whether this should become default for non-katello environments as well.

Langsam wird ein Schuh draus ;-)

So the real problem is that the auto-updates repo and the later configured repo are different repos, and thus can be out of sync and we end up doing certain operations twice, prolonging the update. That's certainly something we should improve!

And maybe more to the "why": https://projects.theforeman.org/issues/35397 speaks about missing parameters, like keyboard, cool, but I can't find a ubuntu_mirror_uri parameter anywhere.

In general, it is useful to have this variable especially in case of a smart proxy setup. Depending on the configuration, the local repo might be on the foreman itself, but in other cases the smart proxy could contain a copy of that repo. I thought that a host parameter would solve this issue quite elegantly, since we can now define the parameter per host group. However, this is a different feature that is not mentioned in the issue - should I create another one for this change?

A "normal" smart proxy will never have repos, it has no such capability. A Katello one will, but then I would actually expect Katello to be able to tell us the repo url with the right proxy used, so we don't have to guess and/or set it manually as a parameter?

And yes, I think we should split this one into one PR for keyboard/locale, that gets an instant ack and can be merged, and then we can discuss the details of the repo part.

* Param 'keyboard' to set default layout
* Param 'lang' to set default locale
* Adapt template snapshot accordingly

Co-authored-by: Bastian Schmidt <schmidt@atix.de>
Co-authored-by: Evgeni Golov <evgeni@golov.de>
@bastian-src bastian-src force-pushed the fix_35397_ubuntu_autoinstall_host_params branch from 397c91b to 01f051d Compare November 4, 2022 12:26
@evgeni evgeni merged commit 5339e9f into theforeman:develop Nov 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants