Fix crash with logs view #4376

Nokel81 · 2021-11-17T22:07:39Z

Switch to saving only IDs of pods, owners, and containers instead of
whole objects
Add more checks about data integrety

Signed-off-by: Sebastian Malton [email protected]

Fixes secondary crash reported in #3782

aleksfront

Nice improvement and overall cleaning! However, found couple of issues:

Do we actually need to prevent logs to show up if pod doesn't have any owners? I'm not sure how often pods can be created directly, but nothing blocks us to open logs for them? For example, here's what I see if created raw Pod from Create Resource tab:
If the viewed pod has suddenly disappeared, maybe it's better to show sibling pod if any? Instead of this message:
For pods without (?) logs I don't see any message like "No logs available", just empty log area. Create a deployment from template dropdown to reproduce:

This probably not affected by current PR, but it's annoying: search box is changing its width if some elements found:

src/renderer/components/dock/log-list.tsx

src/renderer/components/dock/log-resource-selector.tsx

Nokel81 · 2021-11-30T15:45:02Z

Nice improvement and overall cleaning! However, found couple of issues:

1. Do we actually need to prevent logs to show up if pod doesn't have any owners? I'm not sure how often pods can be created directly, but nothing blocks us to open logs for them? For example, here's what I see if created raw Pod from `Create Resource` tab:

Fair enough, I agree that we shouldn't have this limitation.

2. If the viewed pod has suddenly disappeared, maybe it's better to show sibling pod if any? Instead of this message:

Sounds good

3. For pods without (?) logs I don't see any message like "No logs available", just empty log area. Create a deployment from template dropdown to reproduce:

Sounds good

4. This probably not affected by current PR, but it's annoying: search box is changing its width if some elements found:

I will try and fix this in this PR anyway

Nokel81 · 2021-12-02T19:14:46Z

@aleksfront Resolved, can you PTALA?

src/renderer/components/dock/log-tab.store.ts

aleksfront · 2021-12-06T07:24:51Z

src/renderer/components/dock/log-tab.store.ts

+  /**
+   * Get a set of namespaces which pod tabs care about
+   */
+  public getNamespaces(): string[] {


I don't get why we should get namespaces in some way and watch them in logs.tsx. The pod which logs we're looking at (as well as it's "sibling" pods with same owner) could have only 1 namespace. So why we need to collect more than one? Isn't it enough to pick namespace attribute from the previewed pod like pod.getNs()?

It is, however, this gets the namespaces for all tabs not just the currently open one.

This is needed for when we reopen Lens and the pods view might not be open yet. So the Logs component needs to register a watch for pods in all of these namespaces.

I guess I could watch pods in a single namespace for each tab. WDYT?

aleksfront · 2021-12-06T08:05:05Z

src/common/k8s-api/kube-object.store.ts

    if (merge) {
-      const namespaces = partialItems.map(item => item.getNs());
+      const uids = new Set(partialItems.map(item => item.getId()));

      items = [
-        ...this.items.filter(existingItem => !namespaces.includes(existingItem.getNs())),
+        ...this.items.filter(existingItem => !uids.has(existingItem.getId())),
        ...partialItems,
      ];


Not obvious why this is needed?

I'll add a comment then, but the main gist is that stores cannot assume that a given call to mergeItems represents all namespaces being watched.

Specifically, a Logs tab might be viewing logs from a pod in default while the pods view is showing only the ones in kube-system

src/renderer/components/dock/logs.tsx

aleksfront · 2021-12-06T08:18:08Z

@Nokel81 Most of the issues are gone, thanks for fixing those.

This is still an issue:

If the viewed pod has suddenly disappeared, maybe it's better to show sibling pod if any? Instead of this message:

Suggestions about tab naming. First, I doubt we should add namespace before workload name. It makes impossible to get meaningful information from tab because ns will always be long enough to cover all available space.

Maybe it's just me, but kind of strange to see titles like "ReplicaSet logs..." if you just just looking at the Pod logs:

job.logs.mov

You've marked this PR as a candidate to fix Blank Screen when accessing one (out of three) clusters #4018 (Blank Screen when accessing one (out of three) clusters #4018 (comment)). Maybe to move things forward, extract needed parts for 4018 and pull a separate PR? Or it's better to keep with current one?

Nokel81 · 2021-12-06T20:40:46Z

Or it's better to keep with current one?

I think it would be better to keep it here because the fix it to move out the reloadLogs call in render but that requires some other changes I think too.

Nokel81 · 2021-12-06T20:45:17Z

This is still an issue:

Oh, I thought I fixed that...

Suggestions about tab naming. First, I doubt we should add namespace before workload name. It makes impossible to get meaningful information from tab because ns will always be long enough to cover all available space.

Sounds good about the naming.

Maybe it's just me, but kind of strange to see titles like "ReplicaSet logs..." if you just just looking at the Pod logs:

I can revert that change but let me explain why I made it. I made it because I felt that each tab was both about logs and about switching between pods and containers. And to be honest we aren't really even viewing pod logs, but instead viewing container logs.

src/common/utils/comparer.ts

src/common/utils/reactions.ts

src/main/cluster.ts

src/renderer/components/cluster-manager/cluster-status.tsx

src/renderer/components/dock/logs.tsx

aleksfront · 2021-12-08T13:57:53Z

Additional observations:

We have blink of unstyled placeholder on cluster connect / window reload:

placeholder.blink.mov

Lets revert tab title to have "Pod Logs" names:
Need to style this placeholder a bit, make it white and put into center, wdyt?

unstyles.placeholder.mov

For some reason now we have 2 separate spinners around:

2.different.spinners.mov

aleksfront

Problems solved, looking good!

aleksfront · 2021-12-10T13:14:37Z

src/renderer/components/dock/logs.scss

+.PodLogs {
+  .pod-not-found {
+    margin: auto;
+    color: var(--terminalBrightWhite);
+  }
+}


Minor thing, but it's better to stick with css modules to properly isolate styles.

I'll make a follow up PR to move pod logs to modules.

github-actions · 2021-12-17T15:29:42Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2021-12-17T16:11:49Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

github-actions · 2022-01-07T13:40:43Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

github-actions · 2022-01-07T21:06:41Z

Conflicts have been resolved. A maintainer will review the pull request shortly.

- Switch to saving only IDs of pods, owners, and containers instead of whole objects - Add more checks about data integrety Signed-off-by: Sebastian Malton <[email protected]>

Iku-turso

Uh. To be blunt, there are too many problems with this PR to approve. We propose live session to cover all more efficiently. Also PR #4653 (ie. a big splash) makes significant improvements in code touched here. The improvements include getting rid of global shared state and involuntary re-renders which also cause side-effects.

Iku-turso · 2022-01-11T07:28:47Z

src/common/ipc/ipc.ts

@@ -34,7 +34,9 @@ const electronRemote = (() => {
  if (ipcRenderer) {
    try {
      return require("@electron/remote");
-    } catch {}
+    } catch {
+      // ignore temp


I can see that there is a wild-card try-catch, but the code does not explain why. If the try/catch is indeed an absolute necessity, I would like to know why. Better comment, or preferrably lovingly named function, would be better. Future improvement.

Iku-turso · 2022-01-11T07:35:24Z

src/common/k8s-api/kube-watch-api.ts

-    if (this.#watch.inc(store) > 1) {
+    const isNamespaceFilterWatch = !namespaces;
+
+    namespaces ??= KubeWatchApi.context?.contextNamespaces ?? [];


Global shared state, fixed elsewhere.

Iku-turso · 2022-01-11T07:35:57Z

src/common/k8s-api/kube-watch-api.ts

+
+    namespaces ??= KubeWatchApi.context?.contextNamespaces ?? [];
+
+    if (isNamespaceFilterWatch && this.#watch.inc(store) > 1) {


Named function required. Future improvement.

Iku-turso · 2022-01-11T07:36:15Z

src/common/k8s-api/kube-watch-api.ts

@@ -167,7 +172,7 @@ export class KubeWatchApi {
      : noop; // don't watch namespaces if namespaces were provided

    return () => {
-      if (this.#watch.dec(store) === 0) {
+      if (!isNamespaceFilterWatch || this.#watch.dec(store) === 0) {


Named function required. Future improvement.

Iku-turso · 2022-01-11T07:42:12Z

src/renderer/components/dock/__test__/log-resource-selector.test.tsx

-    const tabData = getOnePodTabData();
-    const { getByText } = render(getComponent(tabData));
+    const props = getOnePodTabProps();
+    const { getByText } = render(<LogResourceSelector {...props} />);

    expect(getByText("dockerExporter")).toBeInTheDocument();


Notice: Multiple expects in a test is very bad.

Iku-turso · 2022-01-11T08:53:51Z

src/renderer/components/dock/log.store.ts

-  get lines(): number {
-    return this.logs.length;
-  }
+  getLogs(tabId: TabId, { showTimestamps }: LogTabData): string[] {


This needs to be reactive. The big-splash makes this reactive, among other things. The big-splash needs to be merged first because this.

Iku-turso · 2022-01-11T08:54:39Z

src/renderer/components/dock/log.store.ts

-    if (!extraction || extraction.length < 3) {
-      return ["", logs];
+    if (!timestamps) {
+      return "";


Empty string should be null when possible. Future improvement.

Iku-turso · 2022-01-11T08:57:18Z

src/renderer/components/dock/logs.tsx

+      ], {
+        namespaces: logTabStore.getNamespaces(),
+      }),
+      when(() => this.canLoad, () => this.load(true)),


Initialization in wrong place (component). State should be ready when rendering. Future improvement.

Iku-turso · 2022-01-11T08:57:49Z

src/renderer/components/dock/logs.tsx

+   * Only used in `componentDidMount`
+   */
+  private changeSelectedPodWhenCurrentDisappears() {
+    return reaction(() => this.getPods(this.tabData), (data) => {


Preventable reaction. Use action.

Iku-turso · 2022-01-11T09:02:13Z

src/renderer/utils/createStorage.ts

@@ -50,8 +51,11 @@ export function createStorage<T>(key: string, defaultValue: T) {

      try {
        storage.data = await fse.readJson(filePath);


Notice: explicit access to filesystem! Explicit side-effect.

github-actions · 2022-01-17T18:57:22Z

This pull request has conflicts, please resolve those before we can evaluate the pull request.

Nokel81 · 2022-01-24T15:10:39Z

Closing this PR as the commit 9d33fff fixes a lot of the issues that this did as well.

I will recreate the changes to the storage layer in a new PR (for 5.5) once #3738 is merged.

Nokel81 added the bug Something isn't working label Nov 17, 2021

Nokel81 requested a review from a team as a code owner November 17, 2021 22:07

Nokel81 requested review from jweak and jakolehm and removed request for a team November 17, 2021 22:07

Nokel81 marked this pull request as draft November 18, 2021 04:07

Nokel81 marked this pull request as ready for review November 18, 2021 14:04

Nokel81 requested a review from aleksfront November 18, 2021 14:04

Nokel81 mentioned this pull request Nov 19, 2021

Blank Screen when accessing one (out of three) clusters #4018

Closed

Nokel81 added the area/logs label Nov 22, 2021

Nokel81 mentioned this pull request Nov 29, 2021

App crash at / #4442

Closed

Nokel81 linked an issue Nov 29, 2021 that may be closed by this pull request

App crash at / #4442

Closed

aleksfront reviewed Nov 30, 2021

View reviewed changes

src/renderer/components/dock/log-list.tsx Outdated Show resolved Hide resolved

src/renderer/components/dock/log-resource-selector.tsx Outdated Show resolved Hide resolved

Nokel81 linked an issue Dec 2, 2021 that may be closed by this pull request

Blank Screen when accessing one (out of three) clusters #4018

Closed

Nokel81 force-pushed the fix/pod-logs-storage branch from 69a3583 to 0b0ae6e Compare December 2, 2021 19:10

Nokel81 requested a review from aleksfront December 2, 2021 19:14

aleksfront reviewed Dec 6, 2021

View reviewed changes

src/renderer/components/dock/logs.tsx Outdated Show resolved Hide resolved

aleksfront mentioned this pull request Dec 6, 2021

Lens crashed while viewing pod logs; now always crashes for this single cluster only #4506

Closed

Nokel81 linked an issue Dec 6, 2021 that may be closed by this pull request

Lens crashed while viewing pod logs; now always crashes for this single cluster only #4506

Closed

aleksfront reviewed Dec 8, 2021

View reviewed changes

aleksfront previously approved these changes Dec 10, 2021

View reviewed changes

Nokel81 dismissed aleksfront’s stale review via 5aa2084 December 10, 2021 14:07

Nokel81 requested a review from aleksfront December 14, 2021 15:43

github-actions bot added the PR: needs rebase label Dec 17, 2021

Nokel81 force-pushed the fix/pod-logs-storage branch from 5aa2084 to bc9a350 Compare December 17, 2021 16:11

github-actions bot removed the PR: needs rebase label Dec 17, 2021

Nokel81 added this to the 5.4.0 milestone Jan 5, 2022

github-actions bot added the PR: needs rebase label Jan 7, 2022

Nokel81 force-pushed the fix/pod-logs-storage branch from bc9a350 to a6e307c Compare January 7, 2022 21:06

github-actions bot removed the PR: needs rebase label Jan 7, 2022

Fix crash with logs view

cea7a34

- Switch to saving only IDs of pods, owners, and containers instead of whole objects - Add more checks about data integrety Signed-off-by: Sebastian Malton <[email protected]>

Nokel81 force-pushed the fix/pod-logs-storage branch from a6e307c to cea7a34 Compare January 7, 2022 21:10

Iku-turso requested changes Jan 11, 2022

View reviewed changes

github-actions bot added the PR: needs rebase label Jan 17, 2022

Nokel81 closed this Jan 24, 2022

Nokel81 deleted the fix/pod-logs-storage branch January 24, 2022 15:10

Nokel81 restored the fix/pod-logs-storage branch January 25, 2022 21:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix crash with logs view #4376

Fix crash with logs view #4376

Nokel81 commented Nov 17, 2021

aleksfront left a comment •

edited

Loading

Nokel81 commented Nov 30, 2021

Nokel81 commented Dec 2, 2021

aleksfront Dec 6, 2021

Nokel81 Dec 6, 2021

aleksfront Dec 6, 2021

Nokel81 Dec 6, 2021

aleksfront commented Dec 6, 2021

Nokel81 commented Dec 6, 2021

Nokel81 commented Dec 6, 2021

aleksfront commented Dec 8, 2021

aleksfront left a comment

aleksfront Dec 10, 2021

Nokel81 Dec 10, 2021

github-actions bot commented Dec 17, 2021

github-actions bot commented Dec 17, 2021

github-actions bot commented Jan 7, 2022

github-actions bot commented Jan 7, 2022

Iku-turso left a comment

Iku-turso Jan 11, 2022

Iku-turso Jan 11, 2022

Iku-turso Jan 11, 2022

Iku-turso Jan 11, 2022

Iku-turso Jan 11, 2022

Iku-turso Jan 11, 2022

Iku-turso Jan 11, 2022

Iku-turso Jan 11, 2022

Iku-turso Jan 11, 2022

Iku-turso Jan 11, 2022

github-actions bot commented Jan 17, 2022

Nokel81 commented Jan 24, 2022


		namespaces ??= KubeWatchApi.context?.contextNamespaces ?? [];

		if (isNamespaceFilterWatch && this.#watch.inc(store) > 1) {

		@@ -50,8 +51,11 @@ export function createStorage<T>(key: string, defaultValue: T) {

		try {
		storage.data = await fse.readJson(filePath);

Fix crash with logs view #4376

Fix crash with logs view #4376

Conversation

Nokel81 commented Nov 17, 2021

aleksfront left a comment • edited Loading

Choose a reason for hiding this comment

Nokel81 commented Nov 30, 2021

Nokel81 commented Dec 2, 2021

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

aleksfront commented Dec 6, 2021

Nokel81 commented Dec 6, 2021

Nokel81 commented Dec 6, 2021

aleksfront commented Dec 8, 2021

aleksfront left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Dec 17, 2021

github-actions bot commented Dec 17, 2021

github-actions bot commented Jan 7, 2022

github-actions bot commented Jan 7, 2022

Iku-turso 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

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

github-actions bot commented Jan 17, 2022

Nokel81 commented Jan 24, 2022

aleksfront left a comment •

edited

Loading