Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2681693004:
[cipd] ignore package folders missing current link. (Closed)
Description[cipd] ignore package folders missing current link.
For bad packages which include .cipd/pkgs guts, it's possible for empty
folders to pollute the pkgs space with description.json files such that
multiple pkg folders contain 'valid' contents for the same package.
R=vadimsh@chromium.org
BUG=689359
Review-Url: https://codereview.chromium.org/2681693004
Committed: https://github.com/luci/luci-go/commit/c571b954b15cdf9132da0f08c355fdf2a600c2f2
Patch Set 1 #
Total comments: 2
Patch Set 2 : do cleanup instead #Patch Set 3 : more logging #
Total comments: 2
Patch Set 4 : Add tests #Patch Set 5 : skip tests on windows #Patch Set 6 : Make it work on windows #Patch Set 7 : Make it actually work on windows #Patch Set 8 : Add current.txt too #Patch Set 9 : third time is the charm #
Depends on Patchset: Messages
Total messages: 26 (14 generated)
The CQ bit was checked by iannucci@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I'm actually a bit unsure of this change... I'm worried that we might end up accumulating many different useless folders. Is there a safe point in the cipd deployment state machine where I can have it attempt to clean up junk?
On 2017/02/08 09:18:16, iannucci wrote: > I'm actually a bit unsure of this change... I'm worried that we might end up > accumulating many different useless folders. > > Is there a safe point in the cipd deployment state machine where I can have it > attempt to clean up junk? What if I always have it prefer the shortest + lowest (alphanumerically) folder whose description.json matches, and then just delete all the rest? Would that break stuff? And also have it delete any folders which don't have a description.json and also don't have a current link?
On 2017/02/08 09:51:32, iannucci wrote: > On 2017/02/08 09:18:16, iannucci wrote: > > I'm actually a bit unsure of this change... I'm worried that we might end up > > accumulating many different useless folders. > > Hm... We should probably update the deployer to ignore .cipd/* files when extracting a package. It will "fix" historical goma packages (that have nothing important in .cipd/* anyway). It will remove one source of a junk. > > Is there a safe point in the cipd deployment state machine where I can have it > > attempt to clean up junk? > > What if I always have it prefer the shortest + lowest (alphanumerically) folder > whose description.json matches, and then just delete all the rest? Would that > break stuff? > > And also have it delete any folders which don't have a description.json and also > don't have a current link? We can add an installation timestamp in description.json and pick the oldest one? Also, why oldest?.. Deleting directories without _current and description.json seem fine (assuming tmp dirs used during installation are located elsewhere or have a known prefix we can filter on).
https://codereview.chromium.org/2681693004/diff/1/cipd/client/cipd/local/depl... File cipd/client/cipd/local/deployer.go (right): https://codereview.chromium.org/2681693004/diff/1/cipd/client/cipd/local/depl... cipd/client/cipd/local/deployer.go:445: err = fmt.Errorf("invalid package dir, no description.json and no instances found") nit: this error actually looks quite menacing in the cipd client log maybe we can reformulate these warnings somehow to indicate that this is mostly OK, and not a big deal? https://codereview.chromium.org/2681693004/diff/1/cipd/client/cipd/local/depl... cipd/client/cipd/local/deployer.go:453: } else if cur == "" { this will be hit if clients concurrently install a package: 1. Client 1 creates a tempdir, creates description.json, renames it to <number1>. Goes to sleep. 2. Client 2 wakes up, sees there's no _current, skips this directory and creates a new one <number2>. Goes to sleep. 3. Client 1 wakes up, continues installing into <number1>. Eventually creates _current there. 4. Client 2 wakes up and installs stuff into <number2>. As a result we have a mess :(
seems fine... lgtm https://codereview.chromium.org/2681693004/diff/40001/cipd/client/cipd/local/... File cipd/client/cipd/local/deployer.go (right): https://codereview.chromium.org/2681693004/diff/40001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:562: logging.Warningf(ctx, "removing bad directory: %q (%s)", fullPkgPath, err) nit: "removing junk directory" to indicate that it is kind of ok, that shit happens ("bad" is stronger word)
The CQ bit was checked by iannucci@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org Link to the patchset: https://codereview.chromium.org/2681693004/#ps60001 (title: "Add tests")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2681693004/diff/40001/cipd/client/cipd/local/... File cipd/client/cipd/local/deployer.go (right): https://codereview.chromium.org/2681693004/diff/40001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:562: logging.Warningf(ctx, "removing bad directory: %q (%s)", fullPkgPath, err) On 2017/02/09 00:54:53, Vadim Sh. wrote: > nit: "removing junk directory" to indicate that it is kind of ok, that shit > happens ("bad" is stronger word) Done.
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: Luci-go Win Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/3439675722336a10)
The CQ bit was checked by iannucci@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org Link to the patchset: https://codereview.chromium.org/2681693004/#ps80001 (title: "skip tests on windows")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by iannucci@chromium.org
The CQ bit was checked by iannucci@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from vadimsh@chromium.org Link to the patchset: https://codereview.chromium.org/2681693004/#ps160001 (title: "third time is the charm")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 160001, "attempt_start_ts": 1486608773690580,
"parent_rev": "27fda207cc185f378079bfa12f7a6c15f8f84cd4", "commit_rev":
"c571b954b15cdf9132da0f08c355fdf2a600c2f2"}
Message was sent while issue was closed.
Description was changed from ========== [cipd] ignore package folders missing current link. For bad packages which include .cipd/pkgs guts, it's possible for empty folders to pollute the pkgs space with description.json files such that multiple pkg folders contain 'valid' contents for the same package. R=vadimsh@chromium.org BUG=689359 ========== to ========== [cipd] ignore package folders missing current link. For bad packages which include .cipd/pkgs guts, it's possible for empty folders to pollute the pkgs space with description.json files such that multiple pkg folders contain 'valid' contents for the same package. R=vadimsh@chromium.org BUG=689359 Review-Url: https://codereview.chromium.org/2681693004 Committed: https://github.com/luci/luci-go/commit/c571b954b15cdf9132da0f08c355fdf2a600c2f2 ==========
Message was sent while issue was closed.
Committed patchset #9 (id:160001) as https://github.com/luci/luci-go/commit/c571b954b15cdf9132da0f08c355fdf2a600c2f2 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
