hubcio commented on code in PR #3517:
URL: https://github.com/apache/iggy/pull/3517#discussion_r3467787941
##########
core/server_common/src/executor.rs:
##########
@@ -20,6 +20,42 @@ use compio::runtime::Runtime;
const DEFAULT_SHARD_RUNTIME_CAPACITY: u32 = 4096;
const SHARD_RUNTIME_CAPACITY_ENV: &str = "IGGY_SHARD_RUNTIME_CAPACITY";
+// Minimum kernel required by IORING_SETUP_COOP_TASKRUN + full io_uring
feature set.
+const MIN_KERNEL_MAJOR: u32 = 6;
+const MIN_KERNEL_MINOR: u32 = 8;
+
+/// Reads the running kernel version from `/proc/sys/kernel/osrelease` and
returns
+/// `Err` with a human-readable message if the kernel is older than 6.8.
+///
+/// Call this once at process startup, before `create_shard_executor`. Both the
+/// classic server and server-ng entry points do this; tests skip it unless
they
+/// intentionally exercise the check.
+pub fn check_kernel_version() -> Result<(), String> {
+ let raw = std::fs::read_to_string("/proc/sys/kernel/osrelease")
Review Comment:
this reads `/proc/sys/kernel/osrelease` unconditionally, but that path
doesn't exist on macos. read_to_string returns Err, `check_kernel_version`
returns Err, and both `iggy-server` (core/server/src/main.rs) and
`iggy-server-ng` (core/server-ng/src/main.rs) then eprintln and
`std::process::exit(1)` at the top of main. so the server can't boot on macos
at all - `cargo run --bin iggy-server` on apple silicon exits immediately. and
macos arm64 is a real runtime target here: see the `target_os = "macos",
target_arch = "aarch64"` cfg in `create_shard_executor` right below. gate the
call (or the whole fn) behind `#[cfg(target_os = "linux")]`.
##########
core/server_common/src/executor.rs:
##########
@@ -37,16 +73,19 @@ fn shard_capacity_from_env() -> u32 {
/// harness sets to `256` so N nodes * M shards fit under an 8 MiB
/// `RLIMIT_MEMLOCK` budget without `ENOMEM` at ring setup.
///
-/// # Errors
+/// `keep_worker_pool` must be `true` when TCP, HTTP, or WebSocket transports
are
+/// active: those transports dispatch blocking ops through the asyncify thread
pool
+/// and a zero-worker pool panics with "thread pool is needed but no worker
thread
+/// is running". Pass `false` only for pure QUIC-only deployments where every
op
+/// stays on the ring.
///
-/// Returns an `std::io::Error` if the underlying `io_uring` proactor cannot
be initialised.
-/// On `InvalidInput` the kernel rejected the required flags; on `OutOfMemory`
or
-/// `PermissionDenied` the caller should print the appropriate diagnostic
before panicking.
+/// # Errors
///
-/// Shard executors require `IORING_SETUP_COOP_TASKRUN` for predictable
latency.
-/// Falling back to default flags would silently degrade shard performance -
-/// do not add a retry with reduced flags here.
-pub fn create_shard_executor() -> Result<Runtime, std::io::Error> {
+/// Returns an `std::io::Error` if the underlying `io_uring` proactor cannot be
+/// initialised. On `InvalidInput` the kernel rejected the required flags; on
+/// `OutOfMemory` or `PermissionDenied` the caller should print the appropriate
+/// diagnostic before panicking.
+pub fn create_shard_executor(keep_worker_pool: bool) -> Result<Runtime,
std::io::Error> {
Review Comment:
`keep_worker_pool` is only read inside the `#[cfg(not(all(target_os =
"macos", target_arch = "aarch64")))]` block below, so on a macos aarch64 build
it's unused and trips `unused_variables`, which fails `cargo clippy -- -D
warnings` (the mandated check) on a mac. prefix it `_keep_worker_pool` (no
warning even where it is used) or `#[allow(unused_variables)]` the fn for that
target.
##########
core/server_common/src/executor.rs:
##########
@@ -20,6 +20,42 @@ use compio::runtime::Runtime;
const DEFAULT_SHARD_RUNTIME_CAPACITY: u32 = 4096;
const SHARD_RUNTIME_CAPACITY_ENV: &str = "IGGY_SHARD_RUNTIME_CAPACITY";
+// Minimum kernel required by IORING_SETUP_COOP_TASKRUN + full io_uring
feature set.
Review Comment:
this is wrong, and it contradicts the same crate.
`IORING_SETUP_COOP_TASKRUN` and `IORING_SETUP_TASKRUN_FLAG` - the only two
setup flags iggy requests via `.coop_taskrun(true).taskrun_flag(true)` - both
landed in kernel 5.19, not 6.8. compio defaults `single_issuer`/`defer_taskrun`
to false and we never enable them, so their 6.0/6.1 floors don't apply either.
`diagnostics.rs` in this crate already encodes exactly this: `MIN_KERNEL_MAJOR
= 5`, `MIN_KERNEL_MINOR = 19` for these same two flags, with its own
`parse_kernel_version` helper, and `print_invalid_io_uring_args_info` already
prints '>= 5.19' to the operator on EINVAL. so this new const reinvents that
parser and disagrees with it by a full release. if 6.8 is a deliberate
support/maturity floor, say so and reconcile the two constants - otherwise it
should be 5.19.
##########
core/server_common/src/executor.rs:
##########
@@ -127,4 +177,44 @@ mod tests {
assert_eq!(shard_capacity_from_env(),
DEFAULT_SHARD_RUNTIME_CAPACITY);
});
}
+
+ #[test]
+ fn check_kernel_version_accepts_current_kernel() {
+ // This test runs on the CI host; if the host is >= 6.8 the check
passes.
+ // If CI runs on an older kernel the test is skipped rather than
failed,
+ // because the check is a hard requirement for production, not for CI
hosts.
+ let raw = std::fs::read_to_string("/proc/sys/kernel/osrelease");
+ if let Ok(raw) = raw {
+ let ver = raw.trim();
+ let mut p = ver.splitn(3, '.');
+ let major: u32 = p.next().and_then(|s|
s.parse().ok()).unwrap_or(0);
+ let minor: u32 = p
+ .next()
+ .and_then(|s| s.split(|c: char| !c.is_ascii_digit()).next())
+ .and_then(|s| s.parse().ok())
+ .unwrap_or(0);
+ if major > 6 || (major == 6 && minor >= 8) {
+ assert!(check_kernel_version().is_ok(), "kernel {ver} should
pass");
+ }
+ }
+ }
+
+ #[test]
Review Comment:
this test never calls `check_kernel_version` - it re-inlines the parse logic
on `"5.15.0-58-generic"` and asserts on the inlined copy (the comment even says
so), so it'd stay green if the real function broke. and
`check_kernel_version_accepts_current_kernel` only asserts inside `if let
Ok(raw)` and only when the host is >= 6.8, so it's vacuous on macos or older
hosts. pull the comparison into a pure `fn kernel_meets_min(release: &str) ->
bool`, call it from the real fn, and test that directly with old/new strings -
no /proc needed.
##########
core/server_common/src/executor.rs:
##########
@@ -37,16 +73,19 @@ fn shard_capacity_from_env() -> u32 {
/// harness sets to `256` so N nodes * M shards fit under an 8 MiB
/// `RLIMIT_MEMLOCK` budget without `ENOMEM` at ring setup.
///
-/// # Errors
+/// `keep_worker_pool` must be `true` when TCP, HTTP, or WebSocket transports
are
+/// active: those transports dispatch blocking ops through the asyncify thread
pool
+/// and a zero-worker pool panics with "thread pool is needed but no worker
thread
+/// is running". Pass `false` only for pure QUIC-only deployments where every
op
Review Comment:
worth making this invariant explicit, because it's load-bearing and
unguarded. dropping the pool for quic-only is safe today only because nothing
on the quic server path reaches compio's asyncify pool: fs writes use io_uring
opcodes (fsync maps to the `Fsync` op), and the server doesn't do dns
resolution (`resolve_sock_addrs`) or call `compio::fs::set_permissions` - the
paths that go through `spawn_blocking`. nothing enforces that. the moment
someone adds a `spawn_blocking` caller, or an fs opcode isn't supported on the
running kernel and falls back to blocking, a quic-only server silently regains
the 'thread pool is needed but no worker thread is running' panic. either
document the invariant here or just keep the pool unconditionally - the saved
threads aren't worth the footgun.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
