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

root: Added Minimum Memory and CPU Reservation #12313

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

kaindlnetwork
Copy link
Contributor

@kaindlnetwork kaindlnetwork commented Dec 10, 2024

Details

I've added minimum memory and CPU reservations into the docker compose file to ensure that the containers can start properly and to prevent running into out-of-memory issues by informing Docker about the expected resource consumption.


Checklist

  • Local tests pass (ak test authentik/)
  • The code has been formatted (make lint-fix)

If an API change has been made

  • The API schema has been updated (make gen-build)

If changes to the frontend have been made

  • The code has been formatted (make web)

If applicable

  • The documentation has been updated
  • The documentation has been formatted (make website)

Signed-off-by: Kaindl Network <82705244+kaindlnetwork@users.noreply.github.com>
@kaindlnetwork kaindlnetwork requested a review from a team as a code owner December 10, 2024 14:03
Copy link

netlify bot commented Dec 10, 2024

Deploy Preview for authentik-docs canceled.

Name Link
🔨 Latest commit 7981218
🔍 Latest deploy log https://app.netlify.com/sites/authentik-docs/deploys/67584a38739212000880c765

Copy link

netlify bot commented Dec 10, 2024

Deploy Preview for authentik-storybook ready!

Name Link
🔨 Latest commit 7981218
🔍 Latest deploy log https://app.netlify.com/sites/authentik-storybook/deploys/67584a385cffe8000867e35d
😎 Deploy Preview https://deploy-preview-12313--authentik-storybook.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

codecov bot commented Dec 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.62%. Comparing base (83edb0d) to head (7981218).
Report is 2 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main   #12313   +/-   ##
=======================================
  Coverage   92.62%   92.62%           
=======================================
  Files         762      762           
  Lines       38230    38230           
=======================================
  Hits        35409    35409           
  Misses       2821     2821           
Flag Coverage Δ
e2e 48.91% <ø> (-0.01%) ⬇️
integration 24.72% <ø> (ø)
unit 90.23% <ø> (+<0.01%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@rissson
Copy link
Member

rissson commented Dec 10, 2024

I'm not against this change directly, but maybe those should be left commented as it should be up to the user to configure those, especially if they don't have any configured elsewhere on their system. For instance, we don't configure those in the Helm chart, but we do provide examples of how to do so: https://github.com/goauthentik/helm/blob/main/charts/authentik/values.yaml#L352

@kaindlnetwork
Copy link
Contributor Author

kaindlnetwork commented Dec 10, 2024

@rissson Thank you for your feedback!

I see your point that these settings could be considered user-specific. However, I believe setting minimal memory and CPU reservations in the Docker-Compose file could serve as a best practice for several reasons:

  • Stability and Error Prevention: Without resource limits, especially new users might encounter hard-to-diagnose issues.
  • Standardized Configuration: The Docker-Compose file acts as a reference. Clear default settings promote a stable and reproducible setup.
  • Transparent Documentation: Even if these values were commented out, it would highlight their importance for maintaining a stable environment.
    Since Docker-Compose is often used for simpler setups, reasonable default settings could be helpful. Perhaps adding a descriptive comment in the Docker-Compose file would be a good compromise?

What do you think about this approach? I’m open to alternative suggestions.

@rissson
Copy link
Member

rissson commented Dec 17, 2024

Again, I'd like to see that provided to the user in the same fashion we do it for the Helm chart

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.

2 participants