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

[installer]: Allow replica counts to be specified for all components #9495

Merged
merged 8 commits into from
Apr 26, 2022

Conversation

andrew-farries
Copy link
Contributor

@andrew-farries andrew-farries commented Apr 22, 2022

Description

One of the Webapp team's epics for Q2 is to use the Gitpod installer to deploy to Gitpod SaaS. In order to do that we will need to add additional configuration to the installer to make the output suitable for a SaaS deployment as opposed to a self-hosted deployment.

This PR adds the ability to configure the number of replicas for each component, rather than using to a hard-coded value of 1.

Related Issue(s)

Part of #9097

How to test

Create an installer config file containing this experimental section:

experimental:
  common:
    podConfig:
      server:
        replicas: 123
      content-service:
        replicas: 456

Get a versions.yaml for use with the installer:

docker run -it --rm "eu.gcr.io/gitpod-core-dev/build/versions:${version}" cat versions.yaml > versions.yaml

Then invoke the installer as:

installer render --debug-version-file versions.yaml --config /path/to/config --use-experimental-config

The rendered output will have the values from the config for the server and content-service components and the old default of 1 for all other components.

Release Notes

Allow replica counts for each component to be configurable through the installer

Documentation

None.

@andrew-farries andrew-farries requested review from a team April 22, 2022 16:06
@github-actions github-actions bot added team: IDE 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 and removed size/M labels Apr 22, 2022
Copy link
Contributor

@mustard-mh mustard-mh left a comment

Choose a reason for hiding this comment

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

this?

// todo(sje): receive config value
Replicas: pointer.Int32(1),

@mrsimonemms
Copy link
Contributor

mrsimonemms commented Apr 22, 2022

@andrew-farries I hope I'm not coming across as Mr Negative, but I'm not sure this is the right approach. The experimental section is designed as "Gitpod SaaS-only" stuff but there are going to definitely be large self-hosted instances that will need to have multiple replicas. Generally speaking, we're not going to be supporting the use of the experimental section for self-hosted users (with some exceptions).

What might be a better approach is a more nuanced approach.

  1. Adding a section to the main config, something like lite (I don't like that name, but can't think of anything better at the moment) where it defaults to false - if it's enabled, it puts everything into a single replica and if disabled it puts everything that can have multiple replicas to 2 (I believe there are deployments that cannot be replicated, but not sure)
  2. Keep the experimental section as you've done for more fine-grained configuration

Then the value can be 1 or 2 for self-hosted users with a fairly small amount of configuration or n for SaaS users.

I'd also prefer that the experimental section didn't have a CommonConfig struct - I think it may be better having experimental.webapp.dashboard.podConfig.replicas = 2. That said, as this is for Gitpod SaaS, I'm not going to put up a fight if you want to stick with the current approach.

Also, we'll need to have some validation to make sure that the values are an integer. It's worth seeing what happens if you set the value to a string/boolean - hopefully it'll error/panic, but I don't think it would from reading the code

Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

PR looks good, left a comment around general approach to config in go. Would you be able to please also add tests for the common.Replicas function? It's shared across many components and I'd rather we did have some tests to start with as we evolve it.

install/installer/pkg/common/common.go Show resolved Hide resolved
@easyCZ easyCZ self-assigned this Apr 25, 2022
@geropl
Copy link
Member

geropl commented Apr 25, 2022

but I'm not sure this is the right approach. The experimental section is designed as "Gitpod SaaS-only" stuff but there are going to definitely be large self-hosted instances that will need to have multiple replicas

@mrsimonemms this approach was taken after a sync with @corneliusludmann last week which we scheduled for exactly this discussion. In general I agree: This is an option that we will need in for self-hosted at some point. But we decided to solve it this way, and put it into experimental because it's far easier to change afterwards. There is a whole host of config that could go into such a "common"/"generic" podConfig section. But unless we know how that should look like I suggest we decouple that decision from this PR/change. 👍

@geropl
Copy link
Member

geropl commented Apr 25, 2022

/hold

@andrew-farries Could you add unit tests as suggested by @easyCZ ? Thank you! 🙏

@andrew-farries andrew-farries force-pushed the af/installer-specify-replicacount branch 2 times, most recently from 939a2e1 to 3950294 Compare April 25, 2022 11:20
Copy link
Member

@easyCZ easyCZ left a comment

Choose a reason for hiding this comment

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

Approving as I'm happy with the changes. Left a couple of suggestions but up to you if you want to tackle them or how.

@andrew-farries
Copy link
Contributor Author

this?

// todo(sje): receive config value
Replicas: pointer.Int32(1),

Thanks, I missed this one.

@andrew-farries
Copy link
Contributor Author

Also, we'll need to have some validation to make sure that the values are an integer. It's worth seeing what happens if you set the value to a string/boolean - hopefully it'll error/panic, but I don't think it would from reading the code

If the value for replicas is set to a string, the installer fails with:

image

@andrew-farries andrew-farries force-pushed the af/installer-specify-replicacount branch 2 times, most recently from 5599f33 to 2a30a53 Compare April 25, 2022 11:58
Copy link
Contributor

@corneliusludmann corneliusludmann left a comment

Choose a reason for hiding this comment

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

Looks good to me.

Regarding experimental or not: As long as we don't have customers asking for this and we are not sure yet how exactly this config should look like it's much more convenient for us to use the experimental section as incubation for this config and move it to the general (public) section once we are fine with that and want to support this officially. That was the reason for the decision.

Andrew Farries added 6 commits April 25, 2022 14:59
Allow a new `common` field under `experimental` for cross-cutting
concerns such as replica counts.
To allow lookup of replica counts. Defaults to one if there is no
replica count configured for a component.
So that they respect the new
`experimental.common.podConfig.<component>.replicas` setting.
@andrew-farries andrew-farries force-pushed the af/installer-specify-replicacount branch from 2a30a53 to 672ff2e Compare April 25, 2022 13:59
@andrew-farries
Copy link
Contributor Author

andrew-farries commented Apr 25, 2022

/werft run with-clean-slate-deployment=true

👍 started the job as gitpod-build-af-installer-specify-replicacount.9
(with .werft/ from main)

@andrew-farries
Copy link
Contributor Author

andrew-farries commented Apr 25, 2022

/werft run with-vm=true

👍 started the job as gitpod-build-af-installer-specify-replicacount.10
(with .werft/ from main)

@andrew-farries
Copy link
Contributor Author

andrew-farries commented Apr 25, 2022

/werft run with-vm=true

👍 started the job as gitpod-build-af-installer-specify-replicacount.12
(with .werft/ from main)

To avoid problems with import cycles when importing components/*
packages.
@andrew-farries andrew-farries force-pushed the af/installer-specify-replicacount branch from 75a1234 to 725fbd0 Compare April 25, 2022 16:10
@andrew-farries andrew-farries requested a review from easyCZ April 25, 2022 16:21
@andrew-farries
Copy link
Contributor Author

andrew-farries commented Apr 25, 2022

@easyCZ requested a re-review from you as I had to move the tests into an _test package (725fbd0) in order to address this comment.

To run each testcase as a sub-test.
@andrew-farries
Copy link
Contributor Author

/unhold

@roboquat roboquat merged commit 94086f9 into main Apr 26, 2022
@roboquat roboquat deleted the af/installer-specify-replicacount branch April 26, 2022 07:44
@roboquat roboquat added deployed: webapp Meta team change is running in production deployed: workspace Workspace team change is running in production deployed: IDE IDE change is running in production labels Apr 26, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployed: IDE IDE change is running in production deployed: webapp Meta team change is running in production deployed: workspace Workspace team change is running in production release-note size/L team: delivery Issue belongs to the self-hosted team team: IDE team: webapp Issue belongs to the WebApp team team: workspace Issue belongs to the Workspace team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants