Reduce memory requests of minio in preview envs #10158

vulkoingim · 2022-05-20T15:48:12Z

Description

Average memory usage for minio seems to be ~100mb - I've left it a bit more just in case

kubectl top pods minio-859b8f94b5-xztxk --containers
POD                      NAME    CPU(cores)   MEMORY(bytes)   
minio-859b8f94b5-xztxk   minio   2m           87Mi

Reduce memory requests of minio in preview environments

Related Issue(s)

Fixes # #10028

vulkoingim · 2022-05-23T10:56:45Z

/werft run

👍 started the job as gitpod-build-aa-minio-resources.15
(with .werft/ from main)

sagor999

/hold
if you want to address nit.

install/installer/pkg/components/minio/helm.go

install/installer/pkg/config/v1/config.go

easyCZ

LGTM, approving for WebApp

mads-hartmann

Thanks for the PR @vulkoingim! I'd prefer for this change to be a no-op for anyone who doesn't want to set resource requests for minio. That would mean making the config optional, and if the user doesn't set the config, then don't provide defaults.

mads-hartmann · 2022-05-24T06:55:29Z

install/installer/pkg/config/v1/config.go

@@ -152,6 +155,7 @@ type ObjectStorage struct {
 	Azure              *ObjectStorageAzure        `json:"azure,omitempty"`
 	MaximumBackupCount *int                       `json:"maximumBackupCount,omitempty"`
 	BlobQuota          *int64                     `json:"blobQuota,omitempty"`
+	Resources          Resources                  `json:"resources" validate:"required"`


I'm a bit hesitant to make this required as that's a breaking change (as far as I know). Can we make it optional instead?

I realise there might have been some context missing about the installer, please see this comment where I elaborate a bit more ☺️ #10191 (review)

By default minio gets 2G of memory, but it's not obvious where it is set (have to dig to find the original yaml, which gets overwritten eventually) - that's why I opted for making it required as it makes it explicit. I realise I should have left a bit more context.

Setting the default to 2gb sounds good then ☺️ but I'd still prefer to make sure it isn't a required field in the config. Otherwise when this is merged people will get an error when they try to use the new version of the installer with their existing config

When you have a default value, you don't need to set a value in the config. The user-given config from the file is merged with the default config in the code.

When you have a default value, you don't need to set a value in the config. The user-given config from the file is merged with the default config in the code.

I thought it would be like that as well, but it seems that the merging is done at some later stage.

Installer inits a default config

The custom configs in installer.ts get applied

Validation runs and fails if it wasn't set if it was marked as required

https://github.com/gitpod-io/gitpod/blob/main/.werft/jobs/build/deploy-to-preview-environment.ts#L290-L292

@vulkoingim: Sorry, I don't get it. 😇

With this installer: eu.gcr.io/gitpod-core-dev/build/installer:aa-minio-resources.20 (where it was required and not omitempty), you can run

installer init > config.yaml

Remove
resources: requests: memory: 256Mi
from objectStorage block in config.yaml

and installer validate config --config config.yaml success as expected!?

The only difference between omitempty and validate:"required" is whether it is generated by installer init or not. And with omitempty it's not part of the generated config.yaml but I think it should. That makes that config value easier to discover. That's why I would vote for settings is as required and not omitempty. What do you think?

The only difference between omitempty and validate:"required" is whether it is generated by installer init or not. And with omitempty it's not part of the generated config.yaml

Sorry for leading you astray @vulkoingim - I thought that adding validate:"required" meant it had to be part of config.yaml.

@corneliusludmann

The only difference between omitempty and validate:"required" is whether it is generated by installer init or not. And with omitempty it's not part of the generated config.yaml but I think it should.

The required part is also used for validation, not only for generating the config. So it will be generated, but without a default value (or overwritten in installer.ts) it will be set to null, so eventually it will fail the validation. It would look like this:

objectStorage: inCluster: true resources: requests: null

In general I also think that we should set it to required, give it a default value in the config.go:53, and overwrite it in the installer.ts, as I've done so far.

But what Mads also says is true - if we set it to required and someone is using an old config (where this part is not present) with this new version of the installer - it will fail.

{ "valid": false, "fatal": [ "Field 'Config.ObjectStorage.Resources.Requests' is required" ] }

I just tested the behavior for both versions and both fail for this config:

objectStorage: inCluster: true resources: requests: null

However, when the new config value resources in objectStorage is missing entirely (that is what old config files would have, it works with both versions as well.

And: Both versions generate the resources block (I was wrong here and it makes sense since the value is not empty and thus not omitted).

Therefore, I'm fine with the current implementation.

install/installer/pkg/config/v1/config.go

corneliusludmann

Looks good. 🚀

A few random comments:

Please squash your commits into one commit with a meaningful message.
Since you already there, would you mind running the following command and pushing the updated example-config.yaml?
```
cd install/installer && go run . init > example-config.yaml
```
You need to add a release-note block to your pull request description with a meaningful message like this:
```
```release-note
Add your meaningful changelog message here!
```
```

When you are done, remove the do-not-merge/hold label from this pull request.

Thanks a lot! 🚢 🛹

mads-hartmann

Thanks 🎉

roboquat added do-not-merge/work-in-progress do-not-merge/release-note-label-needed size/S labels May 20, 2022

vulkoingim force-pushed the aa/minio-resources branch from afb1515 to b6165d6 Compare May 23, 2022 10:23

vulkoingim changed the title ~~Reduce memory request of minio and workspaces in preview envs~~ Reduce memory requests of minio in preview envs May 23, 2022

vulkoingim force-pushed the aa/minio-resources branch from b6165d6 to 604bad9 Compare May 23, 2022 10:59

vulkoingim marked this pull request as ready for review May 23, 2022 14:56

vulkoingim requested review from a team May 23, 2022 14:56

roboquat removed the do-not-merge/work-in-progress label May 23, 2022

github-actions bot added 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 May 23, 2022

vulkoingim force-pushed the aa/minio-resources branch from 2f412c6 to 3b07ba3 Compare May 23, 2022 14:59

sagor999 approved these changes May 23, 2022

View reviewed changes

install/installer/pkg/components/minio/helm.go Outdated Show resolved Hide resolved

install/installer/pkg/config/v1/config.go Outdated Show resolved Hide resolved

roboquat added the do-not-merge/hold label May 23, 2022

easyCZ approved these changes May 24, 2022

View reviewed changes

mads-hartmann requested changes May 24, 2022

View reviewed changes

vulkoingim requested a review from mads-hartmann May 24, 2022 08:08

corneliusludmann approved these changes May 24, 2022

View reviewed changes

mads-hartmann approved these changes May 24, 2022

View reviewed changes

Reduce minio memory requests in preview environments

9e8c62d

vulkoingim force-pushed the aa/minio-resources branch from 9599e0d to 9e8c62d Compare May 24, 2022 14:47

roboquat added release-note and removed do-not-merge/release-note-label-needed labels May 24, 2022

vulkoingim removed the do-not-merge/hold label May 24, 2022

roboquat merged commit 476851b into main May 24, 2022

roboquat deleted the aa/minio-resources branch May 24, 2022 14:54

vulkoingim mentioned this pull request May 24, 2022

Fix ObjectStorage resources block required even when empty #10223

Merged

roboquat added the deployed: workspace Workspace team change is running in production label May 25, 2022

roboquat added the deployed: webapp Meta team change is running in production label Jun 9, 2022

Reduce memory requests of minio in preview envs #10158

Reduce memory requests of minio in preview envs #10158

Uh oh!

Conversation

vulkoingim commented May 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Related Issue(s)

Uh oh!

vulkoingim commented May 23, 2022 • edited by werft-gitpod-dev-com bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sagor999 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

easyCZ left a comment

Choose a reason for hiding this comment

Uh oh!

mads-hartmann left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vulkoingim May 24, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

corneliusludmann left a comment

Choose a reason for hiding this comment

Uh oh!

mads-hartmann left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

vulkoingim commented May 20, 2022 •

edited

Loading

vulkoingim commented May 23, 2022 •

edited by werft-gitpod-dev-com bot

Loading

vulkoingim May 24, 2022 •

edited

Loading