/juju

Add migration logtransfer endpoint #6588

Merged
merged 8 commits into from Nov 23, 2016

Conversation

Reviewers

@howbazaar howbazaar

@anastasiamac anastasiamac

@mjs mjs

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
5 participants
@babbageclunk @jujubot @mjs @howbazaar @anastasiamac
@babbageclunk
Member

babbageclunk commented Nov 21, 2016 •
Edited 1 time

This lives on the API server at /migrate/logtransfer and is similar to the logsink handler. The main differences are:

  • instead of requiring machine agent credentials it requires a controller superuser
  • the destination model is determined by the X-Juju-Migration-Model-UUID header instead of the url
  • the log records include the entity (rather than it being set as the connected agent), because they're streamed for all entities in the source model.

This will be used by the logtransfer phase of the migrationmaster worker.
@babbageclunk
Member

babbageclunk commented Nov 21, 2016

!!build!!
apiserver/apiserver.go
if err != nil {
return nil, errors.Annotate(err, "creating logsink writer")
}
srv.logSinkWriter = logSinkWriter
+ migratedLogWriter, err := newLogSinkWriter(filepath.Join(srv.logDir, "migrated.log"))
@howbazaar

howbazaar Nov 21, 2016

Owner

Hmm... I'm wondering about this.
Don't you think it would make more sense to have a per-model migration log file?
That way they could be deleted individually at a later date.
This would require the file to be defined at connection time rather than endpoint creation time.

@babbageclunk

babbageclunk Nov 21, 2016

Member

(As discussed) I can see why this would be nice, but having the files per-model would also mean having to come up with another log rotation strategy so that the logs didn't take up unbounded disk space. The current ones get written via lumberjack which handles that for us. (You said you were alright with this given that it's really a backup for the DB logs.)

@mjs

mjs Nov 22, 2016

Contributor

There is still a problem here in that migrated.log and any rotated copies will be left behind unmanaged. Given that migrations will be fairly infrequent this potentially eats up another 900MB of disk on each controller machine with no real purpose.

I wonder if we even need a file backup of the migrated logs? It really is just a last resort thing if mongodb dies and I don't think it's super important for historical logs. Post migration, any new activity on the new controller will be captured in logsink.log. My gut says not to have a log file for migrated logs (i.e. allow the Writer passed to newLogSinkHandler to be nil)

@babbageclunk

babbageclunk Nov 23, 2016

Member

Ok, that seems much simpler.

apiserver/logtransfer.go
+ // Here the log messages are expected to be coming from another
+ // Juju controller, so the version number provided should be the
+ // Juju version of the source controller.
+ ver, err := jujuClientVersionFromReq(req)
@howbazaar

howbazaar Nov 21, 2016

Owner

Isn't it possible that there would be different versions on different log records? If the previous controller or model was upgraded recently? Is the stored client version the model version or the controller version?

@babbageclunk

babbageclunk Nov 21, 2016

Member

The version in the DB doesn't come back from debug-log (which is the source for these logs) at the moment. I could add it there, but I'm not sure why we need it or what the benefit would be. Will chat with Menno about it when he's back in tomorrow. At the moment it'll be the "client" version, which in this case is the jujud version of the source controller I think.

@mjs

mjs Nov 22, 2016

Contributor

As discussed in a hangout yesterday, storing the version per log message is kinda dumb and we shouldn't persist with that mistake with migrations. Sending the client's Juju version on connect is potentially useful though so let's keep doing that.

@babbageclunk

babbageclunk Nov 23, 2016

Member

Oh, you mean write out a version of 0 to the DB log? That makes sense - one less place to change when we rip it out later.

@anastasiamac

anastasiamac Nov 23, 2016

Member

Please add a bug number for version/log message in the codebase - lp#1643743

@mjs

mjs Nov 23, 2016

Contributor

I wasn't suggesting writing out version 0 to the log. Probably just use the target controller's version for that.

@mjs

mjs reviewed Nov 22, 2016
View changes

Looks great overall. We just need to nail down the log file approach.

apiserver/apiserver.go
if err != nil {
return nil, errors.Annotate(err, "creating logsink writer")
}
srv.logSinkWriter = logSinkWriter
+ migratedLogWriter, err := newLogSinkWriter(filepath.Join(srv.logDir, "migrated.log"))
@howbazaar

howbazaar Nov 21, 2016

Owner

Hmm... I'm wondering about this.
Don't you think it would make more sense to have a per-model migration log file?
That way they could be deleted individually at a later date.
This would require the file to be defined at connection time rather than endpoint creation time.

@babbageclunk

babbageclunk Nov 21, 2016

Member

(As discussed) I can see why this would be nice, but having the files per-model would also mean having to come up with another log rotation strategy so that the logs didn't take up unbounded disk space. The current ones get written via lumberjack which handles that for us. (You said you were alright with this given that it's really a backup for the DB logs.)

@mjs

mjs Nov 22, 2016

Contributor

There is still a problem here in that migrated.log and any rotated copies will be left behind unmanaged. Given that migrations will be fairly infrequent this potentially eats up another 900MB of disk on each controller machine with no real purpose.

I wonder if we even need a file backup of the migrated logs? It really is just a last resort thing if mongodb dies and I don't think it's super important for historical logs. Post migration, any new activity on the new controller will be captured in logsink.log. My gut says not to have a log file for migrated logs (i.e. allow the Writer passed to newLogSinkHandler to be nil)

@babbageclunk

babbageclunk Nov 23, 2016

Member

Ok, that seems much simpler.

apiserver/logtransfer.go
+ s.ctxt.release(s.st)
+}
+
+func newMigrationLoggingStrategy(ctxt httpContext, fileLogger io.Writer) LoggingStrategy {
@mjs

mjs Nov 22, 2016

Contributor

move this to the top? we tend to put constructors near the type definition.

@babbageclunk

babbageclunk Nov 23, 2016

Member

Ok

babbageclunk added some commits Nov 13, 2016

@babbageclunk
Add MigrationLoggingStrategy 
Use it to add a migration log transfer endpoint to the the API server.
6bb956e
@babbageclunk
More tests for logtransfer handler
63cbee1
@babbageclunk
Use httpContext.stateForMigration 
Update tests to match new migration auth.
f7a4a12
@babbageclunk
Use pre-existing header constant
b75b7cf
@babbageclunk
Move constructors closer to type definition
bed4b34
@babbageclunk
Remove file logging for logtransfer endpoint 
The file log is just a last resort thing if mongodb dies and it's not
really important for historical logs. Post migration, any new activity
on the new controller will be captured in logsink.log.
170107c
@babbageclunk
Remove version number from DbLogger 
It's not needed and the intent is to remove it from the log entirely -
see https://pad.lv/1643743

For now, just don't log it in the logtransfer handler.
f3e39de
@babbageclunk
Member

babbageclunk commented Nov 23, 2016

!!build!!
@mjs

mjs approved these changes Nov 23, 2016
View changes

Looks good - just some small stuff.

apiserver/apiserver.go
+ add("/model/:modeluuid/logsink", srv.trackRequests(logSinkHandler))
+
+ // We don't need to save the migrated logs to a logfile as well as to the DB.
+ logTransferHandler := newLogSinkHandler(httpCtxt, ioutil.Discard, newMigrationLoggingStrategy)
@mjs

mjs Nov 23, 2016

Contributor

Nice use of Discard

@babbageclunk

babbageclunk Nov 23, 2016

Member

Thanks! Seemed better than checking for nil everywhere.

apiserver/logtransfer.go
@@ -0,0 +1,77 @@
+// Copyright 2015 Canonical Ltd.
@mjs

mjs Nov 23, 2016

Contributor

2016

apiserver/logtransfer.go
+ }
+ fileErr := logToFile(s.fileLogger, s.filePrefix, m)
+ if fileErr != nil {
+ logger.Errorf("logging to logsink.log failed: %v", fileErr)
@mjs

mjs Nov 23, 2016

Contributor

naming logsink.log isn't quite right

state/logs.go
@@ -231,7 +229,6 @@ func (logger *DbLogger) Log(t time.Time, entity string, module string, location
Time: unixEpochNanoUTC,
@mjs

mjs Nov 23, 2016

Contributor

Please add a TODO near logDoc's definition which references the ticket you created about removing Version from the struct.

state/logs.go
- )
+ // TODO(ericsnow) Use a controller-global int sequence for Id.
+
+ // UnixNano() returns the "absolute" (UTC) number of nanoseconds
@mjs

mjs Nov 23, 2016

Contributor

Does this really require a comment?

@babbageclunk
Member

babbageclunk commented Nov 23, 2016

Thanks @mjs - did those.
@babbageclunk
Member

babbageclunk commented Nov 23, 2016

$$merge$$
@jujubot
Contributor

jujubot commented Nov 23, 2016

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

@jujubot jujubot merged commit 1519858 into juju:develop Nov 23, 2016
1 check failed

1 check failed

github-check-merge-juju Built PR, ran unit tests, and tested LXD deploy. Use !!.*!! to request another build. IE, !!build!!, !!retry!!
Details

@babbageclunk babbageclunk deleted the babbageclunk:migration-logtransfer branch Nov 23, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment