Better logging and address review comments

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"}})

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)
 	}

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,63 +270,68 @@ 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)
+					}
 				}
 			}
 		}
 	}, 5*time.Second)
 	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

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.

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)