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

Add workspace class to workspace instance #10454

Merged
merged 3 commits into from
Jun 8, 2022
Merged

Add workspace class to workspace instance #10454

merged 3 commits into from
Jun 8, 2022

Conversation

atduarte
Copy link
Contributor

@atduarte atduarte commented Jun 3, 2022

Description

Adds the workspace class, also known as workspace size, to the workspace instance. This will be relevant to determine usage costs and for our own reporting.

A workspace class determines what type of resources a given workspace instance gets.

Related Issue(s)

Fixes #10430

How to test

Manual test using preview environments, using @jankeromnes suggested steps.

Results

Release Notes

NONE

Documentation

  • /werft with-payment

@@ -501,6 +501,7 @@ export class WorkspaceStarter {

instance.status.phase = "pending";
instance.region = installation;
instance.workspaceClass = startRequest.getSpec()!.getClass();
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

Copy link
Contributor

@jankeromnes jankeromnes left a comment

Choose a reason for hiding this comment

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

Hey, many thanks for implementing this @atduarte! 🎉

Regarding how this can be tested, I'd suggest the following:

  • Add - [x] /werft with-payment at the bottom of your PR description, and trigger a new Werft build (this will enable paid plans)
  • Set up a Chargebee webhook for this PR (I can do that for you)
  • Connect to the database like so:
    • Open the PR in Gitpod
    • Run ./dev/preview/install-k3s-kubeconfig.sh (manually 🙄)
    • Run kubectl port-forward mysql-0 3306 & (to expose the database port)
    • Run mysql -h 127.0.0.1 -pjBzVMe2w4Yi7GagadsyB gitpod (to connect to the database)

Next, you'll be able to test this:

  • Start a new workspace while on the free plan
    • in the database, the workspace instance should have workspaceClass: 'default'
  • Grant yourself access to XL:
    • create a new team (e.g. by visiting /teams/new)
    • upgrade the team to Unleashed (in team settings > billing)
    • in the database, update d_b_team_subscription2 excludeFromMoreResources to 0
  • Start the same workspace again, or a new workspace
    • this workspace should have workspaceClass = 'gitpodio-internal-xl' in the DB

Happy to help with any of this, and/or have a call. 😁

@roboquat roboquat added size/M and removed size/S labels Jun 3, 2022
Copy link
Contributor Author

@atduarte atduarte left a comment

Choose a reason for hiding this comment

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

Thanks for the thoughtful comments and guidance, @jankeromnes! 🧡

I'm currently stuck at "upgrade the team to Unleashed (in team settings > billing)" as I'm required to pay. 🤔 Looked for ways on Slack and Notion without any luck. Got it 🔍

"Payment in Progress". I guess you need to activate the Chargebee webhook now?

@jankeromnes
Copy link
Contributor

jankeromnes commented Jun 4, 2022

Sure, more than happy to help, especially since you spontaneously started implementing this yourself. 💯 Many thanks for that.

"Payment in Progress". I guess you need to activate the Chargebee webhook now?

Ah, yes indeed, forgot to set this up. I went into Chargebee and added a webhook for this PR, so payments should go through now. (Also, didn't share the exact steps, because we're considering moving away from Chargebee soon, but if you're curious I'm still happy to share more anyway)

Regarding the enum, I understand your concerns but I'm not sure using an enum is a good idea. It will depend on the conversation regarding how server will learn about the available classes in the workspace-clusters—it could be dynamic, and we will surely go further than the S-XL 🤔

Given this, do you still have a strong opinion on using enums? Happy to do what you think is best :)

Hmm, that's a good point. I still think we'll need some kind of fixed mapping between particular classes and credits/prices, but maybe that could also be dynamic?

Since this is still a bit unclear and somewhat orthogonal to this contribution, I have no strong feelings about enum or not -- please feel free to go ahead with whatever is most convenient for this PR, and let's figure out the mapping in a future iteration. 👍

@atduarte
Copy link
Contributor Author

atduarte commented Jun 6, 2022

/werft with-payment

@gtsiolis
Copy link
Contributor

gtsiolis commented Jun 6, 2022

/werft run with-payment

👍 started the job as gitpod-build-ad-workspace-class.3
(with .werft/ from main)

@atduarte
Copy link
Contributor Author

atduarte commented Jun 6, 2022

image

😄

@atduarte atduarte marked this pull request as ready for review June 6, 2022 09:27
@atduarte atduarte requested a review from a team June 6, 2022 09:27
@github-actions github-actions bot added the team: webapp Issue belongs to the WebApp team label Jun 6, 2022
@easyCZ
Copy link
Member

easyCZ commented Jun 7, 2022

LGTM. I've 2 questions remaining:

  1. Are we guaranteed to receive the workspace class from the cluster manager? As in, has this been completely rolled out?
  2. Given the code, it's currently possible to end up with 1 Workspace which had multiple Workspace Instances where each instance was of a different class. Is this acceptable?

1 similar comment
@easyCZ
Copy link
Member

easyCZ commented Jun 7, 2022

LGTM. I've 2 questions remaining:

  1. Are we guaranteed to receive the workspace class from the cluster manager? As in, has this been completely rolled out?
  2. Given the code, it's currently possible to end up with 1 Workspace which had multiple Workspace Instances where each instance was of a different class. Is this acceptable?

@geropl
Copy link
Member

geropl commented Jun 8, 2022

Given the code, it's currently possible to end up with 1 Workspace which had multiple Workspace Instances where each instance was of a different class. Is this acceptable?

From my point of view: yes, absolutely. I imagine a extreme case where you start up a workspace in the past, and try to re-start after 1 month. However the config change, the new instance should be guided by that, and not what was configured in the past.

@atduarte
Copy link
Contributor Author

atduarte commented Jun 8, 2022

Are we guaranteed to receive the workspace class from the cluster manager? As in, has this been completely rolled out?

@easyCZ The class is chosen by the server and currently is actually hard-coded here:

let workspaceClass: string = "default";
if (await this.userService.userGetsMoreResources(user)) {
workspaceClass = "gitpodio-internal-xl";
}

@roboquat roboquat merged commit ff84252 into main Jun 8, 2022
@roboquat roboquat deleted the ad-workspace-class branch June 8, 2022 09:19
public async up(queryRunner: QueryRunner): Promise<void> {
if (!(await columnExists(queryRunner, TABLE_NAME, COLUMN_NAME))) {
await queryRunner.query(
`ALTER TABLE ${TABLE_NAME} ADD COLUMN ${COLUMN_NAME} varchar(255) NOT NULL DEFAULT '', ALGORITHM=INPLACE, LOCK=NONE`,
Copy link
Contributor

Choose a reason for hiding this comment

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

🔥 Thanks for adding the ALGORITHM=INPLACE, LOCK=NONE here, which will avoid about 2 hours of db-sync downtime during deployment. 😁 🚀

@roboquat roboquat added deployed: webapp Meta team change is running in production deployed Change is completely running in production labels Jun 9, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: webapp Meta team change is running in production deployed Change is completely running in production release-note-none size/M team: webapp Issue belongs to the WebApp team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Store workspace size used for a given workspace instance
6 participants