Treat missing cgroups files as a quiet fallback, not a stack-traced error - #52
Open
auxten wants to merge 2 commits into
Open
Treat missing cgroups files as a quiet fallback, not a stack-traced error#52auxten wants to merge 2 commits into
auxten wants to merge 2 commits into
Conversation
…rror
In strictly restricted containers (k8s pods with hidden cgroupfs, distroless
images, FaaS sandboxes, ...) neither cgroups v1's `memory/memory.stat` nor
cgroups v2's `memory.current` is accessible. The previous code modeled this
expected condition as a `FILE_DOESNT_EXIST` exception, which ended up dumping
a full C++ stack trace via `tryLogCurrentException` on every embedded server
construction (i.e. essentially every chDB query in short-lived sessions).
The query still completed (Jemalloc was used as the fallback memory source)
but the noise on stderr was alarming, broke log monitoring, and made users
think their queries had failed.
This change reshapes `ICgroupsReader::getCgroupsPath()` to express its
real semantics: it is a probe that may or may not find a usable cgroup
hierarchy. The new `tryGetCgroupsPath()` returns `std::optional`, so:
* Cgroups absent (the expected case in restricted containers): callers
take the empty branch and emit a single `LOG_DEBUG` line.
* Cgroups present but reader construction fails (file vanished,
permissions, etc.): callers still log the full diagnostic via
`tryLogCurrentException`. Truly unexpected errors are preserved.
Both call sites in `MemoryWorker` and `AsynchronousMetrics` are updated
accordingly. The unused `FILE_DOESNT_EXIST` ErrorCodes declaration in
`MemoryWorker.cpp` is removed.
Fixes #51
The "Test chdb DataStore tests against upstream chdb (latest tag)" step
fetches the latest chdb release tag via an unauthenticated curl to
api.github.com, which is subject to a 60 req/hr per-IP rate limit.
On busy CI runners (especially shared GitHub-hosted macOS arm64 boxes)
this regularly returned HTTP 403, the empty body crashed the downstream
`json.load`, and the whole job failed before a single test ran.
This change:
* Sends `Authorization: Bearer $GITHUB_TOKEN` when the variable is set,
raising the rate limit from 60 req/hr to 5000 req/hr.
* Retries the request up to 3 times with backoff to absorb transient
network/rate-limit hiccups.
* Emits a clear actionable error (suggesting `CHDB_TAG=` as a workaround)
when the lookup ultimately fails, instead of an opaque JSONDecodeError.
* Wires `GITHUB_TOKEN: ${{ secrets.GITHUB_TOKEN }}` through to the step
in all four wheel-build workflows so the auth path is exercised in CI.
The script keeps working unauthenticated (e.g. for local runs) when no
GITHUB_TOKEN is set; the retry/auth machinery is purely additive.
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.
Summary
Fixes #51.
In strictly restricted containers (k8s pods with hidden
/sys/fs/cgroup, distroless images, FaaS sandboxes, …) neither cgroups v1memory/memory.statnor cgroups v2memory.currentis accessible. The previous code modeled this expected condition as aFILE_DOESNT_EXISTexception, which ended up dumping a full C++ stack trace viatryLogCurrentExceptionon every embedded server construction — i.e. essentially every chDB query in short-lived sessions:The query still completed (Jemalloc fallback) but the noise on stderr was alarming, broke log monitoring, and made users think their queries had failed.
Approach
Reshape
ICgroupsReader::getCgroupsPath()to express what it really is: a probe that may or may not find a usable cgroup hierarchy. The newtryGetCgroupsPath()returnsstd::optional<std::pair<std::string, CgroupsVersion>>, so callers can distinguish:LOG_DEBUGline. No stack trace, no exception machinery.tryLogCurrentException. Genuinely unexpected errors are preserved.Both call sites in
MemoryWorkerandAsynchronousMetricsare updated. The now-unusedFILE_DOESNT_EXISTErrorCodes declaration inMemoryWorker.cppis removed.Why this shape
Two alternatives were considered:
FILE_DOESNT_EXISTinside the existingcatchand downgrade toLOG_DEBUG. Smallest diff, but still uses exceptions for control flow on a hot path that runs on every embedded session start. Doesn't fix the underlying "this isn't actually an error" mismodeling.tryGetCgroupsPath()non-throwing (this PR). Same diff size at call sites, semantically correct, no exception machinery for the expected case, and existingcreateCgroupsReaderfailures still surface with their full diagnostic.The existing
gtest_cgroups_reader.cpponly exercisescreateCgroupsReader(which is unchanged), so no test surgery is needed.Test plan
gtest_cgroups_reader— should still pass unchanged.LOG_DEBUGfromMemoryWorkershows it picked the cgroups path; behaviour unchanged./sys/fs/cgroupnot mounted (or--security-opt=apparmor=unconfinedstyle hardening that hides it) —stderrshould be clean (no stack trace), andMemoryWorkershould fall back to Jemalloc as before.OS_LINUX-guarded code is excluded, no API breakage.