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

Reduce the number of queries #32

Closed
wants to merge 2 commits into from
Closed

Conversation

ekohl
Copy link
Member

@ekohl ekohl commented Feb 29, 2024

No description provided.

@ianballou
Copy link
Member

I can integrate this into my solution for the container gateway busy errors / race conditions.

This implements a singleton database connection using dependency
injection. It also aims to reduce the number of queries to a minimum,
aiming for a better performance. Especially with concurrent updates in
mind.
end
repositories = database[:repositories]

entries = user_repo_maps['users'].flat_map do |user_repo_map|
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if import can handle this flat map - you may need to run it per user

@@ -25,6 +25,12 @@ class Plugin < ::Proxy::Plugin
validate :pulp_endpoint, url: true

rackup_path File.join(__dir__, 'container_gateway_http_config.ru')

def load_dependency_injection_wirings(container, settings)
Copy link
Member Author

Choose a reason for hiding this comment

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

Oops, this is a DSL, not a separate class:

Suggested change
def load_dependency_injection_wirings(container, settings)
load_dependency_injection_wirings do |container, settings|

Copy link
Member

Choose a reason for hiding this comment

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

Very strange, for some reason, this dependency injecting causes container gateway endpoints to return 404s. Something is bugging Sinatra.

Copy link
Member

Choose a reason for hiding this comment

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

Actually, it seems to be killing the entire container gateway plugin, after refreshing the feature has disappeared. Interesting ...

Copy link
Member

@ianballou ianballou Mar 6, 2024

Choose a reason for hiding this comment

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

Disabling all modules in the group ['container_gateway'] due to a failure in one of them: undefined method 'sqlite_db_path' for #<Hash, okay, getting closer ...

Copy link
Member

Choose a reason for hiding this comment

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

Ah of course, Database was module instead of a class. Things got easier to debug after I found out more logs were available on the smart proxy in the UI.

Copy link
Member Author

Choose a reason for hiding this comment

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

You should also have then in the regular process. Perhaps you need to raise the log level to see them in the console


def migrate_db(db_connection, container_gateway_path)
Sequel::Migrator.run(db_connection, "#{container_gateway_path}/smart_proxy_container_gateway/sequel_migrations")
# TODO: create a background service that does this
Copy link
Member Author

Choose a reason for hiding this comment

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

You can start a service within smart-proxy.

For example: https://github.com/theforeman/smart-proxy/blob/e8fe59f83843ca619264e8d4757bc99fcde93fa9/modules/dhcp_isc/dhcp_isc_plugin.rb#L18

This is configured via: https://github.com/theforeman/smart-proxy/blob/e8fe59f83843ca619264e8d4757bc99fcde93fa9/modules/dhcp_isc/configuration_loader.rb#L28

Note you can use container.get_dependency(:database) there to pass the DB connection.

It's then a regular class: https://github.com/theforeman/smart-proxy/blob/develop/modules/dhcp_common/free_ips.rb

As you scale up, this may work better.

@@ -4,11 +4,13 @@
require 'sequel'
module Proxy
module ContainerGateway
extend Proxy::DHCP::DependencyInjection
Copy link
Member Author

Choose a reason for hiding this comment

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

Clearly this needs to be something else than DHCP, so pretty much a copy of https://github.com/theforeman/smart-proxy/blob/develop/modules/dhcp/dependency_injection.rb

extend ::Proxy::Util
extend ::Proxy::Log

inject_attr :database, :database
Copy link
Member Author

Choose a reason for hiding this comment

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

This provides a property database on an instance, but I see everything is static. It would be better to refactor the class so it can be instantiated and inserted via dependency injection.

Another note: database is an instance of Proxy::ContainerGateway::Database, so you may need to use database.connection (and expose that) or change the dependency injection to provide a Sequel::Database instance.

Comment on lines +6 to +8
class << self
Sequel.extension :migration, :core_extensions
end
Copy link
Member Author

Choose a reason for hiding this comment

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

I don't know if this is a correct implementation. Perhaps it needs to be done before the connect call. It also needs a attr_reader :connection to expose the connection.

@ianballou
Copy link
Member

@ekohl to make sure I understand the thinking behind a single DB connection -- by doing so we lose the ability to do concurrent writes to SQLite (which could be enabled via WAL), but in our case that would solve the race condition problem.
Sequel does have nice support for a database connection pool, but implementing locking could have its own issues.

Even here at the end it mentions the best way to avoid race conditions is by running single threaded: https://sequel.jeremyevans.net/2010/03/09/pessimistic-locking.html

Speaking of which, if we're using a single DB connection, would it also make sense to enable single_threaded for the connection? https://sequel.jeremyevans.net/rdoc/files/doc/opening_databases_rdoc.html#label-General+connection+options

@ianballou
Copy link
Member

ianballou commented Mar 1, 2024

I just did a quick experiment with adding some locks the base code + switching to WAL mode -- the DB did lock up under stress still. Seems pessimistic locking + WAL mode counteract each other.

I think the results are no surprise. I'll do some stress-testing next with the bulk inserts.

diff --git a/lib/smart_proxy_container_gateway/container_gateway_main.rb b/lib/smart_proxy_container_gateway/container_gateway_main.rb
index 22be2d4..5c3169d 100644
--- a/lib/smart_proxy_container_gateway/container_gateway_main.rb
+++ b/lib/smart_proxy_container_gateway/container_gateway_main.rb
@@ -109,12 +109,14 @@ module Proxy
             next if repos.nil?
 
             repos.each do |repo|
-              found_repo = Repository.find(name: repo['repository'],
-                                           auth_required: repo['auth_required'].to_s.downcase == "true")
-              if found_repo.nil?
-                logger.warn("#{repo['repository']} does not exist in this smart proxy's environments")
-              elsif found_repo.auth_required
-                found_repo.add_user(User.find(name: user))
+              Repository.db.transaction do
+                found_repo = Repository.find(name: repo['repository'],
+                                            auth_required: repo['auth_required'].to_s.downcase == "true")&.lock!
+                if found_repo.nil?
+                  logger.debug("#{repo['repository']} does not exist in this smart proxy's environments")
+                elsif found_repo.auth_required
+                  found_repo.add_user(User.find(name: user))
+                end
               end
             end
           end
@@ -123,14 +125,18 @@ module Proxy
 
       # Replaces the user-repo mapping for a single user
       def update_user_repositories(username, repositories)
-        user = User.where(name: username).first
-        user.remove_all_repositories
-        repositories.each do |repo_name|
-          found_repo = Repository.find(name: repo_name)
-          if found_repo.nil?
-            logger.warn("#{repo_name} does not exist in this smart proxy's environments")
-          elsif user.repositories_dataset.where(name: repo_name).first.nil? && found_repo.auth_required
-            user.add_repository(found_repo)
+        User.db.transaction do
+          user = User.where(name: username)&.first&.lock!
+          user.remove_all_repositories
+          repositories.each do |repo_name|
+            Repository.db.transaction do
+              found_repo = Repository.find(name: repo_name)&.lock!
+              if found_repo.nil?
+                logger.debug("#{repo_name} does not exist in this smart proxy's environments")
+              elsif user.repositories_dataset.where(name: repo_name).first.nil? && found_repo.auth_required
+                user.add_repository(found_repo)
+              end
+            end
           end
         end
       end
@@ -176,6 +182,7 @@ module Proxy
         file_path = Proxy::ContainerGateway::Plugin.settings.sqlite_db_path
         sqlite_timeout = Proxy::ContainerGateway::Plugin.settings.sqlite_timeout
         conn = Sequel.connect("sqlite://#{file_path}", timeout: sqlite_timeout)
+        conn.run 'PRAGMA journal_mode=WAL'
         container_gateway_path = $LOAD_PATH.detect { |path| path.include? 'smart_proxy_container_gateway' }
         begin
           Sequel::Migrator.check_current(conn, "#{container_gateway_path}/smart_proxy_container_gateway/sequel_migrations")

end
conn
end
user = database[:users].find_or_create(name: username)
Copy link
Member

Choose a reason for hiding this comment

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

Looks like find_or_create is expected to run on a model rather than a dataset.


repository_users = database[:repository_users]
repository_users.delete
repositories_users.import([:repository_id, :user_id], entries)
Copy link
Member

@ianballou ianballou Mar 6, 2024

Choose a reason for hiding this comment

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

Mental note: If ignoring the changes to use a single DB connection, issues can pop up still during concurrent smart proxy syncs:

2024-03-06T16:20:27 848a5696 [W] Error processing request '848a5696-6ca6-4138-898c-36377e3bfbea: <Sequel::UniqueConstraintViolation>: SQLite3::ConstraintException: UNIQUE constraint failed: repositories_users.repository_id, repositories_users.user_id

Also, I had to switch to importing DB IDs in entries -- seems the Sequel Query there is disagreeing with import. I wonder if it's because the query above repositories.where(name: user_repo_names, auth_required: true).select(:id, user_id: user.id) needs a joins to repositories_users.

@ekohl
Copy link
Member Author

ekohl commented Mar 6, 2024

So you notice it was all more of a general direction first pass attempt. Many details are wrong. Sorry about that, but I wanted to share before heading out for the day

@ianballou
Copy link
Member

So you notice it was all more of a general direction first pass attempt. Many details are wrong. Sorry about that, but I wanted to share before heading out for the day

Totally understand, I was posting some comments to keep track of my thoughts.

I'm pushing my continuation of this work here: #33

@ekohl
Copy link
Member Author

ekohl commented Mar 7, 2024

Totally understand, I was posting some comments to keep track of my thoughts.

And rightfully so. As you could see, I did the same thing with annotating my own thoughts.

@ianballou
Copy link
Member

Closing since f96702c is merged.
Thanks for the help!

@ianballou ianballou closed this Apr 15, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants