-
Notifications
You must be signed in to change notification settings - Fork 60
Repository sync tasks fail with 429 Too Many Requests because IoP vmaas reposcan syncs are requested when one is already in progress #1210
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
Open
Odilhao
wants to merge
4
commits into
theforeman:foreman_3_16
Choose a base branch
from
Odilhao:cut-12.2.19
base: foreman_3_16
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
+301
−2
Open
Changes from 3 commits
Commits
Show all changes
4 commits
Select commit
Hold shift + click to select a range
9f43987
Add VMaaS reposcan sync task for repository sync (#1078)
nofaralfasi 8f27551
Add X-Org-Id header for IoP requests
ShimShtein 407a17b
Fix VmaasReposcanSync errors preventing parent task success (#1180)
jeremylenz ac5316b
Release 12.2.19
Odilhao File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| require 'rest-client' | ||
|
|
||
| module InsightsCloud | ||
| module Async | ||
| # Triggers VMaaS reposcan sync via IoP gateway when repositories are synced | ||
| class VmaasReposcanSync < ::Actions::EntryAction | ||
| include ::ForemanRhCloud::CertAuth | ||
|
|
||
| HTTP_TOO_MANY_REQUESTS = 429 | ||
|
|
||
| # Subscribe to Katello repository sync hook action, if available | ||
| def self.subscribe | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this being cherry-picked to this branch? Without the subscription here, the class is never invoked. That means (as far as I can tell) we are cherry-picking the VmaasReposcanSync feature itself. Is that what's intended? |
||
| 'Actions::Katello::Repository::SyncHook'.constantize | ||
| rescue NameError | ||
| Rails.logger.debug('VMaaS reposcan sync: Repository::SyncHook action not found') | ||
| nil | ||
| end | ||
|
|
||
| def plan(repo, *_args) | ||
| return unless ::ForemanRhCloud.with_iop_smart_proxy? | ||
|
|
||
| repo_id = repo.is_a?(Hash) ? (repo[:id] || repo['id']) : nil | ||
| unless repo_id | ||
| logger.error("VMaaS reposcan sync: missing repository id in SyncHook plan parameters: #{repo.inspect}") | ||
| return | ||
| end | ||
|
|
||
| organization_id = Katello::Repository.find(repo_id).organization_id | ||
|
|
||
| plan_self(organization_id: organization_id) | ||
| end | ||
|
|
||
| def run | ||
| url = ::InsightsCloud.vmaas_reposcan_sync_url | ||
|
|
||
| response = execute_cloud_request( | ||
| organization: organization, | ||
| method: :put, | ||
| url: url, | ||
| headers: { 'Content-Type' => 'application/json' } | ||
| ) | ||
|
|
||
| if response.code >= 200 && response.code < 300 | ||
| message = "VMaaS reposcan sync triggered successfully: #{response.code}" | ||
| logger.info(message) | ||
| else | ||
| message = "VMaaS reposcan sync failed with status: #{response.code}, body: #{response.body}" | ||
| logger.error(message) | ||
| end | ||
| output[:message] = message | ||
|
|
||
| response | ||
| rescue RestClient::ExceptionWithResponse => e | ||
| handle_rest_client_error(e) | ||
| rescue StandardError => e | ||
| handle_standard_error(e) | ||
| end | ||
|
|
||
| def rescue_strategy_for_self | ||
| Dynflow::Action::Rescue::Skip | ||
| end | ||
|
|
||
| def organization | ||
| @organization ||= Organization.find(input[:organization_id]) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def handle_rest_client_error(exception) | ||
| if exception.response&.code == HTTP_TOO_MANY_REQUESTS | ||
| message = "VMaaS reposcan sync skipped: another sync already in progress (#{HTTP_TOO_MANY_REQUESTS})" | ||
| logger.warn(message) | ||
| else | ||
| message = "VMaaS reposcan sync failed: #{exception.response&.code} - #{exception.response&.body}" | ||
| logger.error(message) | ||
| end | ||
| output[:message] = message | ||
| # Do NOT raise - let rescue_strategy_for_self Skip handle this | ||
| end | ||
|
|
||
| def handle_standard_error(exception) | ||
| message = "Error triggering VMaaS reposcan sync: #{exception.message}" | ||
| logger.error(message) | ||
| output[:message] = message | ||
| # Do NOT raise - let rescue_strategy_for_self Skip handle this | ||
| end | ||
|
|
||
| def logger | ||
| action_logger | ||
| end | ||
| end | ||
| end | ||
| end | ||
202 changes: 202 additions & 0 deletions
202
test/unit/lib/insights_cloud/async/vmaas_reposcan_sync_test.rb
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,202 @@ | ||
| require 'test_plugin_helper' | ||
| require 'foreman_tasks/test_helpers' | ||
|
|
||
| class VmaasReposcanSyncTest < ActiveSupport::TestCase | ||
| include ForemanTasks::TestHelpers::WithInThreadExecutor | ||
|
|
||
| setup do | ||
| @organization = FactoryBot.create(:organization) | ||
| # Create a simple repository - we only need id and organization_id for the action | ||
| @repo = ::Katello::Repository.new(id: 1) | ||
| @repo.stubs(:organization_id).returns(@organization.id) | ||
| ::Katello::Repository.stubs(:find).with(1).returns(@repo) | ||
|
|
||
| @repo_payload = { id: @repo.id } | ||
| @expected_url = 'https://example.com/api/v1/vmaas/reposcan/sync' | ||
| InsightsCloud.stubs(:vmaas_reposcan_sync_url).returns(@expected_url) | ||
| ForemanRhCloud.stubs(:with_iop_smart_proxy?).returns(true) | ||
| Organization.stubs(:find).with(@organization.id).returns(@organization) | ||
| end | ||
|
|
||
| # Planning behavior | ||
| test 'plan plans_self when repo payload has id and IoP is available' do | ||
| InsightsCloud::Async::VmaasReposcanSync.any_instance.expects(:plan_self).once | ||
|
|
||
| ForemanTasks.sync_task(InsightsCloud::Async::VmaasReposcanSync, @repo_payload) | ||
| end | ||
|
|
||
| test 'plan does not plan_self when repo payload is missing id' do | ||
| payload_without_id = {} | ||
|
|
||
| mock_logger = mock('logger') | ||
| mock_logger.expects(:error).with { |msg| msg =~ /missing repository id/i } | ||
| InsightsCloud::Async::VmaasReposcanSync.any_instance.stubs(:logger).returns(mock_logger) | ||
| InsightsCloud::Async::VmaasReposcanSync.any_instance.expects(:plan_self).never | ||
|
|
||
| ForemanTasks.sync_task(InsightsCloud::Async::VmaasReposcanSync, payload_without_id) | ||
| end | ||
|
|
||
| test 'plan does not plan_self when IoP smart proxy is not available' do | ||
| ForemanRhCloud.stubs(:with_iop_smart_proxy?).returns(false) | ||
|
|
||
| InsightsCloud::Async::VmaasReposcanSync.any_instance.expects(:plan_self).never | ||
|
|
||
| ForemanTasks.sync_task(InsightsCloud::Async::VmaasReposcanSync, @repo_payload) | ||
| end | ||
|
|
||
| test 'plan skips repo_id validation when IoP smart proxy is not available' do | ||
| ForemanRhCloud.stubs(:with_iop_smart_proxy?).returns(false) | ||
| payload_without_id = {} | ||
|
|
||
| # Logger should not be called since IoP check returns early | ||
| InsightsCloud::Async::VmaasReposcanSync.any_instance.expects(:logger).never | ||
| InsightsCloud::Async::VmaasReposcanSync.any_instance.expects(:plan_self).never | ||
|
|
||
| ForemanTasks.sync_task(InsightsCloud::Async::VmaasReposcanSync, payload_without_id) | ||
| end | ||
|
|
||
| test 'plan does not plan_self when repository is nil' do | ||
| mock_logger = mock('logger') | ||
| mock_logger.expects(:error).with { |msg| msg =~ /missing repository id/i } | ||
| InsightsCloud::Async::VmaasReposcanSync.any_instance.stubs(:logger).returns(mock_logger) | ||
| InsightsCloud::Async::VmaasReposcanSync.any_instance.expects(:plan_self).never | ||
|
|
||
| ForemanTasks.sync_task(InsightsCloud::Async::VmaasReposcanSync, nil) | ||
| end | ||
|
|
||
| # Run behavior | ||
| test 'run triggers VMaaS reposcan sync successfully' do | ||
| mock_response = mock('response') | ||
| mock_response.stubs(:code).returns(200) | ||
|
|
||
| InsightsCloud::Async::VmaasReposcanSync.any_instance | ||
| .expects(:execute_cloud_request) | ||
| .with do |params| | ||
| params[:method] == :put && | ||
| params[:url] == @expected_url && | ||
| params[:headers].is_a?(Hash) && | ||
| params[:headers]['Content-Type'] == 'application/json' && | ||
| params[:organization] == @organization | ||
| end | ||
| .returns(mock_response) | ||
|
|
||
| task = ForemanTasks.sync_task(InsightsCloud::Async::VmaasReposcanSync, @repo_payload) | ||
|
|
||
| assert_equal 'VMaaS reposcan sync triggered successfully: 200', task.output[:message] | ||
| end | ||
|
|
||
| test 'run sets error message in task output for failed response' do | ||
| mock_response = mock('response') | ||
| mock_response.stubs(:code).returns(500) | ||
| mock_response.stubs(:body).returns('Internal Server Error') | ||
|
|
||
| InsightsCloud::Async::VmaasReposcanSync.any_instance | ||
| .stubs(:execute_cloud_request) | ||
| .returns(mock_response) | ||
|
|
||
| task = ForemanTasks.sync_task(InsightsCloud::Async::VmaasReposcanSync, @repo_payload) | ||
|
|
||
| assert_equal 'VMaaS reposcan sync failed with status: 500, body: Internal Server Error', task.output[:message] | ||
| end | ||
|
|
||
| test 'run sets error message in task output for RestClient exception' do | ||
| error_response = mock('error_response') | ||
| error_response.stubs(:code).returns(500) | ||
| error_response.stubs(:body).returns('Server Error') | ||
| exception = RestClient::ExceptionWithResponse.new(error_response) | ||
|
|
||
| InsightsCloud::Async::VmaasReposcanSync.any_instance | ||
| .stubs(:execute_cloud_request) | ||
| .raises(exception) | ||
|
|
||
| task = ForemanTasks.sync_task(InsightsCloud::Async::VmaasReposcanSync, @repo_payload) | ||
|
|
||
| assert_equal 'VMaaS reposcan sync failed: 500 - Server Error', task.output[:message] | ||
| end | ||
|
|
||
| test 'run sets error message in task output for StandardError exception' do | ||
| mock_logger = mock('logger') | ||
| mock_logger.expects(:error).with('Error triggering VMaaS reposcan sync: Network timeout') | ||
| InsightsCloud::Async::VmaasReposcanSync.any_instance.stubs(:logger).returns(mock_logger) | ||
|
|
||
| InsightsCloud::Async::VmaasReposcanSync.any_instance | ||
| .stubs(:execute_cloud_request) | ||
| .raises(StandardError.new('Network timeout')) | ||
|
|
||
| task = ForemanTasks.sync_task(InsightsCloud::Async::VmaasReposcanSync, @repo_payload) | ||
|
|
||
| assert_equal 'Error triggering VMaaS reposcan sync: Network timeout', task.output[:message] | ||
| end | ||
|
|
||
| test 'run logs and handles error response without raising' do | ||
| error_response = mock('error_response') | ||
| error_response.stubs(:code).returns(500) | ||
| error_response.stubs(:body).returns('error') | ||
| exception = RestClient::ExceptionWithResponse.new(error_response) | ||
|
|
||
| InsightsCloud::Async::VmaasReposcanSync.any_instance | ||
| .stubs(:execute_cloud_request) | ||
| .raises(exception) | ||
|
|
||
| task = ForemanTasks.sync_task(InsightsCloud::Async::VmaasReposcanSync, @repo_payload) | ||
|
|
||
| assert_equal 'VMaaS reposcan sync failed: 500 - error', task.output[:message] | ||
| end | ||
|
|
||
| test 'run handles 429 error with warning log level' do | ||
| error_response = mock('error_response') | ||
| error_response.stubs(:code).returns(429) | ||
| error_response.stubs(:body).returns('{"msg": "Another task already in progress"}') | ||
| exception = RestClient::ExceptionWithResponse.new(error_response) | ||
|
|
||
| mock_logger = mock('logger') | ||
| mock_logger.expects(:warn).with('VMaaS reposcan sync skipped: another sync already in progress (429)') | ||
| InsightsCloud::Async::VmaasReposcanSync.any_instance.stubs(:logger).returns(mock_logger) | ||
|
|
||
| InsightsCloud::Async::VmaasReposcanSync.any_instance | ||
| .stubs(:execute_cloud_request) | ||
| .raises(exception) | ||
|
|
||
| task = ForemanTasks.sync_task(InsightsCloud::Async::VmaasReposcanSync, @repo_payload) | ||
|
|
||
| assert_equal 'VMaaS reposcan sync skipped: another sync already in progress (429)', | ||
| task.output[:message] | ||
| end | ||
|
|
||
| test 'run handles non-429 errors with error log level' do | ||
| error_response = mock('error_response') | ||
| error_response.stubs(:code).returns(500) | ||
| error_response.stubs(:body).returns('Internal Server Error') | ||
| exception = RestClient::ExceptionWithResponse.new(error_response) | ||
|
|
||
| mock_logger = mock('logger') | ||
| mock_logger.expects(:error).with('VMaaS reposcan sync failed: 500 - Internal Server Error') | ||
| InsightsCloud::Async::VmaasReposcanSync.any_instance.stubs(:logger).returns(mock_logger) | ||
|
|
||
| InsightsCloud::Async::VmaasReposcanSync.any_instance | ||
| .stubs(:execute_cloud_request) | ||
| .raises(exception) | ||
|
|
||
| task = ForemanTasks.sync_task(InsightsCloud::Async::VmaasReposcanSync, @repo_payload) | ||
|
|
||
| assert_equal 'VMaaS reposcan sync failed: 500 - Internal Server Error', | ||
| task.output[:message] | ||
| end | ||
|
|
||
| test 'run handles RestClient::ExceptionWithResponse with nil response' do | ||
| exception = RestClient::ExceptionWithResponse.new(nil) | ||
|
|
||
| mock_logger = mock('logger') | ||
| mock_logger.expects(:error).with('VMaaS reposcan sync failed: - ') | ||
| InsightsCloud::Async::VmaasReposcanSync.any_instance.stubs(:logger).returns(mock_logger) | ||
|
|
||
| InsightsCloud::Async::VmaasReposcanSync.any_instance | ||
| .stubs(:execute_cloud_request) | ||
| .raises(exception) | ||
|
|
||
| task = ForemanTasks.sync_task(InsightsCloud::Async::VmaasReposcanSync, @repo_payload) | ||
|
|
||
| refute_nil task.output[:message] | ||
| assert_equal 'VMaaS reposcan sync failed: - ', task.output[:message] | ||
| end | ||
| end |
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion (bug_risk): Consider more robust URL construction to avoid double slashes and missing trailing slashes.
String concatenation here assumes a specific trailing-slash convention on
ForemanRhCloud.iop_smart_proxy.url, which can easily change and silently produce malformed URLs (//api/...or missing/). Prefer a URL-joining helper (e.g.,URI.joinor a dedicated method) so the path is composed correctly regardless of configuration.Suggested implementation:
If
URIis not already loaded in this file or a common initializer, addrequire 'uri'near the top oflib/insights_cloud.rb(or in a shared place) soURI.joinis available.