Fixes #37137 - Use insert_all when creating available module streams #10878

jeremylenz · 2024-02-01T22:15:48Z

What are the changes introduced in this pull request?

During a profile upload, a host's module streams are updated in Katello's database.

In #10412 we changed to using create_or_find_by! to create those records. This solved an issue where concurrent host registrations would sometimes fail due to a PostgreSQL error:

ERROR:  duplicate key value violates unique constraint "katello_available_module_streams_name_stream_context"

However, these errors are still inserted into postgres logs. They are harmless, but log files can fill up quickly. This is because there will be one log for every module stream creation attempt, for every host with module streams, not just at registration but also during every profile upload. This results in log files that can quickly build to 200000 lines with just those error logs.

With this change, we're now using insert_all to insert all of the records in a single SQL query, which should avoid the race condition.

Considerations taken when implementing this change?

This required some major refactoring, because

import_module_streams takes module stream data in a list of Hashes, but those don't include Katello's ID. (There's no way they could, as this info is coming from sub-man/dnf.)
In our previous implementation, we relied on the ids returned from create_or_find_by etc. to pass on to the next method, sync_available_module_stream_associations. sync_available_module_stream_associations requires a Hash indexed by database ID.
Previously, the data passed to sync_available_module_stream_associations was all of the host's available module streams, indexed by ID, including the ones we just newly added. Thus, new_available_module_streams was a bit of a misnomer. I did some refactoring to recreate this format of the indexed hash.

What are the testing steps for this pull request?

tail -f /var/lib/pgsql/data/log/postgresql-Thu.log (replace Thu with whatever day it is today)
Register a host (without this patch) - Notice that the file fills up with error logs

2024-02-01 21:57:16 UTC STATEMENT:  INSERT INTO "katello_available_module_streams" ("name", "stream", "context") VALUES ($1, $2, $3) RETURNING "id"
2024-02-01 21:57:16 UTC ERROR:  duplicate key value violates unique constraint "katello_available_module_streams_name_stream_context"
2024-02-01 21:57:16 UTC DETAIL:  Key (name, stream, context)=(virt, rhel, 30b713e6) already exists.

You can also check how many logs there are:

grep 'duplicate key value violates unique constraint "katello_available_module_streams_name_stream_context"' /var/lib/pgsql/data/log/postgresql-Thu.log | wc -l
---
227950
---

Enable or disable a module stream - Notice that more error logs are created.
Check out this PR
Enable or disable a module stream - No new logs should be created.
Register a host - No new logs should be created.

jeremylenz · 2024-02-02T14:44:48Z

fixed rubocop

chris1984 · 2024-02-02T15:41:16Z

Your PR is having the same test failures as mine is

jeremylenz · 2024-02-02T15:42:53Z

Your PR is having the same test failures as mine is

Yeah, apparently #10861 has the fixes. We may have to ignore those for a bit.

jeremylenz · 2024-02-02T17:01:30Z

[test katello]

jeremylenz · 2024-02-09T16:29:07Z

[test katello]

sbernhard · 2024-02-19T10:54:37Z

Ready?

jeremylenz · 2024-02-19T14:30:57Z

Yes, this should be ready for review again 👍

sbernhard · 2024-02-19T16:11:04Z

app/models/katello/concerns/host_managed_extensions.rb

+                                                stream: module_stream["stream"])
+          else
+            stream = AvailableModuleStream.where(name: module_stream["name"],
+            context: module_stream["context"],


is the indention broken?

sbernhard · 2024-02-19T16:13:58Z

app/models/katello/concerns/host_managed_extensions.rb

@@ -348,11 +348,21 @@ def import_enabled_repositories(repos)
      end

      def import_module_streams(module_streams)
+        # create_or_find_by avoids race conditions during concurrent registrations but clogs postgres logs with harmless errors.


instead of two different implementation, would it be possible to catch the "postgres logs with harmless errors" and handle this accordingly?

No, that's the way the race condition is avoided - by letting the Postgres constraint handle it rather than ruby/rails.

ok. make sense.

ianballou

Why won't we hit PG::UniqueViolation errors during concurrent package profile uploads? Is there a difference in concurrency between 10 systems registering at the same time and 10 systems dnf updating at the same time?

ianballou · 2024-02-19T21:50:39Z

app/models/katello/concerns/host_managed_extensions.rb

+        # create_or_find_by avoids race conditions during concurrent registrations but clogs postgres logs with harmless errors.
+        # So we'll use create_or_find_by! during registration and first_or_create! otherwise.
+        registered_time = subscription_facet&.registered_at
+        use_create_or_find_by = registered_time.nil? || registered_time > 1.minute.ago
        streams = {}
        module_streams.each do |module_stream|


I know this code is old, but I wonder if now would be a good time to switch to insert_all or upsert_all. It feels odd to do so many creations one postgres insert at a time in a Ruby loop.

Bulk insertion lets you create all entities in a single Postgres insert call. It takes into account duplicates too from what I remember. You can pass in what the records should be unique_by (in this case name, stream, and context).

There might be a catch, but I think it's worth a thought.

https://apidock.com/rails/v6.0.0/ActiveRecord/Persistence/ClassMethods/insert_all
https://apidock.com/rails/v6.0.0/ActiveRecord/Persistence/ClassMethods/upsert_all

We use them in a couple places:

https://github.com/search?q=repo%3AKatello%2Fkatello%20insert_all&type=code
https://github.com/search?q=repo%3AKatello%2Fkatello+upsert_all&type=code

jeremylenz · 2024-02-20T00:32:54Z

We haven't had any complaints of that, for whatever reason. It could be because most package profile uploads are from rhsmcertd checkins which are staggered automatically with random delay intervals to prevent too many concurrent requests, whereas registrations can happen at any time.

app/models/katello/concerns/host_managed_extensions.rb

jeremylenz · 2024-02-27T20:54:38Z

fixed rubocop

ianballou · 2024-02-27T20:56:44Z

Gonna test first then code review.

ianballou

This is working great! So far I'm not seeing any issues with the code.

I'll give it a final review once the dev logging is cleaned up.

…registration

ianballou

Still working fine for me! Ruby tests are passing now too.

…atello#10878) * Fixes #37137 - Use insert_all when creating available module streams (cherry picked from commit 7458f70)

* Release 4.12.0-rc1 (#10909) * Fixes #37137 - Use insert_all when creating available module streams (#10878) * Fixes #37137 - Use insert_all when creating available module streams (cherry picked from commit 7458f70) * Fixes #37178 - use safe navigation for operatingsystem (#10897) (cherry picked from commit 664488d) * Refs #37202 - display name according to setting in host collection (cherry picked from commit de39a36) * Fixes #37192 - Use with_enabled_email scope (#10901) * Fixes #37192 - Use with_enabled_email scope Instead of typing out the same query explicitly. * Refs #37192 - Bump required foreman version (cherry picked from commit 1ace8e5) * Refs #37203 - display name according to setting in errata host list (cherry picked from commit 2775554) * Fixes #37214 - Change 'default' for limit to environment checkbox in activation-key and content-host (cherry picked from commit 9d0a338) * Fixes #37108 - Preload content_view_components (#10864) (cherry picked from commit d98c17b) * Fixes #36976 - Too many audit records slow down CV loading (#10911) (cherry picked from commit 30c6977) * Refs #37201 - display short host name in activation keys menu (cherry picked from commit 8164043) * Fixes #37187 - Update ACS refresh Pulp fixtures + fix repository_test test (cherry picked from commit 99c7857) * Fixes #37198 - Allow installedDeb package attributes in safemode (cherry picked from commit b5d785f) * Fixes #37197 - Kickstart repository correctly listed on hostgroup (cherry picked from commit 35c49b1) * Fixes #37169 - Managing a Hosts Repository Sets does not behave as expected (#10905) (cherry picked from commit 6420cf8) --------- Co-authored-by: Jeremy Lenz <[email protected]> Co-authored-by: Matěj Mudra <[email protected]> Co-authored-by: Adam Růžička <[email protected]> Co-authored-by: Manisha Singhal <[email protected]> Co-authored-by: Bernhard Suttner <[email protected]> Co-authored-by: Samir Jha <[email protected]> Co-authored-by: ianballou <[email protected]> Co-authored-by: Bernhard Suttner <[email protected]> Co-authored-by: Partha Aji <[email protected]> Co-authored-by: Thorben <[email protected]>

…atello#10878) * Fixes #37137 - Use insert_all when creating available module streams (cherry picked from commit 7458f70)

* Fixes #37187 - Update ACS refresh Pulp fixtures + fix repository_test test (cherry picked from commit 99c7857) * Fixes #37137 - Use insert_all when creating available module streams (#10878) * Fixes #37137 - Use insert_all when creating available module streams (cherry picked from commit 7458f70) * Fixes #37178 - use safe navigation for operatingsystem (#10897) (cherry picked from commit 664488d) * Refs #37202 - display name according to setting in host collection (cherry picked from commit de39a36) * Refs #37201 - display short host name in activation keys menu (cherry picked from commit 8164043) * Refs #37203 - display name according to setting in errata host list (cherry picked from commit 2775554) * Fixes #37214 - Change 'default' for limit to environment checkbox in activation-key and content-host (cherry picked from commit 9d0a338) * Fixes #37198 - Allow installedDeb package attributes in safemode (cherry picked from commit b5d785f) * Fixes #37108 - Preload content_view_components (#10864) (cherry picked from commit d98c17b) * Fixes #37197 - Kickstart repository correctly listed on hostgroup (cherry picked from commit 35c49b1) * Fixes #36976 - Too many audit records slow down CV loading (#10911) (cherry picked from commit 30c6977) * Fixes #37169 - Managing a Hosts Repository Sets does not behave as expected (#10905) (cherry picked from commit 6420cf8) --------- Co-authored-by: ianballou <[email protected]> Co-authored-by: Jeremy Lenz <[email protected]> Co-authored-by: Matěj Mudra <[email protected]> Co-authored-by: Manisha Singhal <[email protected]> Co-authored-by: Bernhard Suttner <[email protected]> Co-authored-by: Bernhard Suttner <[email protected]> Co-authored-by: Partha Aji <[email protected]> Co-authored-by: Samir Jha <[email protected]> Co-authored-by: Thorben <[email protected]>

…atello#10878) * Fixes #37137 - Use insert_all when creating available module streams (cherry picked from commit 7458f70)

* Fixes #37187 - Update ACS refresh Pulp fixtures + fix repository_test test (cherry picked from commit 99c7857) * Fixes #37137 - Use insert_all when creating available module streams (#10878) * Fixes #37137 - Use insert_all when creating available module streams (cherry picked from commit 7458f70) * Fixes #37178 - use safe navigation for operatingsystem (#10897) (cherry picked from commit 664488d) * Refs #37202 - display name according to setting in host collection (cherry picked from commit de39a36) * Refs #37201 - display short host name in activation keys menu (cherry picked from commit 8164043) * Refs #37203 - display name according to setting in errata host list (cherry picked from commit 2775554) * Fixes #37214 - Change 'default' for limit to environment checkbox in activation-key and content-host (cherry picked from commit 9d0a338) * Fixes #37198 - Allow installedDeb package attributes in safemode (cherry picked from commit b5d785f) * Fixes #37108 - Preload content_view_components (#10864) (cherry picked from commit d98c17b) * Fixes #37197 - Kickstart repository correctly listed on hostgroup (cherry picked from commit 35c49b1) * Fixes #36976 - Too many audit records slow down CV loading (#10911) (cherry picked from commit 30c6977) * Fixes #37169 - Managing a Hosts Repository Sets does not behave as expected (#10905) (cherry picked from commit 6420cf8) * Refs #37214 - Use Limit to Environment by default (cherry picked from commit 8bedda9) --------- Co-authored-by: ianballou <[email protected]> Co-authored-by: Jeremy Lenz <[email protected]> Co-authored-by: Matěj Mudra <[email protected]> Co-authored-by: Manisha Singhal <[email protected]> Co-authored-by: Bernhard Suttner <[email protected]> Co-authored-by: Bernhard Suttner <[email protected]> Co-authored-by: Partha Aji <[email protected]> Co-authored-by: Samir Jha <[email protected]> Co-authored-by: Thorben <[email protected]>

pr-processor bot added the Not yet reviewed label Feb 1, 2024

jeremylenz force-pushed the 37137-first-or-create-or-create-or-find-by-oh-my branch from e69483b to 4b73412 Compare February 2, 2024 14:44

sbernhard reviewed Feb 19, 2024

View reviewed changes

ianballou reviewed Feb 19, 2024

View reviewed changes

jeremylenz force-pushed the 37137-first-or-create-or-create-or-find-by-oh-my branch from 4b73412 to 4dfde98 Compare February 20, 2024 23:54

jeremylenz changed the title ~~Fixes #37137 - Revert to old method of creating module streams after registration~~ Fixes #37137 - Use upsert_all when creating available module streams Feb 21, 2024

ianballou reviewed Feb 22, 2024

View reviewed changes

app/models/katello/concerns/host_managed_extensions.rb Show resolved Hide resolved

app/models/katello/concerns/host_managed_extensions.rb Outdated Show resolved Hide resolved

jeremylenz force-pushed the 37137-first-or-create-or-create-or-find-by-oh-my branch from 4dfde98 to 8123b07 Compare February 27, 2024 20:24

jeremylenz changed the title ~~Fixes #37137 - Use upsert_all when creating available module streams~~ Fixes #37137 - Use insert_all when creating available module streams Feb 27, 2024

jeremylenz force-pushed the 37137-first-or-create-or-create-or-find-by-oh-my branch from 8123b07 to 65430af Compare February 27, 2024 20:54

ianballou reviewed Feb 27, 2024

View reviewed changes

jeremylenz force-pushed the 37137-first-or-create-or-create-or-find-by-oh-my branch from 5aad7dc to f0bad71 Compare February 28, 2024 14:28

jeremylenz added 4 commits February 28, 2024 11:09

Fixes #37137 - Revert to old method of creating module streams after …

682568f

…registration

Refs #37137 - use insert_all and index_by

756f496

Refs #37137 - cache AvailableModuleStreams

8982ebc

Refs #37137 - fix test

1f8225e

jeremylenz force-pushed the 37137-first-or-create-or-create-or-find-by-oh-my branch from e3caee8 to 1f8225e Compare February 28, 2024 16:09

ianballou approved these changes Feb 28, 2024

View reviewed changes

pr-processor bot removed the Not yet reviewed label Feb 28, 2024

jeremylenz merged commit 7458f70 into Katello:master Feb 28, 2024
13 of 16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #37137 - Use insert_all when creating available module streams #10878

Fixes #37137 - Use insert_all when creating available module streams #10878

jeremylenz commented Feb 1, 2024 •

edited

Loading

jeremylenz commented Feb 2, 2024

chris1984 commented Feb 2, 2024

jeremylenz commented Feb 2, 2024

jeremylenz commented Feb 2, 2024

jeremylenz commented Feb 9, 2024

sbernhard commented Feb 19, 2024

jeremylenz commented Feb 19, 2024

sbernhard Feb 19, 2024

sbernhard Feb 19, 2024

jeremylenz Feb 19, 2024

sbernhard Feb 19, 2024

ianballou left a comment

ianballou Feb 19, 2024

jeremylenz commented Feb 20, 2024

jeremylenz commented Feb 27, 2024

ianballou commented Feb 27, 2024

ianballou left a comment

ianballou left a comment

Fixes #37137 - Use insert_all when creating available module streams #10878

Fixes #37137 - Use insert_all when creating available module streams #10878

Conversation

jeremylenz commented Feb 1, 2024 • edited Loading

What are the changes introduced in this pull request?

Considerations taken when implementing this change?

What are the testing steps for this pull request?

jeremylenz commented Feb 2, 2024

chris1984 commented Feb 2, 2024

jeremylenz commented Feb 2, 2024

jeremylenz commented Feb 2, 2024

jeremylenz commented Feb 9, 2024

sbernhard commented Feb 19, 2024

jeremylenz commented Feb 19, 2024

sbernhard Feb 19, 2024

Choose a reason for hiding this comment

sbernhard Feb 19, 2024

Choose a reason for hiding this comment

jeremylenz Feb 19, 2024

Choose a reason for hiding this comment

sbernhard Feb 19, 2024

Choose a reason for hiding this comment

ianballou left a comment

Choose a reason for hiding this comment

ianballou Feb 19, 2024

Choose a reason for hiding this comment

jeremylenz commented Feb 20, 2024

jeremylenz commented Feb 27, 2024

ianballou commented Feb 27, 2024

ianballou left a comment

Choose a reason for hiding this comment

ianballou left a comment

Choose a reason for hiding this comment

jeremylenz commented Feb 1, 2024 •

edited

Loading