-
Notifications
You must be signed in to change notification settings - Fork 55
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
COO-169: feat: add troubleshooting panel and distributed tracing ui-plugin #480
Conversation
@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:
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. |
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 Once the patch is verified, the new status will be reflected by the 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. |
@@ -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(), |
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 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
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 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.
@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:
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: 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:
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. |
03dd41c
to
8e8c731
Compare
@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:
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: 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:
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. |
de84a0d
to
f5ebcf9
Compare
@PeterYurkovich: No Jira issue is referenced in the title of this pull request. In response to this:
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. |
b32f71f
to
11ae0af
Compare
/ok-to-test |
/lgtm |
/hold |
@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. |
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.
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.
Changed it in the PR #497
// 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"` |
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.
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.
Removed this in PR #497
/unhold |
/approve |
[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 |
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.
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"` |
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.
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.
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.
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.
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.
"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.
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.
Removed it in #497
// TroubleshootingPanel contains configuration for the troubleshooting console plugin. | ||
// | ||
// +kubebuilder:validation:Optional | ||
TroubleshootingPanel *TroubleshootingPanelConfig `json:"troubleshootingPanel,omitempty"` |
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.
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.
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'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"` |
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.
same remark here, we need to check the type to enforce that the field isn't defined when type != DistributedTracing
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've created a jira for visibility and will make a follow up PR to address these.
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.