-
Notifications
You must be signed in to change notification settings - Fork 303
Fixes #38954 - N-1/2 smart proxy sync failure due to missing API endpoints #11597
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
Conversation
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.
Hey there - I've reviewed your changes - here's some feedback:
- The
rescue StandardErrorblocks are quite broad here; consider rescuing the specific error types you expect (e.g.,RestClient::NotFound,ProxyAPI::ProxyException) to avoid unintentionally hiding unrelated failures. - The NotFound detection and logging logic is duplicated between
update_container_gateway_hostsandupdate_host_container_repo_mapping; consider extracting a small helper method to DRY this up and keep the conditions consistent. - Logging the full backtrace at warn level could be noisy in production; you might want to log the message at warn and move the backtrace to debug or a separate conditional log when more detail is required.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The `rescue StandardError` blocks are quite broad here; consider rescuing the specific error types you expect (e.g., `RestClient::NotFound`, `ProxyAPI::ProxyException`) to avoid unintentionally hiding unrelated failures.
- The NotFound detection and logging logic is duplicated between `update_container_gateway_hosts` and `update_host_container_repo_mapping`; consider extracting a small helper method to DRY this up and keep the conditions consistent.
- Logging the full backtrace at warn level could be noisy in production; you might want to log the message at warn and move the backtrace to debug or a separate conditional log when more detail is required.
## Individual Comments
### Comment 1
<location> `app/models/katello/concerns/smart_proxy_extensions.rb:279-277` </location>
<code_context>
}
end
ProxyAPI::ContainerGateway.new(url: self.url).update_hosts({ hosts: hosts })
+ rescue StandardError => e
+ if e.is_a?(RestClient::NotFound) || (e.is_a?(ProxyAPI::ProxyException) && e.respond_to?(:wrapped_exception) && e.wrapped_exception.is_a?(RestClient::NotFound))
+ Rails.logger.warn("Capsule #{name} does not support the update_hosts endpoint (likely running an older version). Skipping host updates.")
+ else
+ Rails.logger.warn("Failed to update hosts for capsule #{name}: #{e.message}")
+ Rails.logger.warn(e.backtrace.join("\n"))
+ end
end
</code_context>
<issue_to_address>
**issue (bug_risk):** Rescuing `StandardError` and only logging can hide unexpected failures.
Catching `StandardError` here will hide any unexpected failures in building the host list or calling the API, only emitting a warning. That can mask production issues and leave updates partially applied. Prefer rescuing only the known API-related exceptions (e.g., specific `RestClient`/`ProxyAPI` errors), and allow other errors to bubble up or be re-raised after logging (except for the `NotFound` cases you intentionally ignore).
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
ianballou
left a comment
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.
Code looks fine, we just need to text the varying error conditions.
| end | ||
| ProxyAPI::ContainerGateway.new(url: self.url).update_hosts({ hosts: hosts }) | ||
| rescue StandardError => e | ||
| if e.is_a?(RestClient::NotFound) || (e.is_a?(ProxyAPI::ProxyException) && e.respond_to?(:wrapped_exception) && e.wrapped_exception.is_a?(RestClient::NotFound)) |
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.
Since Katello controls all the code here, it's still uncertain if the 404 will return as NotFound or a ProxyException?
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.
It is a ProxyAPI::ProxyException that wraps the RestClient::NotFound error. Checking with or as a precaution but is unnecessary..Let me verify the exception type again to refresh my memory and update this.
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.
Updated to remove the first condition and added the safe &. operator to handle any weirdness..
d5237cc to
a610595
Compare
a610595 to
f3d5713
Compare
ianballou
left a comment
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.
N-2 capsule syncing works now, thanks!
What are the changes introduced in this pull request?
Rescue errors for API endpoints not being found on old N-1, N-2 smart proxies and allow the sync to succeed. Add log warnings when older smart proxy version is detected.
Considerations taken when implementing this change?
This also catches all StandardErrors and logs them as warnings instead of failing the sync cause an error impacts all content types blocking use of proxy whereas a warning would allow proxy to sync and the only issue there will be around container cert auth for hosts registered to the proxy which can be resolved by falling back to basic auth or retrying the sync after debugging the issue in case of failures while keeping proxies usable.
What are the testing steps for this pull request?
With this PR, instead of error, the task will succeed with warning in logs as below:
Summary by Sourcery
Handle missing container gateway API endpoints on older smart proxies without failing sync tasks.
Bug Fixes:
Enhancements: