Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Removes the need to know OS series in many cases #6469
Conversation
| @@ -1780,7 +1780,7 @@ func unitMachineStorageParams(u *Unit) (*machineStorageParams, error) { | ||
| return nil, errors.Annotatef(err, "getting storage instance") | ||
| } | ||
| machineParams, err := machineStorageParamsForStorageInstance( | ||
| - u.st, chMeta, u.UnitTag(), u.Series(), allCons, storage, | ||
| + u.st, chMeta, u.UnitTag(), allCons, storage, |
kat-co
Oct 19, 2016
Contributor
This is referring to a path for another machine; thus we need to use series/OS to determine the correct set of paths.
|
Just to make the comment, I think having the paths aggregated into an appropriate area is good. But because at runtime we can easily want to talk about paths on another machine (the above being one of those cases), then we need to have the groups of paths available, and be able to select which one we want. |
mjs
reviewed
Oct 25, 2016
Although I've raised a few concerns, I think the overall approach here is good. There's lots of great macro and micro improvements.
Obviously needs test updates and TODOs handled before it can be landed.
| - p.ConfDir = newPaths.ConfDir | ||
| - } | ||
| -} | ||
| +// type AgentPaths struct { |
| - DataDir() string | ||
| + DataPath() string | ||
| + | ||
| + StoragePath() string |
| + instancePaths.Temp, | ||
| + instancePaths.Data, | ||
| + instancePaths.Log, | ||
| + instancePaths.MetricsSpool, |
mjs
Oct 25, 2016
Contributor
Why not pass instancePaths (i.e. a paths.Collection) instead of passing all the paths individually?
| + machineId, | ||
| + nonce, | ||
| + modelConfig.ImageStream(), | ||
| + osType, |
mjs
Oct 25, 2016
Contributor
osType isn't used by NewInstanceConfig so I don't think there's a reason to add this arg.
Sorting this out allows simplification in other parts of this PR.
kat-co
Oct 27, 2016
Contributor
NewInstanceConfig now uses osType, specifically to store it so that downstream consumers of InstanceConfig can use it instead of re-deriving os.OSType. This was the goal of bringing this into InstanceConfig as it's something pertinent to starting instances.
| if dataDir != "" { | ||
| - icfg.DataDir = dataDir | ||
| + icfg.DataPath = dataDir |
| @@ -32,6 +33,8 @@ type CloudConfig interface { | ||
| // GetSeries returns the series this CloudConfig was made for. | ||
| GetSeries() string | ||
| + DataPath() string |
| + tempPath string, | ||
| + dataPath string, | ||
| + logPath string, | ||
| + metricsSpoolPath string, |
| config controller.Config, | ||
| cons, modelCons constraints.Value, | ||
| + os os.OSType, |
mjs
Oct 25, 2016
Contributor
Again, given that NewInstanceConfig doesn't use os, this doesn't need it either.
| + // This is a non-critical error. The inability to determine | ||
| + // the series of the machine running the Juju command does not | ||
| + // preclude people from actually using Juju. | ||
| + logger.Warningf("%v", errors.Annotatef(err, "cannot determine whether to warn about Juju 1.x")) |
mjs
Oct 25, 2016
Contributor
I agree with your reasoning but I wonder if this should even be Debugf.
| @@ -78,6 +86,10 @@ func (c *agentConf) DataDir() string { | ||
| return c.dataDir | ||
| } | ||
| +func (c *agentConf) StoragePath() string { |
| @@ -83,14 +84,17 @@ func UnregisterImageDataSourceFunc(id string) { | ||
| func ImageMetadataSources(env Environ) ([]simplestreams.DataSource, error) { | ||
| config := env.Config() | ||
| + // TODO(katco): Pass this in (does it belong in Environ?) |
| + return errors.Trace(err) | ||
| + } | ||
| + | ||
| + toolsSeries.Add(series) |
| -) | ||
| - | ||
| -type osVarType int | ||
| +// collection couples together all the paths Juju knows about into a |
| + Storage: "C:/Juju/lib/juju/storage", | ||
| + Temp: "C:/Juju/tmp", | ||
| + } | ||
| +) |
mjs
Oct 25, 2016
Contributor
A helper which takes an OS type and returns a Collection could be nice.
| + | ||
| +package paths | ||
| + | ||
| +var Defaults = Nix |
mjs
Oct 25, 2016
Contributor
Defaults seems dangerous to me. It could easily end up being used inappropriately.
Would you mind seeing how hard it is to do without the existence of Defaults. I'd be a lot happier if we had to think about which variant to use rather than automatically (and sometimes incorrectly) using Defaults.
| @@ -184,6 +184,8 @@ type ModelArgs struct { | ||
| // MigrationMode is the initial migration mode of the model. | ||
| MigrationMode MigrationMode | ||
| + | ||
| + StoragePath string |
| - doc: *udoc, | ||
| + // TODO(katco): Pass this in; unsure if I should store this in | ||
| + // Mongo or not. | ||
| + storagePath: paths.Defaults.Storage, |
mjs
Oct 25, 2016
Contributor
This is certainly wrong as we could end up using the local machines OS to determine the storage path of a remote unit. To avoid the possibility of errors shouldn't the storage path always be calculated using the OS of the host machine?
I don't think this should be stored in mongo, nor passed in (at this level at least).
kat-co
added some commits
Oct 18, 2016
|
Current status of tests: http://pastebin.ubuntu.com/23485814/ |
| @@ -251,7 +183,7 @@ type Config interface { | ||
| // MetricsSpoolDir returns the spool directory where workloads store |
|
Oops, sorry for any added noise. I hadn't realised my debugging of the jenkins merge check job was actually commenting on the PRs. |
|
With no one driving this, the massive change, and work needed, I'm closing this. |
kat-co commentedOct 18, 2016
This patch does a few things:
This will allow new versions of OSs that fall outside of our release cycle (namely MacOS) to run the juju binary without panics or errors.
I HAVE NOT ADDRESSED TESTS UNTIL I GET FEEDBACK ON THIS APPROACH. TESTS WILL FAIL