On not copy-pasting state and params struct conversions
Hi all, Anastasia raised an issue in http://reviews.vapour.ws/r/885/ about how to cut down on struct conversions between params, state, and domain packages. In this case we're talking about storage. The following API server facades currently participate in storage: - client - storage - provisioner - diskmanager (to be renamed, this lists block devices on machines) - diskformatter - storageworker (to be renamed, this is the dynamic storage provisioner) Each facade have some overlap in dealing with storage entities, e.g. the diskformatter and diskmanager each need to deal with block devices. This leads to much duplication of struct copying/conversion code when toing and froing between state and clients. I don't want to go adding conversion code to the params, state or storage packages, as they really shouldn't have dependencies on each other. Does anyone have a good idea about where to put this common functionality? Maybe api/common/storage, apiserver/common/storage? Does not appeal, but I can't think of a better option. Cheers, Andrew -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: On not copy-pasting state and params struct conversions
So, I think you *do* need conversion code specific to state -- the methods exported from state should be in terms of the storage types, because they're the language we've chosen to model storage, so there needs to be conversion code in state (or possibly in an internal package -- but either way there's no reason to expose it to anyone else). WRT params, though, we already have two groups of client packages, so the same forces don't quite apply: and I still prefer that we *not* import other packages into params [0]. How about a single params/convert package? Cheers William [0] this is because the temptation to use a storage.Foo in place of a params.Foo has historically been overwhelming (and while it's *very dangerous* to do that it's hard to make the danger sufficiently obvious to dissuade distracted devs from doing so). On Tue, Feb 10, 2015 at 11:07 AM, Andrew Wilkins andrew.wilk...@canonical.com wrote: Hi all, Anastasia raised an issue in http://reviews.vapour.ws/r/885/ about how to cut down on struct conversions between params, state, and domain packages. In this case we're talking about storage. The following API server facades currently participate in storage: - client - storage - provisioner - diskmanager (to be renamed, this lists block devices on machines) - diskformatter - storageworker (to be renamed, this is the dynamic storage provisioner) Each facade have some overlap in dealing with storage entities, e.g. the diskformatter and diskmanager each need to deal with block devices. This leads to much duplication of struct copying/conversion code when toing and froing between state and clients. I don't want to go adding conversion code to the params, state or storage packages, as they really shouldn't have dependencies on each other. Does anyone have a good idea about where to put this common functionality? Maybe api/common/storage, apiserver/common/storage? Does not appeal, but I can't think of a better option. Cheers, Andrew -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: A home for underappreciated errors
On Tue, Feb 10, 2015 at 12:13 PM, William Reade william.re...@canonical.com wrote: really ought to include a bunch more mechanism for finding out *precisely* what should have been assigned to what To be clear: I do not think we should do this, because the interpretation of those entities would themselves be domain-specific -- if client code is written to expect a unit id and ends up suddenly seeing a volume id, the *best* case is that it fails -- the worst is that it keeps on running with bad data and smearing the effects of the bug further through the system with arbitrary further fallout. +100 to domain-specific errors. -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: A home for underappreciated errors
On Tue, Feb 10, 2015 at 6:16 PM, roger peppe roger.pe...@canonical.com wrote: On 10 February 2015 at 10:06, Andrew Wilkins andrew.wilk...@canonical.com wrote: On Tue, Feb 10, 2015 at 5:57 PM, roger peppe roger.pe...@canonical.com wrote: On 10 February 2015 at 08:55, Andrew Wilkins andrew.wilk...@canonical.com wrote: Hi all, Ian raised a good point in http://reviews.vapour.ws/r/885/ (caution advised, may cause eyebleed) about a change I made to errors: https://github.com/juju/errors/pull/17 The NotAssigned error, which previously was only raised by state.Unit.AssignedMachineId, is now also raised by state.Volume.StorageInstance. I removed the error from the state package so we didn't have apiserver/common poking its head in there, and moved it over to the juju/errors package. The issue Ian raised is that juju/errors should contain relatively generic error types, and we should keep domain-specific things elsewhere. Examples are NotAssigned, and NotProvisioned. We're thinking of moving these domain-specific error types into a new package, juju/juju/errors. What do you think about this? I agree with Ian here. Personally, I think that specific error types/values work well when defined in or close to the module that returns them, which is aligned to my general feeling that the set of possible returned errors is part of the contract of the method it was returned from. For example, the mgo package returns mgo.ErrNotFound when an item isn't found, and that's fine - we translate from that error to a more local not-found error when necessary. For errors that pass over an API, they can be considered a part of the API and defined along with the other API types. So, rather than a single package for juju-related errors, I would suggest that domain-specific errors are defined in packages close to the domain in question rather than in a single package used by everything. To my mind this is a more modular and scalable approach. That all sounds good, but it doesn't work very well with the existing error code thingy in apiserver/common, unless we're okay with apiserver/common poking into state. I'm not a fan of that. Perhaps we just need a state-internal package which registers predicates in apiserver/common to do that magic. That wouldn't be so bad IMO. I don't understand. Can't you just define NotAssignedError in state and have the usual predicate operations in apiserver/common ? (you can't import apiserver/common from state BTW - you'd get a cycle) Yeah, I didn't think that through well. Long day. Having it state dependency from apiserver/common is, as William points out, fine. It's all going to state anyway. -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: On not copy-pasting state and params struct conversions
I think that the danger here is one of backwards incompatibility, and that's a danger that's possible to alleviate with some tooling support. Writing tons of boilerplate code to manually convert between types at different levels has always seemed like a poor use of developer resources to me (and opens up more possibility for conversion errors). When things *do* change in a backwardly incompatible way, then of course conversion code would need to be written, but if there is a tool that can automatically test that things remain compatible, this could be only a small percentage of what is being done up front now. cheers, rog. On 10 February 2015 at 09:07, Andrew Wilkins andrew.wilk...@canonical.com wrote: Hi all, Anastasia raised an issue in http://reviews.vapour.ws/r/885/ about how to cut down on struct conversions between params, state, and domain packages. In this case we're talking about storage. The following API server facades currently participate in storage: - client - storage - provisioner - diskmanager (to be renamed, this lists block devices on machines) - diskformatter - storageworker (to be renamed, this is the dynamic storage provisioner) Each facade have some overlap in dealing with storage entities, e.g. the diskformatter and diskmanager each need to deal with block devices. This leads to much duplication of struct copying/conversion code when toing and froing between state and clients. I don't want to go adding conversion code to the params, state or storage packages, as they really shouldn't have dependencies on each other. Does anyone have a good idea about where to put this common functionality? Maybe api/common/storage, apiserver/common/storage? Does not appeal, but I can't think of a better option. Cheers, Andrew -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: A home for underappreciated errors
On Tue, Feb 10, 2015 at 5:57 PM, roger peppe roger.pe...@canonical.com wrote: On 10 February 2015 at 08:55, Andrew Wilkins andrew.wilk...@canonical.com wrote: Hi all, Ian raised a good point in http://reviews.vapour.ws/r/885/ (caution advised, may cause eyebleed) about a change I made to errors: https://github.com/juju/errors/pull/17 The NotAssigned error, which previously was only raised by state.Unit.AssignedMachineId, is now also raised by state.Volume.StorageInstance. I removed the error from the state package so we didn't have apiserver/common poking its head in there, and moved it over to the juju/errors package. The issue Ian raised is that juju/errors should contain relatively generic error types, and we should keep domain-specific things elsewhere. Examples are NotAssigned, and NotProvisioned. We're thinking of moving these domain-specific error types into a new package, juju/juju/errors. What do you think about this? I agree with Ian here. Personally, I think that specific error types/values work well when defined in or close to the module that returns them, which is aligned to my general feeling that the set of possible returned errors is part of the contract of the method it was returned from. For example, the mgo package returns mgo.ErrNotFound when an item isn't found, and that's fine - we translate from that error to a more local not-found error when necessary. For errors that pass over an API, they can be considered a part of the API and defined along with the other API types. So, rather than a single package for juju-related errors, I would suggest that domain-specific errors are defined in packages close to the domain in question rather than in a single package used by everything. To my mind this is a more modular and scalable approach. That all sounds good, but it doesn't work very well with the existing error code thingy in apiserver/common, unless we're okay with apiserver/common poking into state. I'm not a fan of that. Perhaps we just need a state-internal package which registers predicates in apiserver/common to do that magic. That wouldn't be so bad IMO. It's also the standard Go idiom. cheers, rog. -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: On not copy-pasting state and params struct conversions
On Tue, Feb 10, 2015 at 5:44 PM, William Reade william.re...@canonical.com wrote: So, I think you *do* need conversion code specific to state -- the methods exported from state should be in terms of the storage types, because they're the language we've chosen to model storage, so there needs to be conversion code in state (or possibly in an internal package -- but either way there's no reason to expose it to anyone else). Not sure I follow what you're saying about in terms of the storage types, so I'll describe how it is at the moment. The types are in three places: - juju/juju/storage - juju/juju/apiserver/params - juju/juju/state There's a storage.Volume, a params.Volume, and a state.Volume. state.Volume is actually an interface, but there's a VolumeInfo which ~correlates to the other struct types. I think the conversion from params-state all needs to be in apiserver/something. WRT params, though, we already have two groups of client packages, so the same forces don't quite apply: and I still prefer that we *not* import other packages into params [0]. How about a single params/convert package? I could live with it, but I don't like the idea of adding dependencies to every domain for every facade that uses it. Cheers, Andrew Cheers William [0] this is because the temptation to use a storage.Foo in place of a params.Foo has historically been overwhelming (and while it's *very dangerous* to do that it's hard to make the danger sufficiently obvious to dissuade distracted devs from doing so). On Tue, Feb 10, 2015 at 11:07 AM, Andrew Wilkins andrew.wilk...@canonical.com wrote: Hi all, Anastasia raised an issue in http://reviews.vapour.ws/r/885/ about how to cut down on struct conversions between params, state, and domain packages. In this case we're talking about storage. The following API server facades currently participate in storage: - client - storage - provisioner - diskmanager (to be renamed, this lists block devices on machines) - diskformatter - storageworker (to be renamed, this is the dynamic storage provisioner) Each facade have some overlap in dealing with storage entities, e.g. the diskformatter and diskmanager each need to deal with block devices. This leads to much duplication of struct copying/conversion code when toing and froing between state and clients. I don't want to go adding conversion code to the params, state or storage packages, as they really shouldn't have dependencies on each other. Does anyone have a good idea about where to put this common functionality? Maybe api/common/storage, apiserver/common/storage? Does not appeal, but I can't think of a better option. Cheers, Andrew -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: A home for underappreciated errors
On 10 February 2015 at 08:55, Andrew Wilkins andrew.wilk...@canonical.com wrote: Hi all, Ian raised a good point in http://reviews.vapour.ws/r/885/ (caution advised, may cause eyebleed) about a change I made to errors: https://github.com/juju/errors/pull/17 The NotAssigned error, which previously was only raised by state.Unit.AssignedMachineId, is now also raised by state.Volume.StorageInstance. I removed the error from the state package so we didn't have apiserver/common poking its head in there, and moved it over to the juju/errors package. The issue Ian raised is that juju/errors should contain relatively generic error types, and we should keep domain-specific things elsewhere. Examples are NotAssigned, and NotProvisioned. We're thinking of moving these domain-specific error types into a new package, juju/juju/errors. What do you think about this? I agree with Ian here. Personally, I think that specific error types/values work well when defined in or close to the module that returns them, which is aligned to my general feeling that the set of possible returned errors is part of the contract of the method it was returned from. For example, the mgo package returns mgo.ErrNotFound when an item isn't found, and that's fine - we translate from that error to a more local not-found error when necessary. For errors that pass over an API, they can be considered a part of the API and defined along with the other API types. So, rather than a single package for juju-related errors, I would suggest that domain-specific errors are defined in packages close to the domain in question rather than in a single package used by everything. To my mind this is a more modular and scalable approach. It's also the standard Go idiom. cheers, rog. -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: A home for underappreciated errors
I think that NotAssigned is *too* generic. UnitNotAssignedToMachineErr is one thing, and VolumeNotAssignedToStorageInstance is another; if we use the same error type to represent those distinct situations, we really ought to include a bunch more mechanism for finding out *precisely* what should have been assigned to what (because refactoring happens, and methods start returning the same generic error for new reasons, potentially breaking client code that makes assumptions about what that generic error type implies. See worker/uniter/filter/filter.go and search for ErrTerminateAgent for a particularly hair-raising example...) I see what you're saying re apiserver/common, but I wouldn't really characterise it as poking into state: apiserver is generally implemented in terms of state, so that's a perfectly reasonable dependency IMO. For exactly the reasons stated re generic error types in code, the generic error codes in the api are the scary bits I now wish we'd done differently... Cheers William On Tue, Feb 10, 2015 at 10:55 AM, Andrew Wilkins andrew.wilk...@canonical.com wrote: Hi all, Ian raised a good point in http://reviews.vapour.ws/r/885/ (caution advised, may cause eyebleed) about a change I made to errors: https://github.com/juju/errors/pull/17 The NotAssigned error, which previously was only raised by state.Unit.AssignedMachineId, is now also raised by state.Volume.StorageInstance. I removed the error from the state package so we didn't have apiserver/common poking its head in there, and moved it over to the juju/errors package. The issue Ian raised is that juju/errors should contain relatively generic error types, and we should keep domain-specific things elsewhere. Examples are NotAssigned, and NotProvisioned. We're thinking of moving these domain-specific error types into a new package, juju/juju/errors. What do you think about this? Cheers, Andrew -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: A home for underappreciated errors
-BEGIN PGP SIGNED MESSAGE- Hash: SHA1 On 10.02.2015 11:57, roger peppe wrote: On 10 February 2015 at 08:55, Andrew Wilkins andrew.wilk...@canonical.com wrote: Hi all, Ian raised a good point in http://reviews.vapour.ws/r/885/ (caution advised, may cause eyebleed) about a change I made to errors: https://github.com/juju/errors/pull/17 The NotAssigned error, which previously was only raised by state.Unit.AssignedMachineId, is now also raised by state.Volume.StorageInstance. I removed the error from the state package so we didn't have apiserver/common poking its head in there, and moved it over to the juju/errors package. The issue Ian raised is that juju/errors should contain relatively generic error types, and we should keep domain-specific things elsewhere. Examples are NotAssigned, and NotProvisioned. We're thinking of moving these domain-specific error types into a new package, juju/juju/errors. What do you think about this? NotAssigned is more generic than NotProvisioned IMO, so if I were to decide which one to move to juju/errors, I'd move the former, not the latter. I agree with Ian here. Personally, I think that specific error types/values work well when defined in or close to the module that returns them, which is aligned to my general feeling that the set of possible returned errors is part of the contract of the method it was returned from. For example, the mgo package returns mgo.ErrNotFound when an item isn't found, and that's fine - we translate from that error to a more local not-found error when necessary. For errors that pass over an API, they can be considered a part of the API and defined along with the other API types. So, rather than a single package for juju-related errors, I would suggest that domain-specific errors are defined in packages close to the domain in question rather than in a single package used by everything. To my mind this is a more modular and scalable approach. +1 I like the idea of having state/errors for sharable-but-domain-specific errors (or network/errors, storage/errors, etc.) and leave generic errors useful it a lot more cases in juju/errors. It's also the standard Go idiom. cheers, rog. - -- Dimiter Naydenov dimiter.nayde...@canonical.com Juju Core Sapphire team http://juju.ubuntu.com -BEGIN PGP SIGNATURE- Version: GnuPG v1 iQEcBAEBAgAGBQJU2d83AAoJENzxV2TbLzHwd20H/i22GBC5dADZnLFAfNhE+X0y E7tZc+UkrsKGUmcAHm1k5tEuv3r9Vk6vLxtEcRVXcWvXqoCCW72Cpsw7lLiAkKoH dYPwyBTVorp/N+Z1hxMN+P3LhctRlGNlz1IR9BLJ+3fM/Dlfp+YfVJ93BfqrcC7f uaUJY0iSdOHuedfs29CtTtamSDNb1s3l8uCo5X0Ihh2zc1SMOHPVFxrdWnLTERpN Vw8C6gv47GETUekTzwoviioiQrDxKijMqjkGB9khrKiXdlSLz0rQV2W+874ztPgo pkcdlSzLCJ89r7WB79kajGYWGrlXD1n6NsTSgywIW+M8teIprHjR/exWDxtyPKs= =dbEQ -END PGP SIGNATURE- -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
juju-core 1.21.2 is proposed for release
# juju-core 1.21.2 A new stable release of Juju, juju-core 1.21.2, is proposed. This release may replace 1.21.1 on 2015-02-12 after a period of evaluation. ## Getting Juju juju-core 1.21.2 is available for utopic and backported to earlier series in the following PPA: https://launchpad.net/~juju/+archive/proposed The proposed packages in this archive use the proposed simple-streams. You must configure the 'agent-stream' option in your environments.yaml to use the matching juju agents. agent-stream: proposed ## Notable Changes This releases addresses stability and performance issues. ## Resolved issues * Unable to override network-bridge if container type is kvm (local provider) Lp 1416134 * Src/bitbucket.org/kardianos/osext/license is wrong Lp 1416425 * Github.com/juju/syslog has contradicting licensing info Lp 1416433 * Some files refer to an include license file that is not included Lp 1416430 * Github.com/juju/utils has contradictory licence info Lp 1416436 * Juju restore no longer works with azure: error: cannot re-bootstrap environment: cannot determine state server instances: environment is not bootstrapped Lp 1417178 * Unit ports not populated by api megawatcher Lp 1418433 * Apt-proxy can be incorrectly set when the fallback from http-proxy is used Lp 1417617 Finally We encourage everyone to subscribe the mailing list at juju-...@lists.canonical.com, or join us on #juju-dev on freenode. -- Curtis Hovey Canonical Cloud Development and Operations http://launchpad.net/~sinzui -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
What are blocks ?
Hello, Can someone direct me to the design documents for the enigmatic 'block' functionality which is showing up in the review queue. Thanks Dave -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
