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 configuration of affinity for server and proxy components #9558

Conversation

andrew-farries
Copy link
Contributor

@andrew-farries andrew-farries commented Apr 26, 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 affinities (both node and inter-pod) for all components.

Following discussion, we now only allow the server and proxy components to configure their pod anti-affinity via a simple boolean toggle (experimental.webapp.usePodAffinity) in the installer config.

Related Issue(s)

Part of #9097

How to test

Create an installer config file containing this experimental section:

experimental:
  webapp:
    usePodAffinity: true

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:

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

The rendered output will set anti-affinity for the server and proxy components.

All other components are unaffected and have their hard coded (node-)affinities.

Release Notes

Allow pod affinities for `server` and `proxy` to be configurable through the installer

Documentation

None.

@andrew-farries andrew-farries requested review from a team April 26, 2022 13:16
@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 labels Apr 26, 2022
@geropl
Copy link
Member

geropl commented Apr 26, 2022

@andrew-farries Let's limit the scope of this PR to IDE + webapp here by just using Affinity to those components.
Especially workspace will be very happy to use just NodeAffinity. And if they need it, they can always change later.

@andrew-farries
Copy link
Contributor Author

@andrew-farries Let's limit the scope of this PR to IDE + webapp here by just using Affinity to those components. Especially workspace will be very happy to use just NodeAffinity. And if they need it, they can always change later.

I think we don't lose anything by applying the change uniformly to all components. Workspace components are still picking up their default node affinity and no pod affinities with these changes.

IMO there is more value in having all components configure their affinity in the same way, even if only a couple use the experimental config to do more than simple node affinity.

@csweichel
Copy link
Contributor

csweichel commented Apr 26, 2022

Could you add some context as to why this is necessary in the first place?
While building the installer we made the very deliberate decision to not make the affinity configurable, because this has led to considerable complexity (i.e. variance and infrastructure difference) in the past.

When it comes to anti-affinity for server, can't we just bake that right in? I.e. if this behaviour is good for us, won't it be good for everyone else? (And if it isn't, can't we add a single boolean flag then?)

@geropl
Copy link
Member

geropl commented Apr 26, 2022

When it comes to anti-affinity for server, can't we just bake that right in? I.e. if this behaviour is good for us, won't it be good for everyone else?

This won't work, as anti-affinity is provider specific, and adds complexity where it is not needed.

Could you add some context as to why this is necessary in the first place?
While building the installer we made the very deliberate decision to not make the affinity configurable, because this has led to considerable complexity (i.e. variance and infrastructure difference) in the past.

I think we understand this requirement. But on the other hand we have to cater for a usecase where we additional flexibility solves a real problem, e.g. scalability issues.
I very much agree, though, that we should not make this the default for all pods. 👍

IMO we can shortcut this if we made this webapp component specifc. Or even narrower: server, proxy @csweichel Would that make sense?

@andrew-farries
Copy link
Contributor Author

While building the installer we made the very deliberate decision to not make the affinity configurable, because this has led to considerable complexity (i.e. variance and infrastructure difference) in the past.

I think it's also worth emphasising that this is under the experimental section in the installer config and as such we don't intend to expose these settings to users; rather they are just for our use to tailor the installer output to make it suitable for installing to Gitpod SaaS.

@geropl geropl mentioned this pull request Apr 26, 2022
53 tasks
@csweichel
Copy link
Contributor

csweichel commented Apr 26, 2022

When it comes to anti-affinity for server, can't we just bake that right in? I.e. if this behaviour is good for us, won't it be good for everyone else?

This won't work, as anti-affinity is provider specific, and adds complexity where it is not needed.

I don't think that's correct. The anti-affinity we need for our components (proxy, server and ws-proxy) uses our own structure (see below). It even becomes easier exactly because we do not allow users to configure the required node labels.

      affinity:
        nodeAffinity:
          requiredDuringSchedulingIgnoredDuringExecution:
            nodeSelectorTerms:
            - matchExpressions:
              - key: gitpod.io/workload_services
                operator: In
                values:
                - "true"
        podAntiAffinity:
          preferredDuringSchedulingIgnoredDuringExecution:
          - podAffinityTerm:
              labelSelector:
                matchExpressions:
                - key: component
                  operator: In
                  values:
                  - server
              topologyKey: gitpod.io/workload_services
            weight: 100

Could you add some context as to why this is necessary in the first place?
While building the installer we made the very deliberate decision to not make the affinity configurable, because this has led to considerable complexity (i.e. variance and infrastructure difference) in the past.

I think we understand this requirement. But on the other hand we have to cater for a usecase where we additional flexibility solves a real problem, e.g. scalability issues. I very much agree, though, that we should not make this the default for all pods. 👍

Not questioning that we need degrees of freedom to do our job. I'm merely questioning if we need them here.

IMO we can shortcut this if we made this webapp component specifc. Or even narrower: server, proxy @csweichel Would that make sense?

If we had to do this (and I'm not convinced that is the case yet), this would be the way to go.

@csweichel
Copy link
Contributor

I think it's also worth emphasising that this is under the experimental section in the installer config and as such we don't intend to expose these settings to users; rather they are just for our use to tailor the installer output to make it suitable for installing to Gitpod SaaS.

Fair point - that said, it's not just the promises we make to users w.r.t. to the config surface, but predominantly the complexity and installation variants we ourselves need to maintain and test.

@aledbf
Copy link
Member

aledbf commented Apr 26, 2022

but predominantly the complexity and installation variants we ourselves need to maintain and test.

This also opens the door to support questions in discord for the DCS/self-hosted teams.

@csweichel
Copy link
Contributor

@andrew-farries @geropl how do we continue with this one?

@andrew-farries
Copy link
Contributor Author

@andrew-farries @geropl how do we continue with this one?

The consensus here seems to be that we make this less generic; ie only allow the few components that need to configure their affinities for a SaaS deployment able to do so.

@geropl
Copy link
Member

geropl commented Apr 27, 2022

@andrew-farries @geropl how do we continue with this one?

The consensus here seems to be that we make this less generic; ie only allow the few components that need to configure their affinities for a SaaS deployment able to do so.

Agreed. This removes the incidental complexity while keeping the flexibility where we'd like to keep it. @andrew-farries Let's sync on the concrete code changes. 👍

@corneliusludmann
Copy link
Contributor

corneliusludmann commented Apr 27, 2022

Given the discussion above, is this pull request still ready for review, @andrew-farries? If not (because there are ongoing discussions that need to be finished first) would you mind marking this pull request as “draft” again so that it is clear for all requested reviews whether they should review this pull request or not? Thanks. 🙏

@andrew-farries andrew-farries marked this pull request as draft April 27, 2022 08:52
@andrew-farries andrew-farries force-pushed the af/installer-specify-pod-affinity branch from f6118dc to 97e4854 Compare April 27, 2022 13:23
@roboquat roboquat added size/M and removed size/L labels Apr 27, 2022
@andrew-farries
Copy link
Contributor Author

Rebased (and updated PR description) to allow only the server and proxy components to configure their pod affinity/anti-affinity.

This should address the concerns above about allowing too much variation via the installer. @csweichel are you ok with this, or would you rather make this more restrictive by further limiting the pod affinity/anti-affinty that these components are allowed to set?

@csweichel
Copy link
Contributor

I still don't understand why anity-affinity needs to be configurable to this degree, i.e. why devolve the concrete configuration/parametrisation to the experimental config. IMHO that introduces unnecessary degrees of freedom.
Why not just make that part of the installer itself - maybe add a boolean flag to enable the anti-affinity settings.

It would seem that I'm missing some info here, or am not seeing a concrete scenario that you're designing for.

@andrew-farries
Copy link
Contributor Author

I still don't understand why anity-affinity needs to be configurable to this degree, i.e. why devolve the concrete configuration/parametrisation to the experimental config. IMHO that introduces unnecessary degrees of freedom.
Why not just make that part of the installer itself - maybe add a boolean flag to enable the anti-affinity settings.

The server configures its anti-affinity as follows:

          podAntiAffinity:
            preferredDuringSchedulingIgnoredDuringExecution:
              - podAffinityTerm:
                  labelSelector:
                    matchExpressions:
                      - key: component
                        operator: In
                        values:
                          - server
                  topologyKey: gitpod.io/workload_services
                weight: 100

and for proxy:

          podAntiAffinity:
            preferredDuringSchedulingIgnoredDuringExecution:
              - podAffinityTerm:
                  labelSelector:
                    matchExpressions:
                      - key: component
                        operator: In
                        values:
                          - proxy
                  topologyKey: gitpod.io/workload_services
                weight: 100

tbh, I'm not sure how much variance we expect to have to support here; if this is all we need then we could simplify this further to a simple toggle as you say. @geropl would you be happy with that change?

@geropl
Copy link
Member

geropl commented Apr 28, 2022

Why not just make that part of the installer itself - maybe add a boolean flag to enable the anti-affinity settings.
It would seem that I'm missing some info here, or am not seeing a concrete scenario that you're designing for.

My reasoning is more that we should not overload installer with these very specific optimizations/concerns here. I also see the point of unnecessary freedom, but maybe I interpreted the experimental section slightly differently. But I don't want to expand the discussion here too much. Let's move on with the minimal consensus, and expand on concrete usecases, as usual! 👍 💯

tbh, I'm not sure how much variance we expect to have to support here; if this is all we need then we could simplify this further to a simple toggle as you say. @geropl would you be happy with that change?

@andrew-farries Let's do that!

Andrew Farries added 3 commits April 28, 2022 10:13
@andrew-farries andrew-farries force-pushed the af/installer-specify-pod-affinity branch from 97e4854 to 8258c5f Compare April 28, 2022 10:51
@andrew-farries
Copy link
Contributor Author

tbh, I'm not sure how much variance we expect to have to support here; if this is all we need then we could simplify this further to a simple toggle as you say. @geropl would you be happy with that change?

@andrew-farries Let's do that!

Rebased to make the pod anti-affinity for server and proxy configurable via a simple toggle under webapp.experimental.

@andrew-farries andrew-farries marked this pull request as ready for review April 28, 2022 11:02
@andrew-farries andrew-farries changed the title [installer] Allow configuration of affinity for all components [installer] Allow configuration of affinity for server and proxy components Apr 28, 2022
Server *ServerConfig `json:"server,omitempty"`
PublicAPI *PublicAPIConfig `json:"publicApi,omitempty"`
Server *ServerConfig `json:"server,omitempty"`
UsePodAffinity bool `json:"usePodAffinity"`
Copy link
Member

Choose a reason for hiding this comment

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

nit: Should read anti: UsePodAntiAffinity bool json:"usePodAntiAffinity"`

Copy link
Member

@geropl geropl left a comment

Choose a reason for hiding this comment

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

LGTM!
Please have a look at the comment above ☝️

@roboquat roboquat merged commit 6141d4a into af/installer-specify-resourcerequirements Apr 28, 2022
@roboquat roboquat deleted the af/installer-specify-pod-affinity branch April 28, 2022 12:15
@geropl
Copy link
Member

geropl commented Apr 28, 2022

@andrew-farries The PR is based on another branch, please re-create against main.

@andrew-farries
Copy link
Contributor Author

The branch was stacked on #9545 to avoid merge conflicts. I'll address your comment in a follow-up

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note size/M 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.

6 participants