Issue 91390043: -
Description
Rudimentary beginnings of architecture docs. More to come, but no point keeping these secret.
https://code.launchpad.net/~fwereade/juju-core/arch-docs-1/+merge/219190
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 40
MessagesTotal messages: 7
Please take a look.
Sign in to reply to this message.
I have some comments, and a couple spelling errors, but I think it flows pretty well. LGTM. We might want someone less familiar with the internals to give it a look to see if it is coherent for them. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt File doc/architectural-overview.txt (right): https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:27: application, and the `juju-deployer` python tool, are other examples. I'd probably take the commas out of the sentence as it seems to flow just fine without. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:46: (and, honestly, to lock it down further and better than we currently have). Possible not worth including the side comment here? I agree with the sentiment but it might not have place in the first run overview of the system. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:66: password, if the one they used has been stored persistently somewhere and is "stored persistently" outside of our control? Certainly writing it to agent.conf stores it persistently and you have to do that *somewhere*. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:80: differences. I think the details here might not be important enough to dedicate a whole paragraph. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:118: reasonably combined with the apiaddressupdater, but there's no guarantee that "that ... "? https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:126: * Handle SIGABRT and permanently stop the agent (`worker/terminationworker`) and trigger a cleanup? https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:148: basicaly all the information in agent config which *isn't* about contacting an basically https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:162: container provisioners run in all machine agents anyway) , similar to how container provisioners work in each machine agent ? https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:170: Do we have nested sections? "singular" workers might be better as a subsection. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:241: worker facades. In these cases, the method is implemented on a spearate type, which separate https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:282: Basically, if an environ config gets off a state server, we've screwed up. Probably add a point here that we do have too much code already that calls API.EnvironConfig() when it should be using a different call to get the specific details it actually cares about.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt File doc/architectural-overview.txt (right): https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:18: *units* that comprise those services, and the *machines* on which those units run. shouldn't that be "and of the *units* that those services are comprised of" ? https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:73: When comparing the unit agent with the machine agent, the truth of the above You've started talking about machine and unit agents here without explaining what they are. I'd add a brief statement saying that there are (currently) two types of agents: machine and unit, each of which is responsible for maintaining a machine or unit entity. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:118: reasonably combined with the apiaddressupdater, but there's no guarantee that Why would you want to combine them? Sure they both make changes based on API server addresses, but they are doing quite different things with them... I'm not sure anything after "--" is worth keeping. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:129: the two sets of machines will be the same for ever. I can't parse this second line. Two sets of machines? Does this belong here? https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:131: (`worker/provisioner`) Maybe a note here that the provisioner may also provision/decommision provider instances (see State Server Workers below). https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:138: be flawed or incomplete). Those that do run the `worker/deployer` code which "Those that do" or "Those that do run"? How about "Machine agents having JobHostUnits run"? Wordier, but unambiguous. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:145: information it really shouldn't have access to -- ie the running provider type s/ie/i.e./ https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:150: down and (2) just encourages worse layering violations as we progress. Do we want to say anything about capabilities here? Maybe too low level. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:217: XXXX Looking forward to reading the following sections.
Sign in to reply to this message.
https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt File doc/architectural-overview.txt (right): https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:156: the following. s/./:/
Sign in to reply to this message.
All the "done"s mean "done in followup". https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt File doc/architectural-overview.txt (right): https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:18: *units* that comprise those services, and the *machines* on which those units run. On 2014/05/13 12:56:00, axw wrote: > shouldn't that be > "and of the *units* that those services are comprised of" > ? I think my usage is correct, actually. Google claims it means (among other things) "make up or constitute (a whole)". (I also use the other meaning, "consist of; be made up of" in the previous paragraph. Ah, the joys of English.) https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:27: application, and the `juju-deployer` python tool, are other examples. On 2014/05/13 09:33:27, jameinel wrote: > I'd probably take the commas out of the sentence as it seems to flow just fine > without. Done. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:46: (and, honestly, to lock it down further and better than we currently have). On 2014/05/13 09:33:26, jameinel wrote: > Possible not worth including the side comment here? I agree with the sentiment > but it might not have place in the first run overview of the system. I think I'll generally be leaving the backchat in. I think it's worse to have it recorded nowhere than to have it recorded in an imperfect location. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:66: password, if the one they used has been stored persistently somewhere and is On 2014/05/13 09:33:27, jameinel wrote: > "stored persistently" outside of our control? > Certainly writing it to agent.conf stores it persistently and you have to do > that *somewhere*. Done. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:73: When comparing the unit agent with the machine agent, the truth of the above On 2014/05/13 12:56:00, axw wrote: > You've started talking about machine and unit agents here without explaining > what they are. I'd add a brief statement saying that there are (currently) two > types of agents: machine and unit, each of which is responsible for maintaining > a machine or unit entity. Done. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:80: differences. On 2014/05/13 09:33:27, jameinel wrote: > I think the details here might not be important enough to dedicate a whole > paragraph. I've given it an <h6> called "Note" and quoted it. Might do the same in another couple of places. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:118: reasonably combined with the apiaddressupdater, but there's no guarantee that On 2014/05/13 12:56:00, axw wrote: > Why would you want to combine them? Sure they both make changes based on API > server addresses, but they are doing quite different things with them... > I'm not sure anything after "--" is worth keeping. Yeah, just dropping that. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:126: * Handle SIGABRT and permanently stop the agent (`worker/terminationworker`) On 2014/05/13 09:33:27, jameinel wrote: > and trigger a cleanup? Doesn't do that, as far as I'm aware. In general I think it's a bad idea to SIGABRT machine agents. axw, would you remind me exactly what this behaviour supports? I know it's something to do with the manual provider... https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:129: the two sets of machines will be the same for ever. On 2014/05/13 12:56:00, axw wrote: > I can't parse this second line. Two sets of machines? Does this belong here? Ha! it came from the dangling "that" above. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:131: (`worker/provisioner`) On 2014/05/13 12:56:00, axw wrote: > Maybe a note here that the provisioner may also provision/decommision provider > instances (see State Server Workers below). Done. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:138: be flawed or incomplete). Those that do run the `worker/deployer` code which On 2014/05/13 12:55:59, axw wrote: > "Those that do" or "Those that do run"? > How about "Machine agents having JobHostUnits run"? Wordier, but unambiguous. Done. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:145: information it really shouldn't have access to -- ie the running provider type On 2014/05/13 12:55:59, axw wrote: > s/ie/i.e./ Done. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:148: basicaly all the information in agent config which *isn't* about contacting an On 2014/05/13 09:33:26, jameinel wrote: > basically Done. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:150: down and (2) just encourages worse layering violations as we progress. On 2014/05/13 12:56:00, axw wrote: > Do we want to say anything about capabilities here? Maybe too low level. Maybe we do, but I'm not quite sure what you mean -- expand please? https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:162: container provisioners run in all machine agents anyway) On 2014/05/13 09:33:26, jameinel wrote: > , similar to how container provisioners work in each machine agent > > ? Done. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:170: On 2014/05/13 09:33:26, jameinel wrote: > Do we have nested sections? "singular" workers might be better as a subsection. Yeah, number of leading #s down to <h6>. Done. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:241: worker facades. In these cases, the method is implemented on a spearate type, which On 2014/05/13 09:33:27, jameinel wrote: > separate Done. https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:282: Basically, if an environ config gets off a state server, we've screwed up. On 2014/05/13 09:33:27, jameinel wrote: > Probably add a point here that we do have too much code already that calls > API.EnvironConfig() when it should be using a different call to get the specific > details it actually cares about. Done.
Sign in to reply to this message.
https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt File doc/architectural-overview.txt (right): https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... doc/architectural-overview.txt:150: down and (2) just encourages worse layering violations as we progress. On 2014/05/15 08:58:15, fwereade wrote: > On 2014/05/13 12:56:00, axw wrote: > > Do we want to say anything about capabilities here? Maybe too low level. > > Maybe we do, but I'm not quite sure what you mean -- expand please? We have briefly discussed in the past a concept of environment capabilities, such as "capable of global firewalling", which would be used to turn on the firewaller worker. I think ignore this for now, since it's just a concept and hasn't really been discussed in detail (unless before my time, perhaps)
Sign in to reply to this message.
On 2014/05/15 09:10:31, axw wrote: > https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt > File doc/architectural-overview.txt (right): > > https://codereview.appspot.com/91390043/diff/1/doc/architectural-overview.txt... > doc/architectural-overview.txt:150: down and (2) just encourages worse layering > violations as we progress. > On 2014/05/15 08:58:15, fwereade wrote: > > On 2014/05/13 12:56:00, axw wrote: > > > Do we want to say anything about capabilities here? Maybe too low level. > > > > Maybe we do, but I'm not quite sure what you mean -- expand please? > > We have briefly discussed in the past a concept of environment capabilities, > such as "capable of global firewalling", which would be used to turn on the > firewaller worker. I think ignore this for now, since it's just a concept and > hasn't really been discussed in detail (unless before my time, perhaps) Ah, got you. Good point.I *should* talk about it, but I think not here; I'd like the API<->agent comms to avoid using vocab intended for the state<->environ level. A Job STM to be the best way to express the desire to have particular agents act differently.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
