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

feat: switch currently viewed cluster #499

Merged
merged 53 commits into from
Nov 6, 2024
Merged

feat: switch currently viewed cluster #499

merged 53 commits into from
Nov 6, 2024

Conversation

TristanHoladay
Copy link
Contributor

@TristanHoladay TristanHoladay commented Oct 24, 2024

Description

Adds the ability for users to change which cluster they are viewing in Runtime by selecting from a list produced from the available clusters in their local Kubeconfig.

See Design Doc for more details.

Related Issue

Resolves #348

Switch -- Error state then Success
Screencast from 11-05-2024 10:35:53 AM.webm

Switch after disconnection event occurred:
Screencast from 11-05-2024 10:37:07 AM.webm

@TristanHoladay TristanHoladay marked this pull request as ready for review October 29, 2024 20:47
@TristanHoladay TristanHoladay requested a review from a team as a code owner October 29, 2024 20:47
src/pkg/api/handlers.go Outdated Show resolved Hide resolved
@BillyFigueroa
Copy link
Contributor

BillyFigueroa commented Nov 4, 2024

This is a question...

Do we need the name to display twice like it does now? I think the prefix is the context, so will that ever be different than the second half which I guess is the cluster name?

i.e.
instead of k3d-uds.k3d-uds it's just k3d-uds

Screenshot 2024-11-04 at 12 48 50 PM

Also, do we want to make the font inside the drop down a little smaller? the text on the dropdown seems smaller

@TristanHoladay
Copy link
Contributor Author

@TristanHoladay are you handling the case where a cluster is displayed on the menu but the cluster is stopped? For example, I was just running uds-runtime cluster and have the uds cluster but is stopped. The stopped cluster still shows up on the drop down menu and when I click it just goes on forever. Video included

nice catch! i know exactly what this is cause the same thing happened when i had to update the reconnection logic. We're able to successfully make the client because it exists in the config but when the cache gets made it sits indefinitely in the cache.WaitForCacheSync() because none of the informers can connect to the cluster. i'll update with the fix.

@TristanHoladay
Copy link
Contributor Author

@TristanHoladay are you handling the case where a cluster is displayed on the menu but the cluster is stopped? For example, I was just running uds-runtime cluster and have the uds cluster but is stopped. The stopped cluster still shows up on the drop down menu and when I click it just goes on forever. Video included

Avoids starting new cache if connection to client is refused.
Screenshot from 2024-11-04 13-53-12

@TristanHoladay
Copy link
Contributor Author

This is a question...

Do we need the name to display twice like it does now? I think the prefix is the context, so will that ever be different than the second half which I guess is the cluster name?

i.e. instead of k3d-uds.k3d-uds it's just k3d-uds

Also, do we want to make the font inside the drop down a little smaller? the text on the dropdown seems smaller

It is possible to have a different context name than cluster name, and its imperative we have that for the backend. As far as showing that to the users 🤷‍♂️ It is possible that you have 2 different contexts each with a cluster named the same. So at least for duplicate cluster names the context name will be important. @Madeline-UX do we want to show context name only if there are other contexts with duplicate cluster names?

Also, for the font, Figma shows them as the same font size but i don't have a strong opinion there.

@decleaver
Copy link
Contributor

Unable to reconnect to original cluster after attempting to connect to bad cluster.
Steps to reproduce:

  1. Start runtime connected to a good cluster
  2. Attempt to switch to bad cluster

You get an error message, the selected cluster in the dropdown remains as the original good cluster but there is no way to get it to reconnect, without refreshing the browser

Would recommend the drop down updating to the bad cluster you tried to connect to, that way you can select the working cluster to reconnect.

image

@TristanHoladay
Copy link
Contributor Author

Unable to reconnect to original cluster after attempting to connect to bad cluster. Steps to reproduce:

  1. Start runtime connected to a good cluster
  2. Attempt to switch to bad cluster

You get an error message, the selected cluster in the dropdown remains as the original good cluster but there is no way to get it to reconnect, without refreshing the browser

Would recommend the drop down updating to the bad cluster you tried to connect to, that way you can select the working cluster to reconnect.

image

oh man good catch! i think the issue is not that the connection to the original cluster doesn't work (the K8sSession hasn't been updated if the switch fails), but that there's no mechanism for setting the loading to false in this case, so it remains on the error version of the loading page indefinitely.

Changing the selected cluster does make sense except we'd have to update the K8sSession with no client or cache just to re-create the good one again. I'm going to see if there's a different solution first.

@decleaver
Copy link
Contributor

can make this a follow on issue, but i have old contexts from CMS days that are really long. Looks like the dropdown truncates these (which looks nice but doesn't let me see the whole name). Maybe add a tool tip on hover to see entire name? The selected context shows the full name which looks kind of weird, so not sure how we want to handle it.

image

@TristanHoladay
Copy link
Contributor Author

can make this a follow on issue, but i have old contexts from CMS days that are really long. Looks like the dropdown truncates these (which looks nice but doesn't let me see the whole name). Maybe add a tool tip on hover to see entire name? The selected context shows the full name which looks kind of weird, so not sure how we want to handle it.

yeah i think there are some design decisions i want to get with @Madeline-UX on. i'll make a separate issue and add this to it. Thanks for checking that though! super helpful to see more real-life cluster names.

Copy link
Contributor

@decleaver decleaver left a comment

Choose a reason for hiding this comment

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

🚀

@TristanHoladay TristanHoladay merged commit 4403aa4 into main Nov 6, 2024
14 checks passed
@TristanHoladay TristanHoladay deleted the cluster-switch branch November 6, 2024 19:56
// Set ready to false to block cluster check ticker since switching
ks.ready = false

defer r.Body.Close()
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: this should be safe to remove.

Suggested change
defer r.Body.Close()

See https://cs.opensource.google/go/go/+/refs/tags/go1.23.2:src/net/http/request.go;l=179-182

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks @catsby. i'll add this to the follow on PR i'll be doing.

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

Successfully merging this pull request may close these issues.

Enable Switching between Clusters
5 participants