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.

Sign up for GitHub

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

Jump to bottom

Install PodDistruptionBudgets when replica greater than 1 #1509

Open
wants to merge 27 commits into
base: main
Choose a base branch
from
Open

Install PodDistruptionBudgets when replica greater than 1 #1509

wants to merge 27 commits into from

Conversation

rgarcia89
Copy link
Contributor

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 rgarcia89 changed the title Add PodDistruptionBudgets when replica greater than 1 Install PodDistruptionBudgets when replica greater than 1 May 17, 2023
@rgarcia89
Copy link
Contributor Author

@Vad1mo ready for review

@Vad1mo
Copy link
Member

Vad1mo commented May 23, 2023

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

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

@rgarcia89
Copy link
Contributor Author

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.

@MarcHenriot
Copy link

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
Copy link

MarcHenriot commented Jul 12, 2023 •
edited
Loading

@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

Shengwen Yu and others added 19 commits July 12, 2023 14:54
@rgarcia89
feat: add JOBSERVICE_WEBHOOK_JOB_MAX_RETRY and JOBSERVICE_WEBHOOK_JOB…
30f262c 
…_HTTP_CLIENT_TIMEOUT

Signed-off-by: Shengwen Yu <[email protected]>
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@geneliu @rgarcia89
feat: add configmap checksum annotation for portal
45881e8 
Signed-off-by: Gene Liu <[email protected]>
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@rgarcia89
feat: add config of reaper.max_update_hours & reaper.max_dangling_hours
b7a24d6 
Signed-off-by: Shengwen Yu <[email protected]>
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@jouve @rgarcia89
extraEnvVars for all workloads
4758272 
Signed-off-by: Cyril Jouve <[email protected]>
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@boniek83 @rgarcia89
feat: add podLabels
46f67fb 
Signed-off-by: Rafał Boniecki <[email protected]>
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@rgarcia89
Update logo path to main for new releases
5b76956 
Signed-off-by: OrlinVasilev <[email protected]>
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@Vad1mo @rgarcia89
Update README.md (goharbor#1488)
7a9bcca 
Based on multiple discussions and questions from the community. https://cloud-native.slack.com/archives/CC1E09J6S/p1682520074518419

It would make sense to update the readme and clarify the use of username password

Signed-off-by: Raul Garcia Sanchez <[email protected]>
@PhilipNelson5 @rgarcia89
Allow CONFIG_OVERWRITE_JSON to be set in values file (goharbor#1147)
363710d 
The JSON string set in core.configureUserSettings string
will be added to the core secret and loaded into the
environment variable CONFIG_OVERWRITE_JSON

Signed-off-by: Philip Nelson <[email protected]>
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@rgarcia89
feat: remove notary
16e1d08 
Signed-off-by: Shengwen Yu <[email protected]>
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@rgarcia89
fix: add github-ghcr to the list of PERMITTED_REGISTRY_TYPES_FOR_PROX…
2d87984 
…Y_CACHE

Signed-off-by: Shengwen Yu <[email protected]>
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@kariya-mitsuru @rgarcia89
Add TLS 1.3 support for harbor-nginx
1374b17 
Signed-off-by: Mitsuru Kariya <[email protected]>
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@kariya-mitsuru @rgarcia89
Add TLS 1.3 support for harbor-portal
36ee315 
Signed-off-by: Mitsuru Kariya <[email protected]>
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@arjungandhi @rgarcia89
Fix incorrect comment on values.yaml
93092f8 
the commend above the existing secret field specified the wrong information for the what the key of the secret should be

Signed-off-by: Arjun Gandhi <[email protected]>
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@rgarcia89
Add tls checksum
21b8211 
Signed-off-by: Hein-Jan Vervoorn <[email protected]>
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@rgarcia89
Allow user to set topologySpreadConstraints
a20d33c 
Co-authored-by: Stephan Mauermann <[email protected]>
Co-authored-by: Stephan Mauermann <[email protected]>
Signed-off-by: Diogo Guerra <[email protected]>
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@sudermanjr @rgarcia89
Fix goharbor#1127 - remove capabilities check for prometheus
94d71ed 
Signed-off-by: Andy Suderman <[email protected]>
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@MarcHenriotMMS @rgarcia89
feat: add pdb
f5b1043 
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@MarcHenriotMMS @rgarcia89
feat: add pdb
fce0169 
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@MarcHenriotMMS @rgarcia89
fix: set podDisruptionBudget.enabled to false
62f997c 
Signed-off-by: Raul Garcia Sanchez <[email protected]>
@rgarcia89
Copy link
Contributor Author

@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

@MarcHenriot
Copy link

MarcHenriot commented Jul 12, 2023 •
edited
Loading

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

@rgarcia89
Copy link
Contributor Author

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

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

@rgarcia89
Copy link
Contributor Author

@Vad1mo I think this PR is ready to be merged

@MarcHenriot
Copy link

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

It misses matchLabels in the selector section.

@rgarcia89
Copy link
Contributor Author

@Vad1mo any idea when this can be merged?

@OrlinVasilev OrlinVasilev requested a review from zyyw December 13, 2023 10:49
@OrlinVasilev
Copy link
Member

@zyyw can you please review that PR thank you!

@rgarcia89
Copy link
Contributor Author

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

@slushysnowman
Copy link

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

@Kajot-dev
Copy link
Contributor

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

@balonik
Copy link

balonik commented Jul 31, 2024

@zyyw please review and approve this MR. It is pending just single approval.

@flaviomoringa
Copy link

Hi, we just had a MAJOR issue on our setup due to the lack of a PDB on Harbor. This feature is definetly needed ASAP, and I'm quite perplex how this is not there on the default HR since the beggining.

Please approve and add this to the next release ASAP.

Thanks a lot for this amazing piece of software.

@OrlinVasilev
Copy link
Member

@Vad1mo I think that's good to go

@phantasm66
Copy link

phantasm66 commented Nov 20, 2024 •
edited
Loading

It looks like the only failing workflow integration test for this PR is due to an outdated workflow action (deprecated version of actions/upload-artifact: v2). Can that be addressed and this feature be merged? 🙏

@rgarcia89
Copy link
Contributor Author

All cleared and ready to be merged

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@OrlinVasilev OrlinVasilev OrlinVasilev approved these changes

@Vad1mo Vad1mo Vad1mo approved these changes

@zyyw zyyw Awaiting requested review from zyyw

Assignees
No one assigned
Labels
kind/requirement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.