New issue
Advanced search Search tips

Issue 846712 link

Starred by 1 user

Issue metadata

Status: Fixed
Owner:
Closed: Jun 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug

Blocking:
issue 842802



Sign in to add a comment

isolate go tar archiver duplicates files

Project Member Reported by mar...@chromium.org, May 25 2018

Issue description

The Go implementation of the isolate archiver occasionally duplicate files in a tarfile (!)

It hasn't been too much of a problem yet because of the compression but this is not really efficient use and this hinders marking the tree as read only.

AI:
- Find the reason for duplication. Some files are even duplicated 3 times.
- Fix it.
- Deploy the fixed binary everywhere.
 
Project Member

Comment 1 by bugdroid1@chromium.org, Jun 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/753b186a947b4f98c78115a78ad1f41d18622572

commit 753b186a947b4f98c78115a78ad1f41d18622572
Author: Marc-Antoine Ruel <maruel@chromium.org>
Date: Fri Jun 01 17:19:21 2018

isolate: Remove ArchiveOptions reference from TarringArchiver

This was a layering error. This is in preparation to move TarringArchiver into
package archiver, where it should have been put initially. Then I'll do bug
fixes.

Remove t.Parallel() from test cases that spew to stdout/stderr. We should silence
these annoying tests unless testing.Verbose() is true but at least make sure
they are not corrupting the test output. :(

Bug:  846712 
Change-Id: Ib0229d1bb12751161e21746009a41a8f035808f4
Reviewed-on: https://chromium-review.googlesource.com/1082376
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Commit-Queue: Marc-Antoine Ruel <maruel@chromium.org>

[modify] https://crrev.com/753b186a947b4f98c78115a78ad1f41d18622572/client/cmd/isolate/archive.go
[modify] https://crrev.com/753b186a947b4f98c78115a78ad1f41d18622572/client/cmd/isolate/batch_archive.go
[modify] https://crrev.com/753b186a947b4f98c78115a78ad1f41d18622572/client/cmd/isolate/batch_archive_test.go
[modify] https://crrev.com/753b186a947b4f98c78115a78ad1f41d18622572/client/cmd/isolate/tarring_archiver.go
[modify] https://crrev.com/753b186a947b4f98c78115a78ad1f41d18622572/client/cmd/isolate/upload_tracker_test.go

Project Member

Comment 2 by bugdroid1@chromium.org, Jun 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/b0580dda2539d1b5b9c4e7886460599f3363c30d

commit b0580dda2539d1b5b9c4e7886460599f3363c30d
Author: Marc-Antoine Ruel <maruel@chromium.org>
Date: Fri Jun 01 19:25:21 2018

archiver: rename Item to PendingItem

There's a conflict with an Item symbol in cmd/isolate. I found it easier to
rename the one in package archiver.

Also remove a few \n that were triggering a presubmit check for good reason.

R=mknyszek@google.com

Bug:  846712 
Change-Id: I22537bff2ead08d13c0766ab1a4293a2d50f639e
Reviewed-on: https://chromium-review.googlesource.com/1082755
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Commit-Queue: Marc-Antoine Ruel <maruel@chromium.org>

[modify] https://crrev.com/b0580dda2539d1b5b9c4e7886460599f3363c30d/client/archiver/archiver.go
[modify] https://crrev.com/b0580dda2539d1b5b9c4e7886460599f3363c30d/client/archiver/archiver_test.go
[modify] https://crrev.com/b0580dda2539d1b5b9c4e7886460599f3363c30d/client/archiver/directory.go
[modify] https://crrev.com/b0580dda2539d1b5b9c4e7886460599f3363c30d/client/isolate/isolate.go
[modify] https://crrev.com/b0580dda2539d1b5b9c4e7886460599f3363c30d/client/isolated/archive.go

Project Member

Comment 3 by bugdroid1@chromium.org, Jun 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/1bd1967d1e5c92f607174f3ba3c25b893344e108

commit 1bd1967d1e5c92f607174f3ba3c25b893344e108
Author: Marc-Antoine Ruel <maruel@chromium.org>
Date: Fri Jun 01 19:39:41 2018

archiver: move tar archiver from cmd/isolate

That's where it should have been in the first place.

Do as little mutation as possible to keep the move reviewable.

That makes package archiver a sad package, with incompatible duplicate
implementations. :/ But it's still better than keeping one of the implementation
in a main package. A follow up unexports a few symbols.

R=mknyszek@google.com

Bug:  846712 
Change-Id: I14f947fd9ea9a87fa8788296a7009fac131c4422
Reviewed-on: https://chromium-review.googlesource.com/1082813
Commit-Queue: Marc-Antoine Ruel <maruel@chromium.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>

[rename] https://crrev.com/1bd1967d1e5c92f607174f3ba3c25b893344e108/client/archiver/checker.go
[rename] https://crrev.com/1bd1967d1e5c92f607174f3ba3c25b893344e108/client/archiver/checker_test.go
[rename] https://crrev.com/1bd1967d1e5c92f607174f3ba3c25b893344e108/client/archiver/partitioning_walker_test.go
[rename] https://crrev.com/1bd1967d1e5c92f607174f3ba3c25b893344e108/client/archiver/tar_archiver.go
[rename] https://crrev.com/1bd1967d1e5c92f607174f3ba3c25b893344e108/client/archiver/tar_archiver_test.go
[rename] https://crrev.com/1bd1967d1e5c92f607174f3ba3c25b893344e108/client/archiver/tarring_archiver.go
[rename] https://crrev.com/1bd1967d1e5c92f607174f3ba3c25b893344e108/client/archiver/upload_tracker.go
[rename] https://crrev.com/1bd1967d1e5c92f607174f3ba3c25b893344e108/client/archiver/upload_tracker_test.go
[rename] https://crrev.com/1bd1967d1e5c92f607174f3ba3c25b893344e108/client/archiver/uploader.go
[rename] https://crrev.com/1bd1967d1e5c92f607174f3ba3c25b893344e108/client/archiver/uploader_test.go
[modify] https://crrev.com/1bd1967d1e5c92f607174f3ba3c25b893344e108/client/cmd/isolate/archive.go
[modify] https://crrev.com/1bd1967d1e5c92f607174f3ba3c25b893344e108/client/cmd/isolate/batch_archive.go

Project Member

Comment 4 by bugdroid1@chromium.org, Jun 1 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/e8e20691754f2fa91feab8a1506db09b7f6b4310

commit e8e20691754f2fa91feab8a1506db09b7f6b4310
Author: Marc-Antoine Ruel <maruel@chromium.org>
Date: Fri Jun 01 19:58:51 2018

archiver: unexport ItemBundle and ShardItems

They didn't need to be exported, making the package more cluttered than
necessary. The package is still a mess but it's less than it used to be. :)

R=mknyszek@google.com

Bug:  846712 
Change-Id: I62182b07a86e96b3caef095c4f22f1e34cd08fab
Reviewed-on: https://chromium-review.googlesource.com/1082714
Commit-Queue: Marc-Antoine Ruel <maruel@chromium.org>
Reviewed-by: Michael Knyszek <mknyszek@google.com>

[modify] https://crrev.com/e8e20691754f2fa91feab8a1506db09b7f6b4310/client/archiver/tar_archiver.go
[modify] https://crrev.com/e8e20691754f2fa91feab8a1506db09b7f6b4310/client/archiver/tar_archiver_test.go
[modify] https://crrev.com/e8e20691754f2fa91feab8a1506db09b7f6b4310/client/archiver/upload_tracker.go

Project Member

Comment 5 by bugdroid1@chromium.org, Jun 2 2018

The following revision refers to this bug:
  https://chromium.googlesource.com/infra/luci/luci-go.git/+/f41ecd629b2a49d7affcf2729726a293fcf70d32

commit f41ecd629b2a49d7affcf2729726a293fcf70d32
Author: Marc-Antoine Ruel <maruel@chromium.org>
Date: Sat Jun 02 00:53:21 2018

archiver: keep a map of seen files to skip redundant ones

During 'batch_archive', the same file may be mapped in multiple input lists
(isolate), resulting the file to be archived multiple time.

Skip duplicates at the directory walk, otherwise this causes a lot of redundant
work and may cause the same file to be stored multiple times in a tar file,
causing problems when unpacking the tarfile.

Bug:  846712 
Change-Id: Ic6ff65b07930cfc6a72e8addf93b17111672b5bc
Reviewed-on: https://chromium-review.googlesource.com/1082732
Reviewed-by: Andrii Shyshkalov <tandrii@chromium.org>
Commit-Queue: Marc-Antoine Ruel <maruel@chromium.org>

[modify] https://crrev.com/f41ecd629b2a49d7affcf2729726a293fcf70d32/client/archiver/partitioning_walker_test.go
[modify] https://crrev.com/f41ecd629b2a49d7affcf2729726a293fcf70d32/client/archiver/tarring_archiver.go

Comment 6 by mar...@chromium.org, Jun 11 2018

Status: Fixed (was: Assigned)

Sign in to add a comment