frontend: add node shell #1603

farodin91 · 2023-12-16T14:37:23Z

Follow up PR:

Windows Support
Improved Error handling

fixes #996

farodin91 · 2024-01-11T11:47:37Z

@joaquimrocha What do you think of this?

joaquimrocha · 2024-01-11T14:16:59Z

@farodin91 We had the intention of adding this but then things got stalled since apparently it's not possible to run it in all k8s environments. I will review it now though as I'd like to indeed have this.

frontend/src/components/common/Shell.tsx

frontend/src/components/node/Details.tsx

joaquimrocha · 2024-01-11T14:27:10Z

frontend/src/components/node/Details.tsx

+                icon="mdi:console"
+                onClick={() => setShowShell(true)}
+                iconButtonProps={{
+                  disabled: item?.status?.nodeInfo?.operatingSystem !== 'linux',


We only disable buttons in the header actions if they are to be shown by default because the user can ever use them (e.g. no permissions -> hide button).
If the user has permissions but the OS is not Linux, I feel a bit divided because of the cases where the user never has a Linux node (so just like not having permissions to exec, we shouldn't show that action).

If it makes sense to have it disabled anyway, then we'd have to explain why it's disabled in the tooltip (we do this for the "show previous" button in the logs viewer dialog).

What do you think of my change?

frontend/src/lib/k8s/node.ts

joaquimrocha

Pretty cool to have this feature! I left a few comments. Let me know if you have any questions or need to discuss it (we can also do it in the community slack).

frontend/src/components/App/Settings/SettingsCluster.tsx

joaquimrocha · 2024-01-17T10:51:11Z

frontend/src/components/App/Settings/SettingsCluster.tsx

  const invalidNamespaceMessage = t(
    "translation|Namespaces must contain only lowercase alphanumeric characters or '-', and must start and end with an alphanumeric character."
  );
+  const isValidNewAllowedNamespace = isValidNamespaceFormat(newAllowedNamespace);


Why was this moved here?

frontend/src/components/App/Settings/SettingsCluster.tsx

joaquimrocha · 2024-01-17T11:03:04Z

frontend/src/components/App/Settings/SettingsCluster.tsx

+                  value={dropShellLinuxImage}
+                  placeholder={DEFAULT_DROP_SHELL_LINUX_IMAGE}
+                  helperText={t(
+                    'translation|The default image is used for dropping a shell into a node (when not specified directly).'


What does "when not specified directly" refer to?

frontend/src/components/node/Details.tsx

frontend/src/helpers/index.ts

frontend/src/i18n/locales/de/glossary.json

joaquimrocha · 2024-01-17T11:15:04Z

frontend/src/lib/k8s/node.ts

+    const url = `/api/v1/namespaces/kube-system/pods/${podName}/exec?container=shell${commandStr}&stdin=${
+      stdin ? 1 : 0
+    }&stderr=${stderr ? 1 : 0}&stdout=${stdout ? 1 : 0}&tty=${tty ? 1 : 0}`;


The 1st time I tried your PR, it failed to show any terminal. I think it's because the image was not pulled and it took longer? After re-trying it did work. Maybe you can reproduce it by using an always pull policy in the pod during dev.

I wonder if we can somehow show a spinner and wait till the pod is available, for a better UX.

frontend/src/components/common/Terminal.tsx

joaquimrocha · 2024-01-17T11:39:19Z

frontend/src/components/common/Terminal.tsx

I think making the Terminal dependent on a node or pod is not the right direction here. After all we are execing into a pod.
I wonder if instead of passing the node so it executes the shell command there, we could instead override the exec method for that pod instance, either directly in that instance, or by creating a new Pod class (NodeShellPod) with that new exec method, and we instantiate it from the json of the created Pod.

I understand the exec is not the only thing you're branching on. But e.g. things like hiding the container selector could be a new property like showContainerSelector?: boolean; not circling through the shells could be achieved by overriding the list of shells from a property too, and if this list has only 1 element, there's no point in circling (shellCommand?: string[] | string); the title can be a new title property.

I hope we don't end up with 10 more properties but perhaps just 3 or 4. This would make the terminal more flexible without special casing it for the node. These changes should be independent commits.

I know it's yet more work, but do you think this makes sense?

I started to work on it.

I'm currently think of splitting up the terminal any logic such as attach or not or container selector and building a Terminal dialog for pods and nodes. What do you think?

Having a Terminal dialog for pods and nodes is fine if that makes the code simpler, as long as they use the same underlying component. Since we are in any case execing into the same type of resource, it's just some options on how to connect and what to show that change, I think it's probably easier to just have those exposed in the Terminal and calling it differently depending on whether it's the Node shell or the Pod one.

joaquimrocha · 2024-01-17T11:40:52Z

BTW, forgot to mention that one difference I see in UX with the regular pod exec functionality is that the node one shows a very narrow dialog for a little bit until the terminal is actually rendered. The same doesn't happen for the pod exec as far as I could test.

farodin91 · 2024-01-20T08:52:57Z

I'm vacation at moment. I will come back in one half week.

joaquimrocha

I commented on your last reply. I see you updated the branch but the logic still needs to address the review, right?

joaquimrocha · 2024-02-07T17:46:15Z

frontend/src/components/common/Terminal.tsx

Having a Terminal dialog for pods and nodes is fine if that makes the code simpler, as long as they use the same underlying component. Since we are in any case execing into the same type of resource, it's just some options on how to connect and what to show that change, I think it's probably easier to just have those exposed in the Terminal and calling it differently depending on whether it's the Node shell or the Pod one.

illume · 2024-05-13T10:57:43Z

I went through and closed convos that were done. It should be a bit easier to read the PR now.

farodin91 · 2024-11-03T14:54:54Z

@illume I have rebased my changes and now they pass.

Fixes headlamp-k8s#996 Signed-off-by: farodin91 <[email protected]>

farodin91 · 2024-11-28T22:58:36Z

I thought about moving it to plugins. As a Plugin it would be possible to ship only if wanted and develop further. Any thoughts?

farodin91 changed the title ~~Add node shell~~ frontend: add node shell Dec 18, 2023

farodin91 force-pushed the add-node-shell branch 2 times, most recently from c35b741 to 7f240c1 Compare December 18, 2023 19:13

farodin91 force-pushed the add-node-shell branch from 7f240c1 to dd556db Compare January 9, 2024 21:58

farodin91 marked this pull request as ready for review January 10, 2024 15:06

joaquimrocha requested changes Jan 11, 2024

View reviewed changes

farodin91 force-pushed the add-node-shell branch 3 times, most recently from c171dd8 to 7e87ed8 Compare January 14, 2024 17:49

joaquimrocha requested changes Jan 17, 2024

View reviewed changes

farodin91 force-pushed the add-node-shell branch from 7e87ed8 to be70ef5 Compare February 1, 2024 21:51

joaquimrocha reviewed Feb 7, 2024

View reviewed changes

illume requested a review from joaquimrocha May 13, 2024 10:57

illume mentioned this pull request Jul 8, 2024

Add support for execing into a node #996

Open

farodin91 force-pushed the add-node-shell branch from be70ef5 to 03ef10f Compare November 3, 2024 14:17

dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Nov 3, 2024

frontend: add node shell

d0eb6f3

Fixes headlamp-k8s#996 Signed-off-by: farodin91 <[email protected]>

farodin91 force-pushed the add-node-shell branch from 03ef10f to d0eb6f3 Compare November 21, 2024 20:28

farodin91 requested a review from illume November 22, 2024 10:28

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

frontend: add node shell #1603

frontend: add node shell #1603

farodin91 commented Dec 16, 2023 •

edited

Loading

farodin91 commented Jan 11, 2024

joaquimrocha commented Jan 11, 2024

joaquimrocha Jan 11, 2024

farodin91 Jan 14, 2024

joaquimrocha left a comment

joaquimrocha Jan 17, 2024

joaquimrocha Jan 17, 2024

joaquimrocha Jan 17, 2024

joaquimrocha Jan 17, 2024

farodin91 Feb 1, 2024

joaquimrocha Feb 7, 2024

joaquimrocha commented Jan 17, 2024

farodin91 commented Jan 20, 2024

joaquimrocha left a comment

joaquimrocha Feb 7, 2024

illume commented May 13, 2024

farodin91 commented Nov 3, 2024

farodin91 commented Nov 28, 2024

frontend: add node shell #1603

Are you sure you want to change the base?

frontend: add node shell #1603

Conversation

farodin91 commented Dec 16, 2023 • edited Loading

farodin91 commented Jan 11, 2024

joaquimrocha commented Jan 11, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joaquimrocha left a 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

Choose a reason for hiding this comment

joaquimrocha commented Jan 17, 2024

farodin91 commented Jan 20, 2024

joaquimrocha left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

illume commented May 13, 2024

farodin91 commented Nov 3, 2024

farodin91 commented Nov 28, 2024

farodin91 commented Dec 16, 2023 •

edited

Loading