downgrade log messages for killing containers #80
Conversation
| } else { | ||
| p.logger.Warningf("successfully killed container %s", containerName) | ||
| p.logger.Infof("successfully killed container %s", containerName) |
There was a problem hiding this comment.
Is this even worth logging? Should it be a debug severity instead?
There was a problem hiding this comment.
Hm. It could go either way. If it gets here, it means it was still running. I think it's better for it to finish on its own. But debug instead of no logging for sure.
| @@ -86,9 +86,9 @@ func (p *cliWrapper) KillAndClean(containerName string) error { | |||
| cmdKill := p.getPodmanCmd("kill", containerName) | |||
| p.logger.Debugf("Killing with command %v", cmdKill.Args) | |||
| if err := cmdKill.Run(); err != nil { | |||
| p.logger.Warningf("failed to kill pod %s, probably the execution terminated earlier", containerName) | |||
| p.logger.Infof("failed to kill pod %s, probably the execution terminated earlier", containerName) | |||
There was a problem hiding this comment.
This one's tough without knowing the full context. If this is routine and expected, maybe it should be downgraded to debug. Maybe there should be more analysis of the response to determine whether "execution terminated earlier" (which is probably not worth logging at all) or whether the attempt failed for some other reason (which might warrant a warning or even error).
There was a problem hiding this comment.
I think this is a good thing, so debug or no logging.
In reality it should check to see if the container is still running instead of blindly killing it.
|
I'm closing this for another PR that addresses the source of the issue. |
Changes introduced with this PR
Downgrade the log messages regarding killing containers from warning to info to reduce noise.
By contributing to this repository, I agree to the contribution guidelines.