lcow: fix debug in startServiceVMIfNotRunning() #37446
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
When go-1.11beta1 is used for building, the following error is reported: > 14:56:20 daemon\graphdriver\lcow\lcow.go:236: Debugf format %s reads > arg #2, but call has 1 arg While fixing this, let's also fix a few other things in this very function (startServiceVMIfNotRunning): 1. Do not use fmt.Printf when not required. 2. Use `title` whenever possible. 3. Don't add `id` to messages as `title` already has it. 4. Remove duplicated colons. 5. Try to unify style of messages. 6. s/startservicevmifnotrunning/startServiceVMIfNotRunning/ ... In general, logging/debugging here is a mess and requires much more love than I can give it at the moment. Areas for improvement: 1. Add a global var logger = logrus.WithField("storage-driver", "lcow") and use it everywhere else in the code. 2. Use logger.WithField("id", id) whenever possible (same for "context" and other similar fields). 3. Revise all the errors returned to be uniform. 4. Make use of errors.Wrap[f] whenever possible. Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
|
@johnstep @ jhowardmsft PTAL |
|
This is separated from #37358 |
Merged
|
Sure. LGTM |
lowenna
approved these changes
Jul 12, 2018
|
z and ppc failures are obviously unrelated (some glitch in |
thaJeztah
approved these changes
Jul 12, 2018
| @@ -219,21 +219,21 @@ func (d *Driver) startServiceVMIfNotRunning(id string, mvdToAdd []hcsshim.Mapped | |||
| // Use the global ID if in global mode | |||
| id = d.getVMID(id) | |||
|
|
|||
| title := fmt.Sprintf("lcowdriver: startservicevmifnotrunning %s:", id) | |||
| title := "lcowdriver: startServiceVMIfNotRunning " + id | |||
There was a problem hiding this comment.
Perhaps for a follow up, but we should logrus.WithFields for this information (similar to what we do for other storage drivers)
There was a problem hiding this comment.
I concur (see the patch description).
There was a problem hiding this comment.
...but I'd rather do it in a separate PR later as this fix is quite big already -- and initially it supposed to be a one-liner 🤣
There was a problem hiding this comment.
D'oh!! Didn't read that far into your description 🤦♂️
| if err := svm.getStopError(); err != nil { | ||
| logrus.Debugf("%s: VM %s did not stop successfully: %s", title, id, err) | ||
| logrus.Debugf("%s: VM did not stop successfully: %s", title, err) |
There was a problem hiding this comment.
logrus.WithError(err).Debug(....)
johnstep
approved these changes
Jul 12, 2018
|
PowerPC https://jenkins.dockerproject.org/job/Docker-PRs-powerpc/10507/console failure is unrelated, and tracked through #37408 |
thaJeztah
added
the
area/lcow
8 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When go-1.11beta1 is used for building, the following error is reported:
While fixing this, let's also fix a few other things in this very function (
startServiceVMIfNotRunning()):fmt.Printfwhen not required.titlewhenever possible.idto messages astitlealready has it.s/startservicevmifnotrunning/startServiceVMIfNotRunning/...
In general, logging/debugging here is a mess and requires much more love than I can give it at the moment. Ideas for improvement:
var logger = logrus.WithField("storage-driver", "lcow")and use it everywhere else in the code.logger.WithField("id", id)whenever possible (same for "context" and other similar fields).errors.Wrap[f]whenever possible.