Issue 5541054: Subordinate Services Spec (Closed)
DescriptionBased on input from the week of the rally here are the proposed changes to the
subordinates specification.
This (final) revision contains two storage containment models that differ only in that container relations
nest the traditional ZK relation structure under the id of the container its child nodes interact under.
https://code.launchpad.net/~bcsaller/juju/subordinate-spec/+merge/88515
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 51
Patch Set 2 : Subordinate Services Spec #
Total comments: 82
Patch Set 3 : Subordinate Services Spec #
Total comments: 14
Patch Set 4 : Subordinate Services Spec #
Total comments: 12
Patch Set 5 : Subordinate Services Spec #Patch Set 6 : Subordinate Services Spec #
Total comments: 15
Patch Set 7 : Subordinate Services Spec #
Total comments: 4
Patch Set 8 : Subordinate Services Spec #
Total comments: 8
Patch Set 9 : Subordinate Services Spec #
Total comments: 12
Patch Set 10 : Subordinate Services Spec #
MessagesTotal messages: 29
Please take a look.
Sign in to reply to this message.
a few initial minor comments on the implicit-interfaces doc. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... File docs/source/drafts/implicit-interfaces.rst (right): https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:4: Juju has the ability to provide specialized interfaces to any service The meaning of this sentence is unclear to me. How about this? "Every juju service provides an implicit, specialized, interface to any service attempting to relate to it. It requires no specific declarations on the part of the charm author." Although I don't quite understand why this interface is more "specialized" than any other. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:6: on the part of the charm author. Implicit interface allow for s/interface/interfaces/ https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:7: interested services to gather broad lifecycle oriented events and very s/lifecycle oriented/lifecycle-oriented/ i think! https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:8: general data about other services with out expecting or requiring any s/with out/without/ https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:20: `juju-info` provides name, unit addresses an allows for traditional s/name, unit addresses an /name and unit addresses, and/
Sign in to reply to this message.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... File docs/source/drafts/implicit-interfaces.rst (right): https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:4: Juju has the ability to provide specialized interfaces to any service On 2012/01/13 15:58:22, rog wrote: Removed the first couple sentences based on verbal feedback from Gustavo. > The meaning of this sentence is unclear to me. > How about this? > "Every juju service provides an implicit, specialized, interface to any service > attempting to relate to it. It requires no specific declarations on the part of > the charm author." > > Although I don't quite understand why this interface is more "specialized" than > any other. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:6: on the part of the charm author. Implicit interface allow for On 2012/01/13 15:58:22, rog wrote: > s/interface/interfaces/ Done. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:7: interested services to gather broad lifecycle oriented events and very On 2012/01/13 15:58:22, rog wrote: > s/lifecycle oriented/lifecycle-oriented/ > i think! Done. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:8: general data about other services with out expecting or requiring any On 2012/01/13 15:58:22, rog wrote: > s/with out/without/ Done. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:20: `juju-info` provides name, unit addresses an allows for traditional On 2012/01/13 15:58:22, rog wrote: > s/name, unit addresses an /name and unit addresses, and/ Done.
Sign in to reply to this message.
Looks good, the implicit-interfaces doc could use a once over, and i wouldn't mind chatting about some of the implementation details as regards the persistence of the subordinate relation, specifically the topology modifications and the persistence of the 1-1 unit association.. Also i didn't see any docs regarding the physical zk layout of the new relation type. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... File docs/source/drafts/implicit-interfaces.rst (right): https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:4: Juju has the ability to provide specialized interfaces to any service Seems like this could use a nicer intro sentence(s). ie. "Juju services normally only allow for relations based on their charm declared interfaces. Juju also provides for certain specialized interfaces automatically given to any service." https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:7: interested services to gather broad lifecycle oriented events and very i'd drop some of the adjectives here. s/broad/ and s/very general/generic https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:9: participation on the part of the providing charm author. maybe s/participation/work or perhaps more aptly categorized as 'charm modification' https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:12: the arrival of this feature. Charms attempting to provide new present tense, not future participle, Juju reserves .. s/with the arrival of this/for implicit interfaces. s/provide new/provide https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:14: possible to augment these interfaces in a general way through the I'd drop possible to augment in a general way, more concrete less abstract... ie. A charm however may choose to implement hooks for these implicit interfaces to customize the charms behavior using standard hook mechanisms. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:18: available from all services. s/At the time of this writing. Juju currently provides one implicit interface to all deployed services. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:23: default implentation of the provides side. Specify the values provided/contained by the juju-info relation data. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:28: was explicitly requested. intro The role of the ``juju-info`` implicit interface will adapt dynamically at establishment time based on the expectations of the joined services. For example.. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... File docs/source/drafts/subordinate-internals.rst (right): https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-internals.rst:29: file. The user doc noted this was also a required interface, is that the case? https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-internals.rst:54: this looks strange, why are public-addresses being stored in the topology? i thought we walked this back to a new relation type as stated earlier in this doc. its not clear why the related units are being stored here, is that to materialize the 1-1 rel for the unit instances? Couldn't that be materialized directly on the unit state. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-internals.rst:104: update the mapping using the unit_name of the principal. This paragraph is a little unclear. Its also unclear why the mapping would best be suited to the topology. The topology stores a user defined state, the association and creation of subordinate units is going to happen both at rel def time, but also within the scope of add_unit_state, and actualized by the unit itself, so storing it on the unit state or a contained node seems more appropriate. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-internals.rst:127: `juju add-relation` and `juju remove-relation` nust trigger the s/nust/must https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-internals.rst:129: watch machinery outlined above. The principal service will deploy a s/A principal service unit https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-internals.rst:149: we create a relationship of the new `subordinate-principal` type. why is it called subordinate-principal instead of just subordinate https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... File docs/source/drafts/subordinate-services.rst (right): https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-services.rst:7: machine with no knowledge of other services deployed onto the same knowledge or access https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-services.rst:36: Nice intro. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-services.rst:76: unit (using the new `subordinate-principal` relationship type). A s/proper.. re unit and subordinate unit, i'd add a note that this is as opposed to a standard relation which is defined and actuated between services. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-services.rst:84: Subordinate services imply changes to how we actualize relations tense.. imply changes from what?... perhaps make the distinction against standard relations. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-services.rst:123: Charm Changes s/Charm Changes/Declaring Subordinate Charms https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-services.rst:128: metadata is required. Declaring a required interface with `container: if the label is subordinate for all the docs, sounds like it should just be 'subordinate' for the interface metadata as well instead of introducing a new vocab word 'container' https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-services.rst:137: Implications for Status tense ..s/Impliciations for Status/Subordinate status output https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-services.rst:216: flag. This information looks like it should go into the internals docs, its not really at the level of end user concern.
Sign in to reply to this message.
ptal https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... File docs/source/drafts/implicit-interfaces.rst (right): https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:7: interested services to gather broad lifecycle oriented events and very On 2012/01/18 18:32:15, hazmat wrote: > i'd drop some of the adjectives here. s/broad/ > and s/very general/generic Done. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:12: the arrival of this feature. Charms attempting to provide new On 2012/01/18 18:32:15, hazmat wrote: > present tense, not future participle, Juju reserves .. > > s/with the arrival of this/for implicit interfaces. > > s/provide new/provide Done. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:14: possible to augment these interfaces in a general way through the On 2012/01/18 18:32:15, hazmat wrote: > I'd drop possible to augment in a general way, more concrete less abstract... > ie. > > A charm however may choose to implement hooks for these implicit interfaces to > customize the charms behavior using standard hook mechanisms. Done. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:18: available from all services. On 2012/01/18 18:32:15, hazmat wrote: > s/At the time of this writing. > > Juju currently provides one implicit interface to all deployed services. Done. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:28: was explicitly requested. On 2012/01/18 18:32:15, hazmat wrote: > intro The role of the ``juju-info`` implicit interface will adapt dynamically at > establishment time based on the expectations of the joined services. For > example.. My wording here might still be a little off, but I did update it https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... File docs/source/drafts/subordinate-internals.rst (right): https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-internals.rst:29: file. On 2012/01/18 18:32:15, hazmat wrote: > The user doc noted this was also a required interface, is that the case? Yes, I've expanded this section to include both that information and an example interface declaration. Likewise the example is included in the user-oriented docs https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-internals.rst:54: On 2012/01/18 18:32:15, hazmat wrote: > this looks strange, why are public-addresses being stored in the topology? i > thought we walked this back to a new relation type as stated earlier in this > doc. its not clear why the related units are being stored here, is that to > materialize the 1-1 rel for the unit instances? Couldn't that be materialized > directly on the unit state. Yes, this was an error, it should have read private-address, as per indicated in ServiceRelationState.add_unit_state where private address is currently stored. It seemed this was a reasonable place to keep the 1-1 mapping as well. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-internals.rst:104: update the mapping using the unit_name of the principal. On 2012/01/18 18:32:15, hazmat wrote: > This paragraph is a little unclear. Its also unclear why the mapping would best > be suited to the topology. The topology stores a user defined state, the > association and creation of subordinate units is going to happen both at rel def > time, but also within the scope of add_unit_state, and actualized by the unit > itself, so storing it on the unit state or a contained node seems more > appropriate. This section doesn't refer to the topology, only the state objects which can manipulate either the normal ZK nodes or the topology. In this case I'd only thought to explore how ZK's top level /relations tree is managed in such a way as to make the 1-1 watches simpler. The title on a previous section did refer to the topology though and that has been changed to "ZooKeeper State". That said its still very possible these two sections are incorrect and with the additional clarity(?) I've added I hope you can again provide feedback. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-internals.rst:127: `juju add-relation` and `juju remove-relation` nust trigger the On 2012/01/18 18:32:15, hazmat wrote: > s/nust/must Done. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-internals.rst:129: watch machinery outlined above. The principal service will deploy a On 2012/01/18 18:32:15, hazmat wrote: > s/A principal service unit Done. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-internals.rst:149: we create a relationship of the new `subordinate-principal` type. On 2012/01/18 18:32:15, hazmat wrote: > why is it called subordinate-principal instead of just subordinate in reference to client-server, the endpoints then have the roles of subordinate and principal. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... File docs/source/drafts/subordinate-services.rst (right): https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-services.rst:7: machine with no knowledge of other services deployed onto the same On 2012/01/18 18:32:15, hazmat wrote: > knowledge or access Done. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-services.rst:36: On 2012/01/18 18:32:15, hazmat wrote: > Nice intro. Thanks :) https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-services.rst:84: Subordinate services imply changes to how we actualize relations On 2012/01/18 18:32:15, hazmat wrote: > tense.. imply changes from what?... perhaps make the distinction against > standard relations. I think I've improved this https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-services.rst:123: Charm Changes On 2012/01/18 18:32:15, hazmat wrote: > s/Charm Changes/Declaring Subordinate Charms Done. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-services.rst:128: metadata is required. Declaring a required interface with `container: On 2012/01/18 18:32:15, hazmat wrote: > if the label is subordinate for all the docs, sounds like it should just be > 'subordinate' for the interface metadata as well instead of introducing a new > vocab word 'container' The thought here was that the edge case of two subordinate services talking to the same principal might later need a relationship between themselves that is only actualized within the same container. They wouldn't be subordinate to one another. This is why I advocated scope: container before and I still like that spelling. It seems more future proof. The spelling here was change based on feedback from Gustavo, though given this current version of things he might agree now that subordinate: true is better. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-services.rst:137: Implications for Status On 2012/01/18 18:32:15, hazmat wrote: > tense ..s/Impliciations for Status/Subordinate status output Done. https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/subordinate-... docs/source/drafts/subordinate-services.rst:216: flag. On 2012/01/18 18:32:15, hazmat wrote: > This information looks like it should go into the internals docs, its not really > at the level of end user concern. Indeed, this is left over from the first run through the spec and attempted to capture the higher level changes in one place. Its replaced by the other doc and will be removed, sorry
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... File docs/source/drafts/implicit-interfaces.rst (right): https://codereview.appspot.com/5541054/diff/1/docs/source/drafts/implicit-int... docs/source/drafts/implicit-interfaces.rst:6: on the part of the charm author. Implicit interface allow for I suggest starting the sentence with "Implicit interfaces ...". https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-... File docs/source/drafts/implicit-interfaces.rst (right): https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-... docs/source/drafts/implicit-interfaces.rst:7: author. s/author./author of the other service./ https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-... docs/source/drafts/implicit-interfaces.rst:10: to provide new interfaces in this namespace will trigger an error. A To put this sentence in context it should look like this: """ Implicit interfaces are named after the juju-* reserved namespace, which must be used only as specified. """ https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-... docs/source/drafts/implicit-interfaces.rst:13: mechanisms. Please drop this sentence. It's not clear what this is really saying, and it doesn't look necessary. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-... docs/source/drafts/implicit-interfaces.rst:21: specify the values provided by the juju-info relation data. Please rephrase: The `juju-info` relation, implicitly provided by all charms, enables the requiring unit to obtain basic details about the related-to unit. The following settings will be implicitly provided by the remote unit in a relation with a `juju-info` interface: <fill it in> https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-... docs/source/drafts/implicit-interfaces.rst:27: if the remote role were `client`. This shouldn't be in user-facing documentation. These are implementation details that we don't talk about, and we probably shouldn't because it's very confusing without the proper background. What's a server/client here? Why isn't subordinate a client since it still "requires"? I suggest dropping that. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... File docs/source/drafts/subordinate-internals.rst (right): https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-internals.rst:15: communication to only service units in the same container. The container relation type is not specific to principal and subordinate. It should be possible to establish a container relation between two subordinate services that live in the same container too. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-internals.rst:19: principals and subordinate units. The new relation type is constructed This is too vague to be understandable at this stage, and too specific to be part of the Overview section. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-internals.rst:32: satisfied. :: Please drop this from this file. This doesn't qualify as "internals", and is already described in detail in the high-level documented referred to in the introduction. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-internals.rst:45: units in the same container is efficient. :: I don't think we need an entirely new relation type to handle the use case, and we don't even need changes in the ZooKeeper storage format. Container relations are still client/server in the sense that we have two sides: a providing one, and a requiring one. They should also work between two subordinate services that sit within the same container, as described above. The only difference is really that we should only fire events across well defined pairs, and we can tell which pairs these are just by looking at the topology (e.g. in the same way you can tell which services are under which service when you render the status). https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-internals.rst:71: lifecycle. That's not necessarily the case. If two subordinate services are linked together with a container relation, they shouldn't cause the service to be deployed. What causes the deployment is the knowledge of where to deploy the service in, and this may occur either by linking a subordinate with a principal, or by linking a subordinate with another subordinate that already has a principal. When the subordinate is linked with a principal, it should also check if any other subordinates are linked to it and should be deployed together as well. It should be an error to attempt to link any of the subordinates in the group to two different principal services. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-internals.rst:75: subordinate units in the same container. This should indeed only fire when appropriate, but there's no need for a new relation type. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-internals.rst:82: satisfied. Must be adapted. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-internals.rst:95: running it, i.e, that of the principal's service unit. "inherit" here is making things less clear than they could be. There's no such thing as an inheritance of settings. What is happening in practice, and why? https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-internals.rst:102: of. Sounds good. It should probably be an error to attempt to assign a machine to a subordinate service then. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-internals.rst:111: update the mapping using the unit_name of the principal. I won't review the Python-related changes in detail, but this must be adapted as well to consider the previous comments. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-internals.rst:117: subordinate. Ditto. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-internals.rst:138: relationship is added. Ditto. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-internals.rst:148: ------------------- This section feels a bit confusing given what's actually going on. There's nothing special about implicit interfaces in this context. I suggest dropping it. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-internals.rst:154: fallback to an implicit `juju-info` interface. When we detect that we No fallback seems necessary in relation matching. The implementation of juju-info should put it as a normal interface in all services, and consuming that interface by a subordinate service should be done explicitly. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-internals.rst:161: this can be added at a later time however. The existence of juju-info and subordinate services is orthogonal. If you're doing something that requires further action down the road, it's probably not being implemented correctly. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... File docs/source/drafts/subordinate-services.rst (right): https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:10: of each other. This sentence is very good. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:14: ----------------- Please realign the underlining of all headers with the text. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:19: subordinate to their service. This sentence reads very poorly. I suggest dropping it entirely. The next sentence explains the same idea more clearly. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:25: interface pairing. Requiring specified relationships implies that s/interface/relation/ https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:31: Please remove one of the empty lines here. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:42: A traditional charm/ service in whose container subordinate charm/ service => service or charm https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:46: A service designed for and deployed to the running container of s/service/service or charm/ https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:47: another service. s/service/service unit/ https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:49: Subordinate Relation s/Rel/rel/, since the other two are lowercase too. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:52: traditional relationships juju only implements the relationship s/juju/, juju/ https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:78: relations established with any unit in its environment as usual. This section is problematic. You have to keep in mind that thus far the user never heard about subordinate units before. He's got no idea about how to tag such a unit as a subordinate unit, and doesn't know what "subordinate-principal" relationship type is either. To the user, you just said "Here is a subordinate deployment", and showed commands that he's used to but that just because you say, it's doing something magic that he doesn't understand. Which of those services/charm are subordinate? Why are you using the same command set? Why and how does it work differently? I suggest moving that to the end of this file, and removing the "subordinate-principal relationship type" mention altogether (the term doesn't exist in this file). https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:96: traditional way. It is actually not different from traditional relationships in that way, because "this" is a description of how subordinate services are deployed, which works fine with traditional relationships too. Please use this in place of the above paragraph: """ When a traditional relation is added between two services, all the service units for the first service will receive relation events about all service units for the second service. Subordinate services have a very tight relationship with their principal service, so it makes sense to be able to restrict that communication in some cases so that they only receive events about each other. That's precisely what happens when a relation is tagged as being a scoped for the container. """ https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:98: Subordinate relations exist because they simplify responsibilities for s/Subordinate/Container/ https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:103: properly (and the defining need for subordinate services). Please drop the sentence starting in "This change". It's not clear what it means, and it's not necessary. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:109: aware of the connection. I don't see why that'd be the case. It could just ask the principal service what state it is in through the container relation. It'd be more clear with something like: """ If a subordinate service needs to communicate with all units of the principal service, it can still establish a traditional (non-container) relationship to it. """ https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:122: principal defines the network setup. Please drop the sentence starting in "This works because". It will work because it's implemented properly, but we don't have to tell all the small details about how we did it in user-facing documentation. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:132: in a subordinate deployment. Subordinate services can still declare There's a mismatch here. Your argument to have *container* relations (which I agreed with) was that they are a more general concept. But here you're saying that the way to mark a subordinate service as such is simply by adding a container relation. This feels improper. If container relations are a concept on their own, they should stand on their own in the declaration too, and we should mark a subordinate service as such by having a top-level "subordinate: true" setting. Otherwise you're restricting the existence of container relations between principal and subordinate. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:135: satisfied. Please reword the sentence starting in "Subordinate services" as: """ Subordinate services may still declare traditional relations to any service. The deployment is delayed until a container relation is added. """ https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:137: The example below shows adding a contained relationship to a charm. :: s/contained relationship/container relation/ https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:149: Status reports subordinates under each existing `units` entry of each Please reword as: """ The status output contains details about subordinate units under the status of the principal service unit that it is sharing the container with. """ https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:150: service. The subordinates unit output matches the formatting of s/subordinates/subordinate's/ https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:152: `subordinates` from its output (those are all inherited from the s/are all inherited from/are the same as/ https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:156: dictionary in an abbreviated form. The `subordinate: true` indicator Rather than subordinate: true, we can be more helpful by saying, e.g.: subordinate-to: [ wordpress ] It should be listed even if there are no services related to yet, as: subordinate-to: [] https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:158: relationships showing only any possible relations between this service Why? The relation information continues to be interesting, and it's very awkward to say "add-relation A B" and then see that A has an empty relation list. Let's please keep the relation value as usual for subordinate services. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:161: unit names under which instances of this service are found. Let's please drop the sentence and the described content from the status. Imagine how this will end up with: units: [wordpress/0, wordpress/1, ..., wordpress/42]. The same information is conveyed by the suggested: subordinate-to: [ wordpress ] https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:167: charm local:series/logging-1 Missing colon. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:168: subordinate: true subordinate-to: ... https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:170: units: [ wordpress/0] Let's drop that.
Sign in to reply to this message.
https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-... File docs/source/drafts/implicit-interfaces.rst (right): https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-... docs/source/drafts/implicit-interfaces.rst:7: author. On 2012/01/19 15:06:18, niemeyer wrote: > s/author./author of the other service./ Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-... docs/source/drafts/implicit-interfaces.rst:10: to provide new interfaces in this namespace will trigger an error. A On 2012/01/19 15:06:18, niemeyer wrote: > To put this sentence in context it should look like this: > > """ > Implicit interfaces are named after the juju-* reserved > namespace, which must be used only as specified. > """ Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-... docs/source/drafts/implicit-interfaces.rst:13: mechanisms. On 2012/01/19 15:06:18, niemeyer wrote: > Please drop this sentence. It's not clear what this is really saying, and it > doesn't look necessary. Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-... docs/source/drafts/implicit-interfaces.rst:21: specify the values provided by the juju-info relation data. On 2012/01/19 15:06:18, niemeyer wrote: > Please rephrase: > > The `juju-info` relation, implicitly provided by all charms, enables the > requiring unit to obtain basic details about the related-to unit. The following > settings will be implicitly provided by the remote unit in a relation with a > `juju-info` interface: > > <fill it in> Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/implicit-... docs/source/drafts/implicit-interfaces.rst:27: if the remote role were `client`. On 2012/01/19 15:06:18, niemeyer wrote: > This shouldn't be in user-facing documentation. These are implementation details > that we don't talk about, and we probably shouldn't because it's very confusing > without the proper background. What's a server/client here? Why isn't > subordinate a client since it still "requires"? I suggest dropping that. Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... File docs/source/drafts/subordinate-internals.rst (right): https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-internals.rst:148: ------------------- On 2012/01/19 15:06:18, niemeyer wrote: > This section feels a bit confusing given what's actually going on. There's > nothing special about implicit interfaces in this context. I suggest dropping > it. Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-internals.rst:161: this can be added at a later time however. On 2012/01/19 15:06:18, niemeyer wrote: > The existence of juju-info and subordinate services is orthogonal. If you're > doing something that requires further action down the road, it's probably not > being implemented correctly. We talked about this on G+ and came to some conclusions reflected in the updated specs. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... File docs/source/drafts/subordinate-services.rst (right): https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:14: ----------------- On 2012/01/19 15:06:18, niemeyer wrote: > Please realign the underlining of all headers with the text. Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:19: subordinate to their service. On 2012/01/19 15:06:18, niemeyer wrote: > This sentence reads very poorly. I suggest dropping it entirely. The next > sentence explains the same idea more clearly. Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:25: interface pairing. Requiring specified relationships implies that On 2012/01/19 15:06:18, niemeyer wrote: > s/interface/relation/ Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:31: On 2012/01/19 15:06:18, niemeyer wrote: > Please remove one of the empty lines here. Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:42: A traditional charm/ service in whose container subordinate On 2012/01/19 15:06:18, niemeyer wrote: > charm/ service => service or charm Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:46: A service designed for and deployed to the running container of On 2012/01/19 15:06:18, niemeyer wrote: > s/service/service or charm/ Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:47: another service. On 2012/01/19 15:06:18, niemeyer wrote: > s/service/service unit/ Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:49: Subordinate Relation On 2012/01/19 15:06:18, niemeyer wrote: > s/Rel/rel/, since the other two are lowercase too. Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:52: traditional relationships juju only implements the relationship On 2012/01/19 15:06:18, niemeyer wrote: > s/juju/, juju/ Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:78: relations established with any unit in its environment as usual. On 2012/01/19 15:06:18, niemeyer wrote: > This section is problematic. You have to keep in mind that thus far the user > never heard about subordinate units before. He's got no idea about how to tag > such a unit as a subordinate unit, and doesn't know what "subordinate-principal" > relationship type is either. To the user, you just said "Here is a subordinate > deployment", and showed commands that he's used to but that just because you > say, it's doing something magic that he doesn't understand. Which of those > services/charm are subordinate? Why are you using the same command set? Why and > how does it work differently? > > I suggest moving that to the end of this file, and removing the > "subordinate-principal relationship type" mention altogether (the term doesn't > exist in this file). Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:96: traditional way. On 2012/01/19 15:06:18, niemeyer wrote: > It is actually not different from traditional relationships in that way, because > "this" is a description of how subordinate services are deployed, which works > fine with traditional relationships too. > > Please use this in place of the above paragraph: > > """ > When a traditional relation is added between two services, > all the service units for the first service will receive > relation events about all service units for the second > service. Subordinate services have a very tight relationship > with their principal service, so it makes sense to be able > to restrict that communication in some cases so that they > only receive events about each other. That's precisely what > happens when a relation is tagged as being a scoped for the > container. > """ Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:109: aware of the connection. On 2012/01/19 15:06:18, niemeyer wrote: > I don't see why that'd be the case. It could just ask the principal service what > state it is in through the container relation. > > It'd be more clear with something like: > > """ > If a subordinate service needs to communicate with all > units of the principal service, it can still establish a > traditional (non-container) relationship to it. > """ Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:122: principal defines the network setup. On 2012/01/19 15:06:18, niemeyer wrote: > Please drop the sentence starting in "This works because". It will work because > it's implemented properly, but we don't have to tell all the small details about > how we did it in user-facing documentation. Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:132: in a subordinate deployment. Subordinate services can still declare On 2012/01/19 15:06:18, niemeyer wrote: > There's a mismatch here. Your argument to have *container* relations (which I > agreed with) was that they are a more general concept. But here you're saying > that the way to mark a subordinate service as such is simply by adding a > container relation. This feels improper. If container relations are a concept on > their own, they should stand on their own in the declaration too, and we should > mark a subordinate service as such by having a top-level "subordinate: true" > setting. Otherwise you're restricting the existence of container relations > between principal and subordinate. My explination depended on the notion of actually required relations, a required container relation was the same as a subordinate charm. Given that we are not attempting to implement proper required relations in this cycle adding subordinate true (or checking optional: False) on the relation in this specific case would both work. Preferences welcome. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:135: satisfied. On 2012/01/19 15:06:18, niemeyer wrote: > Please reword the sentence starting in "Subordinate services" as: > > """ > Subordinate services may still declare traditional > relations to any service. The deployment is delayed > until a container relation is added. > """ Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:137: The example below shows adding a contained relationship to a charm. :: On 2012/01/19 15:06:18, niemeyer wrote: > s/contained relationship/container relation/ How we update the example below, to include either subordinate: true at the top level or optional: False to the relation will change the example. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:149: Status reports subordinates under each existing `units` entry of each On 2012/01/19 15:06:18, niemeyer wrote: > Please reword as: > > """ > The status output contains details about subordinate units > under the status of the principal service unit that it is > sharing the container with. > """ Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:152: `subordinates` from its output (those are all inherited from the On 2012/01/19 15:06:18, niemeyer wrote: > s/are all inherited from/are the same as/ Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:156: dictionary in an abbreviated form. The `subordinate: true` indicator On 2012/01/19 15:06:18, niemeyer wrote: > Rather than subordinate: true, we can be more helpful by saying, e.g.: > > subordinate-to: [ wordpress ] > > It should be listed even if there are no services related to yet, as: > > subordinate-to: [] Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:158: relationships showing only any possible relations between this service On 2012/01/19 15:06:18, niemeyer wrote: > Why? The relation information continues to be interesting, and it's very awkward > to say "add-relation A B" and then see that A has an empty relation list. Let's > please keep the relation value as usual for subordinate services. Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:161: unit names under which instances of this service are found. On 2012/01/19 15:06:18, niemeyer wrote: > Let's please drop the sentence and the described content from the status. > Imagine how this will end up with: > > units: [wordpress/0, wordpress/1, ..., wordpress/42]. > > The same information is conveyed by the suggested: > > subordinate-to: [ wordpress ] Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:167: charm local:series/logging-1 On 2012/01/19 15:06:18, niemeyer wrote: > Missing colon. Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:168: subordinate: true On 2012/01/19 15:06:18, niemeyer wrote: > subordinate-to: ... Done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:170: units: [ wordpress/0] On 2012/01/19 15:06:18, niemeyer wrote: > Let's drop that. Done.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Good changes overall, but you've missed comments for a whole file in the review (the internals spec). Even then, to speed things up some further feedback on your current changes follows. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... File docs/source/drafts/subordinate-services.rst (right): https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:46: A service designed for and deployed to the running container of On 2012/01/22 14:27:41, bcsaller wrote: > On 2012/01/19 15:06:18, niemeyer wrote: > > s/service/service or charm/ > > Done. It wasn't actually done. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:132: in a subordinate deployment. Subordinate services can still declare > My explination depended on the notion of actually required relations, a required > container relation was the same as a subordinate charm. Given that we are not > attempting to implement proper required relations in this cycle adding > subordinate true (or checking optional: False) on the relation in this specific > case would both work. Preferences welcome. Even ignoring the fact that we can't write text that depends on required relations now, since they don't exist, required relations would still not solve our problem in this case. If a container relation is to be a generic concept, imagine someone decides to tag a relation as "scope: container" in the *principal* side. There's nothing wrong with that, and it's just a nice hint to the system that the principal service explicitly states that this relation only makes sense within the container, and it's not willing to talk to remotes about it. The principal shouldn't become a subordinate because it's got such a relation. The way to go is what someone (Kapil?) suggested earlier: subordinate: true At the top level. Explicit and unambiguous. If we find a "subordinate: true" without a "scope: container" relation, though, then we should error out because clearly that was a mistake. https://codereview.appspot.com/5541054/diff/9001/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:137: The example below shows adding a contained relationship to a charm. :: > How we update the example below, to include either subordinate: true at the top > level or optional: False to the relation will change the example. See comment above. https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-... File docs/source/drafts/implicit-relations.rst (right): https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-... docs/source/drafts/implicit-relations.rst:2: ------------------- Please realign the underlining. Also, please check all headers and replace them by "Phrase casing". IIRC, I did a pass a while ago to standardize on that format in all documentation, to avoid the several styles we had mixed up. https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-... docs/source/drafts/implicit-relations.rst:17: `juju-info`, specified as :: Let's clarify a bit: `juju-info`, which if specified (it must not be) would look like:: https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-... docs/source/drafts/implicit-relations.rst:28: private-address What about the public-address? https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-... docs/source/drafts/implicit-relations.rst:35: Support logging is a :doc:`subordinate charm<subordinate-services>` s/Support/Suppose/? Also, s/logging/rsyslog, to avoid mixing up relation name and charm name below. https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-... docs/source/drafts/implicit-relations.rst:54: juju add-relation wordpress logging s/logging/rsyslog/ https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-... docs/source/drafts/implicit-relations.rst:57: interface we can safely fallback to using the less-specific (in the s/we can safely/juju will/ https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/relation-... File docs/source/drafts/relation-scope.rst (right): https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/relation-... docs/source/drafts/relation-scope.rst:2: -------------- This doesn't look like a good fit for its own file. Where are we defining the rest of the relation fields? This should be placed there and formatted similarly to the other fields. https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/relation-... docs/source/drafts/relation-scope.rst:17: containers. See :doc:`subordinate-services`. This isn't entirely clear/correct. Relations are added between services. Service units have containers. Services themselves do not have containers. You can add a relation between a principal service and a subordinate service that currently have no units sharing containers, and that will cause the system to deploy the subordinate service unit onto the principal service unit's container. On the other hand, two principals cannot have container relationships established between them. I'm not sure how much of that needs to be explained here. I'd suggest keeping the explanation light, putting more details onto the spec itself, and referring to it from here as already being done. https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/subordina... File docs/source/drafts/subordinate-services.rst (right): https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:131: relations: {} This looks bogus. There's a relation in the example.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
forgot to mail this on the last lbox propose https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-... File docs/source/drafts/implicit-relations.rst (right): https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-... docs/source/drafts/implicit-relations.rst:28: private-address On 2012/01/23 14:49:41, niemeyer wrote: > What about the public-address? Included. I was copying from the ZK state but this is added in the protocol handler so I missed it. https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/relation-... File docs/source/drafts/relation-scope.rst (right): https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/relation-... docs/source/drafts/relation-scope.rst:2: -------------- On 2012/01/23 14:49:41, niemeyer wrote: > This doesn't look like a good fit for its own file. Where are we defining the > rest of the relation fields? This should be placed there and formatted similarly > to the other fields. Yeah, I was keeping it here to keep it in drafts. Moved to charm.rst in the top level https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/relation-... docs/source/drafts/relation-scope.rst:17: containers. See :doc:`subordinate-services`. On 2012/01/23 14:49:41, niemeyer wrote: > This isn't entirely clear/correct. Relations are added between services. Service > units have containers. Services themselves do not have containers. You can add a > relation between a principal service and a subordinate service that currently > have no units sharing containers, and that will cause the system to deploy the > subordinate service unit onto the principal service unit's container. On the > other hand, two principals cannot have container relationships established > between them. > > I'm not sure how much of that needs to be explained here. I'd suggest keeping > the explanation light, putting more details onto the spec itself, and referring > to it from here as already being done. I only say that you can't container related principals now. https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/subordina... File docs/source/drafts/subordinate-services.rst (right): https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/subordina... docs/source/drafts/subordinate-services.rst:131: relations: {} On 2012/01/23 14:49:41, niemeyer wrote: > This looks bogus. There's a relation in the example. Indeed. Good catch
Sign in to reply to this message.
https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-... File docs/source/drafts/implicit-relations.rst (right): https://codereview.appspot.com/5541054/diff/8003/docs/source/drafts/implicit-... docs/source/drafts/implicit-relations.rst:17: `juju-info`, specified as :: On 2012/01/23 14:49:41, niemeyer wrote: > Let's clarify a bit: > > `juju-info`, which if specified (it must not be) would look like:: This wasn't clarified. You can use a different wording if you want, but we have to make clear that the example is *not* to be used/specified, but rather just informative. https://codereview.appspot.com/5541054/diff/15003/docs/source/charm.rst File docs/source/charm.rst (right): https://codereview.appspot.com/5541054/diff/15003/docs/source/charm.rst#newco... docs/source/charm.rst:89: :doc:`subordinate-services`. Very nice, thanks. https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/implicit... File docs/source/drafts/implicit-relations.rst (right): https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/implicit... docs/source/drafts/implicit-relations.rst:3: ==================== Let's please try to use a single header convention for our specifications: === for top headers, --- for inner ones. Please see the other files to make sure. https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/implicit... docs/source/drafts/implicit-relations.rst:42: specified (it must not be) would look like :: That's wrong. You've got the wrong context for the comment. This juju-info may actually be specified. https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/subordin... File docs/source/drafts/subordinate-internals.rst (right): https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:3: ============================================ Header needs updating too. https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:33: subordinate: subordinate_id-000000002 That's the settings node, and is charm-modifiable. We can't depend on that data to implement internal relation establishment logic. Let's talk about this live. https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:61: relations are not deployed until those relationships are satisfied. If a service has multiple container relationships, it doesn't need to satisfy all of them for the charm to be deployed.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
https://codereview.appspot.com/5541054/diff/15003/docs/source/charm.rst File docs/source/charm.rst (right): https://codereview.appspot.com/5541054/diff/15003/docs/source/charm.rst#newco... docs/source/charm.rst:89: :doc:`subordinate-services`. On 2012/02/06 19:46:19, niemeyer wrote: > Very nice, thanks. thanks https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/implicit... File docs/source/drafts/implicit-relations.rst (right): https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/implicit... docs/source/drafts/implicit-relations.rst:3: ==================== On 2012/02/06 19:46:19, niemeyer wrote: > Let's please try to use a single header convention for our specifications: === > for top headers, --- for inner ones. Please see the other files to make sure. Done. https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/implicit... docs/source/drafts/implicit-relations.rst:42: specified (it must not be) would look like :: On 2012/02/06 19:46:19, niemeyer wrote: > That's wrong. You've got the wrong context for the comment. This juju-info may > actually be specified. Oh, I see what I did, fixed above. https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/subordin... File docs/source/drafts/subordinate-internals.rst (right): https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:3: ============================================ On 2012/02/06 19:46:19, niemeyer wrote: > Header needs updating too. Done. https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:33: subordinate: subordinate_id-000000002 This has been updated based on the G+ talk we had. On 2012/02/06 19:46:19, niemeyer wrote: > That's the settings node, and is charm-modifiable. We can't depend on that data > to implement internal relation establishment logic. Let's talk about this live. https://codereview.appspot.com/5541054/diff/15003/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:61: relations are not deployed until those relationships are satisfied. On 2012/02/06 19:46:19, niemeyer wrote: > If a service has multiple container relationships, it doesn't need to satisfy > all of them for the charm to be deployed. Done.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/implicit... File docs/source/drafts/implicit-relations.rst (right): https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/implicit... docs/source/drafts/implicit-relations.rst:59: provides less information) `juju-info` interface. This is the opposite of what we agreed, I think. Doing that would mean no charm containing a juju-info relation will have auto-resolution working properly. It must resolve to logging-directory, unless juju-info is explicitly requested in the command line. https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordin... File docs/source/drafts/subordinate-internals.rst (right): https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:30: container_relations/ s/_/-/ https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:43: indented under it.. Elements preceded by **|** represent a YAML s/.././ https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:44: encoded structure under the preceding ZK node. `container_relations` s/under/within/ https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:61: service-00000002: {name: name, role: role} Let's take the chance to update this, as we discussed online: relations relation-00000001: interface: <name> scope: <name> services: service-00000001: {name: name, role: role} service-00000002: {name: name, role: role} Please mention that this change to the topology had bumped the VERSION to 2. https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:80: have been satisfied. As we discussed online, adding a "scope: container" relation isn't necessarily going to deploy a charm, since it may be established between two subordinate charms. What causes the deployment of the subordinate charm is adding a "scope: container" relation with a "subordinate: false" charm. https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordin... File docs/source/drafts/subordinate-services.rst (right): https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordin... docs/source/drafts/subordinate-services.rst:99: result in a subordinate deployment. Subordinate services may still As we discussed, a subordinate charm must necessarily have a: subordinate: true Principal charms may also have "scope: container" relations, which should only be established with a subordinate charm. These relations shouldn't turn the principal into a subordinate. https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordin... docs/source/drafts/subordinate-services.rst:128: logging: s/logging/rsyslog/, to make the example more clear. Then please review all the relation information below. There are mismatches and typos. https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordin... docs/source/drafts/subordinate-services.rst:163: juju add-relation logging wordpress s/rsyslog/logging/ above and below as well.
Sign in to reply to this message.
https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/implicit... File docs/source/drafts/implicit-relations.rst (right): https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/implicit... docs/source/drafts/implicit-relations.rst:59: provides less information) `juju-info` interface. There was some confusion around my wording but not the intention here. Some wording was changed to be more explicit. On 2012/02/10 00:20:39, niemeyer wrote: > This is the opposite of what we agreed, I think. Doing that would mean no charm > containing a juju-info relation will have auto-resolution working properly. It > must resolve to logging-directory, unless juju-info is explicitly requested in > the command line. https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordin... File docs/source/drafts/subordinate-internals.rst (right): https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:61: service-00000002: {name: name, role: role} On 2012/02/10 00:20:39, niemeyer wrote: > Let's take the chance to update this, as we discussed online: > > relations > relation-00000001: > interface: <name> > scope: <name> > services: > service-00000001: {name: name, role: role} > service-00000002: {name: name, role: role} > > Please mention that this change to the topology had bumped the VERSION to 2. Done. https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:80: have been satisfied. On 2012/02/10 00:20:39, niemeyer wrote: > As we discussed online, adding a "scope: container" relation isn't necessarily > going to deploy a charm, since it may be established between two subordinate > charms. > > What causes the deployment of the subordinate charm is adding a "scope: > container" relation with a "subordinate: false" charm. Done. https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordin... File docs/source/drafts/subordinate-services.rst (right): https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordin... docs/source/drafts/subordinate-services.rst:128: logging: I always have trouble writing these by hand. Hopefully I got it right this time. On 2012/02/10 00:20:39, niemeyer wrote: > s/logging/rsyslog/, to make the example more clear. Then please review all the > relation information below. There are mismatches and typos. https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordin... docs/source/drafts/subordinate-services.rst:163: juju add-relation logging wordpress On 2012/02/10 00:20:39, niemeyer wrote: > s/rsyslog/logging/ above and below as well. Done.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Thanks a lot for the extensive spec, and for keeping up with it Ben. Just a few last minute trivials. LGTM! https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordin... File docs/source/drafts/subordinate-internals.rst (right): https://codereview.appspot.com/5541054/diff/31002/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:44: encoded structure under the preceding ZK node. `container_relations` On 2012/02/10 00:20:39, niemeyer wrote: > s/under/within/ Missing this fix. The YAML content goes within the preceding ZK node. "under" was already used above to give the idea of a sub-node. https://codereview.appspot.com/5541054/diff/33002/docs/source/drafts/subordin... File docs/source/drafts/subordinate-internals.rst (right): https://codereview.appspot.com/5541054/diff/33002/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:68: handled automatically. Please include in the same paragraph: The version of the topology is also changed from 1 to 2. https://codereview.appspot.com/5541054/diff/33002/docs/source/drafts/subordin... File docs/source/drafts/subordinate-services.rst (right): https://codereview.appspot.com/5541054/diff/33002/docs/source/drafts/subordin... docs/source/drafts/subordinate-services.rst:108: `subordinate: true` charms. Nice wording on the new text. Thanks.
Sign in to reply to this message.
https://codereview.appspot.com/5541054/diff/33002/docs/source/drafts/subordin... File docs/source/drafts/subordinate-internals.rst (right): https://codereview.appspot.com/5541054/diff/33002/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:68: handled automatically. On 2012/02/10 18:28:30, niemeyer wrote: > Please include in the same paragraph: > > The version of the topology is also changed from 1 to 2. I hadn't because of the issue of time sensitivity in the document, but I will now. https://codereview.appspot.com/5541054/diff/33002/docs/source/drafts/subordin... File docs/source/drafts/subordinate-services.rst (right): https://codereview.appspot.com/5541054/diff/33002/docs/source/drafts/subordin... docs/source/drafts/subordinate-services.rst:108: `subordinate: true` charms. On 2012/02/10 18:28:30, niemeyer wrote: > Nice wording on the new text. Thanks. Done.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordin... File docs/source/drafts/subordinate-internals.rst (right): https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:30: container/ As discussed online: 1) Drop container/ https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:41: settings/ 2) Move this to within each of the respective units above. https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:50: global/ 3) Drop both global/ and global/global/ https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:51: client/ 4) Move this back to under relation-000000002. https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:81: codebase for dealing with path construction and node access. The text above needs fixing to accommodate the discussed changes. https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:92: interface: name s/name/<name>/, same below. https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:95: service-00000001: {name: name, role: role} s/name: name/name: <name>/, same for role. One is a variable, the other is not. Same below.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
lgtm
Sign in to reply to this message.
https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordin... File docs/source/drafts/subordinate-internals.rst (right): https://codereview.appspot.com/5541054/diff/39001/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:92: interface: name On 2012/02/27 20:24:19, niemeyer wrote: > s/name/<name>/, same below. Still needs addressing. https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordin... File docs/source/drafts/subordinate-internals.rst (right): https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:30: unit_id-00000001/ All the unit ids need fixing in this example. It's something along the lines of "unit-0000000001". https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:32: - unit_id-000000002 The style is also inconsistent. This is a node, just like "unit-id-00000001" below, so both should end in /, or both should start with -. https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:64: relation-id/ relation-<id> https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:65: container_id/ # omitted for global scoped relations unit-<principal id> https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:66: <role>/ {name: name, role: role} {name: <name>, role: <role>} https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:73: store an container information and the normal child nodes are present This needs some fixing ("relations do not store an container information"?). Something like: Globally scoped relations have the "client", "server" and "settings" nodes directly under the "relation-<id>" node.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Thanks for looking this over again, believe I got it all. https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordin... File docs/source/drafts/subordinate-internals.rst (right): https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:30: unit_id-00000001/ On 2012/02/28 00:45:20, niemeyer wrote: > All the unit ids need fixing in this example. It's something along the lines of > "unit-0000000001". Done. https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:32: - unit_id-000000002 On 2012/02/28 00:45:20, niemeyer wrote: > The style is also inconsistent. This is a node, just like "unit-id-00000001" > below, so both should end in /, or both should start with -. I think I understand what you mean here. Hopefully the changes address what you expect. https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:64: relation-id/ On 2012/02/28 00:45:20, niemeyer wrote: > relation-<id> Done. https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:65: container_id/ # omitted for global scoped relations On 2012/02/28 00:45:20, niemeyer wrote: > unit-<principal id> Done. https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:66: <role>/ {name: name, role: role} On 2012/02/28 00:45:20, niemeyer wrote: > {name: <name>, role: <role>} Done. https://codereview.appspot.com/5541054/diff/39003/docs/source/drafts/subordin... docs/source/drafts/subordinate-internals.rst:73: store an container information and the normal child nodes are present On 2012/02/28 00:45:20, niemeyer wrote: > This needs some fixing ("relations do not store an container information"?). > Something like: > > Globally scoped relations have the "client", "server" and "settings" nodes > directly under the "relation-<id>" node. Done.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
