-
Notifications
You must be signed in to change notification settings - Fork 605
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
Bug 1822553: Fix channel issue by narrowing down requests and loading guard #4999
Conversation
@bipuladh: This pull request references Bugzilla bug 1822553, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. 3 validation(s) were run on this bug
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 kubernetes/test-infra repository. |
/kind bug |
/hold |
frontend/packages/operator-lifecycle-manager/src/components/clusterserviceversion.tsx
Outdated
Show resolved
Hide resolved
@bipuladh: This pull request references Bugzilla bug 1822553, which is valid. 3 validation(s) were run on this bug
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 kubernetes/test-infra repository. |
@@ -451,7 +451,7 @@ export class SubscriptionUpdates extends React.Component< | |||
<dl className="co-m-pane__details"> | |||
<dt className="co-detail-table__section-header">Channel</dt> | |||
<dd> | |||
{this.state.waitingForUpdate ? ( | |||
{this.state.waitingForUpdate || !pkg ? ( |
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.
Do we need to differentiate between loading and load error or not found? Otherwise the loading icon could be present forever. We might just disable the button when there's no pkg
since we have a value to show from obj
, but I'm not sure how Patternfly handles disabled buttons of type link
.
I'd think we'd want a similar fix for install plan approvals below.
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 did the change. It however still does not address the issue. It does not show error as mentioned in the bug by disabling the button but the problem I see is that the loading of PackageManifest still takes ages (payload is around 1.6MB in postman). Now the user doesn't even have a loading icon for him to accurately reflect the state(arbitrarily wait for the link to be active).
It only solves the error but the user still has to wait or refresh the page for that button to come alive.
One alternative that I tried which improved the speed was narrowing the query to a particular manifest(using the name of the operator). But then again the solution is not an elegant one.
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 did the change. It however still does not address the issue. It does not show error as mentioned in the bug by disabling the button but the problem I see is that the loading of PackageManifest still takes ages (payload is around 1.6MB in postman). Now the user doesn't even have a loading icon for him to accurately reflect the state(arbitrarily wait for the link to be active).
You change channels rarely. With the loading icon, it means you can't even see the value until the package manifest request completes. So we've made the common case (just looking at the details) much slower.
The size of the response is the real problem. We might want to look at gzipping API responses through the console proxy.
One alternative that I tried which improved the speed was narrowing the query to a particular manifest(using the name of the operator). But then again the solution is not an elegant one.
This is a much better solution if there is a reliable field selector or label selector we can use.
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.
Obviously not for this PR as it's a significant architectural change, but GraphQL could really help here long term.
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 only reliable selector is the name of the operator however since the name is in the name-version-release
we will have to perform string manipulation on it to get the name
part which the package manifest uses. Your thoughts?
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.
@spadgett ^^
@spadgett Just out of curiosity. Does InstallPlan and PackageManifest always share the same namespace of that of the Subscription? https://github.com/openshift/console/blob/master/frontend/packages/operator-lifecycle-manager/src/components/subscription.tsx#L540-L557 |
<Button | ||
type="button" | ||
isInline | ||
onClick={() => channelModal()} |
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.
onClick={() => channelModal()} | |
onClick={channelModal} |
<Button | ||
type="button" | ||
isInline | ||
onClick={() => approvalModal()} |
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.
onClick={() => approvalModal()} | |
onClick={approvalModal} |
Yes. It's the ClusterServiceVersion that can appear in a different namespace. Operators can watch multiple namespaces, so the ClusterServiceVersion is copied into each target namespace. |
variant={!obj ? 'plain' : 'link'} | ||
isDisabled={!obj} |
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, this check might not be needed. It would be a runtime error if obj
is not already defined. I assumed we needed the install plan resource.
/hold cancel |
4406bdb
to
2bb7094
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.
/approve
/lgtm
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: bipuladh, spadgett 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 |
@bipuladh: All pull requests linked via external trackers have merged: openshift/console#4999. Bugzilla bug 1822553 has been moved to the MODIFIED state. 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 kubernetes/test-infra repository. |
/cherry-pick release-4.4 |
@bipuladh: new pull request created: #5339 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 kubernetes/test-infra repository. |
PackageManifest
is loaded.