reimplant PG* env vars for cronjobs, fixes #1747#1756
Conversation
7760611 to
38c76db
Compare
|
|
||
| open my $fh, '<:raw', $envblockfile | ||
| or die "Cannot open '" . $envblockfile . "': $!"; | ||
| my $content = <$fh>; |
There was a problem hiding this comment.
I believe this could miss variables if there's a newline somewhere in an env variable.
There was a problem hiding this comment.
Add local $/; to get the whole file says the internet
| } | ||
| } | ||
|
|
||
| exec @ARGV[1..$#ARGV]; |
There was a problem hiding this comment.
Maybe add an or die so errors are easier to track down?
There was a problem hiding this comment.
exec already complains informatively when, for instance, the specified program can't be found
There was a problem hiding this comment.
From perldoc -f exec:
The "exec" function executes a system command and never
returns; use "system" instead of "exec" if you want it to
return. It fails and returns false only if the command does not
exist and it is executed directly instead of via your system's
command shell (see below).
My understanding is that errors from the invoked command are propagated which is good. In the case of a missing command, though, without doing anything with $!, the script exit code is 0. I still think we should explicitly die in that case though maybe it's not terribly likely.
There was a problem hiding this comment.
Yes we should. So odd, it prints the failure:
Can't exec "tuttifrutti": No such file or directory at with-pgenvblock.pl line 24.
but then indeed exits 0.
Please see #1766
Closes #1747
What has been done to verify that this works as intended?
Automated test: Added a basic test (of the env setting utility itself).
Ad-hoc manual test: Created a cronjob that dumps its env to a file, ran the service container with this here code, saw the file appear with PG* variables inside, rejoiced
But there's no automated end-to-end-test (yet?) asserting that the cronjobs will now
work again and will keep working (specifically, where they failed previously because of absent
PG*env vars).Why is this the best possible solution? Were any other approaches considered?
Discussed on Slack and in a meet.
I opted for using the env representation from
/proc, as it's null-separated, which makes parsing easier!Why? Because IIUC the ASCII null is the only char guaranteed to not appear in varnames or varvalues. From
man 7 environ:Thus IIUC
/proc/$PID/environis pretty close to, or is in fact, the canonical representation of an env. So that's what we'll use then, rather than some intermediary format (such as the output of theenvutility).And I opted against writing the vars out again in some intermediary text-based representation (eg newline-separated
EXPORT bla="boop beep"lines) fit for reading throughsource, as then everything will need to be quoted and escaped, and even if you get that right, it'd be hard to review and test.I think the winning move is to just not to use an intermediary format 😆
And thus there is a wrapper* script for the cronjobs, and the cronjobs themselves don't have to do anything (such as first sourcing some intermediary shell-format assignment list).
*) not strictly a "wrapper" because it uses
execand thus self-dissolves.Implementation notes:
execso that it self-dissolves into the launched program, file descriptors get inherited so the cronjob's std{out,err} and exitcode are what cron "sees", we're not wrapping or layeringPGprefix, so onlyPG*variables get reinjected in the cronjobs. This is because in the meeting @matthew-white argued convincingly against propagating all env vars to the cronjobs right now, not because they shouldn't be (in fact, streamlining the execution environments of the cronjobs and thenodeserver process makes absolute sense), but more because they've been operating without them (for better or worse) up until now and we don't want to have to check whether they won't be upset by them so close to the release.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?
N/A
Does this change require updates to documentation? If so, please file an issue here and include the link below.
N/A
Before submitting this PR, please make sure you have:
nextbranch OR only changed documentation/infrastructure (masteris stable and used in production)