harbor-helm icon indicating copy to clipboard operation
harbor-helm copied to clipboard

Install PodDistruptionBudgets when replica greater than 1

Open rgarcia89 opened this issue 1 year ago • 18 comments

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.

rgarcia89 avatar May 17 '23 08:05 rgarcia89

@Vad1mo ready for review

rgarcia89 avatar May 23 '23 08:05 rgarcia89

I like it. It would be beneficial to make it optional:

  • [ ] add enable/disable flag, on each component an
  • [ ] Allow to set Min amount

Vad1mo avatar May 23 '23 16:05 Vad1mo

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.

rgarcia89 avatar May 24 '23 07:05 rgarcia89

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)

MarcHenriot avatar Jun 02 '23 13:06 MarcHenriot

@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

MarcHenriot avatar Jul 12 '23 01:07 MarcHenriot

@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

rgarcia89 avatar Jul 12 '23 12:07 rgarcia89

I forgot to sign my commits …. What do we do ?

MarcHenriot avatar Jul 12 '23 14:07 MarcHenriot

I forgot to sign my commits …. What do we do ?

Yeah I just signed them one by one. Should be fine now

rgarcia89 avatar Jul 12 '23 14:07 rgarcia89

@Vad1mo I think this PR is ready to be merged

rgarcia89 avatar Jul 21 '23 12:07 rgarcia89

There is typo in all pdb, I will push a fix to your fork.

It misses matchLabels in the selector section.

MarcHenriot avatar Aug 02 '23 21:08 MarcHenriot

@Vad1mo any idea when this can be merged?

rgarcia89 avatar Sep 05 '23 11:09 rgarcia89

@zyyw can you please review that PR thank you!

OrlinVasilev avatar Dec 13 '23 10:12 OrlinVasilev

@MinerYang @Kajot-dev maybe you can have a look here too

rgarcia89 avatar Feb 06 '24 08:02 rgarcia89

Would be great to get this in - at the moment we're manually adding it for our Harbor setup

slushysnowman avatar Feb 06 '24 08:02 slushysnowman

That would be great to have! We're adding these besides the helm chart currently

Kajot-dev avatar Feb 06 '24 10:02 Kajot-dev