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: configure gitaly with recommended resiliency settings #238

Merged

Conversation

JoeHCQ1
Copy link
Contributor

@JoeHCQ1 JoeHCQ1 commented Oct 29, 2024

Added a Pepr policy exemption to enable cgroups, added cgroups variable, added other recommended settings from https://docs.gitlab.com/ee/administration/gitaly/kubernetes.html

Problems:

  1. The exemptions are there regardless of whether or not we wanted to use cgroup policing. Alternative: add a helm chart where I can dynamically add or not add the manifest based on the desired-ness of cgroups.
  2. The cgroups are very your-environment specific. This needs messaged clearly as there's no out-of-the-box solution here.
  3. That this feature weakens security should probably be clearly communicated as a tradeoff.

Note, the only "testing" I forsee being possible in this setup is verifying that the settings took. Fail behavior would require overwhelming our massive gitaly node.

@JoeHCQ1 JoeHCQ1 requested a review from a team as a code owner October 29, 2024 20:38
@JoeHCQ1 JoeHCQ1 linked an issue Oct 29, 2024 that may be closed by this pull request
@jacobbmay
Copy link
Collaborator

jacobbmay commented Oct 29, 2024

  1. The exemption being present whether the cgroups setting is used or not is fine. The expectation is that the cgroups settings are configured because they are recommended by GitLab for stability.
  2. This is true, however we should also set defaults that correspond to whatever the default resource settings are for Gitaly. Just need to document the need to modify them if Gitaly resource allocations are changed.
  3. I don't think this is necessary as I would consider it an essential feature for production ready Gitaly in kubernetes. No different than any of the other exemptions that we already have in place even if this is technically an optional setting.

@JoeHCQ1 JoeHCQ1 self-assigned this Oct 30, 2024
@JoeHCQ1 JoeHCQ1 requested a review from bburky November 6, 2024 17:50
@JoeHCQ1
Copy link
Contributor Author

JoeHCQ1 commented Nov 6, 2024

@bburky this is where we're giving Gitaly the keys to the kingdom.

@JoeHCQ1 JoeHCQ1 changed the title draft: configure gitaly with recommended resiliency settings feat: configure gitaly with recommended resiliency settings Nov 6, 2024
Copy link
Collaborator

@jacobbmay jacobbmay left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@JoeHCQ1 JoeHCQ1 merged commit 63f4989 into main Nov 6, 2024
1 check passed
@JoeHCQ1 JoeHCQ1 deleted the 221-configure-gitaly-with-recommended-resiliency-settings branch November 6, 2024 22:19
This was referenced Dec 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Configure gitaly with recommended resiliency settings
3 participants