harbor-helm
harbor-helm copied to clipboard
Install PodDistruptionBudgets when replica greater than 1
Currently there is no pdb deployed for components were the replica is defined greater than 1. This can lead to an outage of the components during a cluster update. The added pdb definitions will be added automatically once the replica size is greater than 1 and ensure that at least one replica is ready before the pod will be drained.
@Vad1mo ready for review
I like it. It would be beneficial to make it optional:
- [ ] add enable/disable flag, on each component an
- [ ] Allow to set Min amount
Is setting the min amount really necessary? Currently at least one pod is necessary in order for the component to fulfill its tasks. Therefore I think we should be good with the fix value.
Thanks for the implementation, it's really cool!
I quite agree with @Vad1mo and I understand your point of view, but harcoding values is never very good, some users may have use cases that push them to change this number. (If, for example, the harbor has a lot of simultaneous users, perhaps two replicas of a certain service is the bare minimum required to maintain the desired quality of service when operating on Kubernetes nodes.)
In issue #1181 robinkb proposed a rather elegant solution from the oauth2-proxy chart.
I think it would be better to set the default value to 1 and leave it up to the user to choose what best suits their needs. (ref helm doc)
It might also be cool to have the same logic with maxUnavailable fields. (ref #1182)
@rgarcia89 I have pushed a version of the above changes in my fork, can we merge my version to yours ?
Repo : https://github.com/MarcHenriot/harbor-helm
@rgarcia89 I have pushed a version of the above changes in my fork, can we merge my version to yours ?
Repo : https://github.com/MarcHenriot/harbor-helm
Looks good to me. I have merged your fork. Thanks
I forgot to sign my commits …. What do we do ?
I forgot to sign my commits …. What do we do ?
Yeah I just signed them one by one. Should be fine now
@Vad1mo I think this PR is ready to be merged
There is typo in all pdb, I will push a fix to your fork.
It misses matchLabels
in the selector
section.
@Vad1mo any idea when this can be merged?
@zyyw can you please review that PR thank you!
@MinerYang @Kajot-dev maybe you can have a look here too
Would be great to get this in - at the moment we're manually adding it for our Harbor setup
That would be great to have! We're adding these besides the helm chart currently