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

Allow small cpu requests for Synchrony container #650

Merged
merged 3 commits into from
Aug 22, 2023

Conversation

bianchi2
Copy link
Collaborator

@bianchi2 bianchi2 commented Aug 21, 2023

Addresses #644

This PR makes it possible to set smaller cpu requests for Synchrony container. Since we use synchrony.resources.container.requests.cpu in -XX:ActiveProcessorCount in synchrony entrypoint script as a JVM flag, setting anything less than 1 is n't possible since ActiveProcessorCount accepts int only. See: https://eclipse.dev/openj9/docs/xxactiveprocessorcount/. As a result, setting synchrony.resources.container.requests.cpu=100m or synchrony.resources.container.requests.cpu=0.5 results in Synchrony container crashing.

This PR adds a simple check - if synchrony.resources.container.requests.cpu contains m character, -XX:ActiveProcessorCount is set to 1.

I have also added a few comments to values.yaml docs, with a link to https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/#meaning-of-cpu

2 simple unit tests check if -XX:ActiveProcessorCount=1 when synchrony.resources.container.requests.cpu=20m, and
if if -XX:ActiveProcessorCount=5 when synchrony.resources.container.requests.cpu=5

Also, Synchrony is enabled in KinD tests, which wasn't possible because it wasn't possible to have a small cpu claim (GitHub actions have limits)

Checklist

  • I have added unit tests
  • The E2E test has passed (use e2e label)

@bianchi2 bianchi2 added the kind label Aug 21, 2023
@bianchi2 bianchi2 added kind and removed kind labels Aug 21, 2023
resources:
container:
requests:
cpu: 20m
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This will alow schedule synchrony pod, and ActiveProcessorCount will be set to 1.

@bianchi2 bianchi2 added the e2e label Aug 22, 2023
@yzha645
Copy link
Collaborator

yzha645 commented Aug 22, 2023

🚀

@bianchi2 bianchi2 merged commit 3248521 into main Aug 22, 2023
14 checks passed
@bianchi2 bianchi2 deleted the active-processors-count-synchrony branch August 22, 2023 05:14
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.

2 participants