Fixes #34409 - rename existing CentOS OSes to CentOS_Stream #9098

lzap · 2022-02-08T14:11:02Z

Previously, Puppet facts coming out of CentOS Stream box were recognized as a OS with name "CentOS" and title "CentOS Stream 8". After the change 16701e6 its recognized as "CentOS_Stream" and "CentOS Stream 8". That's all expected, however, the patch did not migrate the existing OSes because operating system model enforces the title to be unique.

The way we are parsing facts and create OS entries is bad from the moment we introduced another fact parsing other than Puppet. Foreman needs a proper operating system mapping that would various facter allow to co-exist. Or we could move the responsibility to the user creating a "reported OS name" table which users would need to associate with their operating systems which can now be created either manually or via Katello sync.

The purpose of this patch, however, is not to fix any of this. This introduces a simple migration that should have resolve the upgrade problem. It will help in the particular case mentioned in the RM issue.

theforeman-bot · 2022-02-08T14:11:04Z

Issues: #34409

lzap · 2022-02-09T18:53:43Z

So before merging I am just double-checking it will work as expected.

I installed Satellite 6.10 and registered pure puppet CentOS 8 Stream host, after puppet executed and uploaded facts new OS was created with name=CentOS and major=8 and title=CentOS 8:

#<Redhat id: 2, major: "8", name: "CentOS", minor: "", nameindicator: nil, created_at: "2022-02-09 18:35:22", updated_at: "2022-02-09 18:35:22", release_name: nil, type: "Redhat", description: nil, password_hash: [FILTERED], title: "CentOS 8">

Then I applied the patch 16701e6 and executed puppet agent again this time a new OS was created and associated:

#<Redhat id: 3, major: "8", name: "CentOS_Stream", minor: "", nameindicator: nil, created_at: "2022-02-09 18:44:55", updated_at: "2022-02-09 18:44:55", release_name: nil, type: "Redhat", description: nil, password_hash: [FILTERED], title: "CentOS_Stream 8">

This actually show an important thing I missed - the title is autogenerated on save and it is always "name version" so the migration should have set the title to "CentOS_Stream 8" actually to match this behavior.

Anyway, I did not run into any issue just with Puppet because "CentOS 8" vs "CentOS_Stream 8" are both unique, Foreman just creates a new record and associate the host with the new entry on puppet fact upload. So far so good. Taking a look on RHSM now which might be the offender.

lzap · 2022-02-09T19:10:32Z

OS created via RHSM upload, before the patch:

=> #<Redhat id: 6, major: "8", name: "CentOS", minor: "", nameindicator: nil, created_at: "2022-02-09 19:06:46", updated_at: "2022-02-09 19:06:46", release_name: nil, type: "Redhat", description: nil, password_hash: [FILTERED], title: "CentOS 8">

After the patch:

#<Redhat id: 5, major: "8", name: "CentOS_Stream", minor: "", nameindicator: nil, created_at: "2022-02-09 19:02:59", updated_at: "2022-02-09 19:02:59", release_name: nil, type: "Redhat", description: nil, password_hash: [FILTERED], title: "CentOS_Stream 8">

Also looks good as long as the title has underscore in it.

lzap · 2022-02-09T19:30:36Z

Finally: when I sync CentOS 8 Stream with Katello, it creates:

=> #<Redhat id: 7, major: "8", name: "CentOS_Stream", minor: "", nameindicator: nil, created_at: "2022-02-09 19:16:26", updated_at: "2022-02-09 19:16:26", release_name: nil, type: "Redhat", description: nil, password_hash: [FILTERED], title: "CentOS_Stream 8">

So my conclusion: I was not able to reproduce the issue, in any of my cases a title "CentOS Stream 8" was ever created. I suspect a different puppet agent version of maybe this patch: cc4a95f

Anyway, the way how OS is created on the pipeline tests is very weird: name=CentOS, title=CentOS 8 Stream. The title is set via before_validation callback, fact parsers never set title explicitly. So something else had to rename the OS or modify it in some way. It has to be RHSM but I don't see this behavior, I alway see underscore in the title:

[root@s17 ~]# facter | grep Stream

[root@s17 ~]# subscription-manager facts | grep Stream
distribution.name: CentOS Stream

I have updated this PR so it matches the expected state, eager to hear what @ezr-ondrej found.

ezr-ondrej

I've had simillar results, but only until I figured that the CentOS Stream does not report the facts for description by default and you need to install redhat-lsb-core first for the description to be populated from the facter.

Testing with the redhat-lsb-core installed, on Foreman 2.5 I've registered the system through puppet, and then the offending title CentOS Stream 8 is populated.
After upgrade Facts upload fails with:

2022-02-09T22:36:32 [I|app|1e803d4b] Backtrace for 'Action failed' error (ActiveRecord::RecordInvalid): Validation failed: Description has already been taken, Title has already been taken
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/activerecord-6.0.3.7/lib/active_record/validations.rb:80:in `raise_validation_error'
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/activerecord-6.0.3.7/lib/active_record/validations.rb:53:in `save!'
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/activerecord-6.0.3.7/lib/active_record/transactions.rb:318:in `block in save!'
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/activerecord-6.0.3.7/lib/active_record/transactions.rb:375:in `block in with_transaction_returning_status'
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/activerecord-6.0.3.7/lib/active_record/connection_adapters/abstract/database_statements.rb:280:in `block in transaction'
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/activerecord-6.0.3.7/lib/active_record/connection_adapters/abstract/transaction.rb:280:in `block in within_new_transaction'
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/activesupport-6.0.3.7/lib/active_support/concurrency/load_interlock_aware_monitor.rb:26:in `block (2 levels) in synchronize'
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/activesupport-6.0.3.7/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `handle_interrupt'
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/activesupport-6.0.3.7/lib/active_support/concurrency/load_interlock_aware_monitor.rb:25:in `block in synchronize'
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/activesupport-6.0.3.7/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `handle_interrupt'
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/activesupport-6.0.3.7/lib/active_support/concurrency/load_interlock_aware_monitor.rb:21:in `synchronize'
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/activerecord-6.0.3.7/lib/active_record/connection_adapters/abstract/transaction.rb:278:in `within_new_transaction'
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/activerecord-6.0.3.7/lib/active_record/connection_adapters/abstract/database_statements.rb:280:in `transaction'
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/activerecord-6.0.3.7/lib/active_record/transactions.rb:212:in `transaction'
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/activerecord-6.0.3.7/lib/active_record/transactions.rb:366:in `with_transaction_returning_status'
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/activerecord-6.0.3.7/lib/active_record/transactions.rb:318:in `save!'
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/activerecord-6.0.3.7/lib/active_record/suppressor.rb:48:in `save!'
 1e803d4b | /usr/share/foreman/app/services/puppet_fact_parser.rb:35:in `operatingsystem'
 1e803d4b | /usr/share/foreman/app/models/host/base.rb:161:in `block in set_non_empty_values'
 1e803d4b | /usr/share/foreman/app/models/host/base.rb:160:in `each'
 1e803d4b | /usr/share/foreman/app/models/host/base.rb:160:in `set_non_empty_values'
 1e803d4b | /usr/share/foreman/app/models/host/base.rb:154:in `populate_fields_from_facts'
 1e803d4b | /usr/share/foreman/app/models/host/managed.rb:493:in `populate_fields_from_facts'
 1e803d4b | /usr/share/foreman/app/services/host_fact_importer.rb:50:in `block (2 levels) in parse_facts'
 1e803d4b | /usr/share/foreman/app/services/foreman/telemetry_helper.rb:28:in `telemetry_duration_histogram'
 1e803d4b | /usr/share/foreman/app/services/host_fact_importer.rb:49:in `block in parse_facts'
 1e803d4b | /usr/share/foreman/app/services/host_fact_importer.rb:90:in `block in skipping_orchestration'
 1e803d4b | /usr/share/foreman/app/models/concerns/orchestration.rb:124:in `without_orchestration'
 1e803d4b | /usr/share/foreman/app/services/host_fact_importer.rb:89:in `skipping_orchestration'
 1e803d4b | /usr/share/foreman/app/services/host_fact_importer.rb:45:in `parse_facts'
 1e803d4b | /usr/share/foreman/app/services/host_fact_importer.rb:34:in `import_facts'
 1e803d4b | /usr/share/foreman/app/controllers/api/v2/hosts_controller.rb:310:in `facts'
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/actionpack-6.0.3.7/lib/action_controller/metal/basic_implicit_render.rb:6:in `send_action'
 1e803d4b | /opt/theforeman/tfm/root/usr/share/gems/gems/actionpack-6.0.3.7/lib/abstract_controller/base.rb:195:in `process_action'
... redacted ...

Applying the migration and running db:migrate resolves the issue and the fact upload is successful again.

ezr-ondrej · 2022-02-09T22:43:37Z

db/migrate/20220208134539_rename_cent_os_stream_os.rb

+class RenameCentOsStreamOs < ActiveRecord::Migration[6.0]
+  def up
+    User.without_auditing do
+      Operatingsystem.unscoped.where("type = 'Redhat' and name = 'CentOS' and (description like '%CentOS Stream 8%' or title like '%CentOS Stream 8%')").update_all(name: "CentOS_Stream", title: "CentOS_Stream 8", description: "CentOS_Stream 8")


Do we need to update the description and title?
The issue is fixed by only changing the name.

Well name is not the culprit actually, see the original report exception: Backtrace for 'Action failed' error (ActiveRecord::RecordInvalid): Validation failed: Description has already been taken, Title has already been taken

Foreman operating system model class design:

has a name, major, minor fields with unique constraint in the scope of name-major-minor

has a title field that is automatically generated before validation as follows: "name major(.minor)"

title must be also unique as it is used as "friendly_id"

there is also a description which overrides title if set by a user and it is also unique

when any facts check-in, they try to update OS name, major, minor causing title and description to be regenerated

I can see that description is set automatically on fact upload too but I cannot find the place where it actually gets set

My conclusion: 42
Don't panic, keep a towel nearby.

Correction, I can see where description is set, it is actually explicitly in the fact parser. Title is auto-generated by OS. Well, "makes sense".

The issue is fixed by only changing the name.

Or maybe I am not following what you mean?

lzap · 2022-02-10T09:22:34Z

I was wondering why description is also set, now I found it. When you install redhat-lsb-core puppet reports this:

os => {
  architecture => "x86_64",
  distro => {
    codename => "n/a",
    description => "CentOS Stream release 8",
    id => "CentOSStream",
    release => {
      full => "8",
      major => "8"
    },
    specification => ":core-4.1-amd64:core-4.1-noarch"
  }
}

Fact parser tries to find OS by name, major and minor versions first. If it cannot find a record, it creates new. Then it parses facts and sets os name, major, minor. Now, when description is blank, only then it also sets description from puppet "description" field. Description is filtered through shorten_description which for RedHat removes "release". When OS is saved, title is copied from description if any is present, otherwise is is generated as "name major.minor".

Then this patch is not correct because when description is missing for any reason, there is a difference:

CentOS Stream 8
CentOS_Stream 8

Signed-off-by: Lukas Zapletal <lzap+git@redhat.com>

lzap · 2022-02-10T09:44:14Z

My final conclusion: we need to rename OS name from CentOS to CentOS_Stream for all OSes that have "CentOS Stream" or "CentOS_Stream" in title or description.

The reason is stated in the migration itself:

      # When redhat-lsb-core package is installed, puppet creates
      # description/title "CentOS Stream 8", however, when the package is not
      # present description is unset and title is set to "CentOS_Stream 8" from
      # the OS name and major by the ActiveRecord callback. Let's migrate both
      # cases.

ezr-ondrej

Thanks @lzap ! 👍
This fixes the issue for me. Tested on Foreman 2.5 with CentOS Stream 8 Host, upgraded to nightly, where I've reproduced the issue and this migration has fixed that for me 👍

gvde · 2022-07-12T06:01:29Z

@lzap This will fail if there is already another OS with name 'CentOS_Stream':

PG::UniqueViolation: ERROR:  duplicate key value violates unique constraint "index_operatingsystems_on_name_and_major_and_minor"
DETAIL:  Key (name, major, minor)=(CentOS_Stream, 8, ) already exists.

See also https://community.theforeman.org/t/upgrade-from-3-1-3-4-2-1-to-3-2-1-4-3-1-fails-during-database-migration/29371/2

theforeman-bot added Needs testing Not yet reviewed labels Feb 8, 2022

lzap mentioned this pull request Feb 8, 2022

Fixes #34409 - Revert CentOS_Stream name #9096

Closed

lzap force-pushed the migrate-stream-osname-34409 branch 3 times, most recently from c318ec7 to f6c1a64 Compare February 9, 2022 19:46

ezr-ondrej reviewed Feb 9, 2022

View reviewed changes

Fixes #34409 - rename existing CentOS OSes to CentOS_Stream

1703279

Signed-off-by: Lukas Zapletal <lzap+git@redhat.com>

lzap force-pushed the migrate-stream-osname-34409 branch from f6c1a64 to 1703279 Compare February 10, 2022 09:43

ezr-ondrej closed this Feb 10, 2022

ezr-ondrej reopened this Feb 10, 2022

ezr-ondrej approved these changes Feb 10, 2022

View reviewed changes

theforeman-bot removed the Not yet reviewed label Feb 10, 2022

ezr-ondrej merged commit 3225005 into theforeman:develop Feb 10, 2022

lzap deleted the migrate-stream-osname-34409 branch February 11, 2022 07:52

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #34409 - rename existing CentOS OSes to CentOS_Stream #9098

Fixes #34409 - rename existing CentOS OSes to CentOS_Stream #9098

lzap commented Feb 8, 2022

theforeman-bot commented Feb 8, 2022

lzap commented Feb 9, 2022

lzap commented Feb 9, 2022

lzap commented Feb 9, 2022 •

edited

ezr-ondrej left a comment

ezr-ondrej Feb 9, 2022

lzap Feb 10, 2022

lzap Feb 10, 2022

lzap Feb 10, 2022

lzap commented Feb 10, 2022 •

edited

lzap commented Feb 10, 2022

ezr-ondrej left a comment •

edited

gvde commented Jul 12, 2022

Fixes #34409 - rename existing CentOS OSes to CentOS_Stream #9098

Fixes #34409 - rename existing CentOS OSes to CentOS_Stream #9098

Conversation

lzap commented Feb 8, 2022

theforeman-bot commented Feb 8, 2022

lzap commented Feb 9, 2022

lzap commented Feb 9, 2022

lzap commented Feb 9, 2022 • edited

ezr-ondrej left a comment

Choose a reason for hiding this comment

ezr-ondrej Feb 9, 2022

Choose a reason for hiding this comment

lzap Feb 10, 2022

Choose a reason for hiding this comment

lzap Feb 10, 2022

Choose a reason for hiding this comment

lzap Feb 10, 2022

Choose a reason for hiding this comment

lzap commented Feb 10, 2022 • edited

lzap commented Feb 10, 2022

ezr-ondrej left a comment • edited

Choose a reason for hiding this comment

gvde commented Jul 12, 2022

lzap commented Feb 9, 2022 •

edited

lzap commented Feb 10, 2022 •

edited

ezr-ondrej left a comment •

edited