search

grafana / helm-charts

Apache License 2.0
1.67k stars 2.28k forks source link

[tempo-distributed] Define default affinity for compactor #3422

Open froblesmartin opened 1 week ago

froblesmartin commented 1 week ago

As it is not even defined as null as an example, I guess it was just totally forgotten. 😄 But it is available to be used at https://github.com/grafana/helm-charts/blob/51db949b568a602ce2c6aef88b3c6d76b82ed828/charts/tempo-distributed/templates/compactor/deployment-compactor.yaml#L114-L116

I am defining the same affinities as everywhere else, also the same as in loki-distributed for the compactor.

Please, let me know if everything is alright or if I should change something.

CLAassistant commented 1 week ago

CLA assistant check
All committers have signed the CLA.

froblesmartin commented 1 week ago

Hi @mapno ! :)

I have pushed an update with the README.md updated as well as bumping the patch version. In this case, this change defines a new default value, which could be considered a breaking change 🤔 but in this case, if someone had defined something then it will not affect them, and if they didn't, this is a good to have. Still, someone with a single node could see pods after the 1st one not being scheduled.

Let me know what you think 😉

Thanks!

mapno commented 1 week ago

Hi @mapno ! :)

I have pushed an update with the README.md updated as well as bumping the patch version. In this case, this change defines a new default value, which could be considered a breaking change 🤔 but in this case, if someone had defined something then it will not affect them, and if they didn't, this is a good to have. Still, someone with a single node could see pods after the 1st one not being scheduled.

Let me know what you think 😉

Thanks!

Hi @froblesmartin. I think putting a disclaimer in the README should be enough for this change. A minor version update seems ok. Thanks for the PR!

froblesmartin commented 4 days ago

Hi @froblesmartin. I think putting a disclaimer in the README should be enough for this change. A minor version update seems ok. Thanks for the PR!

@mapno done 😉 bumped a minor version and added some lines in the README 👍