Repository navigation
[Backport release-1.7] fix(seaweedfs): build SeaweedFS in-tree with a bounded S3 read-ahead - #5028
Merged
Merged
Conversation
The S3 gateway is OOM-killed while serving a large object. SeaweedFS starts read-ahead downloads after every read of a sequential stream and counts only the downloads it newly starts toward its prefetch window, so a GET reading 256 KiB at a time out of 4 MiB chunks pulls up to about a gigabyte of the object into memory, past the 512Mi limit of the S3 deployment. No SeaweedFS release carries a fix yet, so there is no release to move to instead. Build weed from the 4.45 sources with a patch that keeps the window at the next prefetchCount chunks, and lay it over the upstream image of the same tag so the rest of the image stays upstream's own. The image build runs the tests of the packages the patch touches, including the regression test the patch adds, because go build never compiles them. Assisted-by: LLM Signed-off-by: Alexey Artamonov <[email protected]> (cherry picked from commit 1913104) Signed-off-by: Myasnikov Daniil <[email protected]>
With the image keys unset the vendored chart renders chrislusf/seaweedfs at its appVersion, so the patched build from images/seaweedfs would be built and never run, and the S3 gateway would keep being OOM-killed on large GETs. Point every component at the in-tree image, pinned by digest like every first-party reference, and hold that in a unit test so a values change cannot quietly fall back to the upstream binary. Assisted-by: LLM Signed-off-by: Alexey Artamonov <[email protected]> (cherry picked from commit d2c6b85) Signed-off-by: Myasnikov Daniil <[email protected]>
The in-tree build pinned its source, base image and toolchain by hand, and nothing moved them when `make update` vendored a new chart, so a bump would build one release's weed into another release's image, or keep patching a release that already carries the fix. Pin the source archive by checksum and the upstream image by digest as build arguments, compile with the toolchain upstream's own builder uses for the tag, and let `make update` re-pin all of them from the vendored chart and fail when the patches no longer apply. The tests of the patched packages now run once rather than once per target platform, and `weed version` names the build. Assisted-by: LLM Signed-off-by: Alexey Artamonov <[email protected]> (cherry picked from commit 5592a3f) Signed-off-by: Myasnikov Daniil <[email protected]>
Renovate would bump the builder, the base image or their digests one at a time into a combination upstream never shipped, and the next `make update` would silently put them back, so it leaves that Dockerfile to the update recipe. The patches are unified diffs of Go sources, whose context lines start with a marker before tabs and blank lines, which git's whitespace checks would otherwise flag or rewrite. Assisted-by: LLM Signed-off-by: Alexey Artamonov <[email protected]> (cherry picked from commit ecdc043) Signed-off-by: Myasnikov Daniil <[email protected]>
The builder, the compile flags and the version stamp changed, so the image built from them has a new digest, and values.yaml has to name it for the components to run that build. Assisted-by: LLM Signed-off-by: Alexey Artamonov <[email protected]> (cherry picked from commit e8635f6) Signed-off-by: Myasnikov Daniil <[email protected]>
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Backport of #4819 to
release-1.7. The five commits are cherry-picked in order from 1913104, d2c6b85, 5592a3f, ecdc043 and e8635f6.One conflict, in
.github/renovate.json: the rule #4819 adds sits next to two e2e sandbox rules from #4867, which is not on this branch. I kept this branch's rules and added only the seaweedfs rule. The diff is the same nine files, +281/-2, as #4819.The bug is the same here. This branch runs the upstream
chrislusf/seaweedfs:4.45image, and its S3 gateway is OOM-killed while serving a large object (#3782).This needs to land before rc.2 is cut, otherwise rc.2 and v1.7.0 ship the upstream image.