From 68097e96dcacdc692d7886934fc69c540eef57b8 Mon Sep 17 00:00:00 2001 From: Christian Bell Date: Mon, 27 Feb 2017 16:08:42 -0800 Subject: [PATCH] Better logging and address review comments --- controllers/gce/controller/fakes.go | 2 +- .../gce/loadbalancers/loadbalancers_test.go | 5 +- controllers/gce/main.go | 67 ++++++++++--------- controllers/gce/storage/configmaps.go | 2 +- controllers/gce/utils/utils.go | 11 +-- 5 files changed, 43 insertions(+), 44 deletions(-) diff --git a/controllers/gce/controller/fakes.go b/controllers/gce/controller/fakes.go index 52927bd99..a4870593c 100644 --- a/controllers/gce/controller/fakes.go +++ b/controllers/gce/controller/fakes.go @@ -49,7 +49,7 @@ func NewFakeClusterManager(clusterName, firewallName string) *fakeClusterManager fakeBackends := backends.NewFakeBackendServices(func(op int, be *compute.BackendService) error { return nil }) fakeIGs := instances.NewFakeInstanceGroups(sets.NewString()) fakeHCs := healthchecks.NewFakeHealthChecks() - namer := utils.NewNamerWithFirewall(clusterName, firewallName) + namer := utils.NewNamer(clusterName, firewallName) nodePool := instances.NewNodePool(fakeIGs) nodePool.Init(&instances.FakeZoneLister{Zones: []string{"zone-a"}}) diff --git a/controllers/gce/loadbalancers/loadbalancers_test.go b/controllers/gce/loadbalancers/loadbalancers_test.go index f1373a933..581f7010a 100644 --- a/controllers/gce/loadbalancers/loadbalancers_test.go +++ b/controllers/gce/loadbalancers/loadbalancers_test.go @@ -236,7 +236,8 @@ func TestUpdateUrlMapNoChanges(t *testing.T) { func TestNameParsing(t *testing.T) { clusterName := "123" - namer := utils.NewNamer(clusterName) + firewallName := clusterName + namer := utils.NewNamer(clusterName, firewallName) fullName := namer.Truncate(fmt.Sprintf("%v-%v", forwardingRulePrefix, namer.LBName("testlb"))) annotationsMap := map[string]string{ fmt.Sprintf("%v/forwarding-rule", utils.K8sAnnotationPrefix): fullName, @@ -308,7 +309,7 @@ func TestClusterNameChange(t *testing.T) { } func TestInvalidClusterNameChange(t *testing.T) { - namer := utils.NewNamer("test--123") + namer := utils.NewNamer("test--123", "test--123") if got := namer.GetClusterName(); got != "123" { t.Fatalf("Expected name 123, got %v", got) } diff --git a/controllers/gce/main.go b/controllers/gce/main.go index ac9bd457d..d71752ce5 100644 --- a/controllers/gce/main.go +++ b/controllers/gce/main.go @@ -257,7 +257,7 @@ func newNamer(kubeClient client.Interface, clusterName string, fwName string) (* return nil, err } - namer := utils.NewNamerWithFirewall(name, fw_name) + namer := utils.NewNamer(name, fw_name) uidVault := storage.NewConfigMapVault(kubeClient, api.NamespaceSystem, uidConfigMapName) // Start a goroutine to poll the cluster UID config map @@ -270,20 +270,25 @@ func newNamer(kubeClient client.Interface, clusterName string, fwName string) (* if err != nil { glog.Errorf("Can't read uidConfigMap %v", uidConfigMapName) } else if !found { - glog.Errorf("Can't read %v from uidConfigMap %v", key, uidConfigMapName) - continue - } - - switch key { - case storage.UidDataKey: - if uid := namer.GetClusterName(); uid != val { - glog.Infof("Cluster uid changed from %v -> %v", uid, val) - namer.SetClusterName(val) + errmsg := fmt.Sprintf("Can't read %v from uidConfigMap %v", key, uidConfigMapName) + if key == storage.UidDataKey { + glog.Errorf(errmsg) + } else { + glog.V(4).Infof(errmsg) } - case storage.ProviderDataKey: - if fw_name := namer.GetFirewallName(); fw_name != val { - glog.Infof("Cluster firewall name changed from %v -> %v", fw_name, val) - namer.SetFirewallName(val) + } else { + + switch key { + case storage.UidDataKey: + if uid := namer.GetClusterName(); uid != val { + glog.Infof("Cluster uid changed from %v -> %v", uid, val) + namer.SetClusterName(val) + } + case storage.ProviderDataKey: + if fw_name := namer.GetFirewallName(); fw_name != val { + glog.Infof("Cluster firewall name changed from %v -> %v", fw_name, val) + namer.SetFirewallName(val) + } } } } @@ -291,42 +296,42 @@ func newNamer(kubeClient client.Interface, clusterName string, fwName string) (* return namer, nil } -// getFlagOrLookupVault returns the name to use associated to a flag and configmap. +// useDefaultOrLookupVault returns either a 'default_name' or if unset, obtains a name from a ConfigMap. // The returned value follows this priority: -// If the provided 'name' is not empty, that name is used. +// If the provided 'default_name' is not empty, that name is used. // This is effectively a client override via a command line flag. -// else, check configmap under 'configmap_name' as a key and if found, use the associated value +// else, check cfgVault with 'cm_key' as a key and if found, use the associated value // else, return an empty 'name' and pass along an error iff the configmap lookup is erroneous. -func getFlagOrLookupVault(cfgVault *storage.ConfigMapVault, cm_key string, name string) (string, error) { - if name != "" { - glog.Infof("Using user provided %v %v", cm_key, name) +func useDefaultOrLookupVault(cfgVault *storage.ConfigMapVault, cm_key, default_name string) (string, error) { + if default_name != "" { + glog.Infof("Using user provided %v %v", cm_key, default_name) // Don't save the uid in the vault, so users can rollback through // setting the accompany flag to "" - return name, nil + return default_name, nil } val, found, err := cfgVault.Get(cm_key) - if found { - glog.Infof("Using %v = %q saved in ConfigMap", cm_key, val) - return val, nil - } else if err != nil { + if err != nil { // This can fail because of: // 1. No such config map - found=false, err=nil // 2. No such key in config map - found=false, err=nil // 3. Apiserver flake - found=false, err!=nil // It is not safe to proceed in 3. - return "", fmt.Errorf("Failed to retrieve %v: %v, using %q as name", cm_key, err, name) + return "", fmt.Errorf("Failed to retrieve %v: %v, returning empty name", cm_key, err) + } else if !found { + // Not found but safe to proceed. + return "", nil } - // Not found but safe to proceed. - return "", nil + glog.Infof("Using %v = %q saved in ConfigMap", cm_key, val) + return val, nil } // getFirewallName returns the firewall rule name to use for this cluster. For // backwards compatibility, the firewall name will default to the cluster UID. // Use getFlagOrLookupVault to obtain a stored or overridden value for the firewall name. // else, use the cluster UID as a backup (this retains backwards compatibility). -func getFirewallName(kubeClient client.Interface, name string, cluster_uid string) (string, error) { +func getFirewallName(kubeClient client.Interface, name, cluster_uid string) (string, error) { cfgVault := storage.NewConfigMapVault(kubeClient, api.NamespaceSystem, uidConfigMapName) - if fw_name, err := getFlagOrLookupVault(cfgVault, storage.ProviderDataKey, name); err != nil { + if fw_name, err := useDefaultOrLookupVault(cfgVault, storage.ProviderDataKey, name); err != nil { return "", err } else if fw_name != "" { return fw_name, cfgVault.Put(storage.ProviderDataKey, fw_name) @@ -344,7 +349,7 @@ func getFirewallName(kubeClient client.Interface, name string, cluster_uid strin // else, allocate a new uid func getClusterUID(kubeClient client.Interface, name string) (string, error) { cfgVault := storage.NewConfigMapVault(kubeClient, api.NamespaceSystem, uidConfigMapName) - if name, err := getFlagOrLookupVault(cfgVault, storage.UidDataKey, name); err != nil { + if name, err := useDefaultOrLookupVault(cfgVault, storage.UidDataKey, name); err != nil { return "", err } else if name != "" { return name, nil diff --git a/controllers/gce/storage/configmaps.go b/controllers/gce/storage/configmaps.go index 7b4f1a42f..6af08b65d 100644 --- a/controllers/gce/storage/configmaps.go +++ b/controllers/gce/storage/configmaps.go @@ -33,7 +33,7 @@ const ( UidDataKey = "uid" // ProviderDataKey is the key used in config maps to store the Provider // UID which we use to ensure unique firewalls. - ProviderDataKey = "providerUid" + ProviderDataKey = "provider-uid" ) // ConfigMapVault stores cluster UIDs in config maps. diff --git a/controllers/gce/utils/utils.go b/controllers/gce/utils/utils.go index 63caccdb3..9d5dbfad1 100644 --- a/controllers/gce/utils/utils.go +++ b/controllers/gce/utils/utils.go @@ -97,15 +97,8 @@ type Namer struct { nameLock sync.Mutex } -// NewNamer creates a new namer. -func NewNamer(clusterName string) *Namer { - namer := &Namer{} - namer.SetClusterName(clusterName) - return namer -} - -// NewNamer creates a new namer with a Firewall Name -func NewNamerWithFirewall(clusterName string, firewallName string) *Namer { +// NewNamer creates a new namer with a Cluster and Firewall name. +func NewNamer(clusterName, firewallName string) *Namer { namer := &Namer{} namer.SetClusterName(clusterName) namer.SetFirewallName(firewallName)