From b2826a0cee9b266060f02896a1d5902d693f5176 Mon Sep 17 00:00:00 2001 From: Alaykumar Patel Date: Thu, 13 Dec 2018 10:36:29 -0500 Subject: [PATCH 1/2] pkg/controller: Fix CR with same name in different namespaces This patch uses / as key to store the cluster in Controller struct to avoid over-writing same named CR's in different namespaces. Fixes #1954 --- pkg/cluster/cluster.go | 2 +- pkg/controller/controller.go | 20 +++++++++------- pkg/controller/controller_test.go | 38 +++++++++++++++++++++++++++---- pkg/controller/informer.go | 2 +- 4 files changed, 48 insertions(+), 14 deletions(-) diff --git a/pkg/cluster/cluster.go b/pkg/cluster/cluster.go index e74a6b9b2..7730334ee 100644 --- a/pkg/cluster/cluster.go +++ b/pkg/cluster/cluster.go @@ -86,7 +86,7 @@ type Cluster struct { } func New(config Config, cl *api.EtcdCluster) *Cluster { - lg := logrus.WithField("pkg", "cluster").WithField("cluster-name", cl.Name) + lg := logrus.WithField("pkg", "cluster").WithField("cluster-name", cl.Name).WithField("cluster-namespace", cl.Namespace) c := &Cluster{ logger: lg, diff --git a/pkg/controller/controller.go b/pkg/controller/controller.go index 499fcc070..2426cf6d0 100644 --- a/pkg/controller/controller.go +++ b/pkg/controller/controller.go @@ -73,7 +73,7 @@ func (c *Controller) handleClusterEvent(event *Event) (bool, error) { if clus.Status.IsFailed() { clustersFailed.Inc() if event.Type == kwatch.Deleted { - delete(c.clusters, clus.Name) + delete(c.clusters, getNamespacedName(clus)) return false, nil } return false, fmt.Errorf("ignore failed cluster (%s). Please delete its CR", clus.Name) @@ -87,30 +87,30 @@ func (c *Controller) handleClusterEvent(event *Event) (bool, error) { switch event.Type { case kwatch.Added: - if _, ok := c.clusters[clus.Name]; ok { + if _, ok := c.clusters[getNamespacedName(clus)]; ok { return false, fmt.Errorf("unsafe state. cluster (%s) was created before but we received event (%s)", clus.Name, event.Type) } nc := cluster.New(c.makeClusterConfig(), clus) - c.clusters[clus.Name] = nc + c.clusters[getNamespacedName(clus)] = nc clustersCreated.Inc() clustersTotal.Inc() case kwatch.Modified: - if _, ok := c.clusters[clus.Name]; !ok { + if _, ok := c.clusters[getNamespacedName(clus)]; !ok { return false, fmt.Errorf("unsafe state. cluster (%s) was never created but we received event (%s)", clus.Name, event.Type) } - c.clusters[clus.Name].Update(clus) + c.clusters[getNamespacedName(clus)].Update(clus) clustersModified.Inc() case kwatch.Deleted: - if _, ok := c.clusters[clus.Name]; !ok { + if _, ok := c.clusters[getNamespacedName(clus)]; !ok { return false, fmt.Errorf("unsafe state. cluster (%s) was never created but we received event (%s)", clus.Name, event.Type) } - c.clusters[clus.Name].Delete() - delete(c.clusters, clus.Name) + c.clusters[getNamespacedName(clus)].Delete() + delete(c.clusters, getNamespacedName(clus)) clustersDeleted.Inc() clustersTotal.Dec() } @@ -132,3 +132,7 @@ func (c *Controller) initCRD() error { } return k8sutil.WaitCRDReady(c.KubeExtCli, api.EtcdClusterCRDName) } + +func getNamespacedName(c *api.EtcdCluster) string { + return fmt.Sprintf("%s%c%s", c.Namespace, '/', c.Name) +} diff --git a/pkg/controller/controller_test.go b/pkg/controller/controller_test.go index fde2100a3..43d2d4cec 100644 --- a/pkg/controller/controller_test.go +++ b/pkg/controller/controller_test.go @@ -62,14 +62,14 @@ func TestHandleClusterEventDeleteFailedCluster(t *testing.T) { Object: clus, } - c.clusters[name] = &cluster.Cluster{} + c.clusters[getNamespacedName(clus)] = &cluster.Cluster{} if _, err := c.handleClusterEvent(e); err != nil { t.Fatal(err) } - if c.clusters[name] != nil { - t.Errorf("failed cluster not cleaned up after delete event, cluster struct: %v", c.clusters[name]) + if c.clusters[getNamespacedName(clus)] != nil { + t.Errorf("failed cluster not cleaned up after delete event, cluster struct: %v", c.clusters[getNamespacedName(clus)]) } } @@ -78,7 +78,8 @@ func TestHandleClusterEventClusterwide(t *testing.T) { clus := &api.EtcdCluster{ ObjectMeta: metav1.ObjectMeta{ - Name: "test", + Name: "test", + Namespace: "a", Annotations: map[string]string{ "etcd.database.coreos.com/scope": "clusterwide", }, @@ -110,6 +111,35 @@ func TestHandleClusterEventClusterwideIgnored(t *testing.T) { } } +func TestHandleClusterEventClusterwideAddTwoCR(t *testing.T) { + c := New(Config{ClusterWide: true}) + clusA := &api.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "a", + }, + } + clusB := &api.EtcdCluster{ + ObjectMeta: metav1.ObjectMeta{ + Name: "test", + Namespace: "b", + }, + } + e := &Event{ + Type: watch.Added, + Object: clusA, + } + ignored, errA := c.handleClusterEvent(e) + if !ignored && errA != nil { + t.Errorf("cluster should be Added") + } + e.Object = clusB + ignored, errB := c.handleClusterEvent(e) + if !ignored && errB != nil { + t.Errorf("cluster should be Added") + } +} + func TestHandleClusterEventNamespacedIgnored(t *testing.T) { c := New(Config{}) diff --git a/pkg/controller/informer.go b/pkg/controller/informer.go index 11424a53b..7d04b4cee 100644 --- a/pkg/controller/informer.go +++ b/pkg/controller/informer.go @@ -129,7 +129,7 @@ func (c *Controller) syncEtcdClus(clus *api.EtcdCluster) { // re-watch or restart could give ADD event. // If for an ADD event the cluster spec is invalid then it is not added to the local cache // so modifying that cluster will result in another ADD event - if _, ok := c.clusters[clus.Name]; ok { + if _, ok := c.clusters[getNamespacedName(clus)]; ok { ev.Type = kwatch.Modified } From 041baa1f5d5afe2e20b68923f5b6a9fd5d04fc26 Mon Sep 17 00:00:00 2001 From: Alaykumar Patel Date: Thu, 13 Dec 2018 13:33:20 -0500 Subject: [PATCH 2/2] changelog: Update CHANGELOG --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 492ceb3d5..14ccc2515 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,8 @@ ### Fixed +- Fixed a bug where `same CR names` in different namespaces with cluster-wide operator were not working as expected [#2026](https://github.com/coreos/etcd-operator/pull/2026) + ### Deprecated ### Security