Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid stdout on all provisioners (except main) #2174

Merged
merged 5 commits into from Jun 4, 2020

Conversation

msaggiorato
Copy link
Member

@msaggiorato msaggiorato commented Jun 2, 2020

This is an attempt to fix #2173 and also clarify a bit of the process to pipe data to a file.

While researching this, i noticed there's a bug in the current implementation which is kind of a pain in the ass to fix, but was not introduced by my refactor.

This basically is a concurrency issue with tee. The way it currently works is that each output (stdout and stderr) are piped to FIFO queues, which is later fed to tee. The script only waits for the queue to be written, but not until tee finishes writing (appending actually) to the file. This is the reason that in some cases the log lines might be out of order, or in a weird situation. Link to an explanation of this here.

I've tried addressing this, but didn't have time to fully fix this, and I think this may be a bit out of the scope of the issue i'm trying to fix (yet it's an issue with the tee piping technique implemented)

After research, the code before my refactor behaved the same as it did before this PR. This PR effectively behaves like @tomjn suggested it should.

@update-docs
Copy link

update-docs bot commented Jun 2, 2020

Thanks for opening this pull request! Make sure CHANGELOG.md gets updated with this change, additionally any docs that need updated can be found at https://github.com/Varying-Vagrant-Vagrants/varyingvagrantvagrants.org

GitHub
The VVV docs and website. Contribute to Varying-Vagrant-Vagrants/varyingvagrantvagrants.org development by creating an account on GitHub.

@tomjn
Copy link
Member

tomjn commented Jun 3, 2020

I'll test this later today, do you have any links for research on the tee issue?

@msaggiorato
Copy link
Member Author

msaggiorato commented Jun 3, 2020

Added link to the PR description.

@Mte90 Mte90 changed the title Avoid stdout on all provisioners (execpt main) Avoid stdout on all provisioners (except main) Jun 3, 2020
@Mte90 Mte90 requested review from tomjn and Mte90 Jun 3, 2020
Mte90
Mte90 approved these changes Jun 3, 2020
Copy link
Member

@Mte90 Mte90 left a comment

FOr me it is working (tested)

@tomjn tomjn merged commit 040f2c4 into Varying-Vagrant-Vagrants:develop Jun 4, 2020
2 checks passed
@msaggiorato msaggiorato deleted the fix/logging-stdout branch Jun 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants