Refactors sockets and moves reconnect logic and error handling into it. #7459

ghost · 2022-11-15T16:32:16Z

Summary

Fixes #5997
Socket subscriptions did not maintain adequate information about their current state to self-heal from an error state. Most notably, there was no information as to which resourceVersion or the age of the resourceVersion being stored in vuex that would allow for a quick recovery if a subscription (watch) was stopped. Subscriptions (watches) were tracked in a store key "started" which didn't adequately describe what was being stored, and a subscription's (watch's) error state was being kept in the key "inError" which also doesn't adequately describe that the key is for use only for subscriptions (watches).

A resourceVersion remains valid to use to resubscribe only if it is less than 5 minutes old, these resourceVersions are not necessarily sequential so they can't be guessed. Current logic was to use the latest resourceVersion from a single resource in a collection rather than the collection resourceVersion, causing issues creating the new subscription (watch).

Occurred changes and/or fixed issues

Websocket subscriptions for resources are referred to as "watches" in the Kubernetes API Concepts documentation so they are referred to as such in code and in this description from here on out.

Moves logic for maintaining resource watches into the socket object itself and extends it so that the socket maintains those subscriptions by itself to consolidate logic and location of socket relevant values.

Technical notes summary

A lot of code removed from shell/plugins/steve/subscribe.js and replaced in shell/utils/socket.js which should hopefully make sockets more modular (this lays a decent amount of groundwork for advanced webworkers as well). Changes outside of these files should be relatively minimal and there shouldn't be much in the way of user impact aside from more stable websocket watches.

Areas or cases that should be tested

High socket traffic
Watches the receive frequent "resource.stop" messages should resubscribe by themselves
Switching between clusters should work properly
resource.error messages should be handled gracefully, if an error uses the word "too old" then that watch should stop (if not already) and rewatch.

Areas which could experience regressions

This is a major change to sockets and watches so those should be thoroughly tested

catherineluse · 2022-11-22T01:53:30Z

@Sean-McQ I'd like to review this PR, but I have found it difficult to reproduce the original issue using Colleen's steps. Do you have any tips to make it easier to repro the original issue?

richard-cox

Haven't done a deep dive in to this yet, but making shell/utils/socket.js steve/norman specific breaks the other places it's used (epinio, harvester, streaming workload logs, shell to deployment). Easiest place to see is Workload list --> Deployment action menu --> Execute Shell

ghost · 2022-11-28T15:18:17Z

For the most part, Steve/Norman are pretty fundamental to the way we use websockets so I still think it makes sense for some of their requirements to be baked into them but I do see where/how that breaks shells atm. Gonna work on a fix for that and put in an update here.

ghost · 2022-11-30T03:16:41Z

As requested, new changes to the socket have been moved to a new class derived from the original socket class (ResourceWatcher) and the ResourceWatcher is in use by the subscribe Vuex methods.

richard-cox

Haven't totally finished reviewing, but hit a functionally blocking bug (see comment in resourceWatcher send(data) { and will pick up the rest tomorrow

richard-cox · 2022-11-30T15:07:52Z

shell/utils/resourceWatcher.js

+const INSECURE = 'ws://';
+const SECURE = 'wss://';
+
+const STATE_DISCONNECTED = 'disconnected';


Lines 8 - 18 are core socket stuff already defined in the root socket class

richard-cox · 2022-11-30T15:10:04Z

shell/utils/resourceWatcher.js

+export const EVENT_CONNECT_ERROR = 'connect_error';
+export const EVENT_DISCONNECT_ERROR = 'disconnect_error';
+
+export const NO_WATCH = 'NO_WATCH';


These and WATCHSTATUSES don't need to be exported

richard-cox · 2022-11-30T15:12:35Z

shell/utils/resourceWatcher.js

+  constructor(url, autoReconnect = true, frameTimeout = null, protocol = null, maxTries = null) {
+    super(url, autoReconnect, frameTimeout, protocol, maxTries);
+
+    this.maintenanceInterval = setInterval(() => {


maintenanceInterval is never stopped. I need to investigate this more, but we should probably wire this in to the connect/disconnect

richard-cox · 2022-11-30T16:48:34Z

shell/utils/resourceWatcher.js

+
+  setUrl(url) {
+    this.baseUrl = self.location.origin + url.replace('subscribe', '');
+    if ( !url.match(/wss?:\/\//) ) {


you could swap this section below out for just super.setUrl(url)

richard-cox · 2022-11-30T16:49:56Z

shell/utils/resourceWatcher.js

+    }
+  }
+
+  isConnecting() {


This can be removed

richard-cox · 2022-11-30T16:52:13Z

shell/utils/resourceWatcher.js

+      }
+    }
+
+    this.dispatchEvent(new CustomEvent(EVENT_MESSAGE, { detail: event }));


This line and lines 246, 247 and 248 are super._onmessage. Could that be used instead and placed either first or last? That would keep all common code in the root class and extended functionality in parent classe's method together

richard-cox · 2022-11-30T18:17:50Z

shell/plugins/steve/subscribe.js

  debug(state, on) {
    state.debugSocket = on !== false;
  },

  resetSubscriptions(state) {
-    clear(state.started);
+    const watcheKeys = Object.keys(state.socket.watches);


this throws an exception when going from the Virtualization Management product to a cluster's explorer product

richard-cox · 2022-11-30T18:28:19Z

shell/utils/resourceWatcher.js

+  // refactoring this to use new specific methods for subscribing and unsubscribing to resource collections which will make recovering stopped subscriptions a little bit easier.
+  send(data) {
+    // we're going to use these values from the data we're sending to determine if we're subscribing or unsubscribing
+    if (data.resourceType) {


This was never true for me (data is always a string), so this.watches is never initialised

richard-cox · 2022-12-19T11:53:14Z

Closing in favour of #7719 / #7671. There'll be another PR up that covers moving rancher socket subscription management into a worker

github-actions bot assigned ghost Nov 15, 2022

ghost marked this pull request as draft November 15, 2022 16:32

ghost requested review from richard-cox, nwmac and mantis-toboggan-md November 16, 2022 13:48

github-actions bot added this to the v2.7.1 milestone Nov 16, 2022

ghost marked this pull request as ready for review November 16, 2022 13:50

catherineluse self-requested a review November 22, 2022 01:51

ghost requested a review from rak-phillip November 22, 2022 17:31

richard-cox requested changes Nov 25, 2022

View reviewed changes

ghost closed this Nov 28, 2022

ghost reopened this Nov 28, 2022

Refactors sockets and moves reconnect logic and error handling into it.

329203f

ghost requested a review from richard-cox November 30, 2022 03:15

richard-cox requested changes Nov 30, 2022

View reviewed changes

richard-cox closed this Dec 19, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactors sockets and moves reconnect logic and error handling into it. #7459

Refactors sockets and moves reconnect logic and error handling into it. #7459

ghost commented Nov 15, 2022 •

edited by zube bot

Loading

catherineluse commented Nov 22, 2022

richard-cox left a comment

ghost commented Nov 28, 2022

ghost commented Nov 30, 2022

richard-cox left a comment

richard-cox Nov 30, 2022

richard-cox Nov 30, 2022

richard-cox Nov 30, 2022

richard-cox Nov 30, 2022

richard-cox Nov 30, 2022

richard-cox Nov 30, 2022

richard-cox Nov 30, 2022

richard-cox Nov 30, 2022

richard-cox commented Dec 19, 2022

Refactors sockets and moves reconnect logic and error handling into it. #7459

Refactors sockets and moves reconnect logic and error handling into it. #7459

Conversation

ghost commented Nov 15, 2022 • edited by zube bot Loading

Summary

Occurred changes and/or fixed issues

Technical notes summary

Areas or cases that should be tested

Areas which could experience regressions

catherineluse commented Nov 22, 2022

richard-cox left a comment

Choose a reason for hiding this comment

ghost commented Nov 28, 2022

ghost commented Nov 30, 2022

richard-cox 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

richard-cox commented Dec 19, 2022

ghost commented Nov 15, 2022 •

edited by zube bot

Loading