From 6ec86cb6e517bfb5ded818244b9db9510a2ed0b9 Mon Sep 17 00:00:00 2001 From: Michael Crosby Date: Fri, 23 May 2014 17:51:16 -0700 Subject: [PATCH] Improve name generation on concurrent requests Fixes #2586 This fixes a few races where the name generator asks if a name is free but another container takes the name before it can be reserved. This solves this by generating the name and setting it. If the set fails with a non unique error then we try again. Docker-DCO-1.1-Signed-off-by: Michael Crosby (github: crosbymichael) --- daemon/daemon.go | 75 ++++++++++++++-------- daemon/utils.go | 19 +----- pkg/namesgenerator/names-generator.go | 21 +++--- pkg/namesgenerator/names-generator_test.go | 28 +------- 4 files changed, 63 insertions(+), 80 deletions(-) diff --git a/daemon/daemon.go b/daemon/daemon.go index 4ea6416ca5..78dee62523 100644 --- a/daemon/daemon.go +++ b/daemon/daemon.go @@ -29,6 +29,7 @@ import ( "github.com/dotcloud/docker/pkg/graphdb" "github.com/dotcloud/docker/pkg/label" "github.com/dotcloud/docker/pkg/mount" + "github.com/dotcloud/docker/pkg/namesgenerator" "github.com/dotcloud/docker/pkg/networkfs/resolvconf" "github.com/dotcloud/docker/pkg/selinux" "github.com/dotcloud/docker/pkg/sysinfo" @@ -250,20 +251,15 @@ func (daemon *Daemon) register(container *Container, updateSuffixarray bool) err func (daemon *Daemon) ensureName(container *Container) error { if container.Name == "" { - name, err := generateRandomName(daemon) + name, err := daemon.generateNewName(container.ID) if err != nil { - name = utils.TruncateID(container.ID) + return err } container.Name = name if err := container.ToDisk(); err != nil { utils.Debugf("Error saving container name %s", err) } - if !daemon.containerGraph.Exists(name) { - if _, err := daemon.containerGraph.Set(name, container.ID); err != nil { - utils.Debugf("Setting default id - %s", err) - } - } } return nil } @@ -370,12 +366,8 @@ func (daemon *Daemon) restore() error { // Any containers that are left over do not exist in the graph for _, container := range containers { // Try to set the default name for a container if it exists prior to links - container.Name, err = generateRandomName(daemon) + container.Name, err = daemon.generateNewName(container.ID) if err != nil { - container.Name = utils.TruncateID(container.ID) - } - - if _, err := daemon.containerGraph.Set(container.Name, container.ID); err != nil { utils.Debugf("Setting default id - %s", err) } registerContainer(container) @@ -470,42 +462,75 @@ func (daemon *Daemon) generateIdAndName(name string) (string, string, error) { ) if name == "" { - name, err = generateRandomName(daemon) - if err != nil { - name = utils.TruncateID(id) - } - } else { - if !validContainerNamePattern.MatchString(name) { - return "", "", fmt.Errorf("Invalid container name (%s), only %s are allowed", name, validContainerNameChars) + if name, err = daemon.generateNewName(id); err != nil { + return "", "", err } + return id, name, nil } + + if name, err = daemon.reserveName(id, name); err != nil { + return "", "", err + } + + return id, name, nil +} + +func (daemon *Daemon) reserveName(id, name string) (string, error) { + if !validContainerNamePattern.MatchString(name) { + return "", fmt.Errorf("Invalid container name (%s), only %s are allowed", name, validContainerNameChars) + } + if name[0] != '/' { name = "/" + name } - // Set the enitity in the graph using the default name specified + if _, err := daemon.containerGraph.Set(name, id); err != nil { if !graphdb.IsNonUniqueNameError(err) { - return "", "", err + return "", err } conflictingContainer, err := daemon.GetByName(name) if err != nil { if strings.Contains(err.Error(), "Could not find entity") { - return "", "", err + return "", err } // Remove name and continue starting the container if err := daemon.containerGraph.Delete(name); err != nil { - return "", "", err + return "", err } } else { nameAsKnownByUser := strings.TrimPrefix(name, "/") - return "", "", fmt.Errorf( + return "", fmt.Errorf( "Conflict, The name %s is already assigned to %s. You have to delete (or rename) that container to be able to assign %s to a container again.", nameAsKnownByUser, utils.TruncateID(conflictingContainer.ID), nameAsKnownByUser) } } - return id, name, nil + return name, nil +} + +func (daemon *Daemon) generateNewName(id string) (string, error) { + var name string + for i := 1; i < 6; i++ { + name = namesgenerator.GetRandomName(i) + if name[0] != '/' { + name = "/" + name + } + + if _, err := daemon.containerGraph.Set(name, id); err != nil { + if !graphdb.IsNonUniqueNameError(err) { + return "", err + } + continue + } + return name, nil + } + + name = "/" + utils.TruncateID(id) + if _, err := daemon.containerGraph.Set(name, id); err != nil { + return "", err + } + return name, nil } func (daemon *Daemon) generateHostname(id string, config *runconfig.Config) { diff --git a/daemon/utils.go b/daemon/utils.go index 15b62e2a06..d60d985152 100644 --- a/daemon/utils.go +++ b/daemon/utils.go @@ -2,10 +2,10 @@ package daemon import ( "fmt" - "github.com/dotcloud/docker/nat" - "github.com/dotcloud/docker/pkg/namesgenerator" - "github.com/dotcloud/docker/runconfig" "strings" + + "github.com/dotcloud/docker/nat" + "github.com/dotcloud/docker/runconfig" ) func migratePortMappings(config *runconfig.Config, hostConfig *runconfig.HostConfig) error { @@ -49,16 +49,3 @@ func mergeLxcConfIntoOptions(hostConfig *runconfig.HostConfig, driverConfig map[ driverConfig["lxc"] = lxc } } - -type checker struct { - daemon *Daemon -} - -func (c *checker) Exists(name string) bool { - return c.daemon.containerGraph.Exists("/" + name) -} - -// Generate a random and unique name -func generateRandomName(daemon *Daemon) (string, error) { - return namesgenerator.GenerateRandomName(&checker{daemon}) -} diff --git a/pkg/namesgenerator/names-generator.go b/pkg/namesgenerator/names-generator.go index 07fadf8171..a89e5b29e2 100644 --- a/pkg/namesgenerator/names-generator.go +++ b/pkg/namesgenerator/names-generator.go @@ -6,10 +6,6 @@ import ( "time" ) -type NameChecker interface { - Exists(name string) bool -} - var ( left = [...]string{"happy", "jolly", "dreamy", "sad", "angry", "pensive", "focused", "sleepy", "grave", "distracted", "determined", "stoic", "stupefied", "sharp", "agitated", "cocky", "tender", "goofy", "furious", "desperate", "hopeful", "compassionate", "silly", "lonely", "condescending", "naughty", "kickass", "drunk", "boring", "nostalgic", "ecstatic", "insane", "cranky", "mad", "jovial", "sick", "hungry", "thirsty", "elegant", "backstabbing", "clever", "trusting", "loving", "suspicious", "berserk", "high", "romantic", "prickly", "evil"} // Docker 0.7.x generates names from notable scientists and hackers. @@ -79,16 +75,17 @@ var ( right = [...]string{"lovelace", "franklin", "tesla", "einstein", "bohr", "davinci", "pasteur", "nobel", "curie", "darwin", "turing", "ritchie", "torvalds", "pike", "thompson", "wozniak", "galileo", "euclid", "newton", "fermat", "archimedes", "poincare", "heisenberg", "feynman", "hawking", "fermi", "pare", "mccarthy", "engelbart", "babbage", "albattani", "ptolemy", "bell", "wright", "lumiere", "morse", "mclean", "brown", "bardeen", "brattain", "shockley", "goldstine", "hoover", "hopper", "bartik", "sammet", "jones", "perlman", "wilson", "kowalevski", "hypatia", "goodall", "mayer", "elion", "blackwell", "lalande", "kirch", "ardinghelli", "colden", "almeida", "leakey", "meitner", "mestorf", "rosalind", "sinoussi", "carson", "mcclintock", "yonath"} ) -func GenerateRandomName(checker NameChecker) (string, error) { - retry := 5 +func GetRandomName(retry int) string { rand.Seed(time.Now().UnixNano()) + +begin: name := fmt.Sprintf("%s_%s", left[rand.Intn(len(left))], right[rand.Intn(len(right))]) - for checker != nil && checker.Exists(name) && retry > 0 || name == "boring_wozniak" /* Steve Wozniak is not boring */ { + if name == "boring_wozniak" /* Steve Wozniak is not boring */ { + goto begin + } + + if retry > 0 { name = fmt.Sprintf("%s%d", name, rand.Intn(10)) - retry = retry - 1 } - if retry == 0 { - return name, fmt.Errorf("Error generating random name") - } - return name, nil + return name } diff --git a/pkg/namesgenerator/names-generator_test.go b/pkg/namesgenerator/names-generator_test.go index bcee7c86a7..2652d42ab4 100644 --- a/pkg/namesgenerator/names-generator_test.go +++ b/pkg/namesgenerator/names-generator_test.go @@ -4,35 +4,9 @@ import ( "testing" ) -type FalseChecker struct{} - -func (n *FalseChecker) Exists(name string) bool { - return false -} - -type TrueChecker struct{} - -func (n *TrueChecker) Exists(name string) bool { - return true -} - -func TestGenerateRandomName(t *testing.T) { - if _, err := GenerateRandomName(&FalseChecker{}); err != nil { - t.Error(err) - } - - if _, err := GenerateRandomName(&TrueChecker{}); err == nil { - t.Error("An error was expected") - } - -} - // Make sure the generated names are awesome func TestGenerateAwesomeNames(t *testing.T) { - name, err := GenerateRandomName(&FalseChecker{}) - if err != nil { - t.Error(err) - } + name := GetRandomName(0) if !isAwesome(name) { t.Fatalf("Generated name '%s' is not awesome.", name) }