|
|
Created:
11 years, 12 months ago by fwereade Modified:
11 years, 11 months ago Reviewers:
mp+105242 Visibility:
Public. |
DescriptionAdd upstart package with Service and Conf types
Service enables control of, and visibility into, the state of an upstart
service; Conf embeds a Service and additionally allows for installation
(both directly, as used by a machine agent creating a unit agent service,
and as a set of commands to run later, as used when constructing a
cloud-init file to run a machine and/or provisioning agent on a new
instance).
https://code.launchpad.net/~fwereade/juju/go-upstart/+merge/105242
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 24
Patch Set 2 : Add upstart package with Service and Conf types #
Total comments: 18
Patch Set 3 : Add upstart package with Service and Conf types #
Total comments: 10
Patch Set 4 : Add upstart package with Service and Conf types #
Total comments: 8
Patch Set 5 : Add upstart package with Service and Conf types #
MessagesTotal messages: 15
Please take a look.
Sign in to reply to this message.
https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go File upstart/upstart.go (right): https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode17 upstart/upstart.go:17: var startedRE = regexp.MustCompile("^.* start/running, process (\\d+)\n$") Really? There's no programmatic way to query it? :-( https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode30 upstart/upstart.go:30: func (s *Service) path() string { s/path/confPath/? https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode52 upstart/upstart.go:52: // Installed returns true if the Service appears to be installed. // Installed returns true if the service configuration exists // in the init directory. https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode60 upstart/upstart.go:60: return s.pid() != -1 Why are we getting into the business of verifying pid numbers and checking for whether the service is being restarted (with Stable)? I'd expect that to be handled by upstart itself, and we can just check status (not every service has a pid, btw, although maybe that doesn't matter for the use case you have in mind). https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode96 upstart/upstart.go:96: // Remove removes the service. // Remove deletes the service configuration from the init directory. This is specially important to point out because it's not the typical way in which a service is disabled with upstart. https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode115 upstart/upstart.go:115: envT = "env %s=%q\n" This would be a lot more clear if defined as a template, in a single string. You can pass Conf itself onto the template and let it render it. https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode116 upstart/upstart.go:116: outT = "/tmp/%s.output" Writing on /tmp/ with well known filenames is a security issue (hint: any user can create a symbolic link from /tmp/jujud.output into /etc/passwd).
Sign in to reply to this message.
Thanks, nice review. A couple of questions... https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go File upstart/upstart.go (right): https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode17 upstart/upstart.go:17: var startedRE = regexp.MustCompile("^.* start/running, process (\\d+)\n$") On 2012/05/17 20:09:14, niemeyer wrote: > Really? There's no programmatic way to query it? :-( Not that I could discover... do you know of something obvious that I'm missing? https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode60 upstart/upstart.go:60: return s.pid() != -1 On 2012/05/17 20:09:14, niemeyer wrote: > Why are we getting into the business of verifying pid numbers and checking for > whether the service is being restarted (with Stable)? I'd expect that to be > handled by upstart itself, and we can just check status (not every service has a > pid, btw, although maybe that doesn't matter for the use case you have in mind). This is in a sense speculative; as I recall I added it to the python version primarily to enable USE_SUDO testing of real services (upstart won't tell us about services it's repeatedly restarting... until it finally gives up, that is, but it takes a good long time before it gives up on a service). I guess we don't need it now though :). I don't anticipate us using PIDless services... do you think we might want to? https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode116 upstart/upstart.go:116: outT = "/tmp/%s.output" On 2012/05/17 20:09:14, niemeyer wrote: > Writing on /tmp/ with well known filenames is a security issue (hint: any user > can create a symbolic link from /tmp/jujud.output into /etc/passwd). I'd be more than happy to make the top-level output optional, or even maybe drop it entirely... tbh I never found it very useful when doing the original stability work, but I'm sure someone was very keen on adding it in review. Thoughts?
Sign in to reply to this message.
superficial comments (i'm not familiar with upstart) https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go File upstart/upstart.go (right): https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode41 upstart/upstart.go:41: match := startedRE.FindStringSubmatch(string(out)) FindSubmatch(out), and then use string(match[1]) below? save a tiny bit of garbage :-) https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode115 upstart/upstart.go:115: envT = "env %s=%q\n" On 2012/05/17 20:09:14, niemeyer wrote: > This would be a lot more clear if defined as a template, in a single string. You > can pass Conf itself onto the template and let it render it. emphatic +1 https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode174 upstart/upstart.go:174: return err perhaps annotate this error. an exit status error from Install because the stop exec fails will be difficult to diagnose. https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode190 upstart/upstart.go:190: fmt.Sprintf("cat >> %s << EOF\n%sEOF\n", c.path(), conf), s/EOF/'EOF'/ (first EOF only) unless you *want* to expand metacharacters in the conf text.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go File upstart/upstart.go (right): https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode30 upstart/upstart.go:30: func (s *Service) path() string { On 2012/05/17 20:09:14, niemeyer wrote: > s/path/confPath/? Done. https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode41 upstart/upstart.go:41: match := startedRE.FindStringSubmatch(string(out)) On 2012/05/21 11:07:38, rog wrote: > FindSubmatch(out), and then use string(match[1]) below? > > save a tiny bit of garbage :-) Done. https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode52 upstart/upstart.go:52: // Installed returns true if the Service appears to be installed. On 2012/05/17 20:09:14, niemeyer wrote: > // Installed returns true if the service configuration exists > // in the init directory. Done. https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode52 upstart/upstart.go:52: // Installed returns true if the Service appears to be installed. On 2012/05/17 20:09:14, niemeyer wrote: > // Installed returns true if the service configuration exists > // in the init directory. Done. https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode60 upstart/upstart.go:60: return s.pid() != -1 On 2012/05/21 10:04:18, fwereade wrote: > On 2012/05/17 20:09:14, niemeyer wrote: > > Why are we getting into the business of verifying pid numbers and checking for > > whether the service is being restarted (with Stable)? I'd expect that to be > > handled by upstart itself, and we can just check status (not every service has > a > > pid, btw, although maybe that doesn't matter for the use case you have in > mind). > > This is in a sense speculative; as I recall I added it to the python version > primarily to enable USE_SUDO testing of real services (upstart won't tell us > about services it's repeatedly restarting... until it finally gives up, that is, > but it takes a good long time before it gives up on a service). I guess we don't > need it now though :). > > I don't anticipate us using PIDless services... do you think we might want to? Done. https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode96 upstart/upstart.go:96: // Remove removes the service. On 2012/05/17 20:09:14, niemeyer wrote: > // Remove deletes the service configuration from the init directory. > > This is specially important to point out because it's not the typical way in > which a service is disabled with upstart. Done. https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode115 upstart/upstart.go:115: envT = "env %s=%q\n" On 2012/05/21 11:07:38, rog wrote: > On 2012/05/17 20:09:14, niemeyer wrote: > > This would be a lot more clear if defined as a template, in a single string. > You > > can pass Conf itself onto the template and let it render it. > > emphatic +1 Done. https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode116 upstart/upstart.go:116: outT = "/tmp/%s.output" On 2012/05/21 10:04:18, fwereade wrote: > On 2012/05/17 20:09:14, niemeyer wrote: > > Writing on /tmp/ with well known filenames is a security issue (hint: any user > > can create a symbolic link from /tmp/jujud.output into /etc/passwd). > > I'd be more than happy to make the top-level output optional, or even maybe drop > it entirely... tbh I never found it very useful when doing the original > stability work, but I'm sure someone was very keen on adding it in review. > Thoughts? Made it optional; we can use it if we want it. https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode174 upstart/upstart.go:174: return err On 2012/05/21 11:07:38, rog wrote: > perhaps annotate this error. an exit status error from Install because the stop > exec fails will be difficult to diagnose. Done. https://codereview.appspot.com/6190061/diff/1/upstart/upstart.go#newcode190 upstart/upstart.go:190: fmt.Sprintf("cat >> %s << EOF\n%sEOF\n", c.path(), conf), On 2012/05/21 11:07:38, rog wrote: > s/EOF/'EOF'/ > (first EOF only) > unless you *want* to expand metacharacters in the conf text. Done.
Sign in to reply to this message.
LGTM https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go File upstart/upstart.go (right): https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode82 upstart/upstart.go:82: {{range $k, $v := .Env}}{{printf "env %s=%q\n" $k $v}}{{end}} The %q here is dubious as discussed on irc. also, i think the printf *might* be more clear expanded into an actual template: {{range $k, $v := .Env}}env {{$k}}={{$v|printf "%q"}} {{end}} then if we do decide to fix the bug sometime, we can replace the printf by a custom quote function. https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode113 upstart/upstart.go:113: func (c *Conf) render() (string, error) { if you return []byte here, you won't have to convert it again to pass to ioutil.WriteFile. https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode117 upstart/upstart.go:117: buf := &bytes.Buffer{} var buf bytes.Buffer ?
Sign in to reply to this message.
https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go File upstart/upstart.go (right): https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode41 upstart/upstart.go:41: cmd := exec.Command("status", s.Name) what about "/sbin/status", so we don't rely on any PATH environment here. Feel free to disregard this if it makes testing impossible. https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode46 upstart/upstart.go:46: return startedRE.FindSubmatch(out) != nil I was disappointed to learn that upstart doesn't use exit codes to indicate the status. https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode54 upstart/upstart.go:54: return exec.Command("start", s.Name).Run() /sbin/start https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode62 upstart/upstart.go:62: return exec.Command("stop", s.Name).Run() /sbin/stop https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode117 upstart/upstart.go:117: buf := &bytes.Buffer{} On 2012/05/28 15:43:15, rog wrote: > var buf bytes.Buffer > ? var buf = new(bytes.Buffer) Execute needs an io.Writer which requires a pointer receiver, not a value. Alternatively var buf bytes.Buffer; confT.Execute(&buf, c) https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode149 upstart/upstart.go:149: "start " + c.Name, /sbin/start
Sign in to reply to this message.
couple more trivial things. https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go File upstart/upstart.go (right): https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode15 upstart/upstart.go:15: var startedRE = regexp.MustCompile("^.* start/running, process (\\d+)\n$") you can use backquotes here and save the \\. (`\n` is interpreted correctly by regexp. https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode46 upstart/upstart.go:46: return startedRE.FindSubmatch(out) != nil return startedRE.Match(out) https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode117 upstart/upstart.go:117: buf := &bytes.Buffer{} On 2012/05/28 22:37:47, dfc wrote: > On 2012/05/28 15:43:15, rog wrote: > > var buf bytes.Buffer > > ? > > var buf = new(bytes.Buffer) > > Execute needs an io.Writer which requires a pointer receiver, not a value. > Alternatively > > var buf bytes.Buffer; confT.Execute(&buf, c) that's what i intended to suggest (even though i didn't mention the &buf bit) i like the fact that the var decl doesn't have any punctuation, but i realise which way to use is very much a matter of taste; my feelings aren't particularly strong here.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go File upstart/upstart.go (right): https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode15 upstart/upstart.go:15: var startedRE = regexp.MustCompile("^.* start/running, process (\\d+)\n$") On 2012/05/29 07:51:01, rog wrote: > you can use backquotes here and save the \\. > > (`\n` is interpreted correctly by regexp. Oh! Well, goodness me :). Thanks. https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode41 upstart/upstart.go:41: cmd := exec.Command("status", s.Name) On 2012/05/28 22:37:47, dfc wrote: > what about "/sbin/status", so we don't rely on any PATH environment here. Feel > free to disregard this if it makes testing impossible. It's all about the testing :(. Better suggestions, anyone? https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode46 upstart/upstart.go:46: return startedRE.FindSubmatch(out) != nil On 2012/05/29 07:51:01, rog wrote: > return startedRE.Match(out) D'oh. Done. https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode82 upstart/upstart.go:82: {{range $k, $v := .Env}}{{printf "env %s=%q\n" $k $v}}{{end}} On 2012/05/28 15:43:15, rog wrote: > The %q here is dubious as discussed on irc. Added BUG comment. > also, i think the printf *might* be more clear expanded into an actual template: > > {{range $k, $v := .Env}}env {{$k}}={{$v|printf "%q"}} > {{end}} > > then if we do decide to fix the bug sometime, we can replace the printf by a > custom quote function. Yeah, that looks nicer. Thanks. https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode117 upstart/upstart.go:117: buf := &bytes.Buffer{} On 2012/05/29 07:51:01, rog wrote: > On 2012/05/28 22:37:47, dfc wrote: > > On 2012/05/28 15:43:15, rog wrote: > > > var buf bytes.Buffer > > > ? > > > > var buf = new(bytes.Buffer) > > > > Execute needs an io.Writer which requires a pointer receiver, not a value. > > Alternatively > > > > var buf bytes.Buffer; confT.Execute(&buf, c) > > that's what i intended to suggest (even though i didn't mention the &buf bit) > > i like the fact that the var decl doesn't have any punctuation, but i realise > which way to use is very much a matter of taste; my feelings aren't particularly > strong here. I went with the original suggestion for no exceptionally good reason.
Sign in to reply to this message.
LGTM. https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go File upstart/upstart.go (right): https://codereview.appspot.com/6190061/diff/7001/upstart/upstart.go#newcode41 upstart/upstart.go:41: cmd := exec.Command("status", s.Name) Apart from putting all the paths into a var block at the top of the file and patching them in the test, nothing comes to mind without introducing a context structure or something. On 2012/05/29 09:58:31, fwereade wrote: > On 2012/05/28 22:37:47, dfc wrote: > > what about "/sbin/status", so we don't rely on any PATH environment here. Feel > > free to disregard this if it makes testing impossible. > > It's all about the testing :(. Better suggestions, anyone?
Sign in to reply to this message.
a few more thoughts after thinking about using this. https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go File upstart/upstart.go (right): https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode20 upstart/upstart.go:20: InitDir string // defaults to "/etc/init" will the upstart daemon notice us if we use a directory other than /etc/init? https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode89 upstart/upstart.go:89: // Conf is responsible for defining and installing upstart services. i think the fields in this struct should be documented. https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode94 upstart/upstart.go:94: Cmd string just a thought after thinking about using this: should this be []string so that it's obvious that it's the command and its arguments? https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode107 upstart/upstart.go:107: return errors.New("missing Desc") should this really be an error?
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go File upstart/upstart.go (right): https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode20 upstart/upstart.go:20: InitDir string // defaults to "/etc/init" On 2012/05/29 16:49:53, rog wrote: > will the upstart daemon notice us if we use a directory other than /etc/init? Depends what directory upstart is looking at ;). It's also useful for testing... https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode89 upstart/upstart.go:89: // Conf is responsible for defining and installing upstart services. On 2012/05/29 16:49:53, rog wrote: > i think the fields in this struct should be documented. Done. https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode94 upstart/upstart.go:94: Cmd string On 2012/05/29 16:49:53, rog wrote: > just a thought after thinking about using this: should this be []string so that > it's obvious that it's the command and its arguments? Hmm, my feeling is that it's obvious enough already, and I don't really want to mess around with quoting args here... counterpoint? https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode107 upstart/upstart.go:107: return errors.New("missing Desc") On 2012/05/29 16:49:53, rog wrote: > should this really be an error? Personally, I think we *should* provide descriptions for our jobs. If you disagree there then, yeah, I guess this shouldn't be an error... but I don't think it's bad to require this.
Sign in to reply to this message.
https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go File upstart/upstart.go (right): https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode94 upstart/upstart.go:94: Cmd string On 2012/05/30 11:22:11, fwereade wrote: > On 2012/05/29 16:49:53, rog wrote: > > just a thought after thinking about using this: should this be []string so > that > > it's obvious that it's the command and its arguments? > > Hmm, my feeling is that it's obvious enough already, and I don't really want to > mess around with quoting args here... counterpoint? counterpoint is that anyone that want to use this package to invoke a command with arguments containing spaces or odd characters is going to need to know the quoting convention anyway, so you might as well put it in here and encapsulate the nih quoting rules, whatever they are. perhaps Cmd string; Args []string to match exec.Command? https://codereview.appspot.com/6190061/diff/7002/upstart/upstart.go#newcode107 upstart/upstart.go:107: return errors.New("missing Desc") On 2012/05/30 11:22:11, fwereade wrote: > On 2012/05/29 16:49:53, rog wrote: > > should this really be an error? > > Personally, I think we *should* provide descriptions for our jobs. If you > disagree there then, yeah, I guess this shouldn't be an error... but I don't > think it's bad to require this. fair enough. https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go File upstart/upstart.go (right): https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go#newcode92 upstart/upstart.go:92: // Desc is the upstart job's "description". s/"description"/description/ https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go#newcode94 upstart/upstart.go:94: // Env holds the environment variables that will be set when the job runs. s/job/command/ ? (to make it clear that the command will have access to those vars, assuming it does)
Sign in to reply to this message.
LGTM https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go File upstart/upstart.go (right): https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go#newcode32 upstart/upstart.go:32: // Installed returns true if the the service configuration exists in the s/the the/the/, but as you're fixing this, would you mind to use the more common convention (it was my mistake to suggest otherwise): // Installed returns whether the service .... https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go#newcode92 upstart/upstart.go:92: // Desc is the upstart job's "description". On 2012/05/30 11:41:32, rog wrote: > s/"description"/description/ +1, and s/job/service/ too. Or, alternatively, expand the abbreviation and document the fact that Conf is the upstart service configuration. The field name would be enough of a hint in this context. Works either way for me, though. https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go#newcode139 upstart/upstart.go:139: return fmt.Errorf("could not remove installed service: %s", err) "upstart: could not ..."
Sign in to reply to this message.
*** Submitted: Add upstart package with Service and Conf types Service enables control of, and visibility into, the state of an upstart service; Conf embeds a Service and additionally allows for installation (both directly, as used by a machine agent creating a unit agent service, and as a set of commands to run later, as used when constructing a cloud-init file to run a machine and/or provisioning agent on a new instance). R=niemeyer, rog, dfc CC= https://codereview.appspot.com/6190061 https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go File upstart/upstart.go (right): https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go#newcode32 upstart/upstart.go:32: // Installed returns true if the the service configuration exists in the On 2012/05/30 20:55:18, niemeyer wrote: > s/the the/the/, but as you're fixing this, would you mind to use the more common > convention (it was my mistake to suggest otherwise): > > // Installed returns whether the service .... Done. https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go#newcode92 upstart/upstart.go:92: // Desc is the upstart job's "description". On 2012/05/30 20:55:18, niemeyer wrote: > On 2012/05/30 11:41:32, rog wrote: > > s/"description"/description/ > > +1, and s/job/service/ too. > > Or, alternatively, expand the abbreviation and document the fact that Conf is > the upstart service configuration. The field name would be enough of a hint in > this context. > > Works either way for me, though. Done. https://codereview.appspot.com/6190061/diff/14002/upstart/upstart.go#newcode94 upstart/upstart.go:94: // Env holds the environment variables that will be set when the job runs. On 2012/05/30 11:41:32, rog wrote: > s/job/command/ ? > (to make it clear that the command will have access to those vars, assuming it > does) Done.
Sign in to reply to this message.
|