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.

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

feat(argo-cd): Add optional mapping of argocd-repo-server emptydir to custom volumes #2410

Merged
merged 17 commits into from
Jan 18, 2024

Conversation

aroundthecode
Copy link
Contributor

@aroundthecode aroundthecode commented Jan 2, 2024

Checklist:

  • I have bumped the chart version according to versioning
  • I have updated the documentation according to documentation
  • I have updated the chart changelog with all the changes that come with this pull request according to changelog.
  • Any new values are backwards compatible and/or have sensible default.
  • I have signed off all my commits as required by DCO.
  • My build is green (troubleshooting builds).

@github-actions github-actions bot added the size/M label Jan 2, 2024
@aroundthecode aroundthecode changed the title Draft: map argocd-repo-server emptydir to custom volumes if needed feat(argo-cd): Add optional mapping of argocd-repo-server emptydir to custom volumes Jan 2, 2024
@pdrastil
Copy link
Member

pdrastil commented Jan 2, 2024

Hi, thanks for PR but we never merged PVC support as this would be breaking for production deployments that use HPA to run more replicas (typically 30-50 in large scale deployments). For this reason the repo-server was intentionally kept as a stateless. See details in #1648

@aroundthecode
Copy link
Contributor Author

aroundthecode commented Jan 2, 2024

Hi @pdrastil the idea was to leave default behaviour as-is with emptydir but allow people with specific use case to be able to customize volumes at will.
The big issue with emptydir is that is triggereing "disk pressure" on k8 nodes, so basically is not production ready as well.

In my setup I had to manually modify Helm chart as per PR to let it work otherwise it woud crash my nodes even with few application.
Using disk mount on tmp folder is the official solution for this so it's quite strange it's not supported at all by helm chart.

Official documentation itself suggests to attach /tmp folder to a persistent volume https://argo-cd.readthedocs.io/en/stable/operator-manual/high_availability/ so I see no reason to have an hardcoded "emptydir" in helm chart

argocd-repo-server clones the repository into /tmp (or the path specified in the TMPDIR env variable). The Pod might run out of disk space if it has too many repositories or if the repositories have a lot of files. To avoid this problem mount a persistent volume.

@snuggie12
Copy link

Hi, thanks for PR but we never merged PVC support as this would be breaking for production deployments that use HPA to run more replicas (typically 30-50 in large scale deployments). For this reason the repo-server was intentionally kept as a stateless. See details in #1648

@pdrastil I agree with @aroundthecode and it would be nice to override this. Currently I have to patch in using ephemeral volume claims which works around the issue you mentioned.

$ cat patches/argocd-repo-server-deploy.patch.yaml
apiVersion: apps/v1
kind: Deployment
metadata:
  name: argocd-repo-server
  namespace: argocd
spec:
  template:
    spec:
      volumes:
        - name: tmp
          emptyDir: null
          ephemeral:
            volumeClaimTemplate:
              metadata:
                labels:
                  type: argocd-repo-server-tmp
              spec:
                accessModes:
                  - ReadWriteOnce
                storageClassName: standard-rwo
                resources:
                  requests:
                    storage: 10Gi

Provide user-driven option to replace emptydir volumes with desired solution

Signed-off-by: Michele Sacchetti <michele.sacchetti@aroundthecode.org>
provide default values with empy object to ensure default behavior is backward-compatible

Signed-off-by: Michele Sacchetti <michele.sacchetti@aroundthecode.org>
bump version

Signed-off-by: Michele Sacchetti <michele.sacchetti@aroundthecode.org>
add changelog

Signed-off-by: Michele Sacchetti <michele.sacchetti@aroundthecode.org>
fix docs on new entry

Signed-off-by: Michele Sacchetti <michele.sacchetti@aroundthecode.org>
Signed-off-by: Michele Sacchetti <michele.sacchetti@aroundthecode.org>
Signoff commit

Signed-off-by: Michele Sacchetti <michele.sacchetti@aroundthecode.org>
Signed-off-by: Michele Sacchetti <michele.sacchetti@aroundthecode.org>
Signed-off-by: Michele Sacchetti <michele.sacchetti@aroundthecode.org>
@aroundthecode
Copy link
Contributor Author

aroundthecode commented Jan 8, 2024

@pdrastil and others, I'd like to know if this PR can be merged or not.
I'm strugglig a bit to get chart-test passing (sorry this is my first PR on this project), but in the last day nobody let it run and I'm keeping the branch updated for other main branch modification.

I think this PR allows to keep things "stateless" by default as @pdrastil suggested but also allow people to customize mountpoints followin best practices if needed.

In my company we are currently using a manually modified version of argo-helm project, so having this PR merged would mean we can go on using it and contributing to it, but I'd like to know if there is any interest on maintainer side to this kind of approach.

@aroundthecode
Copy link
Contributor Author

@mbevc1 @mkilchhofer @yu-croco @jmeridth @pdrastil @tico24 any chance to see this approved?
Turned full green yesteday but got again surpassed by other version and had to merge again.
I'm available for any information/discussion.

@jmeridth
Copy link
Member

As we've had two consumers of this project express the need and one has done the PR to allow the change I'm a fan of merging. This leaves default of emptydir in place.

@yu-croco @pdrastil @mkilchhofer what say you?

Signed-off-by: Michele Sacchetti <michele.sacchetti@aroundthecode.org>
@pdrastil pdrastil merged commit 508162f into argoproj:main Jan 18, 2024
6 checks passed
@pdrastil
Copy link
Member

Thanks for contribution

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants