Skip to content

Commit

Permalink
Merge pull request #12744 from rak-phillip/bugfix/10840-pod-shell
Browse files Browse the repository at this point in the history 
Improve logging for errors in `ContainerShell.vue`
  • Loading branch information
@rak-phillip
rak-phillip authored Dec 4, 2024
2 parents a79df07 + 0568efe commit 4653a0f
Show file tree
Hide file tree
Showing 3 changed files with 45 additions and 26 deletions.
16 changes: 12 additions & 4 deletions shell/assets/translations/en-us.yaml
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1480,16 +1480,16 @@ cluster:
defaultLabel: Use default data directory configuration
commonLabel: Use a common base directory for data directory configuration (sub-directories will be used for the system-agent, provisioning and distro paths)
customLabel: Use custom data directories
common:
common:
label: Data directory base path
tooltip: Data directory base path. We will append the sub-directories appropriate for each directory (/agent, /provisioning and either /rke2 or /k3s)
systemAgent:
systemAgent:
label: System-agent directory path
tooltip: Data directory for the system-agent connection info and plans
provisioning:
provisioning:
label: Provisioning directory path
tooltip: Data directory for provisioning related files
k8sDistro:
k8sDistro:
label: K8s Distro directory path
tooltip: Data directory for the k8s distro
machineConfig:
Expand Down Expand Up @@ -6138,6 +6138,14 @@ wm:
clear: Clear
containerName: "Container: {label}"
failed: "Unable to open a shell to the container (none of the shell commmands succeeded)\n\r"
logLevel:
info: INFO
error: ERROR
warning: WARN
debug: DEBUG
logMessage:
containerError: "{ logLevel }: Container missing shell executable (/bin/sh)"

kubectlShell:
title: "Kubectl: {name}"

Expand Down
17 changes: 13 additions & 4 deletions shell/components/nav/WindowManager/ContainerShell.vue
View file
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import Socket, {
EVENT_CONNECT_ERROR,
} from '@shell/utils/socket';
import Window from './Window';
import dayjs from 'dayjs';
const commands = {
linux: [
Expand Down Expand Up @@ -287,7 +288,7 @@ export default {
// If we had an error message, try connecting with the next command
if (this.errorMsg) {
this.terminal.write(this.errorMsg);
this.terminal.writeln(this.errorMsg);
if (this.backupShells.length && this.retries < 2) {
this.retries++;
// we're not really counting on this being a reactive change so there's no need to fire the whole action
Expand All @@ -299,7 +300,9 @@ export default {
this.connect();
} else {
// Output an message to let he user know none of the shell commands worked
this.terminal.write(this.t('wm.containerShell.failed'));
const timestamp = dayjs().format('YYYY-MM-DD HH:mm:ss');
this.terminal.writeln(`[${ timestamp }] ${ this.t('wm.containerShell.logLevel.info') }: ${ this.t('wm.containerShell.failed') }`);
}
}
});
Expand All @@ -317,10 +320,16 @@ export default {
}
this.terminal.write(msg);
} else {
console.error(msg); // eslint-disable-line no-console
const timestamp = dayjs().format('YYYY-MM-DD HH:mm:ss');
let customError = `[${ timestamp }] ${ this.t('wm.containerShell.logLevel.error') }: ${ this.container }: ${ msg }`;
if (msg.includes('stat /bin/sh: no such file or directory')) {
customError = `[${ timestamp }] ${ this.t('wm.containerShell.logMessage.containerError', { logLevel: this.t('wm.containerShell.logLevel.error') }) }: ${ msg }`;
}
console.error(customError); // eslint-disable-line no-console
if (`${ type }` === '3') {
this.errorMsg = msg;
this.errorMsg = customError;
}
}
});
Expand Down
38 changes: 20 additions & 18 deletions shell/components/nav/WindowManager/__tests__/ContainerShell.test.ts
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ describe('component: ContainerShell', () => {
return { rows: 1 };
});
const write = jest.fn();
const writeln = jest.fn();
const reset = jest.fn();

jest.mock(/* webpackChunkName: "xterm" */ 'xterm', () => {
Expand All @@ -39,6 +40,7 @@ describe('component: ContainerShell', () => {
open = open;
focus = focus;
write = write;
writeln = writeln;
reset = reset
}
};
Expand Down Expand Up @@ -277,10 +279,10 @@ describe('component: ContainerShell', () => {
eventConnected();
eventMessage({ detail: { data: `3${ errorMessage }` } });

expect(consoleError.mock.calls[0][0]).toBe(errorMessage);
expect(consoleError.mock.calls[0][0]).toContain(errorMessage);
expect(wrapper.vm.isOpen).toBe(true);
expect(wrapper.vm.isOpening).toBe(false);
expect(wrapper.vm.errorMsg).toBe(errorMessage);
expect(wrapper.vm.errorMsg).toContain(errorMessage);
expect(wrapper.vm.os).toBe('linux');
});

Expand Down Expand Up @@ -325,10 +327,10 @@ describe('component: ContainerShell', () => {

eventDisconnected();

expect(consoleError.mock.calls[0][0]).toBe(errorMessage);
expect(consoleError.mock.calls[0][0]).toContain(errorMessage);
expect(wrapper.vm.isOpen).toBe(false);
expect(wrapper.vm.isOpening).toBe(false);
expect(wrapper.vm.errorMsg).toBe('eventMessageError');
expect(wrapper.vm.errorMsg).toContain('eventMessageError');
// the backup shell that was leftover was windows so it became the new os in dataprops
expect(wrapper.vm.os).toBeUndefined();
// but we still didn't write it to the pod itself since we don't know if it worked
Expand Down Expand Up @@ -370,11 +372,11 @@ describe('component: ContainerShell', () => {
eventMessage({ detail: { data: `3${ windowsErrorMessage }` } });
eventDisconnected();

expect(consoleError.mock.calls[0][0]).toBe(linuxErrorMessage);
expect(consoleError.mock.calls[1][0]).toBe(windowsErrorMessage);
expect(consoleError.mock.calls[0][0]).toContain(linuxErrorMessage);
expect(consoleError.mock.calls[1][0]).toContain(windowsErrorMessage);
expect(wrapper.vm.isOpen).toBe(false);
expect(wrapper.vm.isOpening).toBe(false);
expect(wrapper.vm.errorMsg).toBe(windowsErrorMessage);
expect(wrapper.vm.errorMsg).toContain(windowsErrorMessage);
expect(wrapper.vm.os).toBeUndefined();
// we never found a shell that worked so we're going to leave the pod os as undefined
expect(defaultContainerShellParams.propsData.pod.os).toBeUndefined();
Expand Down Expand Up @@ -426,11 +428,11 @@ describe('component: ContainerShell', () => {
eventMessage({ detail: { data: `3${ windowsErrorMessage }` } });
eventDisconnected();

expect(consoleError.mock.calls[0][0]).toBe(linuxErrorMessage);
expect(consoleError.mock.calls[1][0]).toBe(windowsErrorMessage);
expect(consoleError.mock.calls[0][0]).toContain(linuxErrorMessage);
expect(consoleError.mock.calls[1][0]).toContain(windowsErrorMessage);
expect(wrapper.vm.isOpen).toBe(false);
expect(wrapper.vm.isOpening).toBe(false);
expect(wrapper.vm.errorMsg).toBe(windowsErrorMessage);
expect(wrapper.vm.errorMsg).toContain(windowsErrorMessage);
expect(wrapper.vm.os).toBeUndefined();
expect(testUndefinedOsParams.propsData.pod.os).toBeUndefined();
expect(connect.mock.calls).toHaveLength(3);
Expand Down Expand Up @@ -472,13 +474,13 @@ describe('component: ContainerShell', () => {
expect(wrapper.vm.backupShells).toHaveLength(1);
expect(wrapper.vm.os).toBeUndefined();
expect(testUndefinedOsParams.propsData.pod.os).toBeUndefined();
expect(wrapper.vm.errorMsg).toBe(linuxErrorMessage);
expect(wrapper.vm.errorMsg).toContain(linuxErrorMessage);

eventConnecting();
eventConnected();
eventMessage({ detail: { data: `1${ windowsShellMessage }` } });

expect(consoleError.mock.calls[0][0]).toBe(linuxErrorMessage);
expect(consoleError.mock.calls[0][0]).toContain(linuxErrorMessage);
expect(consoleError.mock.calls[1]).toBeUndefined();
expect(wrapper.vm.isOpen).toBe(true);
expect(wrapper.vm.isOpening).toBe(false);
Expand Down Expand Up @@ -532,7 +534,7 @@ describe('component: ContainerShell', () => {
expect(wrapper.vm.backupShells).toHaveLength(1);
expect(wrapper.vm.os).toBe('linux');
expect(testNodeDefinedOsParams.propsData.pod.os).toBe('linux');
expect(wrapper.vm.errorMsg).toBe(linuxErrorMessage);
expect(wrapper.vm.errorMsg).toContain(linuxErrorMessage);

eventConnecting();
eventConnected();
Expand All @@ -542,7 +544,7 @@ describe('component: ContainerShell', () => {
expect(wrapper.vm.backupShells).toHaveLength(1);
expect(wrapper.vm.isOpen).toBe(false);
expect(wrapper.vm.isOpening).toBe(false);
expect(wrapper.vm.errorMsg).toBe(linuxErrorMessage);
expect(wrapper.vm.errorMsg).toContain(linuxErrorMessage);
expect(wrapper.vm.os).toBe('linux');
expect(testNodeDefinedOsParams.propsData.pod.os).toBe('linux');
expect(connect.mock.calls).toHaveLength(3);
Expand All @@ -552,13 +554,13 @@ describe('component: ContainerShell', () => {
eventMessage({ detail: { data: `3${ linuxErrorMessage }` } });
eventDisconnected();

expect(consoleError.mock.calls[0][0]).toBe(linuxErrorMessage);
expect(consoleError.mock.calls[1][0]).toBe(linuxErrorMessage);
expect(consoleError.mock.calls[2][0]).toBe(linuxErrorMessage);
expect(consoleError.mock.calls[0][0]).toContain(linuxErrorMessage);
expect(consoleError.mock.calls[1][0]).toContain(linuxErrorMessage);
expect(consoleError.mock.calls[2][0]).toContain(linuxErrorMessage);
expect(wrapper.vm.backupShells).toHaveLength(1);
expect(wrapper.vm.isOpen).toBe(false);
expect(wrapper.vm.isOpening).toBe(false);
expect(wrapper.vm.errorMsg).toBe(linuxErrorMessage);
expect(wrapper.vm.errorMsg).toContain(linuxErrorMessage);
expect(wrapper.vm.os).toBe('linux');
expect(testNodeDefinedOsParams.propsData.pod.os).toBe('linux');
// at some point we have to stop retying and if we're not burning through backup shells, there's a retry limit of 2 for a total of 3 attempts
Expand Down

0 comments on commit 4653a0f

Please sign in to comment.