Re: State should not depend on the API server
On Thu, Sep 11, 2014 at 3:35 PM, Nate Finch nate.fi...@canonical.com wrote: I don't see how having different identical structs that are updated simultaneously in any way prevents any problems with compatibility. If we're updating those structs simultaneously, we're completely missing the point. Once we've defined a pure-data struct that might be persisted or sent over the wire we *must not change that struct* -- if we want to send or store different data, we need a new struct. Maybe I'm missing something from the proposal, but it seems like this just adds busywork without actually solving any problems that weren't caused by incorrect use of the code in the first place. Isn't that tautological? AFAICS, storing a charm.Meta (or a params.anything) directly in the DB *is* incorrect use of the code, but nobody realises that until it's too late: that is the problem, and that's what we're addressing. Separation of logic is absolutely a good thing. Separation of data is not nearly so useful. It's harder than you might think to separate the data from the logic that acts on it ;). Cheers William -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: State should not depend on the API server
On Tue, Sep 16, 2014 at 7:59 PM, William Reade william.re...@canonical.com wrote: On Thu, Sep 11, 2014 at 3:35 PM, Nate Finch nate.fi...@canonical.com wrote: I don't see how having different identical structs that are updated simultaneously in any way prevents any problems with compatibility. If we're updating those structs simultaneously, we're completely missing the point. Once we've defined a pure-data struct that might be persisted or sent over the wire we *must not change that struct* -- if we want to send or store different data, we need a new struct. Maybe I'm missing something from the proposal, but it seems like this just adds busywork without actually solving any problems that weren't caused by incorrect use of the code in the first place. Isn't that tautological? AFAICS, storing a charm.Meta (or a params.anything) directly in the DB *is* incorrect use of the code, but nobody realises that until it's too late: that is the problem, and that's what we're addressing. For example, apiserver/params.StateServingInfo was being stored directly in the database. Woe to the person that updates the api only to discover that data was silently deleted from the state database :( Separation of logic is absolutely a good thing. Separation of data is not nearly so useful. It's harder than you might think to separate the data from the logic that acts on it ;). Cheers William -- 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: State should not depend on the API server
On 16/09/14 19:19, roger peppe wrote: On 16 September 2014 02:12, Tim Penhey tim.pen...@canonical.com wrote: On 12/09/14 01:35, Nate Finch wrote: Separation of logic is absolutely a good thing. Separation of data is not nearly so useful. What I see as the real benefit of this work is based behind the interface segregation principle. Effectively this boils down to don't depend on things you don't need. I agree with this sentiment. Dependency hygiene is very important (though it seems to me that dependencies are perhaps more about implementation than interface). The state package should never depend on packages from the API. This work is not just busy work, but clear separation, and generally what is considered good software development principles. I have difficulty with this though. The state package and the API package are clearly linked - their concerns are not entirely separate. In my view, the state package exists only to serve the purposes of the API, which is *the* externally visible part of Juju. There's a few issues here. The state package as it exists today is a mix of Juju domain business logic and a persistence layer, unfortunately intertwined. What should be the externally visible part of Juju is a service oriented abstraction with coarse grained business methods; we have this with the facades. What comes into those facades over the wire in terms of parameters should be transformed into domain objects for use by the service business logic called by the facade endpoints. On the wire data crosses a system boundary to enter/exit the business services layer and so the data needs to be transformed to avoid unnecessary coupling. As an example, a machine reference comes in over the wire as a machine tag, which is transformed to a machine id for use by the services layer, which us transformed to a global key when passed to the persistence layer. Thus the api params structs contain attributes that are syntactically distinct from what's required by the business services. It's a terrible abstraction leak to do otherwise. Dave has started the process of correcting the implementation issue we have, and in my view, it is necessary and desirable, based on sound design principals. IMO :-) As such, many of the concerns and data structures dealt with by the state package will be the same as those dealt with by the API implementation. This I disagree with - see above. The data structures should not be the same. -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: State should not depend on the API server
On 12/09/14 01:35, Nate Finch wrote: Separation of logic is absolutely a good thing. Separation of data is not nearly so useful. What I see as the real benefit of this work is based behind the interface segregation principle. Effectively this boils down to don't depend on things you don't need. The state package should never depend on packages from the API. This work is not just busy work, but clear separation, and generally what is considered good software development principles. Tim -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: State should not depend on the API server
On 11 September 2014 06:40, John Meinel j...@arbash-meinel.com wrote: ... I have thought for a while that, rather than writing more error-prone code (at 17k LOC, surely the API code is big enough as it is?), it would be good to create a tool that helps us with the underlying problem - incompatible changes made to marshaled types. This would not be too hard - the Go language already has such a tool (see http://golang.org/cmd/api/ ), although it is not currently generally applicable. This could make it possible to provide a much stronger assurance about compatibility, including compatibility between types not defined inside juju-core itself, while keeping the code natural and simple. cheers, rog. So that is interesting, and it might be fruitful to explore. I'll just note that it is actually a pretty small (and reasonably obvious) part of API compatibility. It *doesn't* tell you 1) If passing an empty string suddenly has a different meaning (see Client.ServiceSet where we used to treat as meaning reset-to-default vs Client.NewServiceSetForAPI where we wanted to actually treat as meaning set this value to the empty string.) 2) Returning a new value (Machine.Jobs(), we want to add new Jobs for new behaviors and don't want to confuse old agents until they have finished upgrading). Yes, I agree. There are aspects of compatibility that cannot be checked automatically, and we always need to be careful when changing semantics. I don't think there's any way around that. So while something like the above could tell you has the shape of the API changed, and what I really like about it is that if it had a collapsed view (like a simple text representation), it is easy to keep that around in the source tree and use it for reference. Yes, that's a nice thing about the Go api command, which does use a simple text representation. However, you really do need tests for (1) and (2). (1) we actually broke accidentally because we changed the code inside of state/*, and then had to go back and retrofit the API to use a new code path to preserve compatibility. Yup. And if you need tests, you really still need 2 types to represent the different versions, and if you already need those types, then I would recommend having a regular structure for them. Here's where I don't agree. I don't think, for example, that it's worth creating a new type just because its Jobs field can take on a value that it could not previously have. We might need a new version of the API, sure (although even that might be arguable in this case - agents just ignore unknown job types), but defining a new type seems like overkill to me. Even when we do need to make a new type, I don't really see why having a different type at each level will help us. One will probably need to make the new type at every level too, so having the differently defined types isn't a great benefit over having a single changed type. There will of course be cases when we *do* want different types at different levels, but that's easy to do on a case-by-case basis. I suggest using appropriate types at the different levels *as required by those levels*, rather than expending developer time and generating extra runtime garbage by copying identical representations of the same data around for every single type at every level. 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: State should not depend on the API server
I have been in situations in past jobs where adding one field to one logical entity required changing the signature of a half dozen structures functions that were just copying data back and forth for little to no benefit. This is a pain in the ass, error prone, and should be avoided. One of the things I like best about Go is that structs can be just data. How you interact with the data can be different from layer to layer, but there's no reason why we need different structs with the same layout in different packages just for the illusion of separation of concerns. If you modify one, you have to modify all of them, thus you might as well just have one they all use. At least then it's clear which places in the code depend on this logical data structure, rather than having to try to grep for everywhere we copy the data from one structure to another. This is why having the api/params leaf package is a good idea - it's just data. Any package can use those structs to layout their data in a way that other packages can understand. I don't see how having different identical structs that are updated simultaneously in any way prevents any problems with compatibility. If they're different structs, they cannot possibly actually represent the same data on both sides of the wire, can they? One or both of the structs must have information that is not present on the wire and that information should be in a different type (which may embed the wire-data-struct). Maybe I'm missing something from the proposal, but it seems like this just adds busywork without actually solving any problems that weren't caused by incorrect use of the code in the first place. Separation of logic is absolutely a good thing. Separation of data is not nearly so useful. On Thu, Sep 11, 2014 at 3:46 AM, roger peppe roger.pe...@canonical.com wrote: On 11 September 2014 06:40, John Meinel j...@arbash-meinel.com wrote: ... I have thought for a while that, rather than writing more error-prone code (at 17k LOC, surely the API code is big enough as it is?), it would be good to create a tool that helps us with the underlying problem - incompatible changes made to marshaled types. This would not be too hard - the Go language already has such a tool (see http://golang.org/cmd/api/ ), although it is not currently generally applicable. This could make it possible to provide a much stronger assurance about compatibility, including compatibility between types not defined inside juju-core itself, while keeping the code natural and simple. cheers, rog. So that is interesting, and it might be fruitful to explore. I'll just note that it is actually a pretty small (and reasonably obvious) part of API compatibility. It *doesn't* tell you 1) If passing an empty string suddenly has a different meaning (see Client.ServiceSet where we used to treat as meaning reset-to-default vs Client.NewServiceSetForAPI where we wanted to actually treat as meaning set this value to the empty string.) 2) Returning a new value (Machine.Jobs(), we want to add new Jobs for new behaviors and don't want to confuse old agents until they have finished upgrading). Yes, I agree. There are aspects of compatibility that cannot be checked automatically, and we always need to be careful when changing semantics. I don't think there's any way around that. So while something like the above could tell you has the shape of the API changed, and what I really like about it is that if it had a collapsed view (like a simple text representation), it is easy to keep that around in the source tree and use it for reference. Yes, that's a nice thing about the Go api command, which does use a simple text representation. However, you really do need tests for (1) and (2). (1) we actually broke accidentally because we changed the code inside of state/*, and then had to go back and retrofit the API to use a new code path to preserve compatibility. Yup. And if you need tests, you really still need 2 types to represent the different versions, and if you already need those types, then I would recommend having a regular structure for them. Here's where I don't agree. I don't think, for example, that it's worth creating a new type just because its Jobs field can take on a value that it could not previously have. We might need a new version of the API, sure (although even that might be arguable in this case - agents just ignore unknown job types), but defining a new type seems like overkill to me. Even when we do need to make a new type, I don't really see why having a different type at each level will help us. One will probably need to make the new type at every level too, so having the differently defined types isn't a great benefit over having a single changed type. There will of course be cases when we *do* want different types at different levels, but that's easy to do on a
Re: State should not depend on the API server
... I have thought for a while that, rather than writing more error-prone code (at 17k LOC, surely the API code is big enough as it is?), it would be good to create a tool that helps us with the underlying problem - incompatible changes made to marshaled types. This would not be too hard - the Go language already has such a tool (see http://golang.org/cmd/api/ ), although it is not currently generally applicable. This could make it possible to provide a much stronger assurance about compatibility, including compatibility between types not defined inside juju-core itself, while keeping the code natural and simple. cheers, rog. So that is interesting, and it might be fruitful to explore. I'll just note that it is actually a pretty small (and reasonably obvious) part of API compatibility. It *doesn't* tell you 1) If passing an empty string suddenly has a different meaning (see Client.ServiceSet where we used to treat as meaning reset-to-default vs Client.NewServiceSetForAPI where we wanted to actually treat as meaning set this value to the empty string.) 2) Returning a new value (Machine.Jobs(), we want to add new Jobs for new behaviors and don't want to confuse old agents until they have finished upgrading). So while something like the above could tell you has the shape of the API changed, and what I really like about it is that if it had a collapsed view (like a simple text representation), it is easy to keep that around in the source tree and use it for reference. However, you really do need tests for (1) and (2). (1) we actually broke accidentally because we changed the code inside of state/*, and then had to go back and retrofit the API to use a new code path to preserve compatibility. And if you need tests, you really still need 2 types to represent the different versions, and if you already need those types, then I would recommend having a regular structure for them. John =:- -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: State should not depend on the API server
On 1 September 2014 06:40, David Cheney david.che...@canonical.com wrote: Hello, This is an introductory email to explain my upcoming series of pull requests. The theme is simple: state must not depend on the api server. This is currently not true, state depends on api/params because some of the api types have leaked into the state package and are being directly stored in mongo. Please speak up if you disagree with this proposal. Sorry for not replying earlier. I'm just back from a week's holiday. I'm not sure I agree with the thrust of this proposal. The api/params package is not the API server - it's a leaf package defining types that may be used by the API server. Thus state does not in fact depend on the api server. Furthermore, I am not convinced that redefining the same types in different places is a great help. It means there's more maintenance, and more chance of getting things wrong when writing the code to convert between them. Go makes it easy to define a single type that defines the marshaling format for both client and server sides. This seems to me to be a Good Thing. Within a single code base, surely a given version of the API can define the parameters for the client and server side in the same way, for example? What we really want to do here, IMHO, is guard against incompatible changes made to types that are persistently used, whether that's because they're stored in mongo or used to communicate with other versions of the API server. I have thought for a while that, rather than writing more error-prone code (at 17k LOC, surely the API code is big enough as it is?), it would be good to create a tool that helps us with the underlying problem - incompatible changes made to marshaled types. This would not be too hard - the Go language already has such a tool (see http://golang.org/cmd/api/ ), although it is not currently generally applicable. This could make it possible to provide a much stronger assurance about compatibility, including compatibility between types not defined inside juju-core itself, while keeping the code natural and simple. 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: State should not depend on the API server
On Mon, Sep 1, 2014 at 12:03 AM, John Meinel j...@arbash-meinel.com wrote: FWIW I'd favor 3 layers, though it does mean you have to do copying between structs that would likely otherwise be almost identical. A State layer for saving in the DB, an API layer for communication, and a Model layer for use in the Agents/Clients. This is pretty much what I've done for the new backups in response to conversations with Menno and William. It's worked out fine. The 3 types are similar, but in each case have been slightly different. So the choice of having them different has proved helpful. -eric -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: State should not depend on the API server
FWIW I'd favor 3 layers, though it does mean you have to do copying between structs that would likely otherwise be almost identical. A State layer for saving in the DB, an API layer for communication, and a Model layer for use in the Agents/Clients. John =:- On Mon, Sep 1, 2014 at 9:40 AM, David Cheney david.che...@canonical.com wrote: Hello, This is an introductory email to explain my upcoming series of pull requests. The theme is simple: state must not depend on the api server. This is currently not true, state depends on api/params because some of the api types have leaked into the state package and are being directly stored in mongo. Please speak up if you disagree with this proposal. Dave -- 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: State should not depend on the API server
The goal I have been tasked with is breaking the interdependency between the state package and the _client_ api types in state/api/params. I don't have any opinion on adding additional layers assuming that their dependancies flow downwards, ie labix.org/mgo - juju/state - juju/apiserver On Mon, Sep 1, 2014 at 4:03 PM, John Meinel j...@arbash-meinel.com wrote: FWIW I'd favor 3 layers, though it does mean you have to do copying between structs that would likely otherwise be almost identical. A State layer for saving in the DB, an API layer for communication, and a Model layer for use in the Agents/Clients. John =:- On Mon, Sep 1, 2014 at 9:40 AM, David Cheney david.che...@canonical.com wrote: Hello, This is an introductory email to explain my upcoming series of pull requests. The theme is simple: state must not depend on the api server. This is currently not true, state depends on api/params because some of the api types have leaked into the state package and are being directly stored in mongo. Please speak up if you disagree with this proposal. Dave -- 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: State should not depend on the API server
On 01/09/14 16:03, John Meinel wrote: FWIW I'd favor 3 layers, though it does mean you have to do copying between structs that would likely otherwise be almost identical. A State layer for saving in the DB, an API layer for communication, and a Model layer for use in the Agents/Clients. This is my wish too. A sane, layered architecture, devoid of inappropriate coupling between layers, and a proper separation of concerns; the result of which will lead to, amongst other things, much better and faster, true unit tests. Well, there's a bit more that needs fixing first, eg more IOC, less use of concrete structs etc, but our current woes with unreliable tests is directly attributable to the fact that we often need to bring up the entire stack to test a component. So any progress towards fixing things, in this case eliminating unnecessary and inappropriate coupling between state and api server is much appreciated. John =:- On Mon, Sep 1, 2014 at 9:40 AM, David Cheney david.che...@canonical.com wrote: Hello, This is an introductory email to explain my upcoming series of pull requests. The theme is simple: state must not depend on the api server. This is currently not true, state depends on api/params because some of the api types have leaked into the state package and are being directly stored in mongo. Please speak up if you disagree with this proposal. Dave -- 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: State should not depend on the API server
On Mon, Sep 1, 2014 at 8:03 AM, John Meinel j...@arbash-meinel.com wrote: FWIW I'd favor 3 layers, though it does mean you have to do copying between structs that would likely otherwise be almost identical. A State layer for saving in the DB, an API layer for communication, and a Model layer for use in the Agents/Clients. Yes please, in general: one set of types per layer boundary. Independent of what Dave's doing, which is necessary regardless, I agree with what you're saying: *except* that I think we really have to consider the API layer to be *two* layers. That is to say: if you can change some type in api/params and everything still works, you are Doing It Wrong. We cannot depend on servers and clients always running the same version -- so, every time you thus change server/client in a single motion, you're almost certainly introducing more or less subtle incompatibilities. So, I would be very pleased if we would stop using the same definitions (ie api/params) on both sides of the wire -- it's one of those things that's nice and easy when everyone's running the same version, but an active trap as soon as multiple versions exist (as they do). All the struct-copying is kinda boring, but the layering violations are straight-up evil. Dave, thanks very much for doing this. Cheers William -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
Re: State should not depend on the API server
... So, I would be very pleased if we would stop using the same definitions (ie api/params) on both sides of the wire -- it's one of those things that's nice and easy when everyone's running the same version, but an active trap as soon as multiple versions exist (as they do). All the struct-copying is kinda boring, but the layering violations are straight-up evil. Yeah, we've started running into this a bit as we get into actually revving versions in the API (as yet, we haven't actually bumped an api version though all the infrastructure should be there.) John =:- Dave, thanks very much for doing this. Cheers William -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
State should not depend on the API server
Hello, This is an introductory email to explain my upcoming series of pull requests. The theme is simple: state must not depend on the api server. This is currently not true, state depends on api/params because some of the api types have leaked into the state package and are being directly stored in mongo. Please speak up if you disagree with this proposal. Dave -- Juju-dev mailing list Juju-dev@lists.ubuntu.com Modify settings or unsubscribe at: https://lists.ubuntu.com/mailman/listinfo/juju-dev
