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 #37356 - Allow turning automatic content count updates off #11014

Merged
merged 1 commit into from
Jun 18, 2024

Conversation

sjha4
Copy link
Member

@sjha4 sjha4 commented Jun 4, 2024

What are the changes introduced in this pull request?

Add a setting "automatic_content_count_updates" to control automatic invocation of Update content count task on proxies upon successful syncs.

Considerations taken when implementing this change?

The Update content counts action is triggered asynchronously upon a successful sync on a proxy.
This task can be a long running task for large proxies on a slow connection and this can cause slowness in syncs in the queue.

What are the testing steps for this pull request?

  1. Turn the setting "Calculate content counts on smart proxies automatically" on and off and check if Update content counts is run automatically or not depending on the value of the setting.

@wbclark wbclark self-assigned this Jun 5, 2024
@sjha4 sjha4 force-pushed the content_count_setting branch from 7854239 to 3d4f099 Compare June 5, 2024 18:04
@sjha4
Copy link
Member Author

sjha4 commented Jun 5, 2024

Rebased for pipeline..

Copy link

@vsedmik vsedmik left a comment

Choose a reason for hiding this comment

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

ACK, packit/PRT at the companion PR passed: SatelliteQE/robottelo#15371 (comment)

@sbernhard
Copy link
Member

sbernhard commented Jun 7, 2024

This task can be a long running task for large proxies on a slow connection and this can cause slowness in syncs in the queue.

I wonder, why a async task can slow down other tasks. Instead of adding a new setting and make this feature configuruable, wouldn't it be better to think about how this feature can be improved?

E.g.:
https://github.com/Katello/katello/blame/5c17dce4c6710e8d105794880e1a8baa4f5a726b/app/models/katello/content_view_version.rb#L349 count vs size (see https://medium.com/unagi/ruby-on-rails-length-vs-size-vs-count-with-examples-a32e17274c79)

@sjha4
Copy link
Member Author

sjha4 commented Jun 10, 2024

This seems to happen for users with slow master -> proxy connections and the API call to pulp takes a long time. This is a band-aid for such users to turn it off and run this manually when they need to.

We have some improvements to this in the pipeline which include making the task granular and only run on contents that have actually changed but that will be follow up PR.

Copy link
Member

@sbernhard sbernhard left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@wbclark wbclark left a comment

Choose a reason for hiding this comment

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

Sorry for the delay here and thanks for your patience.

Code LGTM other than the alignment nit.

I thought about asking for a test, but looking at test/actions/katello/capsule_content_test.rb and then app/lib/actions/katello/capsule_content/update_content_counts.rb there doesn't seem enough need

lib/katello/plugin.rb Outdated Show resolved Hide resolved
@sjha4 sjha4 force-pushed the content_count_setting branch from 3d4f099 to 938d092 Compare June 18, 2024 15:23
@sjha4 sjha4 merged commit 98dfce0 into Katello:master Jun 18, 2024
25 checks passed
parthaa pushed a commit to parthaa/katello that referenced this pull request Jun 26, 2024
parthaa pushed a commit that referenced this pull request Jun 26, 2024
sbernhard added a commit to ATIX-AG/katello that referenced this pull request Jul 24, 2024
[PR] Fixes #37356 - Allow turning automatic content count updates off (Katello#11014)

See merge request orcharhino/foreman_components/katello!169
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants