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

[messagebus, bridge] Remove messagebus cross-cluster dependency #7523

Merged
merged 4 commits into from
Feb 17, 2022

Conversation

geropl
Copy link
Member

@geropl geropl commented Jan 10, 2022

Description

This PR contains two complementary changes:

  1. remove the messagebus shovel config, and all non-local exchanges (gitpod.ws) and use local exchanges instead (gitpod.ws.local)
  2. change ws-manager-bridge so that bridges that are non-governing:
    a. still don't write updates to the DB,
    b. but do the same derivation of updates,
    c. and distributes those over their cluster's local messagebus

Note: This PR can be reviewed commit by commit!

Pain points

This polling and hashing feel quite expensive, especially in scenarios where things go haywire already (a lot of workspaces hanging in preparing). But I see no way around it until we pushed image-builder into workspace clusters (#7845). 🤷 Once that's done we should be able to just delete PreparingUpdateEmulator 🗑️ .

Deployment

This PR is meant to be a drop-in replacement for the current setup, which means:

behavior before and after should be identical (order of updates, state persisted to DB before update sent, except for cross-cluster stuff, etc.)
all currently running workspaces should behave as usual, e.g. keep running smoothly
rolling this out will require a messagebus restart, but apart from that behave like a regular deployment

ToDo:

Related Issue(s)

Fixes #7468

How to test

cluster-local works as before

  • start a workspace with devtools open, and watch the websocket messages that keep flowing in
  • note the onInstaceUpdate notifications
  • note how the workspace starts as expected

cross-cluster

We'll test this on staging as the setup is already there, and an alternative setup would eat too much time at the moment.

Release Notes

messagebus: remove cross-cluster dependency

Documentation

@roboquat roboquat added release-note do-not-merge/work-in-progress team: webapp Issue belongs to the WebApp team team: delivery Issue belongs to the self-hosted team size/XXL labels Jan 10, 2022
@codecov
Copy link

codecov bot commented Jan 10, 2022

Codecov Report

Merging #7523 (3eacd33) into main (42d529c) will decrease coverage by 1.13%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #7523      +/-   ##
==========================================
- Coverage   12.31%   11.17%   -1.14%     
==========================================
  Files          20       18       -2     
  Lines        1161      993     -168     
==========================================
- Hits          143      111      -32     
+ Misses       1014      880     -134     
+ Partials        4        2       -2     
Flag Coverage Δ
components-gitpod-cli-app 11.17% <ø> (ø)
components-local-app-app-darwin-amd64 ?
components-local-app-app-darwin-arm64 ?
components-local-app-app-linux-amd64 ?
components-local-app-app-linux-arm64 ?
components-local-app-app-windows-386 ?
components-local-app-app-windows-amd64 ?
components-local-app-app-windows-arm64 ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
components/local-app/pkg/auth/auth.go
components/local-app/pkg/auth/pkce.go

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 42d529c...3eacd33. Read the comment docs.

@geropl geropl force-pushed the gpl/7468-remove-shovel branch from a85ce65 to f9a33e7 Compare January 10, 2022 16:35
@roboquat roboquat added size/L and removed size/XXL labels Jan 10, 2022
@geropl geropl self-assigned this Jan 11, 2022
@geropl geropl mentioned this pull request Jan 12, 2022
@roboquat roboquat added size/XL and removed size/L labels Jan 13, 2022
@stale stale bot added meta: stale This issue/PR is stale and will be closed soon and removed meta: stale This issue/PR is stale and will be closed soon labels Jan 23, 2022
@roboquat roboquat added team: workspace Issue belongs to the Workspace team size/XXL and removed size/XL labels Jan 27, 2022
@geropl geropl force-pushed the gpl/7468-remove-shovel branch from e83c1f4 to 1f259aa Compare January 27, 2022 14:41
@roboquat
Copy link
Contributor

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from geropl and additionally assign mrsimonemms after the PR has been reviewed.
You can assign the PR to them by writing /assign @mrsimonemms in a comment when ready.

Associated issue: #7468

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@roboquat roboquat added size/XL and removed size/XXL labels Jan 27, 2022
@stale stale bot added the meta: stale This issue/PR is stale and will be closed soon label Feb 6, 2022
@geropl geropl force-pushed the gpl/7468-remove-shovel branch from 1f259aa to 0c376d3 Compare February 7, 2022 13:52
@stale stale bot removed the meta: stale This issue/PR is stale and will be closed soon label Feb 7, 2022
@geropl geropl force-pushed the gpl/7468-remove-shovel branch 2 times, most recently from 98c5a5e to 4fac32b Compare February 16, 2022 12:50
@geropl geropl force-pushed the gpl/7468-remove-shovel branch from 4fac32b to 29b17d1 Compare February 16, 2022 13:03
@gitpod-io gitpod-io deleted a comment from stale bot Feb 16, 2022
@gitpod-io gitpod-io deleted a comment from stale bot Feb 16, 2022
@geropl
Copy link
Member Author

geropl commented Feb 16, 2022

/hold

Don't merge before tomorrows deployment 🙂

Now: don't merge when I'm not prepared to monitor deployment. 🕵️‍♂️

@geropl geropl marked this pull request as ready for review February 16, 2022 13:41
@geropl geropl requested review from a team February 16, 2022 13:41
@geropl geropl force-pushed the gpl/7468-remove-shovel branch from 29b17d1 to 3eacd33 Compare February 16, 2022 13:56
@geropl geropl mentioned this pull request Feb 16, 2022
10 tasks
Copy link
Contributor

@JanKoehnlein JanKoehnlein left a comment

Choose a reason for hiding this comment

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

LGTM codewise. Let's merge and test it on staging

@geropl
Copy link
Member Author

geropl commented Feb 17, 2022

/unhold

@roboquat roboquat merged commit a3d1b61 into main Feb 17, 2022
@roboquat roboquat deleted the gpl/7468-remove-shovel branch February 17, 2022 10:43
@roboquat roboquat added the deployed: workspace Workspace team change is running in production label Feb 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: workspace Workspace team change is running in production release-note size/XL team: delivery Issue belongs to the self-hosted team team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

Remove messagebus shovel
4 participants