Skip to content
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

no more logger on dev/urandom #784

Closed
wants to merge 1 commit into from
Closed

no more logger on dev/urandom #784

wants to merge 1 commit into from

Conversation

jessfraz
Copy link
Contributor

i never want to see this error again
.... or hear the work entropy
or touch a computer
k thanks

xoxo,
Jess

Signed-off-by: Jessica Frazelle acidburn@docker.com

i never want to see this error again
.... or hear the work entropy
or touch a computer
k thanks

xoxo,
Jess

Signed-off-by: Jessica Frazelle <acidburn@docker.com>
@tiborvass
Copy link
Contributor

@jfrazelle @icecrime /cc @dmcgowan @stevvooe

I totally understand that we need the CI to be green, and I'm fine merging Jess's PR if we're fine reverting the ability to log this error later. Otherwise I'm -1 on it because this is an issue we have to figure out and burying the symptoms will not help.

I would prefer this solution:

diff --git a/context/context.go b/context/context.go
index 7a3a70e..d9efed8 100644
--- a/context/context.go
+++ b/context/context.go
@@ -27,13 +27,15 @@ func (ic *instanceContext) Value(key interface{}) interface{} {

 var background = &instanceContext{
    Context: context.Background(),
-   id:      uuid.Generate().String(),
 }

 // Background returns a non-nil, empty Context. The background context
 // provides a single key, "instance.id" that is globally unique to the
 // process.
 func Background() Context {
+   if len(background.id) == 0 {
+       background.id = uuid.Generate().String()
+   }
    return background
 }

diff --git a/context/logger.go b/context/logger.go
index b0f0c50..78e4212 100644
--- a/context/logger.go
+++ b/context/logger.go
@@ -3,8 +3,6 @@ package context
 import (
    "fmt"

-   "github.com/docker/distribution/uuid"
-
    "github.com/Sirupsen/logrus"
 )

@@ -101,8 +99,3 @@ func getLogrusLogger(ctx Context, keys ...interface{}) *logrus.Entry {

    return logger.WithFields(fields)
 }
-
-func init() {
-   // inject a logger into the uuid library.
-   uuid.Loggerf = GetLogger(Background()).Warnf
-}

Basically the uuid package is currently called even before all the init() functions are called so there is no way we can override Loggerf. With the patch above, it should be possible to override Loggerf even from docker/docker before uuid.Generate() is called.

And if we really want to play it safe with the initialization orders, we could make it not log by default:

diff --git a/uuid/uuid.go b/uuid/uuid.go
index 02de33f..c9556ee 100644
--- a/uuid/uuid.go
+++ b/uuid/uuid.go
@@ -8,7 +8,6 @@ import (
        "crypto/rand"
        "fmt"
        "io"
-       "log"
        "os"
        "syscall"
        "time"
@@ -30,7 +29,7 @@ var (

        // Loggerf can be used to override the default logging destination. Such
        // log messages in this library should be logged at warning or higher.
-       Loggerf = log.Printf
+       Loggerf func(string, ...interface{})
 )

 // UUID represents a UUID value. UUIDs can be compared and set to other values
@@ -66,7 +65,9 @@ func Generate() (u UUID) {
                        if retryOnError(err) && retries < maxretries {
                                count += n
                                retries++
-                               Loggerf("error generating version 4 uuid, retrying: %v", err)
+                               if Loggerf != nil {
+                                       Loggerf("error generating version 4 uuid, retrying: %v", err)
+                               }
                                continue
                        }

@stevvooe
Copy link
Collaborator

Closing for #792.

@stevvooe stevvooe closed this Jul 30, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants