openshift / troubleshooting-panel-console-plugin

Console Plugin to add a troubleshooting side panel to display Korrel8r data
Apache License 2.0
0 stars 7 forks source link

fix(OU-467): refactor and simplify panel logic to fix races #38

Closed alanconway closed 4 months ago

alanconway commented 4 months ago

See https://issues.redhat.com/browse/OU-476 Repeating the same query gave different results, sometimes success, sometimes a JSON parsing error.

Refactored Korrle8rPanel logic to simplify and remove race conditions:

alanconway commented 4 months ago

This is a pure refactor to simplify the code and remove races. I'll rebase my other PRs on this so please take a look @PeterYurkovich

alanconway commented 4 months ago

Re checking for plugins: I think we should enable korrel8r and show results even if console plugins are missing, and then grey out any nodes in the result that cannot be mapped to a console URL as a result. So the check still belongs in the console, but moves to the URL mapping logic instead of the enable/disable of korrel8r as a whole. Korrel8r is intended to be open ended, and should be able to display partial results even if some components are missing - e.g. lack of netobserve should not prevent showing logging results and vice versa - but we should grey out netobserv odes in the result if we can't create a working URL because there's no netobserv plugin.

PeterYurkovich commented 4 months ago

I think the largest issue with moving the logic to the URL mapping would be that the status of the other plugins would need to be rechecked in a number of different places, rather than just one. For example, the topology view performs a URL mapping to check if the current URL matches the Korrel8r query of a node, and then highlights that node if it does.

I think the purpose of the toURL, fromURL and related functions should be pure and not dependent on whether a plugin exists or not. The expected URL should always be the expected URL, even if it doesn't go anywhere.

One thing I think we can do is to move the check further down the component tree, rather than doing it right when the fetch call happens. We can put it in the new topology component you are creating, return the loading state while the checks are going on, then either pass the status down to the Korrel8rTopology file or perform the filter there. There isn't really a need to wait for the checks to happen before making the query to korrel8r, we just need to make sure the checks are done before we leave the loading state

alanconway commented 4 months ago

+1 - probably belongs in the node display logic. My main concern is the check should govern how nodes are displayed (greyed out or whatever) not cause the entire panel to be disabled. I'll write an issue for that so we can fix it later.

PeterYurkovich commented 4 months ago

As far as making the nodes grayed out rather than hidden, I think it could help show the benefits of utilizing those other plugins and shouldn't cause any visual clutter, so it seems like a good idea. I do think that it exceeds the purpose of a refactor though, and should maybe be moved out into a separate PR or issue

alanconway commented 4 months ago

@PeterYurkovich Fixed all comments I think, will add a new issue for displaying nodes with missing plugins.

alanconway commented 4 months ago

Node display issue: https://github.com/openshift/troubleshooting-panel-console-plugin/issues/39

openshift-ci[bot] commented 4 months ago

@alanconway: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes-sigs/prow](https://github.com/kubernetes-sigs/prow/issues/new?title=Prow%20issue:) repository. I understand the commands that are listed [here](https://go.k8s.io/bot-commands).
PeterYurkovich commented 4 months ago

/lgtm

openshift-ci[bot] commented 4 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: alanconway, PeterYurkovich

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/openshift/troubleshooting-panel-console-plugin/blob/main/OWNERS)~~ [PeterYurkovich] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment