Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2640703002:
Change CIPD internal pkgs directory layout to use numeric indices. (Closed)
DescriptionChange CIPD internal pkgs directory layout to use numeric indices.
Also paves the way for enabling multi-root logic for ensure files.
R=vadimsh@chromium.org
BUG=663843
Review-Url: https://codereview.chromium.org/2640703002
Committed: https://github.com/luci/luci-go/commit/a9cef53c4a1d1cdb4e9d4de59741e0b7282a326a
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add tests and fix some issues #
Total comments: 26
Patch Set 3 : Address comments #
Total comments: 8
Patch Set 4 : Fix one test #Patch Set 5 : fix other test #Patch Set 6 : fix json #Patch Set 7 : Add tests for numSet, fix bugs found #
Total comments: 1
Messages
Total messages: 33 (11 generated)
PTAL; need to add more tests, but the existing tests now pass and seem sane.
self-comments https://codereview.chromium.org/2640703002/diff/1/cipd/client/cipd/local/depl... File cipd/client/cipd/local/deployer.go (right): https://codereview.chromium.org/2640703002/diff/1/cipd/client/cipd/local/depl... cipd/client/cipd/local/deployer.go:449: defer os.RemoveAll(tmpDir) We ignore the error here in the case of failure; it'll lurk under the tmp dir if there's a crash. https://codereview.chromium.org/2640703002/diff/1/cipd/client/cipd/local/depl... cipd/client/cipd/local/deployer.go:452: for attempts := 3; attempts > 0; attempts-- { I put a bound on this... does this seem reasonable, or should we let this go more? https://codereview.chromium.org/2640703002/diff/1/cipd/client/cipd/local/depl... cipd/client/cipd/local/deployer.go:453: n := seenNumbers.smallestNewNum() It's important for both clients to always attempt to pick the same numbers. Otherwise we could end up in a scenario where the same package is in two folders (if I pick 100 and you pick 200, we'll never see each others' folders). https://codereview.chromium.org/2640703002/diff/1/cipd/client/cipd/local/depl... cipd/client/cipd/local/deployer.go:469: die(ctx, "unknown error while creating pkg dir %s: %s", pkgPath, err) I don't think this can ever happen; rename is documented to return *LinkError. https://codereview.chromium.org/2640703002/diff/1/cipd/client/cipd/local/depl... cipd/client/cipd/local/deployer.go:542: // folders present. I'm not sure if that's the right thing to do? https://codereview.chromium.org/2640703002/diff/1/cipd/client/cipd/local/depl... cipd/client/cipd/local/deployer.go:587: err = d.fs.Replace(ctx, tmpName, descriptionPath) this should be idempotent. the only way it might not be is if current switches instances and that also changes the package name. I don't think that's possible, however.
(PS2 is ready for review I think)
https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... File cipd/client/cipd/local/deployer.go (right): https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:240: return common.Pin{}, nil return common.Pin{}, fmt.Errorf("package %s is not installed", pkg) (as line 248 does) CheckDeployed doc: // It returns information about installed version (or error if not installed). https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:279: // Attempt to read the manifest. If it is there -> valid package is found. this comment is no longer correct https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:312: logging.Warningf(ctx, "Package %s not found", packageName) the previous implementation had nice property of nuking all garbage no matter what :) https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:407: die(ctx, "can't read packages dir %q: %s", abs, err) I think it's time to convert packagePath(...) to return (string, error) instead of dying. Previously it was dying only because it was mostly doing string math, plus Abs path convertion that errors only under very special circumstances. Now it has lot more places where it fails. It must return the error as usual. https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:434: if _, err := d.fs.EnsureDirectory(ctx, abs); err != nil { thanks, rietveld... https://screenshot.googleplex.com/fae214tRw84.png https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:438: tmpDir, err := d.TempDir(ctx, strings.Replace(pkg, "/", "_", -1)) do common.ValidatePackageName(pkg) before, just as a precaution. (panicing here is fine, this check should not ever fail). (In fact, it's probably better to make this check when function starts, consider it an assert) https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:447: descFile.Close() check this error too. It is possible a file can be opened and written to, but not flushed. https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:451: defer os.RemoveAll(tmpDir) warn on error https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:454: for attempts := 3; attempts > 0; attempts-- { nit: add small random sleep between attempts, so that two clashing clients have a better chance of resolving the race. (Though is practice it probably doesn't matter). https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:545: func (d *deployerImpl) readDescription(ctx context.Context, pkgDir string) (desc *Description, err error) { one thing that concern me is when this function is run over a directory the client has no longer access to e.g. "sudo cipd ensure ...." "cipd installed" The second "cipd installed" should finish successfully, even though it has no access to drop description.json files. I think in current implementation such uncovertable package will just be considered missing, which is not correct. https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:566: logging.Warningf(ctx, "no current instance id") nit: upper case first letter of a logging messages ("No current instance id in %s", pkgDir). https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:590: err = d.fs.Replace(ctx, tmpName, descriptionPath) Use https://github.com/luci/luci-go/blob/master/cipd/client/cipd/local/fs.go#L184 (instead of lines 579-590). https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... File cipd/client/cipd/local/description.go (right): https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/description.go:5: package local consider moving this to https://github.com/luci/luci-go/blob/master/cipd/client/cipd/local/manifest.go (perhaps renaming manifest.go to desc.go or something).
PTAL! https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... File cipd/client/cipd/local/deployer.go (right): https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:240: return common.Pin{}, nil On 2017/01/18 19:01:53, Vadim Sh. wrote: > return common.Pin{}, fmt.Errorf("package %s is not installed", pkg) > > (as line 248 does) > > CheckDeployed doc: > // It returns information about installed version (or error if not installed). Done. https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:279: // Attempt to read the manifest. If it is there -> valid package is found. On 2017/01/18 19:01:53, Vadim Sh. wrote: > this comment is no longer correct Done. https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:312: logging.Warningf(ctx, "Package %s not found", packageName) On 2017/01/18 19:01:53, Vadim Sh. wrote: > the previous implementation had nice property of nuking all garbage no matter > what :) I'm not sure what you mean; this will happen if the provided package name doesn't exist. What should we be deleting in that case? https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:407: die(ctx, "can't read packages dir %q: %s", abs, err) On 2017/01/18 19:01:53, Vadim Sh. wrote: > I think it's time to convert packagePath(...) to return (string, error) instead > of dying. > > Previously it was dying only because it was mostly doing string math, plus Abs > path convertion that errors only under very special circumstances. > > Now it has lot more places where it fails. It must return the error as usual. Done. https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:434: if _, err := d.fs.EnsureDirectory(ctx, abs); err != nil { On 2017/01/18 19:01:53, Vadim Sh. wrote: > thanks, rietveld... https://screenshot.googleplex.com/fae214tRw84.png :D yeah I know. https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:438: tmpDir, err := d.TempDir(ctx, strings.Replace(pkg, "/", "_", -1)) On 2017/01/18 19:01:53, Vadim Sh. wrote: > do common.ValidatePackageName(pkg) before, just as a precaution. (panicing here > is fine, this check should not ever fail). > > (In fact, it's probably better to make this check when function starts, consider > it an assert) Done. https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:447: descFile.Close() On 2017/01/18 19:01:53, Vadim Sh. wrote: > check this error too. It is possible a file can be opened and written to, but > not flushed. Done. https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:451: defer os.RemoveAll(tmpDir) On 2017/01/18 19:01:53, Vadim Sh. wrote: > warn on error switched to EnsureDirectoryGone https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:454: for attempts := 3; attempts > 0; attempts-- { On 2017/01/18 19:01:53, Vadim Sh. wrote: > nit: add small random sleep between attempts, so that two clashing clients have > a better chance of resolving the race. > > (Though is practice it probably doesn't matter). Done, although the race condition isn't a total abort here; if they're racing for the same ensure file, then they'll pick up each others' directories. https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:545: func (d *deployerImpl) readDescription(ctx context.Context, pkgDir string) (desc *Description, err error) { On 2017/01/18 19:01:53, Vadim Sh. wrote: > one thing that concern me is when this function is run over a directory the > client has no longer access to > > e.g. > "sudo cipd ensure ...." > "cipd installed" > > The second "cipd installed" should finish successfully, even though it has no > access to drop description.json files. > > I think in current implementation such uncovertable package will just be > considered missing, which is not correct. I'll make the failure to add the description.json non-fatal and just return the 'presumed' json. https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:566: logging.Warningf(ctx, "no current instance id") On 2017/01/18 19:01:53, Vadim Sh. wrote: > nit: upper case first letter of a logging messages ("No current instance id in > %s", pkgDir). Done. https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:590: err = d.fs.Replace(ctx, tmpName, descriptionPath) On 2017/01/18 19:01:53, Vadim Sh. wrote: > Use > https://github.com/luci/luci-go/blob/master/cipd/client/cipd/local/fs.go#L184 > (instead of lines 579-590). Duh! Awesome! https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... File cipd/client/cipd/local/description.go (right): https://codereview.chromium.org/2640703002/diff/20001/cipd/client/cipd/local/... cipd/client/cipd/local/description.go:5: package local On 2017/01/18 19:01:53, Vadim Sh. wrote: > consider moving this to > https://github.com/luci/luci-go/blob/master/cipd/client/cipd/local/manifest.go > > (perhaps renaming manifest.go to desc.go or something). Done.
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: Try jobs failed on following builders: Luci-go Win Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/33cd0d563453b210)
lgtm % tests https://codereview.chromium.org/2640703002/diff/40001/cipd/client/cipd/local/... File cipd/client/cipd/local/deployer.go (right): https://codereview.chromium.org/2640703002/diff/40001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:456: err = d.fs.EnsureFile(ctx, filepath.Join(tmpDir, descriptionName), func(f *os.File) error { this is a bit of overkill here, since we now the new dir is empty for sure, but fine https://codereview.chromium.org/2640703002/diff/40001/cipd/client/cipd/local/... File cipd/client/cipd/local/deployer_test.go (right): https://codereview.chromium.org/2640703002/diff/40001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer_test.go:771: func TestUpgradeOldPkgDir(t *testing.T) { this needs GOOS == "windows" variant too (I know about _windows.go, but I believe it's more readable in this case just to have two branches in same file) https://codereview.chromium.org/2640703002/diff/40001/cipd/client/cipd/local/... File cipd/client/cipd/local/json_descs.go (right): https://codereview.chromium.org/2640703002/diff/40001/cipd/client/cipd/local/... cipd/client/cipd/local/json_descs.go:134: Root string `json:"root,omitempty"` https://codereview.chromium.org/2640703002/diff/40001/cipd/client/cipd/local/... cipd/client/cipd/local/json_descs.go:135: PackageName string `json:"package_name"` (for consistency with other JSONs used by CIPD)
oh, also please manually verify end-to-end that upgrade works for real e.g. fetch older client, installs some stuff, update client, update the installed stuff
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...
https://codereview.chromium.org/2640703002/diff/40001/cipd/client/cipd/local/... File cipd/client/cipd/local/deployer.go (right): https://codereview.chromium.org/2640703002/diff/40001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:456: err = d.fs.EnsureFile(ctx, filepath.Join(tmpDir, descriptionName), func(f *os.File) error { On 2017/01/19 01:26:01, Vadim Sh. wrote: > this is a bit of overkill here, since we now the new dir is empty for sure, but > fine Yeah, I know, but it's less code here so *shrug* https://codereview.chromium.org/2640703002/diff/40001/cipd/client/cipd/local/... File cipd/client/cipd/local/deployer_test.go (right): https://codereview.chromium.org/2640703002/diff/40001/cipd/client/cipd/local/... cipd/client/cipd/local/deployer_test.go:771: func TestUpgradeOldPkgDir(t *testing.T) { On 2017/01/19 01:26:01, Vadim Sh. wrote: > this needs GOOS == "windows" variant too > > (I know about _windows.go, but I believe it's more readable in this case just to > have two branches in same file) better: I fixed the test to be cross platform :) https://codereview.chromium.org/2640703002/diff/40001/cipd/client/cipd/local/... File cipd/client/cipd/local/json_descs.go (right): https://codereview.chromium.org/2640703002/diff/40001/cipd/client/cipd/local/... cipd/client/cipd/local/json_descs.go:134: Root string On 2017/01/19 01:26:01, Vadim Sh. wrote: > `json:"root,omitempty"` Done. https://codereview.chromium.org/2640703002/diff/40001/cipd/client/cipd/local/... cipd/client/cipd/local/json_descs.go:135: PackageName string On 2017/01/19 01:26:01, Vadim Sh. wrote: > `json:"package_name"` > > (for consistency with other JSONs used by CIPD) Done.
https://codereview.chromium.org/2640703002/diff/60002/cipd/client/cipd/local/... File cipd/client/cipd/local/deployer.go (right): https://codereview.chromium.org/2640703002/diff/60002/cipd/client/cipd/local/... cipd/client/cipd/local/deployer.go:370: idx := sort.IntSlice((*s)).Search(n) btw, why not container.heap?
On 2017/01/19 03:02:52, Vadim Sh. wrote: > https://codereview.chromium.org/2640703002/diff/60002/cipd/client/cipd/local/... > File cipd/client/cipd/local/deployer.go (right): > > https://codereview.chromium.org/2640703002/diff/60002/cipd/client/cipd/local/... > cipd/client/cipd/local/deployer.go:370: idx := sort.IntSlice((*s)).Search(n) > btw, why not container.heap? container/heap would store in a heap (!= sorted), which would make it hard to find the smallest number (though the current algorithm is very very dumb anyway).
The latest patchset does work as intended (can upgrade, install, remove, etc. as expected).
On 2017/01/19 03:05:33, iannucci wrote: > On 2017/01/19 03:02:52, Vadim Sh. wrote: > > > https://codereview.chromium.org/2640703002/diff/60002/cipd/client/cipd/local/... > > File cipd/client/cipd/local/deployer.go (right): > > > > > https://codereview.chromium.org/2640703002/diff/60002/cipd/client/cipd/local/... > > cipd/client/cipd/local/deployer.go:370: idx := sort.IntSlice((*s)).Search(n) > > btw, why not container.heap? > > container/heap would store in a heap (!= sorted), which would make it hard to > find the smallest number err... the smalles number will be on top of the heap (at 0 index). It's one of the properties of the heap...
lgtm if you are waiting for it
On 2017/01/19 20:32:51, Vadim Sh. wrote: > lgtm if you are waiting for it Ah, derp, no I just got distracted :)
The CQ bit was checked by iannucci@chromium.org
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
Try jobs failed on following builders: Luci-go Linux Precise 32 Tester on luci.infra.try (JOB_FAILED, https://luci-milo.appspot.com/swarming/task/33d179a5a2b1da10)
The CQ bit was checked by iannucci@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
This isn't going to work. Looks like we no longer have Ubuntu-12.04 in pool:Infra pool.
On 2017/01/19 22:56:10, Vadim Sh. wrote: > This isn't going to work. Looks like we no longer have Ubuntu-12.04 in > pool:Infra pool. (And no 32bit test coverage as well).
On 2017/01/19 22:56:10, Vadim Sh. wrote: > This isn't going to work. Looks like we no longer have Ubuntu-12.04 in > pool:Infra pool. lol
CQ is committing da patch.
Bot data: {"patchset_id": 60002, "attempt_start_ts": 1484866478787470,
"parent_rev": "4590aea3339388a2946d2398c536d71789a0ca0a", "commit_rev":
"a9cef53c4a1d1cdb4e9d4de59741e0b7282a326a"}
Message was sent while issue was closed.
Description was changed from ========== Change CIPD internal pkgs directory layout to use numeric indices. Also paves the way for enabling multi-root logic for ensure files. R=vadimsh@chromium.org BUG=663843 ========== to ========== Change CIPD internal pkgs directory layout to use numeric indices. Also paves the way for enabling multi-root logic for ensure files. R=vadimsh@chromium.org BUG=663843 Review-Url: https://codereview.chromium.org/2640703002 Committed: https://github.com/luci/luci-go/commit/a9cef53c4a1d1cdb4e9d4de59741e0b7282a326a ==========
Message was sent while issue was closed.
Committed patchset #7 (id:60002) as https://github.com/luci/luci-go/commit/a9cef53c4a1d1cdb4e9d4de59741e0b7282a326a |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
