Merge ~raharper/curtin:fix/curtin-pack-os-sync into curtin:master

[Ryan Harper]

helpers/shell-archive: drop use of tar --sparse flag

Do not use tar [-S, --sparse] flag: at least one filesystem (btrfs)
requires sync/fsync to ensure proper detection of sparse files.
curtin doesn't contain any sparse files so this does not affect payload
size and this works around bug 1757565.

[Ryan Harper (raharper)]

We use the sync binary as os.sync() in python3.3 and newer will flush all buffers, where as sync --file-system <path> will sync only the filesystem that is holding the path; this should reduce the impact on a multi-user system.

Additionally, os.sync() is not available in python2.7

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:4b635ed84050baf2c1d88a30703b8003d315e3b7
https://jenkins.ubuntu.com/server/job/curtin-ci/873/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/873
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/873
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/873
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/873

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/873/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

Your fix here can only really be the same as 'sleep 1'.

We're not going around the VFS cache in any way here.

A write operation with file close and then a subsequent open and read operation is guaranteed that the read operation will get all the data written.

The operations don't have to make it to a backing disk to guarantee that.
A sync just guarantees that the data hits the disk.

I don't doubt that this "fixes" the issue, as I saw the same wierd behavior.

But I don't understand how the issue we're working around is not a severe kernel bug.

Essentially all we're doing is:

   cp file1 /tmp/file1
   tar -cvzf out.tar.gz /tmp/file1

Am I missing something here?

review: Needs Information

[Ryan Harper (raharper)]

On Tue, Mar 20, 2018 at 9:10 PM, Scott Moser <email address hidden> wrote:
> Review: Needs Information
>
> Your fix here can only really be the same as 'sleep 1'.
>
> We're not going around the VFS cache in any way here.
>
> A write operation with file close and then a subsequent open and read operation is guaranteed that the read operation will get all the data written.
>
> The operations don't have to make it to a backing disk to guarantee that.
> A sync just guarantees that the data hits the disk.
>
> I don't doubt that this "fixes" the issue, as I saw the same wierd behavior.
>
> But I don't understand how the issue we're working around is not a severe kernel bug.
>
> Essentially all we're doing is:
>
> cp file1 /tmp/file1
> tar -cvzf out.tar.gz /tmp/file1
>
> Am I missing something here?

Possibly a python bug then; I suspect if we remove use of shutil and
use cp; it will also just work.

I'll test that today.

> --
> https://code.launchpad.net/~raharper/curtin/+git/curtin/+merge/341780
> You are the owner of ~raharper/curtin:fix/curtin-pack-os-sync.

[Ryan Harper (raharper)]

Updated to drop use the tar -S (--sparse) flag, some filesystems, break sparse file detection heuristics (btrfs is the only known case); however curtin pack isn't archiving any sparse files anyhow so it's not needed and won't impact the size of the archive.

[Server Team CI bot (server-team-bot)]

PASSED: Continuous integration, rev:2dea778b4ba6a3c248a0c38657f3eea7073eead7
https://jenkins.ubuntu.com/server/job/curtin-ci/879/
Executed test runs:
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-amd64/879
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-arm64/879
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-ppc64el/879
    SUCCESS: https://jenkins.ubuntu.com/server/job/curtin-ci/nodes=metal-s390x/879

Click here to trigger a rebuild:
https://jenkins.ubuntu.com/server/job/curtin-ci/879/rebuild

review: Approve (continuous-integration)

[Scott Moser (smoser)]

I approve.
I dont *need* the comment in the code, but its fine.

Basically I think its fairly unlikely that anyone would ever think "we need --sparse on that tar!".

But this is fine.

Thanks!

[Scott Moser (smoser)]

review: Approve

[Scott Moser (smoser)]

An upstream commit landed for this bug.

To view that commit see the following URL:
https://git.launchpad.net/curtin/commit/?id=bd40234f

diff --git a/helpers/shell-archive b/helpers/shell-archive
index 3062e2a..5c6c1fd 100755
--- a/helpers/shell-archive
+++ b/helpers/shell-archive
@@ -109,7 +109,8 @@ dump_bin() {
 
 extract() {
     mkdir "$2" || { error "failed to make '$2'"; return 1; }
-    dump_bin "$1" | tar -Sxzf - -C "$2"
+    # Do not use tar [-S, --sparse] flag, see LP: #1757565
+    dump_bin "$1" | tar -xzf - -C "$2"
 }
 
 main() {
@@ -253,7 +254,8 @@ main() {
     trap cleanup EXIT
 
     local payload="${TEMP_D}/payload.tar.gz" md5=""
-    tar -C "$archive_d" -Sczf "${payload}" . ||
+    # Do not use tar [-S, --sparse] flag, see LP: #1757565
+    tar -C "$archive_d" -czf "${payload}" . ||
         { error "failed to create archive from '${archive_d}'"; return 1; }
 
     md5=$(md5sum < "$payload") ||