|
|
Descriptioncmd/juju: sync-tools fixes
sync-tools now requires the --dev flag before copying development versions,
and no longer copies into a public bucket when the contents of that bucket
are not available to the target environment.
This branch removes the last of the fuzzy multi-bucket tools code.
https://code.launchpad.net/~fwereade/juju-core/environs-tools-final-replacement/+merge/158830
Requires: https://code.launchpad.net/~fwereade/juju-core/environs-tools-provisioning-integration/+merge/158805
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 21
Patch Set 2 : cmd/juju: sync-tools fixes #Patch Set 3 : cmd/juju: sync-tools fixes #
MessagesTotal messages: 9
Please take a look.
Sign in to reply to this message.
LGTM with the "private tools present" error fixed and a couple more minor suggestions. https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go File cmd/juju/synctools.go (right): https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode150 cmd/juju/synctools.go:150: return fmt.Errorf("private tools present: public tools would be ignored") i think this should be just a warning, if anything. just because we have private tools present in the current environment doesn't mean we don't want to make tools available to others in the public bucket. https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode161 cmd/juju/synctools.go:161: case nil, tools.ErrNoMatches, tools.ErrNoTools: still pained by this. https://codereview.appspot.com/8748046/diff/1/environs/tools.go File environs/tools.go (right): https://codereview.appspot.com/8748046/diff/1/environs/tools.go#newcode102 environs/tools.go:102: log.Infof("environs: filtering tools by series: %s", series) these feel like they should be Debugf calls to me (and probably not even that) if you think the log messages are worth it, i'd probably just do: log.Debugf("environs: filtering tools by %+v", filter) just before the Match. https://codereview.appspot.com/8748046/diff/1/environs/tools.go#newcode108 environs/tools.go:108: log.Infof("environs: filtering tools by architecture: %s", *cons.Arch) as above. https://codereview.appspot.com/8748046/diff/1/environs/tools.go#newcode124 environs/tools.go:124: log.Infof("environs: finding exact version %s", vers) Debugf, i think, if anything.
Sign in to reply to this message.
LGTM with a few trivials. https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools_test.go File cmd/juju/synctools_test.go (right): https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools_test.go#newc... cmd/juju/synctools_test.go:116: "1.0.0-precise-amd64", "1.0.0-quantal-amd64", "1.0.0-quantal-i386") please, put these on separate lines (or at least one line per series). https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools_test.go#newc... cmd/juju/synctools_test.go:145: "1.0.0-precise-amd64", "1.0.0-quantal-amd64", "1.0.0-quantal-i386") ditto https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools_test.go#newc... cmd/juju/synctools_test.go:160: "1.0.0-precise-amd64", "1.0.0-quantal-amd64", ditto - make them consistent https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools_test.go#newc... cmd/juju/synctools_test.go:176: "1.0.0-precise-amd64", "1.0.0-quantal-amd64", "1.0.0-quantal-i386") ...and here https://codereview.appspot.com/8748046/diff/1/environs/tools.go File environs/tools.go (right): https://codereview.appspot.com/8748046/diff/1/environs/tools.go#newcode102 environs/tools.go:102: log.Infof("environs: filtering tools by series: %s", series) On 2013/04/15 13:48:21, rog wrote: > these feel like they should be Debugf calls to me (and probably not even that) > > if you think the log messages are worth it, i'd probably just do: > > log.Debugf("environs: filtering tools by %+v", filter) just before the Match. I think it's nice as an Infof, at least it's consistent with the func above. If anything, I'll merge the two messages in one: "environs: filtering tools by version %q and series %q"
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go File cmd/juju/synctools.go (right): https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode150 cmd/juju/synctools.go:150: return fmt.Errorf("private tools present: public tools would be ignored") On 2013/04/15 13:48:21, rog wrote: > i think this should be just a warning, if anything. > just because we have private tools present in the current environment doesn't > mean we don't want to make tools available to others in the public bucket. Honestly, I question the value of --public regardless. If I were setting it up I'd have a shared-tools environ that I never actually used other than to sync tools into, and use its control-bucket as my other environs' public-bucket. Given the current fallback rules, a public sync-tools into an env with private tools will fail to perform the purpose of sync-tools -- make them *available* to the target env -- and so I think it's actually better to fail before performing a ineffectual copy of X MiB via one's local machine. https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode161 cmd/juju/synctools.go:161: case nil, tools.ErrNoMatches, tools.ErrNoTools: On 2013/04/15 13:48:21, rog wrote: > still pained by this. Each non-test file that uses them together also needs to differentiate between the two situations. I don't think it's unreasonable to have distinct errors. https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools_test.go File cmd/juju/synctools_test.go (right): https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools_test.go#newc... cmd/juju/synctools_test.go:116: "1.0.0-precise-amd64", "1.0.0-quantal-amd64", "1.0.0-quantal-i386") On 2013/04/15 15:00:54, dimitern wrote: > please, put these on separate lines (or at least one line per series). heh, they're like this because I was trying to maintain consistency. Done.
Sign in to reply to this message.
https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go File cmd/juju/synctools.go (right): https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode150 cmd/juju/synctools.go:150: return fmt.Errorf("private tools present: public tools would be ignored") On 2013/04/16 08:43:40, fwereade wrote: > On 2013/04/15 13:48:21, rog wrote: > > i think this should be just a warning, if anything. > > just because we have private tools present in the current environment doesn't > > mean we don't want to make tools available to others in the public bucket. > > Honestly, I question the value of --public regardless. If I were setting it up > I'd have a shared-tools environ that I never actually used other than to sync > tools into, and use its control-bucket as my other environs' public-bucket. > > Given the current fallback rules, a public sync-tools into an env with private > tools will fail to perform the purpose of sync-tools -- make them *available* to > the target env -- and so I think it's actually better to fail before performing > a ineffectual copy of X MiB via one's local machine. i think if we're asking it to do something, it should darn well do it. the user may well know best. a warning would be fine, i think. https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode161 cmd/juju/synctools.go:161: case nil, tools.ErrNoMatches, tools.ErrNoTools: On 2013/04/16 08:43:40, fwereade wrote: > On 2013/04/15 13:48:21, rog wrote: > > still pained by this. > > Each non-test file that uses them together also needs to differentiate between > the two situations. the code above demonstrates that statement isn't actually true. taking a quick look through all the places that use the tools.Err* values, i don't see any where the errors are particularly useful, and sometimes they make things more obscure. to try and check that i wasn't on crack, i rustled up a branch which did what i'm suggesting (lose the special errors and the major-version argument to ListTools). overall (not counting the tests, which i didn't touch) it's 27 less lines of code and sync-tools will now work with all tools, not just the same major version. in particular, putting the major-version matching inside Match seems like the Right Thing to do. lp:~rogpeppe/juju-core/fwereade-less-errors
Sign in to reply to this message.
It continues... is there some use case I have failed to properly take into account? https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go File cmd/juju/synctools.go (right): https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode150 cmd/juju/synctools.go:150: return fmt.Errorf("private tools present: public tools would be ignored") On 2013/04/16 10:01:00, rog wrote: > On 2013/04/16 08:43:40, fwereade wrote: > > On 2013/04/15 13:48:21, rog wrote: > > > i think this should be just a warning, if anything. > > > just because we have private tools present in the current environment > doesn't > > > mean we don't want to make tools available to others in the public bucket. > > > > Honestly, I question the value of --public regardless. If I were setting it up > > I'd have a shared-tools environ that I never actually used other than to sync > > tools into, and use its control-bucket as my other environs' public-bucket. > > > > Given the current fallback rules, a public sync-tools into an env with private > > tools will fail to perform the purpose of sync-tools -- make them *available* > to > > the target env -- and so I think it's actually better to fail before > performing > > a ineffectual copy of X MiB via one's local machine. > > i think if we're asking it to do something, it should darn well do it. > the user may well know best. > > a warning would be fine, i think. A sync-tools into a public bucket being used by a running environ with tools *will not work* from the POV of the user. They ask for the latest tools, and juju does a bunch of work, and they don't get the latest tools. If they want fresh tools to run on a real environ, they can destroy-environment, sync-tools --public, bootstrap; and if they're using the environment purely as a frontend to a public tools bucket, they'll never have private tools messing everything up. https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode161 cmd/juju/synctools.go:161: case nil, tools.ErrNoMatches, tools.ErrNoTools: On 2013/04/16 10:01:00, rog wrote: > On 2013/04/16 08:43:40, fwereade wrote: > > On 2013/04/15 13:48:21, rog wrote: > > > still pained by this. > > > > Each non-test file that uses them together also needs to differentiate between > > the two situations. > > the code above demonstrates that statement isn't actually true. ...it's the code above *that* that demonstrates that it in fact just as I stated it. case tools.ErrNoTools: case nil, tools.ErrNoMatches: > taking a quick look through all the places that use the tools.Err* values, i > don't see any where the errors are particularly useful, and sometimes they make > things more obscure. AIUI, what we agreed in atlanta was that private tools would render public ones inaccessible. I don't think that adding an exception to that rule makes anything clearer... I feel twitchy enough about the magic source env and hardcoded use of its public-storage, but I managed to resist the temptation to add a --source env-name feature (which would default to the magic env, and in all cases would cause sync-tools to draw from only the *available* tools in the source env). I haven't yet been convinced that it's sensible to discard the agreed behaviour, but I am becoming convinced that I haven't done it properly and I should get more hardline about copying *available* tools from x such that they become *available* in Y. :) > to try and check that i wasn't on crack, i rustled up a branch which did what > i'm suggesting (lose the special errors and the major-version argument to > ListTools). overall (not counting the tests, which i didn't touch) it's 27 less > lines of code and sync-tools will now work with all tools, not just the same > major version. On first inspection that STM to discard bootstrap tools that should be considered valid (agent-version should beat dev); it makes sync-tools somewhat unhelpful to anyone not using the most recent major version (because they can't copy any useful tools without copying *all* tools); and it makes the first major-version upgrade break upgrade-juju in approximately the same way it breaks sync-tools. I think that fixing all these as well would perhaps make the change a little less compelling, not to mention making the effects less certainly predictable. > in particular, putting the major-version matching inside Match seems like the > Right Thing to do. > > lp:~rogpeppe/juju-core/fwereade-less-errors +0.5 in theory. In practice, looking for major-version-matching tools in one specific bucket is the behaviour we need right now, and I'm not sure it's a win to take that behaviour -- currently implemented OAOO -- and smear it out across all the places that'll have to use it... just to get rid of an error you don't like, that precisely differentiates the cases we care about both in environs/tools.go and in cmd/juju/upgradejuju.go. We will surely reconsider the details when our minds are fully focused on major version upgrades. But today? Let's stick with predictably conservative behaviour, please, and not casually duplicate code to do the same thing in multiple places.
Sign in to reply to this message.
i'm not going to argue these points any more. i have registered my objections, and i don't want to block progress. if you still think it's appropriate, please go ahead. https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go File cmd/juju/synctools.go (right): https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode150 cmd/juju/synctools.go:150: return fmt.Errorf("private tools present: public tools would be ignored") On 2013/04/16 10:58:58, fwereade wrote: > On 2013/04/16 10:01:00, rog wrote: > > On 2013/04/16 08:43:40, fwereade wrote: > > > On 2013/04/15 13:48:21, rog wrote: > > > > i think this should be just a warning, if anything. > > > > just because we have private tools present in the current environment > > doesn't > > > > mean we don't want to make tools available to others in the public bucket. > > > > > > Honestly, I question the value of --public regardless. If I were setting it > up > > > I'd have a shared-tools environ that I never actually used other than to > sync > > > tools into, and use its control-bucket as my other environs' public-bucket. > > > > > > Given the current fallback rules, a public sync-tools into an env with > private > > > tools will fail to perform the purpose of sync-tools -- make them > *available* > > to > > > the target env -- and so I think it's actually better to fail before > > performing > > > a ineffectual copy of X MiB via one's local machine. > > > > i think if we're asking it to do something, it should darn well do it. > > the user may well know best. > > > > a warning would be fine, i think. > > A sync-tools into a public bucket being used by a running environ with tools > *will not work* from the POV of the user. They ask for the latest tools, and > juju does a bunch of work, and they don't get the latest tools. If they specify --public, they're not asking for the latest tools. They're asking them to be made available to the "public". I can easily think of a situation where I'd want to use synctools with an existing environment bootstrapped with --upload-tools; in fact, that's quite possibly the way I'd *normally* use them (juju bootstrap --upload-tools; check it works; juju sync-tools --public) The worst possible down side of letting the sync carry on is that it spends a while copying tools (and you'd get a warning). I can't see that as a significant problem. Aborting with an error seems to me to be patronising behaviour. https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode161 cmd/juju/synctools.go:161: case nil, tools.ErrNoMatches, tools.ErrNoTools: On 2013/04/16 10:58:58, fwereade wrote: > On 2013/04/16 10:01:00, rog wrote: > > On 2013/04/16 08:43:40, fwereade wrote: > > > On 2013/04/15 13:48:21, rog wrote: > > > > still pained by this. > > > > > > Each non-test file that uses them together also needs to differentiate > between > > > the two situations. > > > > the code above demonstrates that statement isn't actually true. > > ...it's the code above *that* that demonstrates that it in fact just as I stated > it. > > case tools.ErrNoTools: > case nil, tools.ErrNoMatches: > > > taking a quick look through all the places that use the tools.Err* values, i > > don't see any where the errors are particularly useful, and sometimes they > make > > things more obscure. > > AIUI, what we agreed in atlanta was that private tools would render public ones > inaccessible. I don't think that adding an exception to that rule makes anything > clearer... i don't think i'm making an exception to that rule. if anything i think it's making *less* exceptions. the user-observable behaviour with my suggested changes should be exactly the same as your branch is now (with the exception that --all actually means "all") > I haven't yet been convinced that it's sensible to discard the agreed behaviour, > but I am becoming convinced that I haven't done it properly and I should get > more hardline about copying *available* tools from x such that they become > *available* in Y. :) that's leaning hard on your particular definition of "available", meaning "available to this particular client". i'd prefer a more general definition: "available to some client". > > to try and check that i wasn't on crack, i rustled up a branch which did what > > i'm suggesting (lose the special errors and the major-version argument to > > ListTools). overall (not counting the tests, which i didn't touch) it's 27 > less > > lines of code and sync-tools will now work with all tools, not just the same > > major version. > > On first inspection that STM to discard bootstrap tools that should be > considered valid (agent-version should beat dev) yes, i missed a "return list, nil" in that if statement. > ; it makes sync-tools somewhat > unhelpful to anyone not using the most recent major version (because they can't > copy any useful tools without copying *all* tools) true, but trivially addressed by adding to the filter above; it adds 4 lines of code. > ; and it makes the first > major-version upgrade break upgrade-juju in approximately the same way it breaks > sync-tools. only if the above point isn't addressed. > We will surely reconsider the details when our minds are fully focused on major > version upgrades. But today? Let's stick with predictably conservative > behaviour, please, and not casually duplicate code to do the same thing in > multiple places. personally i prefer to have less code overall, even if there's some duplication in it (not that i'm sure what "casually duplicated" code you're referring to here).
Sign in to reply to this message.
*** Submitted: cmd/juju: sync-tools fixes sync-tools now requires the --dev flag before copying development versions, and no longer copies into a public bucket when the contents of that bucket are not available to the target environment. This branch removes the last of the fuzzy multi-bucket tools code. R=rog, dimitern CC= https://codereview.appspot.com/8748046
Sign in to reply to this message.
I probably shouldn't keep banging on about this, but some of it's actually really important. https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go File cmd/juju/synctools.go (right): https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode150 cmd/juju/synctools.go:150: return fmt.Errorf("private tools present: public tools would be ignored") On 2013/04/16 11:54:24, rog wrote: > If they specify --public, they're not asking for the latest tools. > They're asking them to be made available to the "public". sync-tools is an environment-level command, which is intended to make tools available to that environment. The --public flag causes it to use the public-bucket in doing so. > I can easily think of a situation where I'd want to use synctools > with an existing environment bootstrapped with --upload-tools; > in fact, that's quite possibly the way I'd *normally* use them > (juju bootstrap --upload-tools; check it works; juju sync-tools --public) Please explain what purpose this serves. > The worst possible down side of letting the sync carry on is that it spends > a while copying tools (and you'd get a warning). I can't see that as a > significant problem. The problem is that, from the perspective of the user who wants to use newer tools, the command has *failed* despite reporting success. > Aborting with an error seems to me to be patronising behaviour. The user has set up their environment such that it cannot use public tools. Wasting the user's time and bandwidth to do something that cannot possibly work and then throwing up our hands and saying "but we warned you" doesn't seem ideal. TBH, I think that has a pretty clear "I told you so but you just wouldn't listen" subtext that's pretty patronising itself. https://codereview.appspot.com/8748046/diff/1/cmd/juju/synctools.go#newcode161 cmd/juju/synctools.go:161: case nil, tools.ErrNoMatches, tools.ErrNoTools: On 2013/04/16 11:54:24, rog wrote: > On 2013/04/16 10:58:58, fwereade wrote: > > AIUI, what we agreed in atlanta was that private tools would render public > ones > > inaccessible. I don't think that adding an exception to that rule makes > anything > > clearer... > > i don't think i'm making an exception to that rule. > if anything i think it's making *less* exceptions. > the user-observable behaviour with my suggested changes > should be exactly the same as your branch is now > (with the exception that --all actually means "all") You are introducing a fuzzy and unnamed concept that maps to a group of environments that happen to share some arbitrary convenient config setting, and you are conflating those with actual environments. I hope I do not need to reiterate that the difference between "working" and "not working", as described above, is in fact user-observable. > that's leaning hard on your particular definition of "available", meaning > "available to this particular client". i'd prefer a more general definition: > "available to some client". I mean "available to the target environment". If we follow your definition, we don't need sync-tools at all, because any syncable tool is inherently accessible by *some* environment. But I don't see how that helps users. > yes, i missed a "return list, nil" in that if statement. > true, but trivially addressed by adding to the filter above; > it adds 4 lines of code. > only if the above point isn't addressed. > personally i prefer to have less code overall, even if there's some duplication > in it (not that i'm sure what "casually duplicated" code you're referring to > here). Actually this is a textbook example. In refactoring for SLOC you have introduced 3 bugs, 2 of which are basically the same but it's now impossible to tell that they are the same. Say your proposal were to land; at some stage we'd get a report of one of the 2 identical ones, and someone would fix it. But they probably wouldn't spot the duplicated bug elsewhere, so when that one's reported someone else is likely to pick it up and fix that in isolation. But those two fixes will probably not be identical, and might not be identical in effect -- and even if they are the situation is no better, because a behaviour change to one will not necessarily be understood to be relevant to the other. And so the tools-selection logic will get a reputation in our minds as flaky and inconsistent, and it'll be a constant (if subtle) drag on development... ...and eventually some poor sod will have to make some sense of it all, and find all the places that get tools.Lists and how they're used and manipulated, and figure out which inconsistent bits are bugs, and which are just implemented differently; and turn that whole mess into something that can be understood in something resembling isolation. (please note: this is what I have *just been doing*, for *this exact functionality*) I do not actually *like* doing that sort of work, because it is hard and ugly and only pays off over the long term, but I do it because I am all too familiar with the costs of failing to do so. All of the above is why duplication is so upsetting to many people -- because they know from bitter experience where it leads. I'm still not sure why it is that you tend to favour this sort of code; have you never experienced the sorts of scenarios I'm describing?
Sign in to reply to this message.
|