From f3d2ae5405c4e4af227146a4395ab2b41f0cbd2e Mon Sep 17 00:00:00 2001 From: rohan47 Date: Wed, 7 Apr 2021 02:37:37 +0530 Subject: [PATCH] ceph: support specifying only public network or only cluster network With this change, the Rook ceph cluster can have only a public network or only a cluster network or both can be specified. Prior to this commit if only a public network was specified, it was copied to the cluster network. Signed-off-by: rohan47 --- pkg/operator/ceph/config/network.go | 4 +- pkg/operator/ceph/config/network_test.go | 179 ++++++++++++++--------- pkg/operator/k8sutil/network.go | 18 ++- pkg/operator/k8sutil/network_test.go | 39 ++++- 4 files changed, 152 insertions(+), 88 deletions(-) diff --git a/pkg/operator/ceph/config/network.go b/pkg/operator/ceph/config/network.go index f3ec864c2..68478f448 100644 --- a/pkg/operator/ceph/config/network.go +++ b/pkg/operator/ceph/config/network.go @@ -50,10 +50,8 @@ func generateNetworkSettings(clusterdContext *clusterd.Context, namespace string cephNetworks := []Option{} for _, selectorKey := range NetworkSelectors { - // This means only "public" was specified and thus we use the same subnet for cluster too + // skip if selector is not specified if _, ok := networkSelectors[selectorKey]; !ok { - cephNetworks = append(cephNetworks, cephNetworks[0]) - cephNetworks[1].Option = fmt.Sprintf("%s_network", selectorKey) continue } diff --git a/pkg/operator/ceph/config/network_test.go b/pkg/operator/ceph/config/network_test.go index e12b1dd50..00df7f677 100644 --- a/pkg/operator/ceph/config/network_test.go +++ b/pkg/operator/ceph/config/network_test.go @@ -31,45 +31,113 @@ import ( ) func TestGenerateNetworkSettings(t *testing.T) { - ctxt := context.TODO() - ns := "rook-ceph" - clientset := testop.New(t, 1) - ctx := &clusterd.Context{ - Clientset: clientset, - NetworkClient: fakenetclient.NewSimpleClientset().K8sCniCncfIoV1(), - } + t.Run("no network definition exists", func(*testing.T) { + ns := "rook-ceph" + clientset := testop.New(t, 1) + ctx := &clusterd.Context{ + Clientset: clientset, + NetworkClient: fakenetclient.NewSimpleClientset().K8sCniCncfIoV1(), + } + netSelector := map[string]string{"public": "public-network-attach-def"} + _, err := generateNetworkSettings(ctx, ns, netSelector) + assert.Error(t, err) - // - // TEST 1: network definition does not exist - // - netSelector := map[string]string{ - "public": "public-network-attach-def", - } + }) - _, err := generateNetworkSettings(ctx, ns, netSelector) - assert.Error(t, err) + t.Run("only cluster network", func(*testing.T) { + netSelector := map[string]string{"cluster": "cluster-network-attach-def"} + networks := []networkv1.NetworkAttachmentDefinition{getClusterNetwork()} + expectedNetworks := []Option{ + { + Who: "global", + Option: "cluster_network", + Value: "172.18.0.0/16", + }, + } + ctxt := context.TODO() + ns := "rook-ceph" + clientset := testop.New(t, 1) + ctx := &clusterd.Context{ + Clientset: clientset, + NetworkClient: fakenetclient.NewSimpleClientset().K8sCniCncfIoV1(), + } - // - // TEST 2: single dedicated networks - // - expectedNetworks := []Option{ - { - Who: "global", - Option: "public_network", - Value: "192.168.0.0/24", - }, - { - Who: "global", - Option: "cluster_network", - Value: "192.168.0.0/24", - }, - } + for i := range networks { + _, err := ctx.NetworkClient.NetworkAttachmentDefinitions(ns).Create(ctxt, &networks[i], metav1.CreateOptions{}) + assert.NoError(t, err) + } + cephNetwork, err := generateNetworkSettings(ctx, ns, netSelector) + assert.NoError(t, err) + assert.ElementsMatch(t, cephNetwork, expectedNetworks, fmt.Sprintf("networks: %+v", cephNetwork)) + }) - // this nad uses whereabouts cni - network := &networkv1.NetworkAttachmentDefinition{ + t.Run("only public network", func(*testing.T) { + ctxt := context.TODO() + ns := "rook-ceph" + clientset := testop.New(t, 1) + ctx := &clusterd.Context{ + Clientset: clientset, + NetworkClient: fakenetclient.NewSimpleClientset().K8sCniCncfIoV1(), + } + netSelector := map[string]string{"public": "public-network-attach-def"} + networks := []networkv1.NetworkAttachmentDefinition{getPublicNetwork()} + expectedNetworks := []Option{ + { + Who: "global", + Option: "public_network", + Value: "192.168.0.0/24", + }, + } + for i := range networks { + _, err := ctx.NetworkClient.NetworkAttachmentDefinitions(ns).Create(ctxt, &networks[i], metav1.CreateOptions{}) + assert.NoError(t, err) + } + cephNetwork, err := generateNetworkSettings(ctx, ns, netSelector) + assert.NoError(t, err) + assert.ElementsMatch(t, cephNetwork, expectedNetworks, fmt.Sprintf("networks: %+v", cephNetwork)) + + }) + + t.Run("public and cluster network", func(*testing.T) { + ctxt := context.TODO() + ns := "rook-ceph" + clientset := testop.New(t, 1) + ctx := &clusterd.Context{ + Clientset: clientset, + NetworkClient: fakenetclient.NewSimpleClientset().K8sCniCncfIoV1(), + } + netSelector := map[string]string{ + "public": "public-network-attach-def", + "cluster": "cluster-network-attach-def", + } + networks := []networkv1.NetworkAttachmentDefinition{getPublicNetwork(), getClusterNetwork()} + expectedNetworks := []Option{ + { + Who: "global", + Option: "public_network", + Value: "192.168.0.0/24", + }, + { + Who: "global", + Option: "cluster_network", + Value: "172.18.0.0/16", + }, + } + for i := range networks { + _, err := ctx.NetworkClient.NetworkAttachmentDefinitions(ns).Create(ctxt, &networks[i], metav1.CreateOptions{}) + assert.NoError(t, err) + } + cephNetwork, err := generateNetworkSettings(ctx, ns, netSelector) + assert.NoError(t, err) + assert.ElementsMatch(t, cephNetwork, expectedNetworks, fmt.Sprintf("networks: %+v", cephNetwork)) + + }) +} + +func getPublicNetwork() networkv1.NetworkAttachmentDefinition { + return networkv1.NetworkAttachmentDefinition{ ObjectMeta: metav1.ObjectMeta{ - Name: "public-network-attach-def", - Namespace: ns, + Name: "public-network-attach-def", }, Spec: networkv1.NetworkAttachmentDefinitionSpec{ Config: `{ @@ -85,39 +153,12 @@ func TestGenerateNetworkSettings(t *testing.T) { }`, }, } +} - // Create public network definition - _, err = ctx.NetworkClient.NetworkAttachmentDefinitions(ns).Create(ctxt, network, metav1.CreateOptions{}) - assert.NoError(t, err) - - cephNetwork, err := generateNetworkSettings(ctx, ns, netSelector) - assert.NoError(t, err) - assert.ElementsMatch(t, cephNetwork, expectedNetworks, fmt.Sprintf("networks: %+v", cephNetwork)) - - // - // TEST 3: two dedicated networks - // - expectedNetworks = []Option{ - { - Who: "global", - Option: "public_network", - Value: "192.168.0.0/24", - }, - { - Who: "global", - Option: "cluster_network", - Value: "172.18.0.0/16", - }, - } - - netSelector = map[string]string{ - "public": "public-network-attach-def", - "cluster": "cluster-network-attach-def", - } - network2 := &networkv1.NetworkAttachmentDefinition{ +func getClusterNetwork() networkv1.NetworkAttachmentDefinition { + return networkv1.NetworkAttachmentDefinition{ ObjectMeta: metav1.ObjectMeta{ - Name: "cluster-network-attach-def", - Namespace: ns, + Name: "cluster-network-attach-def", }, Spec: networkv1.NetworkAttachmentDefinitionSpec{ Config: `{ @@ -133,14 +174,6 @@ func TestGenerateNetworkSettings(t *testing.T) { }`, }, } - - // Create cluster network definition - _, err = ctx.NetworkClient.NetworkAttachmentDefinitions(ns).Create(ctxt, network2, metav1.CreateOptions{}) - assert.NoError(t, err) - - cephNetwork, err = generateNetworkSettings(ctx, ns, netSelector) - assert.NoError(t, err) - assert.ElementsMatch(t, cephNetwork, expectedNetworks, fmt.Sprintf("networks: %+v", cephNetwork)) } func TestGetNetworkRange(t *testing.T) { diff --git a/pkg/operator/k8sutil/network.go b/pkg/operator/k8sutil/network.go index 7d1d7fef1..cef68d9ce 100644 --- a/pkg/operator/k8sutil/network.go +++ b/pkg/operator/k8sutil/network.go @@ -29,6 +29,8 @@ import ( const ( // publicNetworkSelectorKeyName is the network selector key for the ceph public network publicNetworkSelectorKeyName = "public" + // clusterNetworkSelectorKeyName is the network selector key for the ceph cluster network + clusterNetworkSelectorKeyName = "cluster" ) // NetworkAttachmentConfig represents the configuration of the NetworkAttachmentDefinitions object @@ -77,13 +79,16 @@ func ApplyMultus(net rookv1.NetworkSpec, objectMeta *metav1.ObjectMeta) error { shortSyntax = true } - isCsi := strings.Contains(objectMeta.Labels["app"], "csi-") - if isCsi { + var isExcluded bool + for _, clusterNetworkApps := range getClusterNetworkApps() { + isExcluded = strings.Contains(objectMeta.Labels["app"], clusterNetworkApps) + } + if isExcluded { + v = append(v, string(ns)) + } else { if k == publicNetworkSelectorKeyName { v = append(v, string(ns)) } - } else { - v = append(v, string(ns)) } } @@ -104,6 +109,11 @@ func ApplyMultus(net rookv1.NetworkSpec, objectMeta *metav1.ObjectMeta) error { return nil } +// getClusterNetworkApps returns the list of ceph apps that utilize cluster network +func getClusterNetworkApps() []string { + return []string{"osd"} +} + // GetNetworkAttachmentConfig returns the NetworkAttachmentDefinitions configuration func GetNetworkAttachmentConfig(n netapi.NetworkAttachmentDefinition) (NetworkAttachmentConfig, error) { netConfigJSON := n.Spec.Config diff --git a/pkg/operator/k8sutil/network_test.go b/pkg/operator/k8sutil/network_test.go index 0e03f4e89..fec048710 100644 --- a/pkg/operator/k8sutil/network_test.go +++ b/pkg/operator/k8sutil/network_test.go @@ -17,6 +17,7 @@ limitations under the License. package k8sutil import ( + "strings" "testing" netapi "github.com/k8snetworkplumbingwg/network-attachment-definition-client/pkg/apis/k8s.cni.cncf.io/v1" @@ -29,18 +30,37 @@ func TestNetwork_ApplyMultusShort(t *testing.T) { net := rookv1.NetworkSpec{ Provider: "multus", Selectors: map[string]string{ - "server": "macvlan@net1", - "broker": "macvlan@net2", + publicNetworkSelectorKeyName: "macvlan@net1", + clusterNetworkSelectorKeyName: "macvlan@net2", }, } - objMeta := metav1.ObjectMeta{} - err := ApplyMultus(net, &objMeta) - assert.NoError(t, err) + tests := []struct { + name string + labels map[string]string + want []string + }{ + { + name: "for a non osd pod", + want: []string{"macvlan@net1"}, + }, + { + name: "for an osd pod", + labels: map[string]string{"app": "rook-ceph-osd"}, + want: []string{"macvlan@net1", "macvlan@net2"}, + }, + } + for _, test := range tests { + t.Run(test.name, func(t *testing.T) { + objMeta := metav1.ObjectMeta{} + objMeta.Labels = test.labels + err := ApplyMultus(net, &objMeta) + assert.NoError(t, err) + appliedNetworksList := strings.Split(objMeta.Annotations["k8s.v1.cni.cncf.io/networks"], ", ") + assert.ElementsMatch(t, appliedNetworksList, test.want) + }) - assert.Contains(t, objMeta.Annotations, "k8s.v1.cni.cncf.io/networks") - assert.Contains(t, objMeta.Annotations["k8s.v1.cni.cncf.io/networks"], "macvlan@net1") - assert.Contains(t, objMeta.Annotations["k8s.v1.cni.cncf.io/networks"], "macvlan@net2") + } } func TestNetwork_ApplyMultusJSON(t *testing.T) { @@ -53,6 +73,9 @@ func TestNetwork_ApplyMultusJSON(t *testing.T) { } objMeta := metav1.ObjectMeta{} + objMeta.Labels = map[string]string{ + "app": "rook-ceph-osd", + } err := ApplyMultus(net, &objMeta) assert.NoError(t, err)