-
Notifications
You must be signed in to change notification settings - Fork 769
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
Send logrus messages back to caller when building #3215
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt.Fprintf(Err, "Warning: missing %q build argument. Try adding %q to the command line\n", argName, fmt.Sprintf("--build-arg %s=<VALUE>", argName))
Please add newline at end of message
/cc @edsantiago |
This doesn't cover all of the places in the main |
So fix this in Podman. |
@nalind The problem with logrus.SetOutput() is we don't want all the debugging information forwarded to the client. We only want expected error messages. |
LGTM |
Is there a way in logrus, to be selective about messages, IE Error and Warn go to buildOpts.Err |
They all go to the same location (
Why doesn't the client want to know about other warnings and errors? |
sirupsen/logrus#403 |
@nalind, Client/Server reporting is a matrix. Buildah is a little different because of the Stdout/StdErr/Auxout/Reporting BuildOptions. Then we have messages the developers have asked for that were rendered via logrus. Logrus output has been captured by the service and kept on the server. The handlers are responsible for relaying information to the client loosely following the table below. Logging levels: Errors: Always Client and Server |
The standard logger in the logrus package is a globally-shared object, and it is used not only by buildah, but by other dependencies that both buildah and libpod use, and by libpod itself. That is not something your application wants one library in your dependency stack messing with without your full control and knowledge. podman is going to have to manipulate the global standard logger that the logrus library exports to get the results that you're looking for. |
@nalind The global nature of the logger is the issue. If two remote clients are running builds, it is impossible to route any logged message to the client which called imagebuildah.BuildDockerfiles(). Any logrus output has to stay on the server. imagebuildah.BuildDockerfiles() has four writers already associated with it, that is what we are leveraging here. |
imagebuildah/build.go
Outdated
@@ -181,7 +181,7 @@ func warnOnUnsetBuildArgs(node *parser.Node, args map[string]string) { | |||
argHasValue = argFound[argName] | |||
} | |||
if _, ok := args[argName]; !argHasValue && !ok { | |||
logrus.Warnf("missing %q build argument. Try adding %q to the command line", argName, fmt.Sprintf("--build-arg %s=<VALUE>", argName)) | |||
fmt.Fprintf(Err, "Warning: missing %q build argument. Try adding %q to the command line\n", argName, fmt.Sprintf("--build-arg %s=<VALUE>", argName)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Something like (untested)
log := logrus.New()
log.SetOutput(Err)
log.SetLevel(logrus.GetLevel())
log.Warnf(...)
would probably be less disruptive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we could do this one per connection, then this would solve our problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can try to wire in a way to pass a logger to some of our functions, but I don't really think we're going to be able to add a "send logrus output here" parameter to every function that logs things in the packages and libraries that we call into, and that's what'd be necessary to be sure we were catching everything. Not that I know how our caller would filter and route things if all it could control was the output io.Writer
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I say we do our best. So that most of these messages get back to the caller.
Well, so long as we're all aware that this one change isn't going to fix the general case, I guess I've grumbled enough. |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rhatdan The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
buildah.go
Outdated
@@ -89,6 +89,9 @@ const ( | |||
type Builder struct { | |||
store storage.Store | |||
|
|||
// Logger is the logrus loger to write log messages with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Logger is the logrus loger to write log messages with | |
// Logger is the logrus logger to write log messages with |
b.Logger = logrus.New() | ||
b.Logger.SetOutput(os.Stderr) | ||
b.Logger.SetLevel(logrus.GetLevel()) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This snippet belongs somewhere other than in a method called fixupConfig()
. It's also redundant for callers who get here from newBuilder()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where would you want me to put it. fixupConfig is called by both newbuilder and openbuilder. I could make a new function? And call it from both?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OpenBuilder is called from buildah run ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's either going to be do-it-everywhere, or a helper function. Builder objects are also constructed by the variations of OpenBuilder, and by ImportBuilder/ImportBuilderFromImage. Basically, anywhere you see a new Builder object being created, we're going to need to set this field.
imagebuildah/executor.go
Outdated
@@ -300,6 +302,7 @@ func (b *Executor) startStage(ctx context.Context, stage *imagebuilder.Stage, st | |||
volumeCacheInfo: make(map[string]os.FileInfo), | |||
output: output, | |||
stage: stage, | |||
logger: b.logger, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This field isn't needed, since it's never different from the Executor's logger. Use the StageExecutor's executor.logger
in those places instead, similar to how we handle several other fields that we refer to in the Executor.
run.go
Outdated
@@ -70,6 +71,8 @@ const ( | |||
|
|||
// RunOptions can be used to alter how a command is run in the container. | |||
type RunOptions struct { | |||
// Logger is the logrus loger to write log messages with |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// Logger is the logrus loger to write log messages with | |
// Logger is the logrus logger to write log messages with |
run_linux.go
Outdated
@@ -76,7 +76,7 @@ func (b *Builder) Run(command []string, options RunOptions) error { | |||
logrus.Debugf("using %q to hold bundle data", path) | |||
defer func() { | |||
if err2 := os.RemoveAll(path); err2 != nil { | |||
logrus.Error(err2) | |||
b.Logger.Error(err2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b.Logger.Error(err2) | |
options.Logger.Error(err2) |
Also needs to create a logger for options.Logger
if it isn't set.
run_linux.go
Outdated
@@ -120,7 +120,7 @@ func (b *Builder) Run(command []string, options RunOptions) error { | |||
} | |||
defer func() { | |||
if err := b.Unmount(); err != nil { | |||
logrus.Errorf("error unmounting container: %v", err) | |||
b.Logger.Errorf("error unmounting container: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b.Logger.Errorf("error unmounting container: %v", err) | |
options.Logger.Errorf("error unmounting container: %v", err) |
run_linux.go
Outdated
@@ -253,7 +253,7 @@ rootless=%d | |||
|
|||
defer func() { | |||
if err := cleanupRunMounts(runMountTargets, mountPoint); err != nil { | |||
logrus.Errorf("unabe to cleanup run mounts %v", err) | |||
b.Logger.Errorf("unabe to cleanup run mounts %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
b.Logger.Errorf("unabe to cleanup run mounts %v", err) | |
options.Logger.Errorf("unabe to cleanup run mounts %v", err) |
run_linux.go
Outdated
@@ -704,12 +704,14 @@ func runUsingRuntime(isolation define.Isolation, options RunOptions, configureNe | |||
// Lock the caller to a single OS-level thread. | |||
runtime.LockOSThread() | |||
|
|||
logger := options.Logger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This never needs to be different from options.Logger
, so drop the variable and use the passed-in value.
run_linux.go
Outdated
@@ -1069,6 +1071,7 @@ func setupRootlessNetwork(pid int) (teardown func(), err error) { | |||
|
|||
func runConfigureNetwork(isolation define.Isolation, options RunOptions, configureNetworks []string, pid int, containerName string, command []string) (teardown func(), err error) { | |||
var netconf, undo []*libcni.NetworkConfigList | |||
logger := options.Logger |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This doesn't need to ever be different from options.Logger
, and it's used once. Remove the variable and reference options.Logger
directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is the way I originally had it, and I thought this was cleaner, but I will revert.
Embedding another package's API objects in our API means that any stability guarantees we make for our API are also dependent on the stability of their APIs, and this one ties us to a specific implementation rather than an interface. I understand the motivation for this patch, though, so I'm not rejecting it out of hand, but it remains a concern. |
d6cb833
to
51757c2
Compare
imagebuildah/build.go
Outdated
@@ -190,7 +198,7 @@ func warnOnUnsetBuildArgs(node *parser.Node, args map[string]string) { | |||
argHasValue = argFound[argName] | |||
} | |||
if _, ok := args[argName]; !argHasValue && !ok { | |||
logrus.Warnf("missing %q build argument. Try adding %q to the command line", argName, fmt.Sprintf("--build-arg %s=<VALUE>", argName)) | |||
logger.Warnf("missing %q build argument. Try adding %q to the command line\n", argName, fmt.Sprintf("--build-arg %s=<VALUE>", argName)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The \n
was for fmt.Fprintf()
, which doesn't add one by default, but logrus
's formatters always include it, so we don't need that.
We want Info, Warning and Debug logrus messages to be writen to the buildah stderr. this way when podman-remote is using build, it will get the messages back on the client side. [NO TESTS NEEDED] Since this will be tested in Podman. Fixes: containers#3214 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>
We want Info, Warning and Debug logrus messages to be writen to the
buildah stderr. this way when podman-remote is using build, it will
get the messages back on the client side.
[NO TESTS NEEDED] Since this will be tested in Podman.
Fixes: #3214
Signed-off-by: Daniel J Walsh dwalsh@redhat.com