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.

Sign up for GitHub

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

Jump to bottom

Fix concurrent workspace limit #9491

Merged
merged 8 commits into from
May 26, 2022
Merged

Fix concurrent workspace limit #9491

merged 8 commits into from
May 26, 2022

Conversation

andrew-farries
Copy link
Contributor

@andrew-farries andrew-farries commented Apr 22, 2022 •
edited
Loading

Description

Change the initial phase of a workspace from unknown to preparing to avoid filtering them out when calculating a user's concurrent workspace limit.

Builds on #9453 to allow us to distinguish between workspaces waiting for an image build and workspaces that may not need one.

Related Issue(s)

Fixes #9410

How to test

Smoke test various IDEs:

  • In-browser vscode
  • Desktop vscode
  • Goland (via Jetbrains Gateway)
  • local companion app

Release Notes

Correctly enforce the parallel workspace limit

Documentation

None

  • /werft with-vm=true

@andrew-farries andrew-farries force-pushed the af/add-new-workspace-phase branch from c00f147 to 28617ff Compare April 25, 2022 15:58
@meysholdt
Copy link
Member

hey, can you please rebase your branch before you continue working with your preview env? The old base contains a ws-daemon that logs expensively.

To save costs, I'll manually remove the old preview env of this branch. Apologies for the inconvenience.

@andrew-farries andrew-farries force-pushed the af/fix-concurrent-workspace-limit branch from 65b97b8 to 40b7068 Compare April 25, 2022 16:00
@andrew-farries
Copy link
Contributor Author

andrew-farries commented Apr 25, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run

👍 started the job as gitpod-build-af-fix-concurrent-workspace-limit.3
(with .werft/ from main)

@andrew-farries andrew-farries force-pushed the af/add-new-workspace-phase branch 2 times, most recently from 927fcd1 to 9463f6f Compare April 26, 2022 13:22
@csweichel
Copy link
Contributor

@andrew-farries @geropl how do we move forward with this? (post-rebase that is :) )

@andrew-farries
Copy link
Contributor Author

I'm struggling to test this (and the branch on which it's based - #9453) because image builds aren't working for me in preview environments. It needs a a run to check that we haven't broken anything in the transition between workspace phases with the addition of the new 'building' phase.

@geropl geropl force-pushed the af/add-new-workspace-phase branch from 9463f6f to 1ddc7cc Compare April 27, 2022 09:37
@andrew-farries andrew-farries force-pushed the af/add-new-workspace-phase branch from 1ddc7cc to 3150c52 Compare April 27, 2022 15:43
@andrew-farries andrew-farries force-pushed the af/fix-concurrent-workspace-limit branch from 40b7068 to 06d742c Compare April 27, 2022 15:48
@andrew-farries
Copy link
Contributor Author

andrew-farries commented Apr 27, 2022 •
edited by werft-gitpod-dev-com bot
Loading

/werft run with-vm=true

👍 started the job as gitpod-build-af-fix-concurrent-workspace-limit.5
(with .werft/ from main)

Base automatically changed from af/add-new-workspace-phase to main April 27, 2022 19:21
@roboquat roboquat added size/M and removed size/S labels Apr 27, 2022
@geropl
Copy link
Member

geropl commented Apr 28, 2022

@andrew-farries @geropl how do we move forward with this? (post-rebase that is :) )

@csweichel The first PR of the series (the types and backwards-compatible handling) is merged: #9453

Before moving forward with this PR we now have to make sure all clients handle the new "phase".

@andrew-farries andrew-farries force-pushed the af/fix-concurrent-workspace-limit branch from 06d742c to 2fdd520 Compare April 28, 2022 09:23
@roboquat roboquat added size/S and removed size/M labels Apr 28, 2022
@andrew-farries
Copy link
Contributor Author

rebased to resolve merge conflicts and taking this out of 'draft'.

@andrew-farries andrew-farries marked this pull request as ready for review April 28, 2022 09:23
@andrew-farries andrew-farries requested a review from a team April 28, 2022 09:23
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Apr 28, 2022
@geropl geropl marked this pull request as draft April 29, 2022 08:19
@geropl
Copy link
Member

geropl commented Apr 29, 2022

@andrew-farries Moving back to draft because we have to clarify with @gitpod-io/engineering-ide what they have to do before we can merge this one. 👍

AlexTugarev
AlexTugarev reviewed May 25, 2022
View reviewed changes
components/ws-manager-bridge/src/config.ts
@@ -33,6 +33,7 @@ export interface Configuration {
timeouts: {
metaInstanceCheckIntervalSeconds: number;
preparingPhaseSeconds: number;
buildingPhaseSeconds: number;
Copy link
Member

Choose a reason for hiding this comment

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

how do we distribute config changes? do we need to have default values for compatibility?
what's the plan for self-hosted?

Copy link
Member

Choose a reason for hiding this comment

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

@andrew-farries, I believe this needs to be reflected in configmap.go, like the other timeouts.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done in a0e4ea7

@AlexTugarev
Copy link
Member

I'm testing now...

@roboquat roboquat added size/M and removed size/S labels May 25, 2022
@andrew-farries andrew-farries force-pushed the af/fix-concurrent-workspace-limit branch from 99fa3f1 to 6ce07a3 Compare May 25, 2022 12:29
@andrew-farries andrew-farries force-pushed the af/fix-concurrent-workspace-limit branch from eaa9cba to 472adc0 Compare May 25, 2022 15:43
@roboquat roboquat added size/S and removed size/M labels May 25, 2022
@andrew-farries andrew-farries force-pushed the af/fix-concurrent-workspace-limit branch from 472adc0 to e23e74d Compare May 25, 2022 15:47
Add a createWorkspace rate limit
4e84e94 
This was previously in the `default` group.
@andrew-farries andrew-farries force-pushed the af/fix-concurrent-workspace-limit branch from e23e74d to 4e84e94 Compare May 25, 2022 15:47
AlexTugarev
AlexTugarev approved these changes May 25, 2022
View reviewed changes
Copy link
Member

@AlexTugarev AlexTugarev left a comment

Choose a reason for hiding this comment

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

LGTM!
Thanks for checking and testing all of the hidden gems!

There is one comment on a new config entry to consider.

/hold

@andrew-farries
Copy link
Contributor Author

/unhold

@roboquat roboquat merged commit 2bba4a6 into main May 26, 2022
@roboquat roboquat deleted the af/fix-concurrent-workspace-limit branch May 26, 2022 09:19
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels May 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexTugarev AlexTugarev AlexTugarev approved these changes

Assignees
No one assigned
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note size/S team: webapp Issue belongs to the WebApp team
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Max. parallel workspace limit is not correctly enforced
7 participants
@andrew-farries @meysholdt @csweichel @geropl @akosyakov @AlexTugarev @roboquat