Skip to content

Update start-odk.sh memory calculation to support cgroups v2#876

Merged
lognaturel merged 2 commits intogetodk:nextfrom
hotosm:build/cgroupv2
Jan 21, 2025
Merged

Update start-odk.sh memory calculation to support cgroups v2#876
lognaturel merged 2 commits intogetodk:nextfrom
hotosm:build/cgroupv2

Conversation

@spwoodcock
Copy link
Contributor

@spwoodcock spwoodcock commented Jan 16, 2025 •
edited
Loading

Closes #875

Included in this PR

  • The original check for cgroups v1.
  • A check for cgroups v2.
    • If there are memory constraints set, then they are used.
    • If there are no memory constraints, the value will be set to max and we fallback to use the total memory available on the system.
  • Note that it's possible to determine hard and soft memory limits via cgroups, but this is probably overkill.

What has been done to verify that this works as intended?

  • Tested on my local Debian Bookworm setup (using WSL, so it's cgroups v1).
  • Tested on a Debian Bookworm remote server, using cgroups v2.

Why is this the best possible solution? Were any other approaches considered?

  • It seems to be a pretty common way to determine memory available inside a container.
  • Containers are essentially wrappers for standard tools such as cgroups, so the total memory available in the container is determined by a cgroup.
  • It doesn't require any additional tools / only uses standard Linux stuff.
  • This is backwards compatible with cgroups v1 and should work for the foreseeable future (unlikely cgroups will change behaviour for a long time now).

How does this change affect users? Describe intentional changes to behavior and behavior that could have accidentally been affected by code changes. In other words, what are the regression risks?

  • Users that deployed since the commit hotosm@7476a91, and using cgroups v2 in their deployment infrastructure, may have an incorrectly determined number of workers!
  • Likely they may be running with a single worker.
  • This would increase the number of workers, depending on the memory available.

Does this change require updates to documentation? If so, please file an issue here and include the link below.

  • Probably not.

Before submitting this PR, please make sure you have:

  • branched off and targeted the next branch OR only changed documentation/infrastructure (master is stable and used in production)
  • verified that any code or assets from external sources are properly credited in comments or that everything is internally sourced

@spwoodcock spwoodcock mentioned this pull request Jan 16, 2025
Closed
@lognaturel lognaturel changed the base branch from master to next January 16, 2025 22:32
lognaturel
lognaturel requested changes Jan 16, 2025
View reviewed changes
Copy link
Member

@lognaturel lognaturel left a comment

Choose a reason for hiding this comment

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

Thanks! This looks good to me but tests are failing in CircleCI. Is it possible that you branched off of a very old version? Could you please rebase on the latest next so we can get a CircleCI sanity check before merging?

@spwoodcock
Copy link
Contributor Author

spwoodcock commented Jan 16, 2025

Thanks! This looks good to me but tests are failing in CircleCI. Is it possible that you branched off of a very old version? Could you please rebase on the latest next so we can get a CircleCI sanity check before merging?

Ah possibly! I'm afk for the weekend now, but will address this on Monday 😄

@spwoodcock
Copy link
Contributor Author

spwoodcock commented Jan 20, 2025

You were right, I had synced master but not next on our fork - rebased to the latest next branch!

Hopefully the CircleCI build can be re-triggered without much hassle 🙏

@spwoodcock
Copy link
Contributor Author

spwoodcock commented Jan 20, 2025

Made a small fix for shellcheck linting:

In files/service/scripts/start-odk.sh line 76:
export WORKER_COUNT=$(determine_worker_count "$MEMTOT")
       ^----------^ SC2155: Declare and assign separately to avoid masking return values.

For more information:
  https://www.shellcheck.net/wiki/SC2155 -- Declare and assign separately to ...

@spwoodcock spwoodcock requested a review from lognaturel January 20, 2025 13:20
@lognaturel lognaturel merged commit a0f1bdc into getodk:next Jan 21, 2025
1 check passed
@lognaturel
Copy link
Member

lognaturel commented Jan 21, 2025

Thank you!

@spwoodcock spwoodcock deleted the build/cgroupv2 branch January 22, 2025 08:02
@matthew-white matthew-white mentioned this pull request Jan 23, 2025
Closed
yanokwa pushed a commit to yanokwa/odk-central that referenced this pull request Jun 11, 2025
@spwoodcock @yanokwa
Update start-odk.sh memory calculation to support cgroups v2 (getodk#876
a3ee866 
)
@alxndrsn alxndrsn mentioned this pull request Jan 13, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@lognaturel lognaturel lognaturel approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

start-odk.sh memory calculation does not support cgroups v2

2 participants

@spwoodcock @lognaturel