flatcar-install: Wait for subproccesses and report errors#99
Merged
pothos merged 2 commits intoflatcar-masterfrom Jun 26, 2023
Merged
flatcar-install: Wait for subproccesses and report errors#99pothos merged 2 commits intoflatcar-masterfrom
pothos merged 2 commits intoflatcar-masterfrom
Conversation
pothos
commented
Jun 23, 2023
adc7efe to
dec258a
Compare
krnowak
reviewed
Jun 23, 2023
| @@ -702,9 +706,15 @@ function install_from_url() { | |||
| [ ${EEND[2]} -ne 0 ] && echo "${EEND[2]}: GPG signature verification failed for ${IMAGE_NAME}" >&2 | |||
| exit 1 | |||
| fi 3> >(write_to_disk) | |||
Member
There was a problem hiding this comment.
While at it, you probably could fix this hardcoding of file descriptor to 3. It would be better for bash to allocate an unused fd for it with something like this:
local pipefd
exec {pipefd}<> <(:)
…
| tee >(${BZIP_UTIL} -cd >&${pipefd}) \
…
fi ${pipefd}> >(write_to_disk)
Member
Author
There was a problem hiding this comment.
Sounds good, maybe in a separate PR? From what I know the pipe read will behave differently when the pipe was already closed, not sure if this is the case here when using the stdout of : because it terminated already. It could be that something like sleep infinity is better and doesn't lead to reporting EOF early but block as wanted.
Member
|
As I understand it - it replaces #97? |
Member
Author
In its current form yes, but I would hope we can leave it open when it instead tackles the other two trap issues with the subshell trick: #97 (review) |
c272213 to
3c92619
Compare
While bash allows the function brackes to be missing, shellcheck doesn't. Add brackets to demark the function body.
The redirection of FD 3 to the input FD with >(write_to_disk) causes the function to not wait or react on errors from write_to_disk. Before returning, wait for all subprocesses to finsh and then check the success through the out-of-band FIFO. Since we don't need the waiting with a large timeout anymore we can change the wait function to fulfill a new purpose.
3c92619 to
c1fed40
Compare
krnowak
approved these changes
Jun 26, 2023
pothos
added a commit
to flatcar/scripts
that referenced
this pull request
Jun 26, 2023
This pulls in flatcar/init#97 and flatcar/init#99 to work around a bash regression and add handling for disk write errors.
2 tasks
pothos
added a commit
to flatcar/scripts
that referenced
this pull request
Jun 27, 2023
This pulls in flatcar/init#97 and flatcar/init#99 to work around a bash regression and add handling for disk write errors.
pothos
added a commit
to flatcar/scripts
that referenced
this pull request
Jun 27, 2023
This pulls in flatcar/init#97 and flatcar/init#99 to work around a bash regression and add handling for disk write errors.
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
2 participants
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.
flatcar-install: Wait for subproccesses and report errors
The redirection of FD 3 to the input FD with >(write_to_disk) causes
the function to not wait or react on errors from write_to_disk.
Before returning, wait for all subprocesses to finsh and then check the
success through the out-of-band FIFO. Since we don't need the waiting
with a large timeout anymore we can change the wait function to fulfill
a new purpose.
flatcar-install: Function syntax fixes for shellcheck
While bash allows the function brackes to be missing, shellcheck
doesn't.
Add brackets to demark the function body.
How to use
Testing done
Then
scp -r -C -P 2222 -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null bin/flatcar-install core@127.0.0.1:andssh -o StrictHostKeyChecking=no -o UserKnownHostsFile=/dev/null -p 2222 core@127.0.0.1and in the VM:This worked, then I inserted a
falseinwrite_to_disk(after the seconddd) and reransudo ./flatcar-install -d /dev/vdb.It failed with "write_to_disk: Failed writing image to disk" as expected.
Before this change, it was reporting success even though
write_to_diskfailed (if the bash issue is absent which causes a timeout of 2 hours due to the trap not working which is another issue) .Related to flatcar/Flatcar#1059