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

Fix broken pipe error on docker layer creation #101403

Merged
merged 1 commit into from Nov 4, 2020
Merged

Conversation

@mickours
Copy link
Contributor

@mickours mickours commented Oct 22, 2020

This reverts commit f12346d.

This commit was an optimization which was using pipes to read the layer tarball.
It was causing a 141 exit code (means SIGPIPE for tar) while running Layer packing in the VM lauched for the runAsRoot command.

Packing layer...
[    3.755619] reboot: Power down
builder for '/nix/store/nzc2cpi2xh4whzq5h8dg7mgb5zns7993-docker-layer-mycontainer.drv' failed with exit code 141

This error happen consistently on multiple servers running Ubuntu 18.04 servers with Nix 20.09.

I've tested the patch and the error disappeared on all servers.

Motivation for this change

Fix this error by removing the optimization, so it won't happen again.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.
@utdemir
Copy link
Member

@utdemir utdemir commented Oct 22, 2020

Could you share a minimal example? Another way to solve it would be to figure out why we're getting SIGPIPE, and fix that issue instead.

Likely a command fails and the pipe is closed prematurely.

@pedrovelho
Copy link

@pedrovelho pedrovelho commented Oct 23, 2020

Could you share a minimal example? Another way to solve it would be to figure out why we're getting SIGPIPE, and fix that issue instead.

Likely a command fails and the pipe is closed prematurely.

I agree but SIGPIPE is rather a major security breech as is and should not be taken lightly : https://vigilance.fr/vulnerability/QEMU-denial-of-service-via-NBD-SIGPIPE-Signal-23103

@roberth
Copy link
Member

@roberth roberth commented Oct 23, 2020

a major security breech

SIGPIPE is merely a normal way for unix systems to communicate that the process behind a pipe is gone. It is not security problem in and of itself. The issue you've linked is a flaw in QEMU, which is a distinct program that is not involved in this PR. Other than that, the Nix sandbox should not be used for running untrustworthy code because it has not been audited (as far as I know).

@mickours
Copy link
Contributor Author

@mickours mickours commented Oct 28, 2020

Could you share a minimal example? Another way to solve it would be to figure out why we're getting SIGPIPE, and fix that issue instead.

Likely a command fails and the pipe is closed prematurely.

In fact both commands are working as expected because when run sequentially they finished properly.
The issue is in the timing here. It seems that on my servers, tarsum finishes before tar and thus broke the pipe. This is probably due to a delay while tar is syncing the file on the file system (which is a VM and might be slow...).

An interesting post which explains that well: https://stackoverflow.com/a/38549483/2165830

Maybe a better solution would be to ignore the broken pipe using the tee option -p as mentioned in that post.
I'll try that and change the PR if it works.

@mickours mickours changed the title Revert "dockerTools: Calculate tarsum's on the fly" Fix broken pipe error on docker layer creation Oct 30, 2020
@mickours
Copy link
Contributor Author

@mickours mickours commented Oct 30, 2020

Here is the new patch which only add -p option to the tee command (no revert). I've tested it on my servers and works as expected. No more exit 141 errors.

@roberth
Copy link
Member

@roberth roberth commented Oct 30, 2020

@utdemir looks like we need another approval before this can be merged.

Copy link
Contributor

@jonringer jonringer left a comment

please target release-20.09, nixos-20.09 is a protected branch

To comply with CONTRIBUTING.md please have the commit message name be of the format

<pkg-name>: <subject-line>

for more examples, please look at https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md#submitting-changes

in your case, the commit message should be:

docker: Fix broken pipe error on layer creation

@jonringer
Copy link
Contributor

@jonringer jonringer commented Nov 2, 2020

When backporting changes, please follow https://github.com/NixOS/nixpkgs/blob/master/.github/CONTRIBUTING.md#backporting-changes.

Namely, you should be doing git cherry-pick -x <rev> from a commit that has already landed in master. If the branches have diverged, you may alter the commit or add another commit to ensure that the package is able to still evaluate and build

@mickours mickours changed the base branch from nixos-20.09 to master Nov 3, 2020
@mickours
Copy link
Contributor Author

@mickours mickours commented Nov 3, 2020

Sorry @jonringer I should have read the Contributing Guide before, thanks for the advice.

I've change the commit to what you've suggested and rebase my contribution on master. I'll make another PR for the backport into 20.09.

@roberth
Copy link
Member

@roberth roberth commented Nov 3, 2020

@mickours While figuring out what's going on with this PR, I noticed your commit does not have an author e-mail address that matches your GitHub account. You might want to fix that, either in https://github.com/settings/emails or in your git config and then git commit --amend --reset-author.

Other than that, it looks good, but to the person who will merge this, let's alter the branch name in the merge commit message to something sensible.

Add `-p` to the `tee` command to avoid exiting on breaking pipe due to
tarsum finishing before tar which creating docker layers.
@mickours
Copy link
Contributor Author

@mickours mickours commented Nov 3, 2020

@mickours While figuring out what's going on with this PR, I noticed your commit does not have an author e-mail address that matches your GitHub account. You might want to fix that, either in https://github.com/settings/emails or in your git config and then git commit --amend --reset-author.

Other than that, it looks good, but to the person who will merge this, let's alter the branch name in the merge commit message to something sensible.

Done!

@mickours mickours requested a review from jonringer Nov 3, 2020
@roberth
Copy link
Member

@roberth roberth commented Nov 4, 2020

Thank you!

@roberth roberth merged commit 06df746 into NixOS:master Nov 4, 2020
18 checks passed
18 checks passed
@github-actions[bot]
tests
Details
@github-actions[bot]
action
Details
@ofborg[bot]
Evaluation Performance Report Evaluator Performance Report
Details
@github-actions[bot]
Wait for ofborg
Details
@ofborg[bot]
docker, docker.passthru.tests on aarch64-linux Success
Details
@ofborg[bot]
docker, docker.passthru.tests on x86_64-linux Success
Details
@ofborg[bot]
grahamcofborg-eval ^.^!
Details
@ofborg[bot]
grahamcofborg-eval-check-meta config.nix: checkMeta = true
Details
@ofborg[bot]
grahamcofborg-eval-darwin nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="24c5059"; rev="24c5059468026eb8df4c77e98192567378656bc3"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-lib-tests nix-build --arg pkgs import ./. {} ./lib/tests/release.nix
Details
@ofborg[bot]
grahamcofborg-eval-nixos nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="24c5059"; rev="24c5059468026eb8df4c77e98192567378656bc3"; } ./nixos/
Details
@ofborg[bot]
grahamcofborg-eval-nixos-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="24c5059"; rev="24c5059468026eb8df4c77e98192567378656bc3"; } ./nixos/
Details
@ofborg[bot]
grahamcofborg-eval-nixos-options nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="24c5059"; rev="24c5059468026eb8df4c77e98192567378656bc3"; } ./nixos/
Details
@ofborg[bot]
grahamcofborg-eval-nixpkgs-manual nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="24c5059"; rev="24c5059468026eb8df4c77e98192567378656bc3"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-nixpkgs-tarball nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="24c5059"; rev="24c5059468026eb8df4c77e98192567378656bc3"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-nixpkgs-unstable-jobset nix-instantiate --arg nixpkgs { outPath=./.; revCount=999999; shortRev="24c5059"; rev="24c5059468026eb8df4c77e98192567378656bc3"; } ./pkgs/t
Details
@ofborg[bot]
grahamcofborg-eval-package-list nix-env -qa --json --file .
Details
@ofborg[bot]
grahamcofborg-eval-package-list-no-aliases nix-env -qa --json --file . --arg config { allowAliases = false; }
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants