Issue 58510045: debug-log: added new debug log api and command
Descriptiondebug-log: added new debug log api and command
Second approach based on the idea of Roger. It uses an own
handler using a web socket connection for the streaming of
the log. The initial call is a request to the URL
wss://<STATESERVER>:<PORT>/log?lines=<N>&filter=<REGEXP>
The command also handles older environments without the
WatchDebugLog API. This is done as before using SSH or simply
tail the local file. Beside the number of lines the command
and the function take also a regular expression for filtering.
Both are optional. The default number of lines is 10, the
empty filter lets all log lines pass.
juju debug-log -n 25 -f "^machine-1"
The current solution works for both, local and non-local
environments.
What's missing:
- Currently only tested live. Unit tests have to be
developed and added.
https://code.launchpad.net/~themue/juju-core/060-debug-log-api-and-cmd/+merge/203955
(do not edit description out of merge proposal)
Patch Set 1 #
Total comments: 28
Patch Set 2 : debug-log: added new debug log api and command #Patch Set 3 : debug-log: added new debug log api and command #Patch Set 4 : debug-log: added new debug log api and command #
Total comments: 72
Patch Set 5 : debug-log: added new debug log api and command #Patch Set 6 : debug-log: added new debug log api and command #
Total comments: 8
Patch Set 7 : debug-log: added new debug log api and command #
Total comments: 1
Patch Set 8 : debug-log: added new debug log api and command #
Total comments: 17
MessagesTotal messages: 23
Please take a look.
Sign in to reply to this message.
Looking good so far. A few comments and suggestions below. https://codereview.appspot.com/58510045/diff/1/cmd/juju/debuglog.go File cmd/juju/debuglog.go (right): https://codereview.appspot.com/58510045/diff/1/cmd/juju/debuglog.go#newcode46 cmd/juju/debuglog.go:46: f.StringVar(&c.entities, "e", "", "filter the output by entities (environment, machine or unit)") I'm not sure there are any other places on the juju command line that accept entity tags. I think we probably want to translate from ids (e.g. 0, wordpress/0) to tags before sending them server side. https://codereview.appspot.com/58510045/diff/1/cmd/juju/debuglog.go#newcode69 cmd/juju/debuglog.go:69: entities = []string{names.EnvironTag(info.UUID)} If we made client.WatchDebugLog interpret no tags as "give me everything", then this code wouldn't be necessary, and we wouldn't need any special interpretation of environment tags at all. An alternative approach that I quite favour is not to pass entity tags at all, but just a regular expression that filters lines. The command-line interface could still look exactly the same, but when filtering for several tags, it would send the regexp: "^(tag1|tag2|tag3)" This would allow us the future flexibility to do more flexible filtering, while actually simplifying the server-side code. https://codereview.appspot.com/58510045/diff/1/cmd/juju/debuglog.go#newcode72 cmd/juju/debuglog.go:72: entities = strings.Split(c.entities, " ") strings.Fields ? https://codereview.appspot.com/58510045/diff/1/cmd/juju/debuglog.go#newcode80 cmd/juju/debuglog.go:80: reader := bufio.NewReader(debugLog) I think that all the rest can just be: return io.Copy(os.Stdout, debugLog) https://codereview.appspot.com/58510045/diff/1/state/api/apiclient.go File state/api/apiclient.go (right): https://codereview.appspot.com/58510045/diff/1/state/api/apiclient.go#newcode137 state/api/apiclient.go:137: // TODO: Store appropriate websocket config in State. d https://codereview.appspot.com/58510045/diff/1/state/api/client.go File state/api/client.go (right): https://codereview.appspot.com/58510045/diff/1/state/api/client.go#newcode532 state/api/client.go:532: } how about: attrs := url.Values{ "lines": {fmt.Sprint(lines)}, "entities": entities, } cfg.Location = &net.URL{ Scheme: "wss", Host: c.st.serverHostPort, Path: "/log", RawQuery: attrs.Encode(), } instead? https://codereview.appspot.com/58510045/diff/1/state/api/client.go#newcode563 state/api/client.go:563: var req params.EntityLogRequest req := params.EntityLogRequest{ Entities: entities, } ? https://codereview.appspot.com/58510045/diff/1/state/apiserver/common.go File state/apiserver/common.go (right): https://codereview.appspot.com/58510045/diff/1/state/apiserver/common.go#newc... state/apiserver/common.go:20: type errorResponder interface { I don't think we need this. We can use a common type for all error responses (e.g. type ErrorResponse struct { Error string } ) and the code below can just send that. We'll want to make sure that the error JSON is consistent across HTTP requests anyway, and this ensures that. https://codereview.appspot.com/58510045/diff/1/state/apiserver/common.go#newc... state/apiserver/common.go:26: type commonHandler struct { I'm not that keen on this - it doesn't seem like it adds much value over just defining a few functions. What's the gain of embedding it? https://codereview.appspot.com/58510045/diff/1/state/apiserver/log.go File state/apiserver/log.go (right): https://codereview.appspot.com/58510045/diff/1/state/apiserver/log.go#newcode23 state/apiserver/log.go:23: const logLocation = "/var/log/juju/all-machines.log" As Tim pointed out in a previous review, this isn't correct for the local provider. At the least, it needs a comment to that effect. You might want to have a chat with William or Tim at some point to work out a reasonable way to get the info. https://codereview.appspot.com/58510045/diff/1/state/apiserver/log.go#newcode30 state/apiserver/log.go:30: // newLogHandler creates a new log handler. // newLogHandler returns a new http.Handler // that handles debug-log HTTP requests. ? https://codereview.appspot.com/58510045/diff/1/state/apiserver/log.go#newcode56 state/apiserver/log.go:56: lines, err := strconv.Atoi(values["lines"][0]) This will panic if there are no "lines" attributes. I'd do: lines := 0 if linesAttr := values.Get("lines"); linesAttr != "" { var err error lines, err = strconv.Atoi(linesAttr) if err != nil { ... } } https://codereview.appspot.com/58510045/diff/1/state/apiserver/log.go#newcode67 state/apiserver/log.go:67: stream.tomb.Wait() if err := stream.tomb.Wait(); err != nil { logger.Errorf("debug-log handler error: %v", err) } ?
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/58510045/diff/1/cmd/juju/debuglog.go File cmd/juju/debuglog.go (right): https://codereview.appspot.com/58510045/diff/1/cmd/juju/debuglog.go#newcode46 cmd/juju/debuglog.go:46: f.StringVar(&c.entities, "e", "", "filter the output by entities (environment, machine or unit)") On 2014/01/30 14:34:09, rog wrote: > I'm not sure there are any other places on the juju command line that accept > entity tags. I think we probably want to translate from ids > (e.g. 0, wordpress/0) to tags before sending them server side. Not needed anymore with switch to regex. https://codereview.appspot.com/58510045/diff/1/cmd/juju/debuglog.go#newcode69 cmd/juju/debuglog.go:69: entities = []string{names.EnvironTag(info.UUID)} On 2014/01/30 14:34:09, rog wrote: > If we made client.WatchDebugLog interpret no tags as "give me everything", then > this code wouldn't be necessary, and we wouldn't need any special interpretation > of environment tags at all. > > An alternative approach that I quite favour is not to pass entity tags at all, > but just a regular expression that filters lines. > > The command-line interface could still look exactly the same, but > when filtering for several tags, it would send the regexp: > > "^(tag1|tag2|tag3)" > > This would allow us the future flexibility to do more flexible filtering, > while actually simplifying the server-side code. > Also using regex on command side now. The old command has no filtering but pipe to grep is done. So it now can be done in one step. https://codereview.appspot.com/58510045/diff/1/cmd/juju/debuglog.go#newcode72 cmd/juju/debuglog.go:72: entities = strings.Split(c.entities, " ") On 2014/01/30 14:34:09, rog wrote: > strings.Fields ? Not needed anymore with switch to regex. https://codereview.appspot.com/58510045/diff/1/cmd/juju/debuglog.go#newcode80 cmd/juju/debuglog.go:80: reader := bufio.NewReader(debugLog) On 2014/01/30 14:34:09, rog wrote: > I think that all the rest can just be: > > return io.Copy(os.Stdout, debugLog) Done. Cool. https://codereview.appspot.com/58510045/diff/1/state/api/apiclient.go File state/api/apiclient.go (right): https://codereview.appspot.com/58510045/diff/1/state/api/apiclient.go#newcode137 state/api/apiclient.go:137: // TODO: Store appropriate websocket config in State. On 2014/01/30 14:34:09, rog wrote: > d Done. https://codereview.appspot.com/58510045/diff/1/state/api/client.go File state/api/client.go (right): https://codereview.appspot.com/58510045/diff/1/state/api/client.go#newcode532 state/api/client.go:532: } On 2014/01/30 14:34:09, rog wrote: > how about: > > attrs := url.Values{ > "lines": {fmt.Sprint(lines)}, > "entities": entities, > } > cfg.Location = &net.URL{ > Scheme: "wss", > Host: c.st.serverHostPort, > Path: "/log", > RawQuery: attrs.Encode(), > } > > > instead? Nice. https://codereview.appspot.com/58510045/diff/1/state/api/client.go#newcode563 state/api/client.go:563: var req params.EntityLogRequest On 2014/01/30 14:34:09, rog wrote: > req := params.EntityLogRequest{ > Entities: entities, > } > > ? Done. https://codereview.appspot.com/58510045/diff/1/state/apiserver/common.go File state/apiserver/common.go (right): https://codereview.appspot.com/58510045/diff/1/state/apiserver/common.go#newc... state/apiserver/common.go:20: type errorResponder interface { On 2014/01/30 14:34:09, rog wrote: > I don't think we need this. We can use a common type for all error responses > (e.g. > > type ErrorResponse struct { > Error string > } > ) > > and the code below can just send that. > > We'll want to make sure that the error JSON is consistent > across HTTP requests anyway, and this ensures that. The charmsHandler sends a CharmsResponse, which also contains a CharmURL. If this shall be refactored it should be done later in a different CL. https://codereview.appspot.com/58510045/diff/1/state/apiserver/common.go#newc... state/apiserver/common.go:26: type commonHandler struct { On 2014/01/30 14:34:09, rog wrote: > I'm not that keen on this - it doesn't seem like it adds much value over just > defining a few functions. What's the gain of embedding it? Based on your hint to factor out common parts. The methods of this type are used in both, charmsHandler and logHandler. https://codereview.appspot.com/58510045/diff/1/state/apiserver/log.go File state/apiserver/log.go (right): https://codereview.appspot.com/58510045/diff/1/state/apiserver/log.go#newcode23 state/apiserver/log.go:23: const logLocation = "/var/log/juju/all-machines.log" On 2014/01/30 14:34:09, rog wrote: > As Tim pointed out in a previous review, this isn't correct > for the local provider. > > At the least, it needs a comment to that effect. > > You might want to have a chat with William or Tim at > some point to work out a reasonable way to get > the info. I already mentioned it. And a well known bug exists. But I'll add a comment. https://codereview.appspot.com/58510045/diff/1/state/apiserver/log.go#newcode30 state/apiserver/log.go:30: // newLogHandler creates a new log handler. On 2014/01/30 14:34:09, rog wrote: > // newLogHandler returns a new http.Handler > // that handles debug-log HTTP requests. > > ? Done. https://codereview.appspot.com/58510045/diff/1/state/apiserver/log.go#newcode56 state/apiserver/log.go:56: lines, err := strconv.Atoi(values["lines"][0]) On 2014/01/30 14:34:09, rog wrote: > This will panic if there are no "lines" attributes. > > I'd do: > > lines := 0 > if linesAttr := values.Get("lines"); linesAttr != "" { > var err error > lines, err = strconv.Atoi(linesAttr) > if err != nil { > ... > } > } Done. Yep, noticed it too but missed to fix it. Thx. https://codereview.appspot.com/58510045/diff/1/state/apiserver/log.go#newcode67 state/apiserver/log.go:67: stream.tomb.Wait() On 2014/01/30 14:34:09, rog wrote: > if err := stream.tomb.Wait(); err != nil { > logger.Errorf("debug-log handler error: %v", err) > } > > ? Done.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
A few more thoughts. Still waiting for the tests... https://codereview.appspot.com/58510045/diff/1/state/apiserver/common.go File state/apiserver/common.go (right): https://codereview.appspot.com/58510045/diff/1/state/apiserver/common.go#newc... state/apiserver/common.go:20: type errorResponder interface { On 2014/01/30 16:55:20, mue wrote: > On 2014/01/30 14:34:09, rog wrote: > > I don't think we need this. We can use a common type for all error responses > > (e.g. > > > > type ErrorResponse struct { > > Error string > > } > > ) > > > > and the code below can just send that. > > > > We'll want to make sure that the error JSON is consistent > > across HTTP requests anyway, and this ensures that. > > The charmsHandler sends a CharmsResponse, which also contains a CharmURL. If > this shall be refactored it should be done later in a different CL. The charmsHandler does not need to send a CharmsResponse for errors. I would prefer to keep this code simple to start with. If you wish to avoid refactoring charmsHandler, then leave the code there the same as it is now, and change it to call this code later, rather than starting off with something complex here that we'll need to spend time simplifying later. https://codereview.appspot.com/58510045/diff/1/state/apiserver/common.go#newc... state/apiserver/common.go:26: type commonHandler struct { On 2014/01/30 16:55:20, mue wrote: > On 2014/01/30 14:34:09, rog wrote: > > I'm not that keen on this - it doesn't seem like it adds much value over just > > defining a few functions. What's the gain of embedding it? > > Based on your hint to factor out common parts. The methods of this type are used > in both, charmsHandler and logHandler. A function is a fine way of factoring out common parts. Embedding is nice to avoid unless we're actually leveraging the properties of embedding (it actually makes code harder to read). https://codereview.appspot.com/58510045/diff/60001/state/api/client.go File state/api/client.go (right): https://codereview.appspot.com/58510045/diff/60001/state/api/client.go#newcod... state/api/client.go:531: "juju-home": {osenv.JujuHomeDir()}, This doesn't seem quite right. Why are we sending a local directory name to the server? For the local provider, surely the server side can find out where the log file is kept? https://codereview.appspot.com/58510045/diff/60001/state/apiserver/charms.go File state/apiserver/charms.go (right): https://codereview.appspot.com/58510045/diff/60001/state/apiserver/charms.go#... state/apiserver/charms.go:18: envtesting "launchpad.net/juju-core/environs/testing" This is very wrong - production code must not import testing code. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go File state/apiserver/log.go (right): https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:158: _, err := os.Stat(defaultLogLocation) This doesn't seem right. It's possible that a machine with a local provider is also a node in a juju network, so we can't necessarily use the existence of /var/log/juju/all-machines.log as an indicator of whether the environment is local. I would perhaps look at the provider type in the environment config in the state, and change the path accordingly. I'd do that in newLogHandler to avoid doing the calculation on every log http request. One possibility is to make OpenAllMachinesLog a method on environs.EnvironProvider, e.g. // OpenAllMachinesLog opens the all-machines debug log // for reading. It should only be called on a state server // node. OpenAllMachinesLog() (io.ReadCloseSeeker, error) We should perhaps have a discussion about this with others.
Sign in to reply to this message.
Some observations and suggestions. https://codereview.appspot.com/58510045/diff/60001/cmd/juju/debuglog.go File cmd/juju/debuglog.go (right): https://codereview.appspot.com/58510045/diff/60001/cmd/juju/debuglog.go#newco... cmd/juju/debuglog.go:28: Stream the consolidated log file. The consolidated log file contains log messages s/log/debug log/ ? https://codereview.appspot.com/58510045/diff/60001/cmd/juju/debuglog.go#newco... cmd/juju/debuglog.go:59: debugLog, err := client.WatchDebugLog(c.lines, c.filter) You should consider the backwards compatibility when talking to an older apiserver, where WatchDebugLog is not implemented and you get params.IsCodeNotImplemented(err) == true. Look at other *1dot16 methods for examples. The compatibility code should be in a separate function (usually named run1dot16). https://codereview.appspot.com/58510045/diff/60001/state/api/client.go File state/api/client.go (right): https://codereview.appspot.com/58510045/diff/60001/state/api/client.go#newcod... state/api/client.go:561: // SetFilter sets the entity tags that apply to the filter. s/the entity tags that apply to the filter/the filter to apply./ https://codereview.appspot.com/58510045/diff/60001/state/api/client.go#newcod... state/api/client.go:577: // setBasicAuth sets the basic authentication of the passed header. // setBasicAuth creates an Authorization header for HTTP Basic Authentication, using the given username and password ? https://codereview.appspot.com/58510045/diff/60001/state/api/params/params.go File state/api/params/params.go (right): https://codereview.appspot.com/58510045/diff/60001/state/api/params/params.go... state/api/params/params.go:570: // EntityLogRequest describes a filtered request for entity logs. s/EntityLogRequest/DebugLogRequest/ ? There are no entities involved anymore, just a filter. Also, please move this in internal.go, where CharmResponse is. https://codereview.appspot.com/58510045/diff/60001/state/api/params/params.go... state/api/params/params.go:577: Error string `json:",omitempty"` If you define a params.SimpleError { Error string `json:",omitempty"` } you won't need this. See the comments below about sendError. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/apiserver.go File state/apiserver/apiserver.go (right): https://codereview.appspot.com/58510045/diff/60001/state/apiserver/apiserver.... state/apiserver/apiserver.go:156: mux.Handle("/log", newLogHandler(srv.state)) s/newLogHandler/newDebugLogHandler/ https://codereview.appspot.com/58510045/diff/60001/state/apiserver/charms.go File state/apiserver/charms.go (right): https://codereview.appspot.com/58510045/diff/60001/state/apiserver/charms.go#... state/apiserver/charms.go:18: envtesting "launchpad.net/juju-core/environs/testing" On 2014/01/31 15:59:11, rog wrote: > This is very wrong - production code must not import testing code. It's not really testing code, it's just GetEnvironStorage, which is used both in tests and in actual code. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/charms.go#... state/apiserver/charms.go:28: // newCharmsHandler creates a new charms handler. doc comment is not really needed here. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/charms.go#... state/apiserver/charms.go:43: h.sendError(h, w, http.StatusBadRequest, err.Error()) Please see the comments below about sendError - no need to add a responder as a first arg. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/charms.go#... state/apiserver/charms.go:171: // errorResponse wraps the message for an error response. Remove this please, see below. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/common.go File state/apiserver/common.go (right): https://codereview.appspot.com/58510045/diff/60001/state/apiserver/common.go#... state/apiserver/common.go:4: package apiserver Let's call this file something other than common, because there's already a common subpackage. baseHandler type and basehandler.go? https://codereview.appspot.com/58510045/diff/60001/state/apiserver/common.go#... state/apiserver/common.go:19: // errorResponder describes a method returning the wrapped error response instance. I think this is unnecessary. For all errors, you can define a params.SimpleError (in internal.go please, not params.go), which has just Error string `json:",omitempty"`. Then, sendError below can be simplified. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/common.go#... state/apiserver/common.go:25: // commonHandler provides common methods for individual handlers. s/commonHandler/baseHandler/ (see first comment in this file). https://codereview.appspot.com/58510045/diff/60001/state/apiserver/common.go#... state/apiserver/common.go:42: func (h *commonHandler) sendError(responder errorResponder, w http.ResponseWriter, statusCode int, message string, args ...interface{}) error { What's wrong with just func (h *baseHandler) sendError(w http.ResponseWrite r, statusCode int, message string) error ? Then you can just construct a ¶ms.SimpleError{message} inside and send it. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/common.go#... state/apiserver/common.go:47: func (h *commonHandler) sendAuthError(responder errorResponder, w http.ResponseWriter) { like sendError, no responder needed, just use a "unauthorized" message. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/common.go#... state/apiserver/common.go:52: // authenticate provides authentication for the embedding types. this should be an empty method or it should just panic("not implemented") in baseHandler, so the descendants can implement it as needed. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/common.go#... state/apiserver/common.go:57: // authenticateHTTPRequest parses HTTP basic authentication and authorizes the this shouldn't be here - it belongs only in charmsHandler, and it should be called authenticate(req *http.Request) error - you already have h.state, so no need to pass it. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go File state/apiserver/log.go (right): https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:4: package apiserver please, rename this file to debuglog.go, because it's less confusing with logger and other similar things. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:34: // logHandler takes requests to watch the debug log. s/logHandler/debugLogHandler/ (see above). https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:58: h.sendError(h, w, http.StatusInternalServerError, "cannot parse number of lines: %v", err) this looks more like StatusBadRequest https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:88: // errorResponse wraps the message for an error response. Remove this please, as explained before. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:144: var req params.EntityLogRequest how do you stop this handler? there should be a way other than "connection died, and we logged it". https://codereview.appspot.com/58510045/diff/60001/state/environ.go File state/environ.go (right): https://codereview.appspot.com/58510045/diff/60001/state/environ.go#newcode58 state/environ.go:58: // Name returns the name the environment. did you merge trunk by any chance? If not why this?
Sign in to reply to this message.
Forgot to mention, please update the CL description (it no longer takes -e entities).
Sign in to reply to this message.
https://codereview.appspot.com/58510045/diff/60001/state/apiserver/charms.go File state/apiserver/charms.go (right): https://codereview.appspot.com/58510045/diff/60001/state/apiserver/charms.go#... state/apiserver/charms.go:18: envtesting "launchpad.net/juju-core/environs/testing" On 2014/01/31 17:02:29, dimitern wrote: > On 2014/01/31 15:59:11, rog wrote: > > This is very wrong - production code must not import testing code. > > It's not really testing code, it's just GetEnvironStorage, which is used both in > tests and in actual code. It's a testing package, and testing packages are allowed dependencies which production packages are not (for example, it indirectly imports the standard testing package, which defines global environment flags). If we want GetEnvironStorage to be accessible from both tests and production code, it must be factored out into another (non-testing) package, where it can be called from both places. This is very important. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/charms.go#... state/apiserver/charms.go:171: // errorResponse wraps the message for an error response. On 2014/01/31 17:02:29, dimitern wrote: > Remove this please, see below. +1 https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go File state/apiserver/log.go (right): https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:34: // logHandler takes requests to watch the debug log. On 2014/01/31 17:02:29, dimitern wrote: > s/logHandler/debugLogHandler/ (see above). +1 https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:58: h.sendError(h, w, http.StatusInternalServerError, "cannot parse number of lines: %v", err) On 2014/01/31 17:02:29, dimitern wrote: > this looks more like StatusBadRequest +1 https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:115: tailer.Stop() Thinking about dimiter's remark below, perhaps here: case <-stream.srv.tomb.Dying(): wsConn.Close() or perhaps, just after stream.tomb.Done above: defer wsConn.Close() and when the server starts dying, we can just return. Perhaps things might be cleaner if we returned an error from loop and did the tomb cleanup and wsConn closing in the caller. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:144: var req params.EntityLogRequest On 2014/01/31 17:02:29, dimitern wrote: > how do you stop this handler? there should be a way other than "connection died, > and we logged it". I think it's reasonable to have the handler predicated on the existence of the websocket connection, actually, but we should probably close that when the Server is closed, and we should probably add the goroutine to the srv.wg wait group so that things are torn down synchronously. https://codereview.appspot.com/58510045/diff/60001/state/environ.go File state/environ.go (right): https://codereview.appspot.com/58510045/diff/60001/state/environ.go#newcode58 state/environ.go:58: // Name returns the name the environment. On 2014/01/31 17:02:29, dimitern wrote: > did you merge trunk by any chance? If not why this? It's used by the local-provider logfile-path-generation code. I'm not sure it's the right solution.
Sign in to reply to this message.
First feedback before continue on Monday. I'm a bit frustrated. I've got the problem of "late reviewers" here which means I first had an approach together with William. As it has been almost done a new approach came in by Roger. Indeed a very good one, worth the change, but with enough open topics to finish. And now, as it is maybe not yet optimal but working even with the local provider (a very long lasting open issue) a new review by Dimiter with remarks to the parts based on Rogers draft. *sigh* We're on a good way to over-engineer this software. https://codereview.appspot.com/58510045/diff/60001/cmd/juju/debuglog.go File cmd/juju/debuglog.go (right): https://codereview.appspot.com/58510045/diff/60001/cmd/juju/debuglog.go#newco... cmd/juju/debuglog.go:28: Stream the consolidated log file. The consolidated log file contains log messages On 2014/01/31 17:02:29, dimitern wrote: > s/log/debug log/ ? Is it really a debug log? It is the consolidated log of all machines and units, containing not only debug but also other messages with different levels. The more I think about it the more the name "debug-log" is wrong. https://codereview.appspot.com/58510045/diff/60001/cmd/juju/debuglog.go#newco... cmd/juju/debuglog.go:59: debugLog, err := client.WatchDebugLog(c.lines, c.filter) On 2014/01/31 17:02:29, dimitern wrote: > You should consider the backwards compatibility when talking to an older > apiserver, where WatchDebugLog is not implemented and you get > params.IsCodeNotImplemented(err) == true. Look at other *1dot16 methods for > examples. The compatibility code should be in a separate function (usually named > run1dot16). Have to look at it, thanks. https://codereview.appspot.com/58510045/diff/60001/state/api/client.go File state/api/client.go (right): https://codereview.appspot.com/58510045/diff/60001/state/api/client.go#newcod... state/api/client.go:531: "juju-home": {osenv.JujuHomeDir()}, On 2014/01/31 15:59:11, rog wrote: > This doesn't seem quite right. Why are we sending a local directory name to the > server? For the local provider, surely the server side can find out where the > log file is kept? Sure there may be a way to change environ. But the local machine 0 is running as root and doesn't know about the JUJU_HOME of the user who has started it. https://codereview.appspot.com/58510045/diff/60001/state/api/client.go#newcod... state/api/client.go:561: // SetFilter sets the entity tags that apply to the filter. On 2014/01/31 17:02:29, dimitern wrote: > s/the entity tags that apply to the filter/the filter to apply./ Will change. https://codereview.appspot.com/58510045/diff/60001/state/api/client.go#newcod... state/api/client.go:577: // setBasicAuth sets the basic authentication of the passed header. On 2014/01/31 17:02:29, dimitern wrote: > // setBasicAuth creates an Authorization header for HTTP Basic Authentication, > using the given username and password ? Please discuss with Roger. It's based on his draft. https://codereview.appspot.com/58510045/diff/60001/state/api/params/params.go File state/api/params/params.go (right): https://codereview.appspot.com/58510045/diff/60001/state/api/params/params.go... state/api/params/params.go:570: // EntityLogRequest describes a filtered request for entity logs. On 2014/01/31 17:02:29, dimitern wrote: > s/EntityLogRequest/DebugLogRequest/ ? There are no entities involved anymore, > just a filter. Also, please move this in internal.go, where CharmResponse is. Oh, yes, have to rename it as the way of filtering changed. Thanks. But William wanted me to move it from internal to params. *sigh* https://codereview.appspot.com/58510045/diff/60001/state/apiserver/apiserver.go File state/apiserver/apiserver.go (right): https://codereview.appspot.com/58510045/diff/60001/state/apiserver/apiserver.... state/apiserver/apiserver.go:156: mux.Handle("/log", newLogHandler(srv.state)) On 2014/01/31 17:02:29, dimitern wrote: > s/newLogHandler/newDebugLogHandler/ See comment before. Maybe allMachinesLogHandler. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/charms.go File state/apiserver/charms.go (right): https://codereview.appspot.com/58510045/diff/60001/state/apiserver/charms.go#... state/apiserver/charms.go:18: envtesting "launchpad.net/juju-core/environs/testing" On 2014/01/31 15:59:11, rog wrote: > This is very wrong - production code must not import testing code. It's not by me, it's only merged from trunk. So ask the source of this change. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/common.go File state/apiserver/common.go (right): https://codereview.appspot.com/58510045/diff/60001/state/apiserver/common.go#... state/apiserver/common.go:4: package apiserver On 2014/01/31 17:02:29, dimitern wrote: > Let's call this file something other than common, because there's already a > common subpackage. baseHandler type and basehandler.go? See discussion with Roger, he wants a different refactoring. The whole idea here is to factor out common parts of the charmsHandler and the new logWhateverHandler. I'll now reset the whole charms stuff as it has been before and let the log stuff have its copy. A later CL then can do the refactoring. Otherwise we're mixing stuff. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/common.go#... state/apiserver/common.go:19: // errorResponder describes a method returning the wrapped error response instance. On 2014/01/31 17:02:29, dimitern wrote: > I think this is unnecessary. For all errors, you can define a params.SimpleError > (in internal.go please, not params.go), which has just Error string > `json:",omitempty"`. Then, sendError below can be simplified. See above. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/common.go#... state/apiserver/common.go:57: // authenticateHTTPRequest parses HTTP basic authentication and authorizes the On 2014/01/31 17:02:29, dimitern wrote: > this shouldn't be here - it belongs only in charmsHandler, and it should be > called authenticate(req *http.Request) error - you already have h.state, so no > need to pass it. It is used in both handler and Roger refactored it this way. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go File state/apiserver/log.go (right): https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:4: package apiserver On 2014/01/31 17:02:29, dimitern wrote: > please, rename this file to debuglog.go, because it's less confusing with logger > and other similar things. You followed the whole process? It's funny how names and places shall change the more reviewer look into the code (William, Roger and now you). https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:58: h.sendError(h, w, http.StatusInternalServerError, "cannot parse number of lines: %v", err) On 2014/01/31 17:02:29, dimitern wrote: > this looks more like StatusBadRequest Will change, thanks. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:88: // errorResponse wraps the message for an error response. On 2014/01/31 17:02:29, dimitern wrote: > Remove this please, as explained before. Where does this comment hurt? https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:144: var req params.EntityLogRequest On 2014/01/31 17:02:29, dimitern wrote: > how do you stop this handler? there should be a way other than "connection died, > and we logged it". The CLI would not send it, it is stopped with Ctrl-C. But could be interesting for the GUI, yes. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:158: _, err := os.Stat(defaultLogLocation) On 2014/01/31 15:59:11, rog wrote: > This doesn't seem right. It's possible that a machine with a local provider is > also a node in a juju network, so we can't necessarily use the existence of > /var/log/juju/all-machines.log as an indicator of whether the environment is > local. > > I would perhaps look at the provider type in the environment config in the > state, and change the path accordingly. I'd do that in newLogHandler to avoid > doing the calculation on every log http request. > > One possibility is to make OpenAllMachinesLog a method on > environs.EnvironProvider, e.g. > > // OpenAllMachinesLog opens the all-machines debug log > // for reading. It should only be called on a state server > // node. > OpenAllMachinesLog() (io.ReadCloseSeeker, error) > > We should perhaps have a discussion about this with others. If you find a good way I'm happy. I didn't find one. As I said, the location depends on the user who bootstrapped the environment. This solution may not be the best, but it is lightweighted.
Sign in to reply to this message.
Replies.. I didn't mean to suggest against what William and Roger told you, but just to express my thoughts about the CL. As always, it's up to you to actually pick what to change and discuss disagreements if you need. https://codereview.appspot.com/58510045/diff/60001/cmd/juju/debuglog.go File cmd/juju/debuglog.go (right): https://codereview.appspot.com/58510045/diff/60001/cmd/juju/debuglog.go#newco... cmd/juju/debuglog.go:28: Stream the consolidated log file. The consolidated log file contains log messages On 2014/01/31 17:32:37, mue wrote: > On 2014/01/31 17:02:29, dimitern wrote: > > s/log/debug log/ ? > > Is it really a debug log? It is the consolidated log of all machines and units, > containing not only debug but also other messages with different levels. The > more I think about it the more the name "debug-log" is wrong. Until we change the command to something else than debug-log, I think for consistency we should use "debug log" everywhere where the command is involved. https://codereview.appspot.com/58510045/diff/60001/state/api/client.go File state/api/client.go (right): https://codereview.appspot.com/58510045/diff/60001/state/api/client.go#newcod... state/api/client.go:577: // setBasicAuth sets the basic authentication of the passed header. On 2014/01/31 17:32:37, mue wrote: > On 2014/01/31 17:02:29, dimitern wrote: > > // setBasicAuth creates an Authorization header for HTTP Basic Authentication, > > using the given username and password ? > > Please discuss with Roger. It's based on his draft. I'm just suggesting a more appropriate description, based on the one found in http.SetBasicAuth. I'm not suggesting to change the code. https://codereview.appspot.com/58510045/diff/60001/state/api/params/params.go File state/api/params/params.go (right): https://codereview.appspot.com/58510045/diff/60001/state/api/params/params.go... state/api/params/params.go:570: // EntityLogRequest describes a filtered request for entity logs. On 2014/01/31 17:32:37, mue wrote: > On 2014/01/31 17:02:29, dimitern wrote: > > s/EntityLogRequest/DebugLogRequest/ ? There are no entities involved anymore, > > just a filter. Also, please move this in internal.go, where CharmResponse is. > > Oh, yes, have to rename it as the way of filtering changed. Thanks. But William > wanted me to move it from internal to params. *sigh* I'm fine if it's in params.go, but then please move CharmsResponse here as well. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/charms.go File state/apiserver/charms.go (right): https://codereview.appspot.com/58510045/diff/60001/state/apiserver/charms.go#... state/apiserver/charms.go:18: envtesting "launchpad.net/juju-core/environs/testing" On 2014/01/31 17:32:37, mue wrote: > On 2014/01/31 15:59:11, rog wrote: > > This is very wrong - production code must not import testing code. > > It's not by me, it's only merged from trunk. So ask the source of this change. I'm to blame for this, but since it's out of scope for this CL, I vote to keep it like this, and once Frank lands this, I can propose a change to move into a non-testing package. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/common.go File state/apiserver/common.go (right): https://codereview.appspot.com/58510045/diff/60001/state/apiserver/common.go#... state/apiserver/common.go:4: package apiserver On 2014/01/31 17:32:37, mue wrote: > On 2014/01/31 17:02:29, dimitern wrote: > > Let's call this file something other than common, because there's already a > > common subpackage. baseHandler type and basehandler.go? > > See discussion with Roger, he wants a different refactoring. The whole idea here > is to factor out common parts of the charmsHandler and the new > logWhateverHandler. I'll now reset the whole charms stuff as it has been before > and let the log stuff have its copy. A later CL then can do the refactoring. > Otherwise we're mixing stuff. I'm just suggesting to reduce confusion by not having both a common subpackage and a common.go in apiserver. I'm ok with factoring out common parts of both handlers. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/common.go#... state/apiserver/common.go:19: // errorResponder describes a method returning the wrapped error response instance. On 2014/01/31 17:32:37, mue wrote: > On 2014/01/31 17:02:29, dimitern wrote: > > I think this is unnecessary. For all errors, you can define a > params.SimpleError > > (in internal.go please, not params.go), which has just Error string > > `json:",omitempty"`. Then, sendError below can be simplified. > > See above. Roger, can you explain why having an interface{} return is better than not using interface{} at all and instead returning errors using a params.SimpleError { Error string ... } ? https://codereview.appspot.com/58510045/diff/60001/state/apiserver/common.go#... state/apiserver/common.go:57: // authenticateHTTPRequest parses HTTP basic authentication and authorizes the On 2014/01/31 17:32:37, mue wrote: > On 2014/01/31 17:02:29, dimitern wrote: > > this shouldn't be here - it belongs only in charmsHandler, and it should be > > called authenticate(req *http.Request) error - you already have h.state, so no > > need to pass it. > > It is used in both handler and Roger refactored it this way. Sorry, my mistake. I actually meant we don't need both of these methods - just the authenticate(req *http.Request) should work for both handlers (i.e. remove old authenticate() and s/authenticateHTTPRequest/authenticate/). You won't need to pass st *state.State, as you have it in h. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go File state/apiserver/log.go (right): https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:4: package apiserver On 2014/01/31 17:32:37, mue wrote: > On 2014/01/31 17:02:29, dimitern wrote: > > please, rename this file to debuglog.go, because it's less confusing with > logger > > and other similar things. > > You followed the whole process? It's funny how names and places shall change the > more reviewer look into the code (William, Roger and now you). I'm just saying what I think will improve the code. Don't feel obliged to listen to me just for the sake of consistency ;) (joke) https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:88: // errorResponse wraps the message for an error response. On 2014/01/31 17:32:37, mue wrote: > On 2014/01/31 17:02:29, dimitern wrote: > > Remove this please, as explained before. > > Where does this comment hurt? I meant the whole method errorResponse (see previous comments - I also asked Roger why he thinks this is better). https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:115: tailer.Stop() On 2014/01/31 17:27:18, rog wrote: > Thinking about dimiter's remark below, perhaps here: > > case <-stream.srv.tomb.Dying(): > wsConn.Close() > > or perhaps, just after stream.tomb.Done above: > > defer wsConn.Close() > > and when the server starts dying, we can just return. > Perhaps things might be cleaner if we returned an error > from loop and did the tomb cleanup and wsConn closing in > the caller. +1
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/58510045/diff/60001/cmd/juju/debuglog.go File cmd/juju/debuglog.go (right): https://codereview.appspot.com/58510045/diff/60001/cmd/juju/debuglog.go#newco... cmd/juju/debuglog.go:28: Stream the consolidated log file. The consolidated log file contains log messages On 2014/01/31 17:57:09, dimitern wrote: > On 2014/01/31 17:32:37, mue wrote: > > On 2014/01/31 17:02:29, dimitern wrote: > > > s/log/debug log/ ? > > > > Is it really a debug log? It is the consolidated log of all machines and > units, > > containing not only debug but also other messages with different levels. The > > more I think about it the more the name "debug-log" is wrong. > > Until we change the command to something else than debug-log, I think for > consistency we should use "debug log" everywhere where the command is involved. Sounds reasonable. https://codereview.appspot.com/58510045/diff/60001/state/api/client.go File state/api/client.go (right): https://codereview.appspot.com/58510045/diff/60001/state/api/client.go#newcod... state/api/client.go:561: // SetFilter sets the entity tags that apply to the filter. On 2014/01/31 17:32:37, mue wrote: > On 2014/01/31 17:02:29, dimitern wrote: > > s/the entity tags that apply to the filter/the filter to apply./ > > Will change. Done. https://codereview.appspot.com/58510045/diff/60001/state/api/client.go#newcod... state/api/client.go:577: // setBasicAuth sets the basic authentication of the passed header. On 2014/01/31 17:57:09, dimitern wrote: > On 2014/01/31 17:32:37, mue wrote: > > On 2014/01/31 17:02:29, dimitern wrote: > > > // setBasicAuth creates an Authorization header for HTTP Basic > Authentication, > > > using the given username and password ? > > > > Please discuss with Roger. It's based on his draft. > > I'm just suggesting a more appropriate description, based on the one found in > http.SetBasicAuth. I'm not suggesting to change the code. Done. https://codereview.appspot.com/58510045/diff/60001/state/api/params/params.go File state/api/params/params.go (right): https://codereview.appspot.com/58510045/diff/60001/state/api/params/params.go... state/api/params/params.go:570: // EntityLogRequest describes a filtered request for entity logs. On 2014/01/31 17:57:09, dimitern wrote: > On 2014/01/31 17:32:37, mue wrote: > > On 2014/01/31 17:02:29, dimitern wrote: > > > s/EntityLogRequest/DebugLogRequest/ ? There are no entities involved > anymore, > > > just a filter. Also, please move this in internal.go, where CharmResponse > is. > > > > Oh, yes, have to rename it as the way of filtering changed. Thanks. But > William > > wanted me to move it from internal to params. *sigh* > > I'm fine if it's in params.go, but then please move CharmsResponse here as well. Renamed and moved to internal. https://codereview.appspot.com/58510045/diff/60001/state/api/params/params.go... state/api/params/params.go:577: Error string `json:",omitempty"` On 2014/01/31 17:02:29, dimitern wrote: > If you define a params.SimpleError { Error string `json:",omitempty"` } you > won't need this. See the comments below about sendError. Done. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/apiserver.go File state/apiserver/apiserver.go (right): https://codereview.appspot.com/58510045/diff/60001/state/apiserver/apiserver.... state/apiserver/apiserver.go:156: mux.Handle("/log", newLogHandler(srv.state)) On 2014/01/31 17:32:37, mue wrote: > On 2014/01/31 17:02:29, dimitern wrote: > > s/newLogHandler/newDebugLogHandler/ > > See comment before. Maybe allMachinesLogHandler. Renamed in debugLogHandler. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/common.go File state/apiserver/common.go (right): https://codereview.appspot.com/58510045/diff/60001/state/apiserver/common.go#... state/apiserver/common.go:4: package apiserver On 2014/01/31 17:57:09, dimitern wrote: > On 2014/01/31 17:32:37, mue wrote: > > On 2014/01/31 17:02:29, dimitern wrote: > > > Let's call this file something other than common, because there's already a > > > common subpackage. baseHandler type and basehandler.go? > > > > See discussion with Roger, he wants a different refactoring. The whole idea > here > > is to factor out common parts of the charmsHandler and the new > > logWhateverHandler. I'll now reset the whole charms stuff as it has been > before > > and let the log stuff have its copy. A later CL then can do the refactoring. > > Otherwise we're mixing stuff. > > I'm just suggesting to reduce confusion by not having both a common subpackage > and a common.go in apiserver. I'm ok with factoring out common parts of both > handlers. This file doesn't exist anymore. Factoring out common parts will be in a later CL. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go File state/apiserver/log.go (right): https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:4: package apiserver On 2014/01/31 17:57:09, dimitern wrote: > On 2014/01/31 17:32:37, mue wrote: > > On 2014/01/31 17:02:29, dimitern wrote: > > > please, rename this file to debuglog.go, because it's less confusing with > > logger > > > and other similar things. > > > > You followed the whole process? It's funny how names and places shall change > the > > more reviewer look into the code (William, Roger and now you). > > I'm just saying what I think will improve the code. Don't feel obliged to listen > to me just for the sake of consistency ;) (joke) Changed it. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:34: // logHandler takes requests to watch the debug log. On 2014/01/31 17:27:18, rog wrote: > On 2014/01/31 17:02:29, dimitern wrote: > > s/logHandler/debugLogHandler/ (see above). > > +1 Done. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:58: h.sendError(h, w, http.StatusInternalServerError, "cannot parse number of lines: %v", err) On 2014/01/31 17:32:37, mue wrote: > On 2014/01/31 17:02:29, dimitern wrote: > > this looks more like StatusBadRequest > > Will change, thanks. Done. https://codereview.appspot.com/58510045/diff/60001/state/apiserver/log.go#new... state/apiserver/log.go:88: // errorResponse wraps the message for an error response. On 2014/01/31 17:57:09, dimitern wrote: > On 2014/01/31 17:32:37, mue wrote: > > On 2014/01/31 17:02:29, dimitern wrote: > > > Remove this please, as explained before. > > > > Where does this comment hurt? > > I meant the whole method errorResponse (see previous comments - I also asked > Roger why he thinks this is better). Removed all changes on charmsHandler and the common/base part for a later refactoring. https://codereview.appspot.com/58510045/diff/60001/state/environ.go File state/environ.go (right): https://codereview.appspot.com/58510045/diff/60001/state/environ.go#newcode58 state/environ.go:58: // Name returns the name the environment. On 2014/01/31 17:27:18, rog wrote: > On 2014/01/31 17:02:29, dimitern wrote: > > did you merge trunk by any chance? If not why this? > > It's used by the local-provider logfile-path-generation code. I'm not sure it's > the right solution. An alternative solution would be a change of the environ interface and according implementations at the providers. Still the problem there would be that the API server would have to know the JUJU_HOME of the local user who bootstrapped the environment. The log path isn't used anywhere else because it is only hanled during bootstrapping to created the conf of the rsyslog. So that name also that name would have to be persisted.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
A few comments pending unit tests. https://codereview.appspot.com/58510045/diff/100001/cmd/juju/debuglog.go File cmd/juju/debuglog.go (right): https://codereview.appspot.com/58510045/diff/100001/cmd/juju/debuglog.go#newc... cmd/juju/debuglog.go:86: args := append([]string{"0"}, tailcmd) or: args := []string{"0", tailcmd} ? https://codereview.appspot.com/58510045/diff/100001/state/apiserver/debuglog.go File state/apiserver/debuglog.go (right): https://codereview.appspot.com/58510045/diff/100001/state/apiserver/debuglog.... state/apiserver/debuglog.go:44: logLocation := values.Get("location") This is still wrong. It's actually a glaring security hole too, because it allows an API client to read any file on the server with root privileges. If you don't want to fix it now (which is reasonable - it might involve changing a few extra things), then perhaps just make it fail on the local provider and we can fix it later. But we need to delete the "location" attribute. https://codereview.appspot.com/58510045/diff/100001/state/apiserver/debuglog.... state/apiserver/debuglog.go:57: filter := values.Get("filter") It might be better to parse the regexp here while we can still return an error if it fails. https://codereview.appspot.com/58510045/diff/100001/state/apiserver/debuglog.... state/apiserver/debuglog.go:174: stream.filterRx, err = regexp.Compile(filter) If the regexp fails, I'm not sure we want to disable all filtering. I think a better approach might be to leave the log filtering unchanged.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/58510045/diff/100001/cmd/juju/debuglog.go File cmd/juju/debuglog.go (right): https://codereview.appspot.com/58510045/diff/100001/cmd/juju/debuglog.go#newc... cmd/juju/debuglog.go:86: args := append([]string{"0"}, tailcmd) On 2014/02/03 14:01:12, rog wrote: > or: > args := []string{"0", tailcmd} > > ? Done. https://codereview.appspot.com/58510045/diff/100001/state/apiserver/debuglog.go File state/apiserver/debuglog.go (right): https://codereview.appspot.com/58510045/diff/100001/state/apiserver/debuglog.... state/apiserver/debuglog.go:44: logLocation := values.Get("location") On 2014/02/03 14:01:12, rog wrote: > This is still wrong. It's actually a glaring security hole too, because it > allows an API client to read any file on the server with root privileges. > > If you don't want to fix it now (which is reasonable - it might involve changing > a few extra things), then perhaps just make it fail on the local provider and we > can fix it later. But we need to delete the "location" attribute. Removed it as a first step to find a better way for the local provider issue later. https://codereview.appspot.com/58510045/diff/100001/state/apiserver/debuglog.... state/apiserver/debuglog.go:57: filter := values.Get("filter") On 2014/02/03 14:01:12, rog wrote: > It might be better to parse the regexp here while we can still return an error > if it fails. Done. https://codereview.appspot.com/58510045/diff/100001/state/apiserver/debuglog.... state/apiserver/debuglog.go:174: stream.filterRx, err = regexp.Compile(filter) On 2014/02/03 14:01:12, rog wrote: > If the regexp fails, I'm not sure we want to disable all filtering. I think a > better approach might be to leave the log filtering unchanged. Done.
Sign in to reply to this message.
Looks good, thanks, pending unit tests. https://codereview.appspot.com/58510045/diff/120001/cmd/juju/debuglog.go File cmd/juju/debuglog.go (right): https://codereview.appspot.com/58510045/diff/120001/cmd/juju/debuglog.go#newc... cmd/juju/debuglog.go:90: grepCmd := exec.Command("grep", c.filter) I think you probably want grep -E here for compatibility with the regexps accepted by the juju version.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
A few comments; I'd like rog to check it as well, please. https://codereview.appspot.com/58510045/diff/140001/cmd/juju/debuglog.go File cmd/juju/debuglog.go (right): https://codereview.appspot.com/58510045/diff/140001/cmd/juju/debuglog.go#newc... cmd/juju/debuglog.go:67: logger.Debugf("CALLING WATCH DEBUG LOG") d? https://codereview.appspot.com/58510045/diff/140001/cmd/juju/debuglog.go#newc... cmd/juju/debuglog.go:76: _, err = io.Copy(os.Stdout, debugLog) stdout should come from ctx https://codereview.appspot.com/58510045/diff/140001/cmd/juju/debuglog.go#newc... cmd/juju/debuglog.go:90: logLocation := fmt.Sprintf("%s/%s/log/all-machines.log", osenv.JujuHomeDir(), name) if there isn't some convenient LogDir func somewhere, there probably should be. https://codereview.appspot.com/58510045/diff/140001/cmd/juju/debuglog.go#newc... cmd/juju/debuglog.go:98: grepCmd.Stderr = os.Stderr stdout/stderr should come from ctx https://codereview.appspot.com/58510045/diff/140001/cmd/juju/debuglog_test.go File cmd/juju/debuglog_test.go (left): https://codereview.appspot.com/58510045/diff/140001/cmd/juju/debuglog_test.go... cmd/juju/debuglog_test.go:74: c.Assert(err, gc.ErrorMatches, "invalid value \"fnord\" for flag -n: invalid number of lines") I think we should probably have some more tests than this..? https://codereview.appspot.com/58510045/diff/140001/environs/config/config.go File environs/config/config.go (right): https://codereview.appspot.com/58510045/diff/140001/environs/config/config.go... environs/config/config.go:65: DefaultLogLocation string = "/var/log/juju/all-machines.log" I feel like this should be the logging directory, not the eventually-aggregated log file, otherwise how do we put the sibling log files in the right place? https://codereview.appspot.com/58510045/diff/140001/environs/config/config.go... environs/config/config.go:167: c.defined["log-location"] = DefaultLogLocation this feels like it should take the local provider into account so that the debug-log command can use it, perhaps? https://codereview.appspot.com/58510045/diff/140001/environs/config/config.go... environs/config/config.go:809: "log-location", ...and tbh I'm worried that making this user-configurable will lead to all sorts of ugliness. What happens if they set-env in a running environment? Does rsyslog *always* use the right one? etc etc https://codereview.appspot.com/58510045/diff/140001/state/api/params/internal.go File state/api/params/internal.go (right): https://codereview.appspot.com/58510045/diff/140001/state/api/params/internal... state/api/params/internal.go:549: } public API, so not for internal.go, right? https://codereview.appspot.com/58510045/diff/140001/state/apiserver/debuglog.go File state/apiserver/debuglog.go (right): https://codereview.appspot.com/58510045/diff/140001/state/apiserver/debuglog.... state/apiserver/debuglog.go:173: defer stream.mux.Unlock() no need to lock until we've compiled the regex, surely?
Sign in to reply to this message.
Some responses to William's remarks. https://codereview.appspot.com/58510045/diff/140001/environs/config/config.go File environs/config/config.go (right): https://codereview.appspot.com/58510045/diff/140001/environs/config/config.go... environs/config/config.go:167: c.defined["log-location"] = DefaultLogLocation On 2014/02/17 13:34:42, fwereade wrote: > this feels like it should take the local provider into account so that the > debug-log command can use it, perhaps? That seems like it would be a boundary violation. If anything, I'd suggest that there is no default log location - it must be set by the provider, otherwise you get no logging. https://codereview.appspot.com/58510045/diff/140001/environs/config/config.go... environs/config/config.go:809: "log-location", On 2014/02/17 13:34:42, fwereade wrote: > ...and tbh I'm worried that making this user-configurable will lead to all sorts > of ugliness. What happens if they set-env in a running environment? Does rsyslog > *always* use the right one? etc etc I queried that too, but it's in immutableAttributes, so won't it be ok? https://codereview.appspot.com/58510045/diff/140001/state/api/params/internal.go File state/api/params/internal.go (right): https://codereview.appspot.com/58510045/diff/140001/state/api/params/internal... state/api/params/internal.go:549: } On 2014/02/17 13:34:42, fwereade wrote: > public API, so not for internal.go, right? +1 https://codereview.appspot.com/58510045/diff/140001/state/apiserver/debuglog.go File state/apiserver/debuglog.go (right): https://codereview.appspot.com/58510045/diff/140001/state/apiserver/debuglog.... state/apiserver/debuglog.go:173: defer stream.mux.Unlock() On 2014/02/17 13:34:42, fwereade wrote: > no need to lock until we've compiled the regex, surely? That's strictly true, but you could make an argument that since we know that the filtering is about to change, it's ok to hold up the stream for a moment while we decide what the new filter should be. I don't mind either way.
Sign in to reply to this message.
responses to responses ;) https://codereview.appspot.com/58510045/diff/140001/environs/config/config.go File environs/config/config.go (right): https://codereview.appspot.com/58510045/diff/140001/environs/config/config.go... environs/config/config.go:167: c.defined["log-location"] = DefaultLogLocation On 2014/02/17 14:01:28, rog wrote: > On 2014/02/17 13:34:42, fwereade wrote: > > this feels like it should take the local provider into account so that the > > debug-log command can use it, perhaps? > > That seems like it would be a boundary violation. > If anything, I'd suggest that there is no default log location - it must be set > by the provider, otherwise you get no logging. agree boundary violation; I sort of feel that if logging config is anything it's *machine*-level config, because that's the actual granularity of variation. https://codereview.appspot.com/58510045/diff/140001/environs/config/config.go... environs/config/config.go:809: "log-location", On 2014/02/17 14:01:28, rog wrote: > On 2014/02/17 13:34:42, fwereade wrote: > > ...and tbh I'm worried that making this user-configurable will lead to all > sorts > > of ugliness. What happens if they set-env in a running environment? Does > rsyslog > > *always* use the right one? etc etc > > I queried that too, but it's in immutableAttributes, so won't it be ok? Good point about immutableAttributes, but I'm still worried: I just counted 31 `/var/log/juju`s in trunk, and if we're not fixing all of them then this configurability is just going to break stuff.
Sign in to reply to this message.
Hey, another thought. `filter=<REGEXP>` is a bit of a layer violation -- it means that clients need to know our log format to filter, and changing our log format can break the API. Shouldn't we still be talking in terms of tags like we were originally? (regexp filter *as well* would be fine, I think, but I think we want to do the entity filtering properly)
Sign in to reply to this message.
https://codereview.appspot.com/58510045/diff/140001/environs/config/config.go File environs/config/config.go (right): https://codereview.appspot.com/58510045/diff/140001/environs/config/config.go... environs/config/config.go:809: "log-location", On 2014/02/18 08:39:36, fwereade wrote: > On 2014/02/17 14:01:28, rog wrote: > > On 2014/02/17 13:34:42, fwereade wrote: > > > ...and tbh I'm worried that making this user-configurable will lead to all > > sorts > > > of ugliness. What happens if they set-env in a running environment? Does > > rsyslog > > > *always* use the right one? etc etc > > > > I queried that too, but it's in immutableAttributes, so won't it be ok? > > Good point about immutableAttributes, but I'm still worried: I just counted 31 > `/var/log/juju`s in trunk, and if we're not fixing all of them then this > configurability is just going to break stuff. I count only 11, but point taken. We should probably change things so that the user *cannot* specify the log-location (for example by changing environs.Prepare to check that it's not set). Then it can only be set by the provider. But that's not to say that all the /var/log/juju cases are currently correct, even in the existing situation. We should check. For the record: ./log/syslog/config.go:63: :syslogtag, startswith, "juju{{namespace}}-" /var/log/juju{{namespace}}/all-machines.log;JujuLogFormat{{namespace}} ./log/syslog/config.go:123: LogDir: "/var/log/juju", ./log/syslog/config.go:138: LogDir: "/var/log/juju", ./container/lxc/lxc.go:55: logdir := "/var/log/juju" ./container/lxc/lxc.go:187: const internalLogDirTemplate = "%s/%s/rootfs/var/log/juju" ./container/kvm/kvm.go:62: conf.LogDir = "/var/log/juju" ./provider/local/environ.go:162: logfile := fmt.Sprintf("/var/log/juju-%s/all-machines.log", env.config.namespace()) ./cmd/plugins/juju-restore/restore.go:105: rm -r /var/lib/juju /var/log/juju ./cmd/juju/debuglog.go:85: tailcmd := fmt.Sprintf("tail -n %s -f /var/log/juju/all-machines.log", &c.lines) ./worker/deployer/simple.go:69: logDir: "/var/log/juju", ./environs/cloudinit.go:29: const LogDir = "/var/log/juju"
Sign in to reply to this message.
On 2014/02/18 10:06:21, rog wrote: > https://codereview.appspot.com/58510045/diff/140001/environs/config/config.go > File environs/config/config.go (right): > > https://codereview.appspot.com/58510045/diff/140001/environs/config/config.go... > environs/config/config.go:809: "log-location", > On 2014/02/18 08:39:36, fwereade wrote: > > On 2014/02/17 14:01:28, rog wrote: > > > On 2014/02/17 13:34:42, fwereade wrote: > > > > ...and tbh I'm worried that making this user-configurable will lead to all > > > sorts > > > > of ugliness. What happens if they set-env in a running environment? Does > > > rsyslog > > > > *always* use the right one? etc etc > > > > > > I queried that too, but it's in immutableAttributes, so won't it be ok? > > > > Good point about immutableAttributes, but I'm still worried: I just counted 31 > > `/var/log/juju`s in trunk, and if we're not fixing all of them then this > > configurability is just going to break stuff. > > I count only 11, but point taken. I think I probably counted test files too. And I *think* that's the right thing to do when considering impact of a fix. But, ehh. > We should probably change things so that the user *cannot* specify > the log-location (for example by changing environs.Prepare to check > that it's not set). Then it can only be set by the provider. > > But that's not to say that all the /var/log/juju cases are currently > correct, even in the existing situation. We should check. Could do, but I think I like your agent.Conf idea more. For the record, I'm rejecting this specific branch so that Frank can go work on hardware enablement bugs; dimitern's taking over this work in the hope of landing it for 1.18.
Sign in to reply to this message.
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
