-
Notifications
You must be signed in to change notification settings - Fork 365
Drop Diagnostics > Collect Logs #9704
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
9718849 to
16a5f7e
Compare
|
This does not completely remove FileDepot, but it gets rid of one of the largest users of it. This should help narrow down the scope for eventually getting rid of it. |
16a5f7e to
7a3db06
Compare
|
|
||
| # TODO: I think we can drop this now that log collection is dropped, however, PXE needs FileDepot validation, and | ||
| # I'm not sure if this method is also used by PXE validation. Note that application_controller has a log_depot_validate | ||
| # as well, which might be the one used for PXE. By extension, build_uri_settings can probably also be dropped. |
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.
Leaving this TODO in here for now, and I will handle this in a follow up. I don't want to mess with the PXE path and overcomplicate this PR.
| end | ||
| else | ||
| @sb[:active_tab] = "diagnostics_collect_logs" # setting it to show collect logs tab as first tab for the servers that are not started | ||
| @sb[:active_tab] = "diagnostics_summary" # setting it to summary tab as first tab for the servers that are not started |
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.
I'm not entirely sure how to test this as I can't figure out how the code hits this else condition.
|
|
||
| # moved this method here so it can be accessed from pxe_server controller as well | ||
| # this is a terrible name, it doesn't validate log_depots | ||
| # TODO: I think we can move this back to the pxe_server_controller and eliminate all non-pxe code paths |
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.
Also not changing this at the moment as I don't want to mess with PXE.
| "miqValidateButtons('show', 'default_');" | ||
| else | ||
| "miqValidateButtons('hide', 'default_');" | ||
| end |
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.
I honestly don't understand this code nor why there was anything log_depot related in the miq_ae_class_controller in the first place - feels like a copypaste error.
I did verify that on the AeClass method page, that the Validate button correctly enables/disables itself, and it seems to be working as expected.
f8b361b to
6ea45a9
Compare
| end | ||
| end | ||
|
|
||
| context "Toolbar buttons render" do |
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.
I'm not sure about removing this spec - Not sure if it's testing this tab, or just happens to be using this tab to test toolbar button rendering.
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.
I searched the history and this test was added specifically for log collection, so we can drop it.
In containerized deployments we can't support log collection, because we can't mount the log depot. In appliance deployments, we're trying to get away from the application UI having anything to do with the system it's running on to avoid the need to elevate permissions. As such, this commit removes log collection from the UI. Instead, on appliances, users can manually mount an NFS server (or really any server type - even more than our UI supports), and then use the /var/www/miq/vmdb/tools/collect_logs tool. If the user needs scheduling, they can schedule a cron on the appliance to run the tool. Replaces ManageIQ#8235 Co-Authored-By: Melody-Ann-Seda-Marotte <[email protected]>
6ea45a9 to
cef484d
Compare
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.
LGTM. We reviewed this together. Ultimately, there's more to be done but we need to do it in stages. This looks like a good start and should handle the situations where the backend still has things configured to use log depot settings. We'll do that part in the core and schema changes to come.
Follow up to ManageIQ#9704, this drops the log_depot_validate methods. After reviewing it further, these are not used at all. The PxeServer screens were converted to react and call a dedicated validation endpoint under /pxe/pxe_server_async_cred_validation. I also do not see any evidence of scheduled verifications being needed anymore.
In containerized deployments we can't support log collection, because we can't mount the log depot. In appliance deployments, we're trying to get away from the application UI having anything to do with the system it's running on to avoid the need to elevate permissions. As such, this commit removes log collection from the UI.
Instead, on appliances, users can manually mount an NFS server (or really any server type - even more than our UI supports), and then use the /var/www/miq/vmdb/tools/collect_logs tool. If the user needs scheduling, they can schedule a cron on the appliance to run the tool.
Replaces #8235 (cc @MelsHyrule)
@jrafanie @GilbertCherrie Please review.
✂️ 🔥 ✂️ 🔥 ✂️ 🔥 ✂️
Followups:
WIP because I still need to do extensive testing and there are a few questions of PXE still needing file depots, but there being incorrectly named methods like log_depot_validate even when it's not a log depot.