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

COO-169: feat: add troubleshooting panel and distributed tracing ui-plugin #480

Conversation

PeterYurkovich
Copy link
Contributor

@PeterYurkovich PeterYurkovich commented May 9, 2024

COO-64
COO-169
OU-378
OU-314
OU-423

This PR adds the troubleshooting panel console plugin and distributed tracing console plugin to the observability operator.

@PeterYurkovich PeterYurkovich requested a review from a team as a code owner May 9, 2024 21:18
@PeterYurkovich PeterYurkovich requested review from jan--f and slashpai and removed request for a team May 9, 2024 21:18
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented May 9, 2024

@PeterYurkovich: This pull request references OU-378 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target either version "4.16." or "openshift-4.16.", but it targets "OpenShift 4.16" instead.

In response to this:

This PR addresses OU-378 and looks to add the Troubleshooting Panel console plugin to COO.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

Copy link

openshift-ci bot commented May 9, 2024

Hi @PeterYurkovich. Thanks for your PR.

I'm waiting for a rhobs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

@PeterYurkovich PeterYurkovich changed the title OU-378: Add Troubleshooting Panel OU-378: Add UIPlugin for Troubleshooting Panel May 10, 2024
@@ -251,7 +251,8 @@ func (rm resourceManager) registerPluginWithConsole(ctx context.Context, pluginI
OperatorSpec: operatorv1.OperatorSpec{
ManagementState: operatorv1.Managed,
},
Plugins: clusterPlugins,
Plugins: clusterPlugins,
Customization: *cluster.Spec.Customization.DeepCopy(),
Copy link
Contributor

Choose a reason for hiding this comment

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

I wonder if this is really needed, this is a last resource path we are using and we should not change other things unrelated to plugins

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had found that the spec.customization.customLogoFile.key was being unset when the Patch method was called in reconciler.go for the Merger.Reconcile. This leads the Console Operator to get stuck in a reconciliation loop. This issue will pop up on all consoles using a custom image like ROSA. All this line does is ensure that the customization is available on both sides of the patch.

However, although this only happened with the customization it's possible it could happen with other items. I'm not exactly certain how to go about debugging this, but I'd be happy to if someone could point me in the right direction.

pkg/controllers/uiplugin/components.go Outdated Show resolved Hide resolved
pkg/controllers/uiplugin/components.go Outdated Show resolved Hide resolved
@PeterYurkovich PeterYurkovich marked this pull request as draft May 13, 2024 14:28
@PeterYurkovich PeterYurkovich changed the title OU-378: Add UIPlugin for Troubleshooting Panel OU-378: Add UIPlugin for Troubleshooting Panel and Distributed Tracing Console Plugins May 17, 2024
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented May 17, 2024

@PeterYurkovich: This pull request references OU-378 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target either version "4.16." or "openshift-4.16.", but it targets "OpenShift 4.16" instead.

In response to this:

This PR addresses OU-378 and OU-314 and looks to add the Troubleshooting Panel console plugin and distributed tracing console plugin to the observability operator.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@PeterYurkovich PeterYurkovich marked this pull request as ready for review May 17, 2024 01:37
@PeterYurkovich PeterYurkovich changed the title OU-378: Add UIPlugin for Troubleshooting Panel and Distributed Tracing Console Plugins OU-378 OU-314: Add UIPlugin for Troubleshooting Panel and Distributed Tracing Console Plugins May 17, 2024
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented May 17, 2024

@PeterYurkovich: This pull request references OU-314 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.16.0" version, but no target version was set.

In response to this:

This PR addresses OU-378 and OU-314 and looks to add the Troubleshooting Panel console plugin and distributed tracing console plugin to the observability operator.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@PeterYurkovich PeterYurkovich force-pushed the ou-378-add-troubleshooting-panel branch 3 times, most recently from 03dd41c to 8e8c731 Compare May 21, 2024 18:09
@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented May 21, 2024

@PeterYurkovich: This pull request references OU-314 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.17.0" version, but no target version was set.

In response to this:

This PR addresses looks to address OU-378, OU-314, OU-423, andCOO-64. It adds the troubleshooting panel console plugin and distributed tracing console plugin to the observability operator.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@openshift-ci-robot
Copy link
Collaborator

openshift-ci-robot commented May 21, 2024

@PeterYurkovich: This pull request references OU-314 which is a valid jira issue.

Warning: The referenced jira issue has an invalid target version for the target branch this PR targets: expected the task to target the "4.17.0" version, but no target version was set.

In response to this:

This PR addresses looks to address OU-378, OU-314, OU-423, and COO-64. It adds the troubleshooting panel console plugin and distributed tracing console plugin to the observability operator.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@PeterYurkovich PeterYurkovich force-pushed the ou-378-add-troubleshooting-panel branch 2 times, most recently from de84a0d to f5ebcf9 Compare May 22, 2024 13:30
@PeterYurkovich PeterYurkovich changed the title OU-378 OU-314: Add UIPlugin for Troubleshooting Panel and Distributed Tracing Console Plugins feat: add troubleshooting panel and distributed tracing ui-plugin May 22, 2024
@openshift-ci-robot
Copy link
Collaborator

@PeterYurkovich: No Jira issue is referenced in the title of this pull request.
To reference a jira issue, add 'XYZ-NNN:' to the title of this pull request and request another refresh with /jira refresh.

In response to this:

This PR addresses looks to address OU-378, OU-314, OU-423, and COO-64. It adds the troubleshooting panel console plugin and distributed tracing console plugin to the observability operator.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

@PeterYurkovich PeterYurkovich force-pushed the ou-378-add-troubleshooting-panel branch from b32f71f to 11ae0af Compare June 4, 2024 15:00
@danielmellado
Copy link
Contributor

/ok-to-test

@danielmellado
Copy link
Contributor

/lgtm

@danielmellado
Copy link
Contributor

/hold

@danielmellado
Copy link
Contributor

@tremes @simonpasquier this lgtm now but I'd like another set of eyes, holding until then!


### Troubleshooting Panel

The plugin will connect to a Korrel8r instance named `korrel8r` in the `korrel8r` namespace. A "Troubleshooting Panel" button is added to the alerts page, which will convert the current alert into a Korrel8r query, then retrieve related neighbors and display them in a topology view.
Copy link

Choose a reason for hiding this comment

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

This is not accurate but can be changed with #497 by @shwetaap

Copy link
Contributor

Choose a reason for hiding this comment

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

Changed it in the PR #497

Comment on lines +57 to +61
// korrel8r defines the Korrel8r instance that the troubleshooting panel plugin will connect to
//
// +optional
// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Korrel8r Instance"
Korrel8r TroubleshootingPanelKorrel8rConfig `json:"korrel8r,omitempty"`
Copy link

Choose a reason for hiding this comment

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

Again not accurate when #497 lands because we can eliminate this field and the struct below. (cc @shwetaap )

Copy link
Contributor

@shwetaap shwetaap Jun 6, 2024

Choose a reason for hiding this comment

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

Removed this in PR #497

pkg/apis/uiplugin/v1alpha1/types.go Show resolved Hide resolved
@openshift-ci openshift-ci bot removed the lgtm label Jun 5, 2024
@openshift-ci openshift-ci bot added the lgtm label Jun 5, 2024
@danielmellado
Copy link
Contributor

/unhold

@danielmellado
Copy link
Contributor

/approve

@openshift-ci openshift-ci bot added the approved label Jun 5, 2024
Copy link

openshift-ci bot commented Jun 5, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: danielmellado, periklis, PeterYurkovich, tremes

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:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit db0b62f into rhobs:main Jun 5, 2024
11 checks passed
Copy link
Contributor

@simonpasquier simonpasquier 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 late review. My comments weren't meant to be blocking but it'd be good to address sooner than later.

//
// +optional
// +operator-sdk:csv:customresourcedefinitions:type=spec,displayName="Korrel8r Instance Name"
Name string `json:"name,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Should it really be optional?

Reading the code we assume a default value of "korrel8r" for both but I'd rather make these fields mandatory to be explicit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The korrel8r deployment is being attached to the troubleshooting panel UIPlugin in #497 so this specific comment might be helpful to address in that PR.

Copy link
Contributor

Choose a reason for hiding this comment

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

"Korrel8r" will always be deployed when the Troubleshooting Panel is installed in the same namespace where the observability-operator is installed. So there is no need for any configuration.

Copy link
Contributor

Choose a reason for hiding this comment

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

Removed it in #497

// TroubleshootingPanel contains configuration for the troubleshooting console plugin.
//
// +kubebuilder:validation:Optional
TroubleshootingPanel *TroubleshootingPanelConfig `json:"troubleshootingPanel,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Similar to what's being done in #477 we need some additional validation to ensure that when type != TroubleshootingPanel, this field isn't defined.
And if we make TroubleshootingPanelKorrel8rConfig a mandatory field, it should be validated too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created a jira for visibility and will make a follow up PR to address these.

// DistributedTracing contains configuration for the distributed tracing console plugin.
//
// +kubebuilder:validation:Optional
DistributedTracing *DistributedTracingConfig `json:"distributedTracing,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

same remark here, we need to check the type to enforce that the field isn't defined when type != DistributedTracing

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've created a jira for visibility and will make a follow up PR to address these.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants