Multiversion: backport execing and hardcode 0.15.3 as having 0.15.4 available #1935
Conversation
cb22
force-pushed
the
re-release
branch
6 times, most recently
from
70598cc
to
6204906
Compare
|
@sentientwaffle this is ready for review; but leaving as a draft PR since the merging / build process will be... Interesting. |
src/tigerbeetle/main.zig
Outdated
| var binary_path = replica_release_execute_args.?.next().?; | ||
| assert(std.fs.path.isAbsolute(binary_path)); |
There was a problem hiding this comment.
Is the global replica_release_execute_args needed by "real" multiversioning too?
If so, maybe we should just use that instead of std.os.argv for macos/linux too, to avoid some of the branching on .windows at the top-level and in main().
...Then again, which how much type casting is going on, maybe it wouldn't actually simplify anything.
There was a problem hiding this comment.
Is the global replica_release_execute_args needed by "real" multiversioning too?
Nope, there it's handled inside multiversioning.zig.
I've removed the branch for now, at the cost of a little extra memory on Linux / macOS, but since this is a transitional release only I think that's OK!
src/tigerbeetle/main.zig
Outdated
| const cast_envp: [*:null]const ?[*:0]const u8 = @ptrCast(std.os.environ.ptr); | ||
|
|
||
| std.log.info("replica_release_execute: executing '{s}'...\n", .{binary_path}); | ||
| std.os.execveZ(binary_path, cast_args, cast_envp) catch @panic("execveZ failed"); |
There was a problem hiding this comment.
If execveZ fails, the error would be nice to have. Maybe replace the panic with a |err| log.fatal(..., .{err})? (Likewise for windows.)
src/io/windows.zig
Outdated
| @@ -956,7 +956,7 @@ pub const IO = struct { | |||
|
|
|||
| pub const INVALID_FILE = os.windows.INVALID_HANDLE_VALUE; | |||
|
|
|||
| fn open_file_handle(relative_path: []const u8, method: enum { create, open }) !os.fd_t { | |||
| fn open_file_handle(relative_path: []const u8, method: enum { create, open, open_wait }) !os.fd_t { | |||
There was a problem hiding this comment.
Just curious, but why not .deinit() the IO to unlock the data file before starting the new process?
There was a problem hiding this comment.
No reason - that's a great idea! I think I should be able to deinit the replica and storage; afaik it's the port and the data file that we need to ensure are free.
(I was using the file as a proxy for both, here)
cb22
force-pushed
the
re-release
branch
3 times, most recently
from
cd94e07
to
05d2eb0
Compare
…vailable 0.15.4 will be the first version with the ability to read the multiversion metadata embedded in TigerBeetle. This presents a bit of a bootstrapping problem: * Operator replaces 0.15.3 with 0.15.4, * 0.15.4 starts up, re-execs into 0.15.3, * 0.15.3 knows nothing about 0.15.4 or how to check it's available, so we just hang. Work around this by hardcoding that if 0.15.3 is present in a pack, 0.15.4 must be too.
| const cast_args: [*:null]const ?[*:0]const u8 = @ptrCast(std.os.argv.ptr); | ||
| const cast_envp: [*:null]const ?[*:0]const u8 = @ptrCast(std.os.environ.ptr); | ||
|
|
||
| std.log.info("replica_release_execute: executing '{s}'...\n", .{binary_path}); |
There was a problem hiding this comment.
Nit, but you could use log_main for the scoped logger.
Also, log.info() doesn't need an explicit \n.
There was a problem hiding this comment.
Nit, but you could use log_main for the scoped logger.
Ah yes!
Also, log.info() doesn't need an explicit
\n.
Indeed - this is to make the logging output when its exec'ing through multiple versions easier to read - that'll be the last line printed before the new process takes over.
0.15.4 will be the first version with the ability to read the multiversion metadata embedded in TigerBeetle. This presents a bit of a bootstrapping problem:
Work around this by hardcoding that if 0.15.3 is present in a pack, 0.15.4 must be too.
Additionally, fix up setting
CLOEXECon the listening sockets. We were only setting it correctly on the accepted sockets.