New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

devicemapper: rework logging and add --storage-opt dm.libdm_log_level #33845

Merged
merged 4 commits into from Jul 13, 2017

Conversation

@cyphar
Contributor

cyphar commented Jun 27, 2017

Because libdm doesn't log anything if we have a logging callback
registered, we have to forward our logs to logrus. While this was done
previously in e2f8fbf ("devicemapper: split out devicemapper bindings"),
it was modified in e07d3cd ("devmapper: Fix libdm logging") to remove
the debugging information because it was too voluminous.

However, this has caused us several issues in debugging production
systems because libdm LOG{NOTICE,INFO,DEBUG} logs contain information
that cannot be obtained without recompiling the source.

In order to aid debugging while avoiding large amounts of logs, forward
LOG{NOTICE,INFO} into our debug logs and only output _LOG_DEBUG if an
additional storage-driver specific flag is specified. This respects the
fact that most users aren't going to be debugging devicemapper issues,
while also avoiding the source recompilation burden when debugging.

kitteh

Cosy cat by David O'Hare, on Flickr.

Signed-off-by: Aleksa Sarai asarai@suse.de

/cc @vbatts @rhvgoyal

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jun 27, 2017

Contributor

As an aside, I wanted to update the docs for dockerd to include the new storagedriver option. I was trying to figure out where the man pages live and it looks like they're no longer in this repo? Where are they now?

/cc @thaJeztah ?

Contributor

cyphar commented Jun 27, 2017

As an aside, I wanted to update the docs for dockerd to include the new storagedriver option. I was trying to figure out where the man pages live and it looks like they're no longer in this repo? Where are they now?

/cc @thaJeztah ?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jun 27, 2017

Contributor

This looks pretty much like a dup of #32541

Contributor

cpuguy83 commented Jun 27, 2017

This looks pretty much like a dup of #32541

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jun 27, 2017

Contributor

I didn't see that change beforehand. However, this change actually does a lot more than #32541. In particular:

  • It cleans up the exposed callback code (which didn't make sense because we always needed to register a callback anyway).
  • Cleans up the C callback code to not be limited to 256 bytes.
  • Allows a user to specify what their choice of maximum logging is (which is the main point of this patch).
  • Removes some of the other old cruft from the logging code that #32541 didn't touch.

I do like some of the stuff in p/d/log.go though, we might want to merge those changes as well. How would you like to go on from here?

Contributor

cyphar commented Jun 27, 2017

I didn't see that change beforehand. However, this change actually does a lot more than #32541. In particular:

  • It cleans up the exposed callback code (which didn't make sense because we always needed to register a callback anyway).
  • Cleans up the C callback code to not be limited to 256 bytes.
  • Allows a user to specify what their choice of maximum logging is (which is the main point of this patch).
  • Removes some of the other old cruft from the logging code that #32541 didn't touch.

I do like some of the stuff in p/d/log.go though, we might want to merge those changes as well. How would you like to go on from here?

@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jun 27, 2017

Contributor

Maybe we can just pull those changes in here? (minus the logrus dep in the p/d package).

Contributor

cpuguy83 commented Jun 27, 2017

Maybe we can just pull those changes in here? (minus the logrus dep in the p/d package).

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jun 27, 2017

Contributor

Sure, will do. I'm splitting up this patch now so it's more obvious what I'm doing (I squashed it pre-emptively because I was planning on backporting it).

Contributor

cyphar commented Jun 27, 2017

Sure, will do. I'm splitting up this patch now so it's more obvious what I'm doing (I squashed it pre-emptively because I was planning on backporting it).

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jun 27, 2017

Contributor

Actually I just realised that most of the code in pkg/devicemapper/log.go is not really useful if you don't want to have a logrus dependency inside that file (given the rest of my patchset). Effectively you just get a glorified Sprintf wrapper because you can't write a nice Logf without being able to log.

Contributor

cyphar commented Jun 27, 2017

Actually I just realised that most of the code in pkg/devicemapper/log.go is not really useful if you don't want to have a logrus dependency inside that file (given the rest of my patchset). Effectively you just get a glorified Sprintf wrapper because you can't write a nice Logf without being able to log.

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Jun 27, 2017

Contributor

I have not looked at this yet. But we very much require something like this as well. Recompiling source code does not work when debugging a system in production. Restarting docker with additional flag is much easier.

So yes, we do need a knob to get fine control of logging by libdevmapper when used with docker.

Contributor

rhvgoyal commented Jun 27, 2017

I have not looked at this yet. But we very much require something like this as well. Recompiling source code does not work when debugging a system in production. Restarting docker with additional flag is much easier.

So yes, we do need a knob to get fine control of logging by libdevmapper when used with docker.

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Jun 27, 2017

Contributor

BTW, I love the idea of breaking down patches in a series. That way one can focus on core bits first and then focus on less important bits later. And its much easier to review.

Contributor

rhvgoyal commented Jun 27, 2017

BTW, I love the idea of breaking down patches in a series. That way one can focus on core bits first and then focus on less important bits later. And its much easier to review.

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jun 28, 2017

Member

As an aside, I wanted to update the docs for dockerd to include the new storagedriver option. I was trying to figure out where the man pages live and it looks like they're no longer in this repo? Where are they now?

The CLI docs were moved to the https://github.com/docker/cli repository (yes, dockerd could have been left in this repository, but after debating the pros/cons we decided that it made things easier to have them in that repository 😅)

Member

thaJeztah commented Jun 28, 2017

As an aside, I wanted to update the docs for dockerd to include the new storagedriver option. I was trying to figure out where the man pages live and it looks like they're no longer in this repo? Where are they now?

The CLI docs were moved to the https://github.com/docker/cli repository (yes, dockerd could have been left in this repository, but after debating the pros/cons we decided that it made things easier to have them in that repository 😅)

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jun 28, 2017

Contributor

@thaJeztah Alright, I'll match a PR there once this is merged.

Contributor

cyphar commented Jun 28, 2017

@thaJeztah Alright, I'll match a PR there once this is merged.

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jun 28, 2017

Contributor

@rhvgoyal Fix'd. PTAL.

/cc @vbatts

Contributor

cyphar commented Jun 28, 2017

@rhvgoyal Fix'd. PTAL.

/cc @vbatts

@cyphar cyphar changed the title from devicemapper: rework logging and add --storage-opt dm.log_debug to devicemapper: rework logging and add --storage-opt dm.libdm_log_level Jun 28, 2017

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jun 29, 2017

Contributor

/cc @runcom You probably want to port this to containers/storage. Or if you prefer I can push this PR there as well.

Contributor

cyphar commented Jun 29, 2017

/cc @runcom You probably want to port this to containers/storage. Or if you prefer I can push this PR there as well.

@runcom

This comment has been minimized.

Show comment
Hide comment
@runcom

runcom Jun 29, 2017

Member

cc @nalind for c/storage (thanks for the ping!)

Member

runcom commented Jun 29, 2017

cc @nalind for c/storage (thanks for the ping!)

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jun 29, 2017

Contributor
Contributor

cyphar commented Jun 29, 2017

vsnprintf(buffer, 256, f, ap);
va_end(ap);
va_start(ap, f);
vasprintf(&buffer, f, ap);

This comment has been minimized.

@rhvgoyal

rhvgoyal Jun 29, 2017

Contributor

I get following warning during compilation.

Building: bundles/17.06.0-dev/dynbinary-daemon/dockerd-17.06.0-dev

github.com/docker/docker/pkg/devicemapper

.gopath/src/github.com/docker/docker/pkg/devicemapper/devmapper_wrapper.go: In function ‘log_cb’:
.gopath/src/github.com/docker/docker/pkg/devicemapper/devmapper_wrapper.go:20:2: warning: implicit declaration of function ‘vasprintf’; did you mean ‘vsprintf’? [-Wimplicit-function-declaration]
vasprintf(&buffer, f, ap);
^~~~~~~~~
vsprintf

@rhvgoyal

rhvgoyal Jun 29, 2017

Contributor

I get following warning during compilation.

Building: bundles/17.06.0-dev/dynbinary-daemon/dockerd-17.06.0-dev

github.com/docker/docker/pkg/devicemapper

.gopath/src/github.com/docker/docker/pkg/devicemapper/devmapper_wrapper.go: In function ‘log_cb’:
.gopath/src/github.com/docker/docker/pkg/devicemapper/devmapper_wrapper.go:20:2: warning: implicit declaration of function ‘vasprintf’; did you mean ‘vsprintf’? [-Wimplicit-function-declaration]
vasprintf(&buffer, f, ap);
^~~~~~~~~
vsprintf

This comment has been minimized.

@rhvgoyal

rhvgoyal Jun 29, 2017

Contributor

Adding #define _GNU_SOURCE seems to make it go away.

@rhvgoyal

rhvgoyal Jun 29, 2017

Contributor

Adding #define _GNU_SOURCE seems to make it go away.

This comment has been minimized.

@cyphar

cyphar Jun 29, 2017

Contributor

Yup. For some reason I didn't get the same warning. Hmm.

@cyphar

cyphar Jun 29, 2017

Contributor

Yup. For some reason I didn't get the same warning. Hmm.

This comment has been minimized.

@cyphar

cyphar Jun 29, 2017

Contributor

Fixed.

@cyphar

cyphar Jun 29, 2017

Contributor

Fixed.

@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Jun 29, 2017

Contributor

I have disabled deferred removal and deferred deletion. And I launch a container and do "unshare -m" in a second window to make sure container mount point leaks. And then exit the container. I expected that container exit will try to remove device in a busy loop but exit seemed to happen immediately. I don't think its because of your patches but something has changed. Previously it did not behave like this.

BTW, how do I test this. How do I force an error condition so that I can see libdm messages in journal.

Contributor

rhvgoyal commented Jun 29, 2017

I have disabled deferred removal and deferred deletion. And I launch a container and do "unshare -m" in a second window to make sure container mount point leaks. And then exit the container. I expected that container exit will try to remove device in a busy loop but exit seemed to happen immediately. I don't think its because of your patches but something has changed. Previously it did not behave like this.

BTW, how do I test this. How do I force an error condition so that I can see libdm messages in journal.

@@ -264,11 +264,6 @@ func UdevWait(cookie *uint) error {
return nil
}
// LogInitVerbose is an interface to initialize the verbose logger for the device mapper library.
func LogInitVerbose(level int) {

This comment has been minimized.

@rhvgoyal

rhvgoyal Jun 29, 2017

Contributor

How do we know that there are no external users of devicemapper package which are not making use of LogInitVerbose.

I think that might have been the thought in my mind and left LogInitVerbose() behind.

@rhvgoyal

rhvgoyal Jun 29, 2017

Contributor

How do we know that there are no external users of devicemapper package which are not making use of LogInitVerbose.

I think that might have been the thought in my mind and left LogInitVerbose() behind.

This comment has been minimized.

@cyphar

cyphar Jun 30, 2017

Contributor

LogInitVerbose is meaningless if you are using callbacks. It only has meaning for the default logger, which we don't use because we need callbacks to get errors.

@cyphar

cyphar Jun 30, 2017

Contributor

LogInitVerbose is meaningless if you are using callbacks. It only has meaning for the default logger, which we don't use because we need callbacks to get errors.

This comment has been minimized.

@rhvgoyal

rhvgoyal Jun 30, 2017

Contributor

When you say We, you mean docker. I am concerned about other tools (outside docker) making use of devicemapper package. They can make use of it, right?

@rhvgoyal

rhvgoyal Jun 30, 2017

Contributor

When you say We, you mean docker. I am concerned about other tools (outside docker) making use of devicemapper package. They can make use of it, right?

This comment has been minimized.

@rhvgoyal

rhvgoyal Jun 30, 2017

Contributor

I am not saying somebody is making use of it. Just that it is an interface exported by package and if we remove an existing interface (whether it makes much sense or not), there is always potential to break an existing user out there somewhere.

@rhvgoyal

rhvgoyal Jun 30, 2017

Contributor

I am not saying somebody is making use of it. Just that it is an interface exported by package and if we remove an existing interface (whether it makes much sense or not), there is always potential to break an existing user out there somewhere.

This comment has been minimized.

@cyphar

cyphar Jun 30, 2017

Contributor

When you say we, you mean docker.

No, I'm not talking about Docker, I'm talking about any potential user of this pkg.

LogInitVerbose literally does not mean anything outside of the default libdm logger, and because we (or any user of this package) need to set our own callbacks in order to get errBusy (which we use internally for a bunch of things) and so on, exposing this API is confusing and pointless. If you register your own logging callback, you get all messages without any filter.

If you believe I'm mistaken, you can look at the source of lvm2:

void dm_log_init_verbose(int level)
{
	_verbose = level;
}

And _verbose is only used in the default logger:

% git grep '\b_verbose\b' libdm/
libdm/libdm-common.c:static int _verbose = 0;
libdm/libdm-common.c:   if (level <= _LOG_WARN || _verbose) {
libdm/libdm-common.c:   _verbose = level;
/*
 * Library users can provide their own logging
 * function.
 */

__attribute__((format(printf, 5, 0)))
static void _default_log_line(int level,
	    const char *file __attribute__((unused)),
	    int line __attribute__((unused)), int dm_errno_or_class,
	    const char *f, va_list ap)
{
	static int _abort_on_internal_errors = -1;
	FILE *out = log_stderr(level) ? stderr : stdout;

	level = log_level(level);

	if (level <= _LOG_WARN || _verbose) {
		if (level < _LOG_WARN)
			out = stderr;
		vfprintf(out, f, ap);
		fputc('\n', out);
	}

	if (_abort_on_internal_errors < 0)
		/* Set when env DM_ABORT_ON_INTERNAL_ERRORS is not "0" */
		_abort_on_internal_errors =
			strcmp(getenv("DM_ABORT_ON_INTERNAL_ERRORS") ? : "0", "0");

	if (_abort_on_internal_errors &&
	    !strncmp(f, INTERNAL_ERROR, sizeof(INTERNAL_ERROR) - 1))
		abort();
}

And when you register your own logger, it replaces the default one. I checked all of this before removing this API, because I was also confused why it was present in this library. If anyone is using it, it's acting as a noop.

@cyphar

cyphar Jun 30, 2017

Contributor

When you say we, you mean docker.

No, I'm not talking about Docker, I'm talking about any potential user of this pkg.

LogInitVerbose literally does not mean anything outside of the default libdm logger, and because we (or any user of this package) need to set our own callbacks in order to get errBusy (which we use internally for a bunch of things) and so on, exposing this API is confusing and pointless. If you register your own logging callback, you get all messages without any filter.

If you believe I'm mistaken, you can look at the source of lvm2:

void dm_log_init_verbose(int level)
{
	_verbose = level;
}

And _verbose is only used in the default logger:

% git grep '\b_verbose\b' libdm/
libdm/libdm-common.c:static int _verbose = 0;
libdm/libdm-common.c:   if (level <= _LOG_WARN || _verbose) {
libdm/libdm-common.c:   _verbose = level;
/*
 * Library users can provide their own logging
 * function.
 */

__attribute__((format(printf, 5, 0)))
static void _default_log_line(int level,
	    const char *file __attribute__((unused)),
	    int line __attribute__((unused)), int dm_errno_or_class,
	    const char *f, va_list ap)
{
	static int _abort_on_internal_errors = -1;
	FILE *out = log_stderr(level) ? stderr : stdout;

	level = log_level(level);

	if (level <= _LOG_WARN || _verbose) {
		if (level < _LOG_WARN)
			out = stderr;
		vfprintf(out, f, ap);
		fputc('\n', out);
	}

	if (_abort_on_internal_errors < 0)
		/* Set when env DM_ABORT_ON_INTERNAL_ERRORS is not "0" */
		_abort_on_internal_errors =
			strcmp(getenv("DM_ABORT_ON_INTERNAL_ERRORS") ? : "0", "0");

	if (_abort_on_internal_errors &&
	    !strncmp(f, INTERNAL_ERROR, sizeof(INTERNAL_ERROR) - 1))
		abort();
}

And when you register your own logger, it replaces the default one. I checked all of this before removing this API, because I was also confused why it was present in this library. If anyone is using it, it's acting as a noop.

This comment has been minimized.

@rhvgoyal

rhvgoyal Jun 30, 2017

Contributor

Ok, given other core interfaces like RemoveDevice and DeleteDevice rely on dmSawBusy and it will be set only if user of package calls LogInit() (Either with or without a logger). Otherwise package will always return error even in case of busy device and send messages from libdm to stdout.

IOW, for devicemapper pkg to work properly, this package always need to register a callback function with libdm. And right now unfortunately it is initiated from the user of package.

So I agree that cleaning this up makes sense. While there might be a very strange user out there but given how twisted this internface is, chances of this happening are small. If it does happen, we will try to advise them to move to new interface.

So, I am fine with removing LogInitVerbose. Please do capture some of this information in changelog for future reference.

@rhvgoyal

rhvgoyal Jun 30, 2017

Contributor

Ok, given other core interfaces like RemoveDevice and DeleteDevice rely on dmSawBusy and it will be set only if user of package calls LogInit() (Either with or without a logger). Otherwise package will always return error even in case of busy device and send messages from libdm to stdout.

IOW, for devicemapper pkg to work properly, this package always need to register a callback function with libdm. And right now unfortunately it is initiated from the user of package.

So I agree that cleaning this up makes sense. While there might be a very strange user out there but given how twisted this internface is, chances of this happening are small. If it does happen, we will try to advise them to move to new interface.

So, I am fine with removing LogInitVerbose. Please do capture some of this information in changelog for future reference.

This comment has been minimized.

@cyphar

cyphar Jun 30, 2017

Contributor

Will do, sorry for the semi-long rant. I cannot describe in words how silly I find it that libdm requires you to register a logging callback in order to understand what an error code of 1 actually means. The fact we're having this conversation is ridiculous -- the premise is flawed IMO. 😉

@cyphar

cyphar Jun 30, 2017

Contributor

Will do, sorry for the semi-long rant. I cannot describe in words how silly I find it that libdm requires you to register a logging callback in order to understand what an error code of 1 actually means. The fact we're having this conversation is ridiculous -- the premise is flawed IMO. 😉

func LogInit(logger DevmapperLogger) {
dmLogger = logger
LogWithErrnoInit()
}

This comment has been minimized.

@rhvgoyal

rhvgoyal Jun 29, 2017

Contributor

I am again concerned about somebody using devicemapper package and using this interface and they will be broken now.

I think rancher folks were using pkg/devicemapper.

This change does not seem fundamental to what you are trying to do. So why not first focus on the core change instead of removing existing interfaces of pkg/devicemapper/

@rhvgoyal

rhvgoyal Jun 29, 2017

Contributor

I am again concerned about somebody using devicemapper package and using this interface and they will be broken now.

I think rancher folks were using pkg/devicemapper.

This change does not seem fundamental to what you are trying to do. So why not first focus on the core change instead of removing existing interfaces of pkg/devicemapper/

This comment has been minimized.

@cyphar

cyphar Jun 30, 2017

Contributor

The semantics of this function never made sense because you need to have callbacks registered for this package to function. Which meant that a user of this package had to explicitly call this function so that the package will work as intended. That doesn't seem like a sane interface to me.

If you like, I can re-add LogInit but change it's semantics so that it acts as an additional logger (or a replacement for the logrus one we have now). But this will also "break" users in a more drastic way (rather than not compiling, code that worked previously will have slightly different semantics).

As for breaking people, surely everyone uses vendor these days so I'm not really sure what you mean by this. Yes, when they update they will have to make very minor changes but the alternative is to keep an old interface that will lead people to think that the semantics are unchanged (or that the semantics are different to what they actually are).

@cyphar

cyphar Jun 30, 2017

Contributor

The semantics of this function never made sense because you need to have callbacks registered for this package to function. Which meant that a user of this package had to explicitly call this function so that the package will work as intended. That doesn't seem like a sane interface to me.

If you like, I can re-add LogInit but change it's semantics so that it acts as an additional logger (or a replacement for the logrus one we have now). But this will also "break" users in a more drastic way (rather than not compiling, code that worked previously will have slightly different semantics).

As for breaking people, surely everyone uses vendor these days so I'm not really sure what you mean by this. Yes, when they update they will have to make very minor changes but the alternative is to keep an old interface that will lead people to think that the semantics are unchanged (or that the semantics are different to what they actually are).

This comment has been minimized.

@cyphar

cyphar Jun 30, 2017

Contributor

Just to be clear, I do understand wanted to provide logging hooks, so maybe we should re-add it. I'm just trying to point out that the old API was not correct. Sorry for the strong reply.

@cyphar

cyphar Jun 30, 2017

Contributor

Just to be clear, I do understand wanted to provide logging hooks, so maybe we should re-add it. I'm just trying to point out that the old API was not correct. Sorry for the strong reply.

This comment has been minimized.

@rhvgoyal

rhvgoyal Jun 30, 2017

Contributor

I like the idea of retaining LogInit() but changing the semantics so that caller can register its own logger and get all the messages. IOW, it replaced logrus.

So if existing users were using it without any logger LogInit(), they will not see any change except that logs will start going to journal. Those who were registering their own logger, they will also not see any change as they will continue to get all the logs and then they can decide what to do with those logs.

IOW, default logger of pkg/devicemapper will be logrus until and unless overwridden by LogInit.

Does this sound reasonable to you?

@rhvgoyal

rhvgoyal Jun 30, 2017

Contributor

I like the idea of retaining LogInit() but changing the semantics so that caller can register its own logger and get all the messages. IOW, it replaced logrus.

So if existing users were using it without any logger LogInit(), they will not see any change except that logs will start going to journal. Those who were registering their own logger, they will also not see any change as they will continue to get all the logs and then they can decide what to do with those logs.

IOW, default logger of pkg/devicemapper will be logrus until and unless overwridden by LogInit.

Does this sound reasonable to you?

This comment has been minimized.

@cyphar

cyphar Jun 30, 2017

Contributor

That sounds fine to me, I'll update it accordingly.

@cyphar

cyphar Jun 30, 2017

Contributor

That sounds fine to me, I'll update it accordingly.

@@ -2761,6 +2761,14 @@ func NewDeviceSet(root string, doInit bool, options []string, uidMaps, gidMaps [
return nil, errors.New("dm.thinp_autoextend_threshold must be greater than 0 and less than 100")
}
lvmSetupConfig.AutoExtendThreshold = per
case "dm.libdm_log_level":

This comment has been minimized.

@rhvgoyal

rhvgoyal Jun 29, 2017

Contributor

Somewhere in the comment it will be useful to put what values of libdm_log_level users can pass and what these map to.

@rhvgoyal

rhvgoyal Jun 29, 2017

Contributor

Somewhere in the comment it will be useful to put what values of libdm_log_level users can pass and what these map to.

This comment has been minimized.

@cyphar

cyphar Jun 30, 2017

Contributor

I am planning on putting that in the CLI docs, but they map precisely to the libdm log levels. I'll add a comment though.

@cyphar

cyphar Jun 30, 2017

Contributor

I am planning on putting that in the CLI docs, but they map precisely to the libdm log levels. I'll add a comment though.

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jun 30, 2017

Contributor

BTW, how do I test this. How do I force an error condition so that I can see libdm messages in journal.

If you enable debug logging you'll see all of the debug messages, just use --storage-opt dm.libdm_log_level=7 --debug and you'll see plenty of logrus debug messages fly by when you start and stop containers. If you forcefully kill the daemon you also might see some libdm errors as well.

Contributor

cyphar commented Jun 30, 2017

BTW, how do I test this. How do I force an error condition so that I can see libdm messages in journal.

If you enable debug logging you'll see all of the debug messages, just use --storage-opt dm.libdm_log_level=7 --debug and you'll see plenty of logrus debug messages fly by when you start and stop containers. If you forcefully kill the daemon you also might see some libdm errors as well.

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jun 30, 2017

Contributor

And I launch a container and do "unshare -m" in a second window to make sure container mount point leaks. And then exit the container. I expected that container exit will try to remove device in a busy loop but exit seemed to happen immediately. I don't think its because of your patches but something has changed. Previously it did not behave like this.

I don't think that my patches could've caused that. Did you make sure you remounted / with mount --make-rprivate /? Or did we conclude that wasn't actually the part that caused the issue?

Contributor

cyphar commented Jun 30, 2017

And I launch a container and do "unshare -m" in a second window to make sure container mount point leaks. And then exit the container. I expected that container exit will try to remove device in a busy loop but exit seemed to happen immediately. I don't think its because of your patches but something has changed. Previously it did not behave like this.

I don't think that my patches could've caused that. Did you make sure you remounted / with mount --make-rprivate /? Or did we conclude that wasn't actually the part that caused the issue?

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jun 30, 2017

Contributor

Alright, I've fixed up your concerns @rhvgoyal. WDYT?

Contributor

cyphar commented Jun 30, 2017

Alright, I've fixed up your concerns @rhvgoyal. WDYT?

cyphar added some commits Jun 27, 2017

devicemapper: remove 256 character limit of libdm logs
This limit is unecessary and can lead to the truncation of long libdm
logs (which is quite annoying).

Fixes: b440ec0 ("device-mapper: Move all devicemapper spew to log through utils.Debugf().")
Signed-off-by: Aleksa Sarai <asarai@suse.de>
devicemapper: actually remove DmLogInitVerbose
e07d3cd ("devmapper: Fix libdm logging") removed all of the callers of
DmLogInitVerbose, but we still kept around the wrapper. However, the
libdm dm_log_init_verbose API changes the verbosity of the *default*
libdm logger. Because pkg/devicemapper internally *relies* on using
logging callbacks to understand what errors were encountered by libdm,
this wrapper is useless (it only makes sense for the default logger
which we do not user).

Any user not inside Docker of this function almost certainly was not
using this API correctly, because pkg/devicemapper will misbehave if our
logging callbacks were not registered.

Signed-off-by: Aleksa Sarai <asarai@suse.de>
devicemapper: change LogInit and move all callbacks to pkg
LogInit used to act as a manual way of registering the *necessary*
pkg/devicemapper logging callbacks. In addition, it was used to split up
the logic of pkg/devicemapper into daemon/graphdriver/devmapper (such
that some things were logged from libdm).

The manual aspect of this API was completely non-sensical and was just
begging for incorrect usage of pkg/devicemapper, so remove that semantic
and always register our own libdm callbacks.

In addition, recombine the split out logging callbacks into
pkg/devicemapper so that the default logger is local to the library and
also shown to be the recommended logger. This makes the code
substantially easier to read. Also the new DefaultLogger now has
configurable upper-bound for the log level, which allows for dynamically
changing the logging level.

Signed-off-by: Aleksa Sarai <asarai@suse.de>
devicemapper: add --storage-opt dm.libdm_log_level=X option
Because we use our own logging callbacks in order to use libdm
effectively, it is quite difficult to debug complicated devicemapper
issues (because any warnings or notices from libdm are muted by our own
callback function). e07d3cd ("devmapper: Fix libdm logging") further
reduced the ability of this debugging by only allowing _LOG_FATAL errors
to be passed to the output.

Unfortunately libdm is very chatty, so in order to avoid making the logs
even more crowded, add a dm.libdm_log_level storage option that allows
people who are debugging the lovely world of libdm to be able to dive in
without recompiling binaries.

The valid values of dm.libdm_log_level map directly to the libdm logging
levels, and are in the range [2,7] as of the time of writing with 7
being _LOG_DEBUG and 2 being _LOG_FATAL. The default is _LOG_FATAL.

Signed-off-by: Aleksa Sarai <asarai@suse.de>
@rhvgoyal

This comment has been minimized.

Show comment
Hide comment
@rhvgoyal

rhvgoyal Jul 3, 2017

Contributor

LGTM

@cyphar thanks for implementing this knob. This is really required to be able to debug some docker issues without recompiling.

Rest of the cleanup is also nice. Makes reading this code easier.

Contributor

rhvgoyal commented Jul 3, 2017

LGTM

@cyphar thanks for implementing this knob. This is really required to be able to debug some docker issues without recompiling.

Rest of the cleanup is also nice. Makes reading this code easier.

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jul 3, 2017

Contributor

I'm using this patchset to debug some production issues right now. 😉

Contributor

cyphar commented Jul 3, 2017

I'm using this patchset to debug some production issues right now. 😉

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar

cyphar Jul 7, 2017

Contributor

/ping @cpuguy83

Contributor

cyphar commented Jul 7, 2017

/ping @cpuguy83

@coolljt0725

This comment has been minimized.

Show comment
Hide comment
@coolljt0725
Contributor

coolljt0725 commented Jul 12, 2017

ping @cpuguy83

@cpuguy83

LGTM

Tested, looks good.

@cyphar

This comment has been minimized.

Show comment
Hide comment
@cyphar
Contributor

cyphar commented Jul 12, 2017

@thaJeztah

LGTM

@thaJeztah thaJeztah merged commit 00b2182 into moby:master Jul 13, 2017

6 checks passed

dco-signed All commits are signed
experimental Jenkins build Docker-PRs-experimental 35439 has succeeded
Details
janky Jenkins build Docker-PRs 44052 has succeeded
Details
powerpc Jenkins build Docker-PRs-powerpc 4424 has succeeded
Details
windowsRS1 Jenkins build Docker-PRs-WoW-RS1 15386 has succeeded
Details
z Jenkins build Docker-PRs-s390x 4128 has succeeded
Details
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 13, 2017

Member

thanks @cyphar !

Member

thaJeztah commented Jul 13, 2017

thanks @cyphar !

@cyphar cyphar deleted the cyphar:devicemapper-show-me-your-logs branch Jul 13, 2017

albers added a commit to albers/docker-cli that referenced this pull request Jul 14, 2017

Add bash completion for `--storage-opt dm.libdm_log_level`
This adds bash completion for moby/moby#33845.

Signed-off-by: Harald Albers <github@albersweb.de>

kolyshkin added a commit to kolyshkin/moby that referenced this pull request Jul 18, 2017

devmapper_wrapper.go: fix gcc warning
I am getting the following warning from gcc when compiling the daemon:

> # github.com/docker/docker/pkg/devicemapper
> pkg/devicemapper/devmapper_wrapper.go: In function ‘log_cb’:
> pkg/devicemapper/devmapper_wrapper.go:20:2: warning: ignoring return
> value of ‘vasprintf’, declared with attribute warn_unused_result
> [-Wunused-result]
>  vasprintf(&buffer, f, ap);
>  ^

vasprintf(3) man page says if the function returns -1, the buffer is
undefined, so we should not use it. In practice, I assume, this never
happens so we just return.

Introduced by moby#33845 that resulted in
commit 63328c6 ("devicemapper: remove 256 character limit of libdm logs")

Cc: Aleksa Sarai <asarai@suse.de>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

andrewhsu pushed a commit to docker/docker-ce that referenced this pull request Jul 20, 2017

devmapper_wrapper.go: fix gcc warning
I am getting the following warning from gcc when compiling the daemon:

> # github.com/docker/docker/pkg/devicemapper
> pkg/devicemapper/devmapper_wrapper.go: In function ‘log_cb’:
> pkg/devicemapper/devmapper_wrapper.go:20:2: warning: ignoring return
> value of ‘vasprintf’, declared with attribute warn_unused_result
> [-Wunused-result]
>  vasprintf(&buffer, f, ap);
>  ^

vasprintf(3) man page says if the function returns -1, the buffer is
undefined, so we should not use it. In practice, I assume, this never
happens so we just return.

Introduced by moby/moby#33845 that resulted in
commit 63328c6 ("devicemapper: remove 256 character limit of libdm logs")

Cc: Aleksa Sarai <asarai@suse.de>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
Upstream-commit: 7da12bc
Component: engine

bonczj added a commit to bonczj/moby-json-file-logger that referenced this pull request Jul 25, 2017

devmapper_wrapper.go: fix gcc warning
I am getting the following warning from gcc when compiling the daemon:

> # github.com/docker/docker/pkg/devicemapper
> pkg/devicemapper/devmapper_wrapper.go: In function ‘log_cb’:
> pkg/devicemapper/devmapper_wrapper.go:20:2: warning: ignoring return
> value of ‘vasprintf’, declared with attribute warn_unused_result
> [-Wunused-result]
>  vasprintf(&buffer, f, ap);
>  ^

vasprintf(3) man page says if the function returns -1, the buffer is
undefined, so we should not use it. In practice, I assume, this never
happens so we just return.

Introduced by moby#33845 that resulted in
commit 63328c6 ("devicemapper: remove 256 character limit of libdm logs")

Cc: Aleksa Sarai <asarai@suse.de>
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>

riyazdf added a commit to riyazdf/cli that referenced this pull request Aug 2, 2017

Add bash completion for `--storage-opt dm.libdm_log_level`
This adds bash completion for moby/moby#33845.

Signed-off-by: Harald Albers <github@albersweb.de>

vieux pushed a commit to vieux/docker-ce that referenced this pull request Aug 10, 2017

Add bash completion for `--storage-opt dm.libdm_log_level`
This adds bash completion for moby/moby#33845.

Signed-off-by: Harald Albers <github@albersweb.de>
Upstream-commit: b8710ccef398bdfcd5ba323a39b16c5ab15e530a
Component: cli
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment