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

Ensure too old messages for watches in individual resources are handled correctly #10541

Merged
merged 4 commits into from
Mar 8, 2024

Conversation

richard-cox
Copy link
Member

@richard-cox richard-cox commented Mar 1, 2024

Requires v2.9-head at or passed 793118f (from late 2024-03-07)

Summary

Fixes #10540

Occurred changes and/or fixed issues

  • See issue description for detail. Description on how sockets work in [SURE-7122] Excessive WebSocket activity when watching resources with permission by name rancher#41809 (comment)
  • Ensure that watches on single resources that fail with too old fetch the latest version of the resource and re-watch without a revision
  • There were three bugs
    • core issue - the re-watch used the same too old revision from the resource, resulting in the too old message again ad nauseam
    • the http request to fetch the latest version failed for namespaced resources
    • the inError state is fetched via a key constructed of id, namespace and selector, however the inError state from resource.error messages only used the key... which meant for watches that used id or selector the watches were never classed as in error

Technical notes summary

  • The first bug needed the plumbing for namespace to be fixed.... though it's not actually honoured via the watch (it just watches all resources with the same id
  • TODO: the fix for the second bug involves NOT sending a revision. this currently mean all changes are replayed over socket instead of just changes occurring after the watch Not sending a revision means we get the latest version of the resource in a resource.create event (it's not the original resource.create followed by everything else)

Areas or cases that should be tested

As per repo steps

Areas which could experience regressions

  • changes to a resource following a refresh on the details page not showing up correctly

Checklist

  • The PR is linked to an issue and the linked issue has a Milestone, or no issue is needed
  • The PR has a Milestone
  • The PR template has been filled out
  • The PR has been self reviewed
  • The PR has a reviewer assigned
  • The PR has automated tests or clear instructions for manual tests and the linked issue has appropriate QA labels, or tests are not needed
  • The PR has reviewed with UX and tested in light and dark mode, or there are no UX changes

@richard-cox richard-cox added this to the v2.9.0 milestone Mar 1, 2024
@richard-cox richard-cox self-assigned this Mar 1, 2024
…dled correctly

- also fix issue where `find` on `resyncWatch` didn't work with namespaced resources
…ts still happened

- caused because
  - resource.stop is sent alongside resource.error
  - on resource.error we set the state of the watch to errored
    - this was just using the root type as key and nothing else
  - on resource.stop we call resource.start
  - on resource.start we check for inError and bail if so
    - this didn't happen, as we checked the inError using more than just root type
- fix is to make sure setInError useds same key process as inError getter
  - this follows other places it's used
@richard-cox richard-cox requested a review from nwmac March 7, 2024 18:16
@richard-cox richard-cox marked this pull request as ready for review March 7, 2024 18:16
- better comments
- improved tests
- handle trailing url `/`
const urlObj = new URL(url);
const path = urlObj.pathname;

if (!!path?.length && path[path.length - 1] === '/') {
Copy link
Member

Choose a reason for hiding this comment

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

Could have used endsWith

@richard-cox richard-cox merged commit cb2ab78 into rancher:master Mar 8, 2024
23 checks passed
@richard-cox richard-cox deleted the detail-socket-spam branch March 8, 2024 12:10
@richard-cox
Copy link
Member Author

/backport v2.8.next1 release-2.8

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.

Steve socket is spammed on fresh visit to a resource detail page
2 participants