CONSOLE-3171, CONSOLE-3329: Re-initialize graphql client on cluster change #12425

rawagner · 2023-01-10T21:44:30Z

No description provided.

rawagner · 2023-01-10T21:45:43Z

frontend/packages/console-app/src/components/nav/NavHeader.tsx

+      startGQLClient();
      dispatch(clearSSARFlags());
      dispatch(detectFeatures());


I think we should create a function which will do all the necessary dispatches as these 3 function calls are being repeated in couple more places.

Also I think we should re-run API discovery

Feel free to add the api discovery here. We only left it out because we knew it wouldn't work yet :)

I will create a separate PR for that. There is some caching involved on frontend side so Im not sure that running just dispatch(startAPIDiscovery()) will be correct.

rawagner · 2023-01-10T21:47:26Z

pkg/server/server.go

+	handle(graphQLEndpoint, authHandlerWithUser(func(user *auth.User, w http.ResponseWriter, r *http.Request) {
+		cluster := serverutils.GetCluster(r)
+		k8sProxyConfig := s.getK8sProxyConfig(cluster)
+		k8sProxy := proxy.NewProxy(k8sProxyConfig)
+		k8sResolver := resolver.K8sResolver{K8sProxy: k8sProxy}
+		rootResolver := resolver.RootResolver{K8sResolver: &k8sResolver}
+		schema := graphql.MustParseSchema(string(graphQLSchema), &rootResolver, opts...)
+		handler := graphqlws.NewHandler()
+		handler.InitPayload = resolver.InitPayload
+		graphQLHandler := handler.NewHandlerFunc(schema, &relay.Handler{Schema: schema})


@TheRealJon you already had the backend part implemented :)

I'm about to change this implementation a little, but it should be an easy adjustment. Instead of creating a new proxy instance on every request, we should create one on the first request, then reuse the same proxy instance on subsequent requests.

that is happening already IIUC. The handle function is executed once, when graphl client tries to connect to graphql server - we create a proxy and graphql handler here. The rest of the communication is done via websocket and the graphql handler is always using the same proxy instance.
When the cluster is changed in the UI, the websocket is closed and new one is created, executing the handle function again (with new cluster as parameter)

Yes, but I think the number of proxy instances still scales up with the number of requests, which is probably not a scalable solution. With many users/sessions, proxy instances increase.

TheRealJon

A couple of questions and nits, but this is awesome!

frontend/public/graphql/client.ts

TheRealJon · 2023-01-18T21:11:30Z

pkg/server/server.go

+	handle(graphQLEndpoint, authHandlerWithUser(func(user *auth.User, w http.ResponseWriter, r *http.Request) {
+		cluster := serverutils.GetCluster(r)
+		k8sProxyConfig := s.getK8sProxyConfig(cluster)
+		k8sProxy := proxy.NewProxy(k8sProxyConfig)
+		k8sResolver := resolver.K8sResolver{K8sProxy: k8sProxy}
+		rootResolver := resolver.RootResolver{K8sResolver: &k8sResolver}
+		schema := graphql.MustParseSchema(string(graphQLSchema), &rootResolver, opts...)
+		handler := graphqlws.NewHandler()
+		handler.InitPayload = resolver.InitPayload
+		graphQLHandler := handler.NewHandlerFunc(schema, &relay.Handler{Schema: schema})


I'm about to change this implementation a little, but it should be an easy adjustment. Instead of creating a new proxy instance on every request, we should create one on the first request, then reuse the same proxy instance on subsequent requests.

TheRealJon · 2023-01-18T21:12:56Z

frontend/packages/console-app/src/components/nav/NavHeader.tsx

+      startGQLClient();
      dispatch(clearSSARFlags());
      dispatch(detectFeatures());


Feel free to add the api discovery here. We only left it out because we knew it wouldn't work yet :)

jhadvig

/lgtm

QE Approver:
 /assign @yapei
Docs Approver:
 /assign @opayne1
PX Approver:
/assign @RickJWagner

openshift-ci · 2023-02-01T15:27:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jhadvig, rawagner

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:

~~frontend/OWNERS~~ [jhadvig,rawagner]
~~pkg/OWNERS~~ [jhadvig]

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

opayne1

/label docs-approved

RickJWagner · 2023-02-01T18:56:43Z

/label px-approved

jhadvig · 2023-02-03T15:04:41Z

/test e2e-gcp-console

yapei · 2023-02-06T01:20:01Z

@rawagner Could you help rebase to master?

openshift-ci · 2023-02-06T08:10:47Z

New changes are detected. LGTM label has been removed.

yapei · 2023-02-08T08:48:30Z

websocket connection to /api/graphql is re-established whenever we switch to managed cluster

wss://<console_route>/api/graphql?cluster=local-cluster
wss://<console_route>/api/graphql?cluster=usmng-176108

/label qe-approved

jhadvig · 2023-02-08T09:30:48Z

/test e2e-gcp-console

openshift-ci · 2023-02-08T10:46:49Z

@rawagner: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-gcp-console	`6916fae`	link	true	`/test e2e-gcp-console`

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

TheRealJon

During our testathon, these changes were deployed and a few regressions were noted that are related:

User settings fail to load when managed clusters are selected since the user UID changes. We are only fetching the user settings configmap from the hub cluster, and we use the user UID as part of the configmap name. We probably need to either track the hub cluster user UID separately to make sure we fetch the correct configmap, or update the user settings logic to fall back to local storage in a multicluster environment.
I experienced 401 responses from graphql when logged in as a user with cluster viewer permissions.
API discovery still needs to be addressed.

We can probably merge this and address the issues in a follow-on PR, but I just wanted to make sure we have these documented.

jhadvig · 2023-02-20T16:18:07Z

/hold
until we figure out what needs to be done with issues, mentioned in Jon's comment

rawagner · 2023-02-21T08:43:34Z

Great findings, @TheRealJon
1 and 3 are pretty clear but Im not sure why 2 would occur. I dont know anything about cluster-viewer role.
Can we create sub-tasks for all of these and address them separately ?

TheRealJon · 2023-02-21T15:12:42Z

Great findings, @TheRealJon 1 and 3 are pretty clear but Im not sure why 2 would occur. I dont know anything about cluster-viewer role. Can we create sub-tasks for all of these and address them separately ?

I'm hesitant to merge this without at least addressing 1 and 2 because those are pretty serious regressions. The API discovery issue might already have a bug in Jira and I'm fine tracking it separately since it's not introduced by these changes.

TheRealJon · 2023-03-23T17:40:42Z

Multicluster feature has been deferred. Closing.

TheRealJon · 2023-03-23T17:41:44Z

/jira refresh

openshift-ci-robot · 2023-03-23T17:41:47Z

@TheRealJon: This pull request references CONSOLE-3329 which is a valid jira issue.

In response to this:

/jira refresh

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.

TheRealJon · 2023-03-23T17:46:06Z

Both related Jira issues:
https://issues.redhat.com//browse/CONSOLE-3329
https://issues.redhat.com//browse/CONSOLE-3171

openshift-ci bot requested review from florkbr and kyoto January 10, 2023 21:45

openshift-ci bot added component/backend Related to backend component/core Related to console core functionality component/shared Related to console-shared labels Jan 10, 2023

rawagner commented Jan 10, 2023

View reviewed changes

rawagner changed the title ~~CONSOLE-3171, CONSOLE-3329: Re-initialize graphql client on cluster change~~ CONSOLE-3171: Re-initialize graphql client on cluster change Jan 10, 2023

rawagner force-pushed the restart_gql_multicluster branch from b7a2416 to 7cdd4da Compare January 11, 2023 10:14

rawagner changed the title ~~CONSOLE-3171: Re-initialize graphql client on cluster change~~ CONSOLE-3171, CONSOLE-3329: Re-initialize graphql client on cluster change Jan 11, 2023

TheRealJon reviewed Jan 18, 2023

View reviewed changes

rawagner force-pushed the restart_gql_multicluster branch from 7cdd4da to a312dee Compare January 19, 2023 07:16

jhadvig approved these changes Feb 1, 2023

View reviewed changes

openshift-ci bot assigned jhadvig Feb 1, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Feb 1, 2023

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 1, 2023

opayne1 reviewed Feb 1, 2023

View reviewed changes

openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Feb 1, 2023

openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Feb 1, 2023

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 5, 2023

Re-initialize graphql client on cluster change

6916fae

rawagner force-pushed the restart_gql_multicluster branch from a312dee to 6916fae Compare February 6, 2023 08:10

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Feb 6, 2023

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Feb 6, 2023

openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Feb 8, 2023

TheRealJon reviewed Feb 15, 2023

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Feb 20, 2023

TheRealJon closed this Mar 23, 2023

openshift-ci-robot added the jira/valid-reference Indicates that this PR references a valid Jira ticket of any type. label Mar 23, 2023

TheRealJon added the multicluster label Mar 23, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

CONSOLE-3171, CONSOLE-3329: Re-initialize graphql client on cluster change #12425

CONSOLE-3171, CONSOLE-3329: Re-initialize graphql client on cluster change #12425

rawagner commented Jan 10, 2023

rawagner Jan 10, 2023 •

edited

TheRealJon Jan 18, 2023

rawagner Jan 19, 2023

rawagner Jan 10, 2023

TheRealJon Jan 18, 2023

rawagner Jan 19, 2023

TheRealJon Feb 1, 2023

TheRealJon left a comment

TheRealJon Jan 18, 2023

TheRealJon Jan 18, 2023

jhadvig left a comment

openshift-ci bot commented Feb 1, 2023

opayne1 left a comment

RickJWagner commented Feb 1, 2023

jhadvig commented Feb 3, 2023

yapei commented Feb 6, 2023

openshift-ci bot commented Feb 6, 2023

yapei commented Feb 8, 2023 •

edited

jhadvig commented Feb 8, 2023

openshift-ci bot commented Feb 8, 2023

TheRealJon left a comment

jhadvig commented Feb 20, 2023

rawagner commented Feb 21, 2023

TheRealJon commented Feb 21, 2023

TheRealJon commented Mar 23, 2023

TheRealJon commented Mar 23, 2023

openshift-ci-robot commented Mar 23, 2023 •

edited by openshift-ci bot

TheRealJon commented Mar 23, 2023

CONSOLE-3171, CONSOLE-3329: Re-initialize graphql client on cluster change #12425

CONSOLE-3171, CONSOLE-3329: Re-initialize graphql client on cluster change #12425

Conversation

rawagner commented Jan 10, 2023

rawagner Jan 10, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TheRealJon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jhadvig left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Feb 1, 2023

opayne1 left a comment

Choose a reason for hiding this comment

RickJWagner commented Feb 1, 2023

jhadvig commented Feb 3, 2023

yapei commented Feb 6, 2023

openshift-ci bot commented Feb 6, 2023

yapei commented Feb 8, 2023 • edited

jhadvig commented Feb 8, 2023

openshift-ci bot commented Feb 8, 2023

TheRealJon left a comment

Choose a reason for hiding this comment

jhadvig commented Feb 20, 2023

rawagner commented Feb 21, 2023

TheRealJon commented Feb 21, 2023

TheRealJon commented Mar 23, 2023

TheRealJon commented Mar 23, 2023

openshift-ci-robot commented Mar 23, 2023 • edited by openshift-ci bot

TheRealJon commented Mar 23, 2023

rawagner Jan 10, 2023 •

edited

yapei commented Feb 8, 2023 •

edited

openshift-ci-robot commented Mar 23, 2023 •

edited by openshift-ci bot