From e65fbe9862f1d4aee7104482ced1a652d52ce597 Mon Sep 17 00:00:00 2001 From: Humble Chirammal Date: Tue, 17 Aug 2021 20:40:24 +0530 Subject: [PATCH 01/21] rebase: make use of v0.10.0 of csi-lib-utils Signed-off-by: Humble Chirammal --- go.mod | 2 +- go.sum | 5 ++--- .../kubernetes-csi/csi-lib-utils/connection/connection.go | 3 ++- vendor/modules.txt | 2 +- 4 files changed, 6 insertions(+), 6 deletions(-) diff --git a/go.mod b/go.mod index 18f8dd288..39552dd3f 100644 --- a/go.mod +++ b/go.mod @@ -12,7 +12,7 @@ require ( github.com/grpc-ecosystem/go-grpc-middleware v1.3.0 github.com/grpc-ecosystem/go-grpc-prometheus v1.2.0 github.com/hashicorp/vault/api v1.0.5-0.20200902155336-f9d5ce5a171a - github.com/kubernetes-csi/csi-lib-utils v0.9.1 + github.com/kubernetes-csi/csi-lib-utils v0.10.0 github.com/kubernetes-csi/external-snapshotter/client/v4 v4.2.0 github.com/libopenstorage/secrets v0.0.0-20210709082113-dde442ea20ec github.com/onsi/ginkgo v1.16.4 diff --git a/go.sum b/go.sum index 250fbaadb..bbf5fa9d3 100644 --- a/go.sum +++ b/go.sum @@ -187,7 +187,6 @@ github.com/cockroachdb/datadriven v0.0.0-20200714090401-bf6692d28da5/go.mod h1:h github.com/cockroachdb/errors v1.2.4/go.mod h1:rQD95gz6FARkaKkQXUksEje/d9a6wBJoCr5oaCLELYA= github.com/cockroachdb/logtags v0.0.0-20190617123548-eb05cc24525f/go.mod h1:i/u985jwjWRlyHXQbwatDASoW0RMlZ/3i9yJHE2xLkI= github.com/codegangsta/inject v0.0.0-20150114235600-33e0aa1cb7c0/go.mod h1:4Zcjuz89kmFXt9morQgcfYZAYZ5n8WHjt81YYWIwtTM= -github.com/container-storage-interface/spec v1.2.0/go.mod h1:6URME8mwIBbpVyZV93Ce5St17xBiQJQY67NDsuohiy4= github.com/container-storage-interface/spec v1.5.0 h1:lvKxe3uLgqQeVQcrnL2CPQKISoKjTJxojEs9cBk+HXo= github.com/container-storage-interface/spec v1.5.0/go.mod h1:8K96oQNkJ7pFcC2R9Z1ynGGBB1I93kcS6PGg3SsOk8s= github.com/containerd/cgroups v0.0.0-20190919134610-bf292b21730f/go.mod h1:OApqhQ4XNSNC13gXIwDjhOQxjWa/NxkwZXJ1EvqT0ko= @@ -638,8 +637,8 @@ github.com/kr/pty v1.1.1/go.mod h1:pFQYn66WHrOpPYNljwOMqo10TkYh1fy3cYio2l3bCsQ= github.com/kr/text v0.1.0/go.mod h1:4Jbv+DJW3UT/LiOwJeYQe1efqtUx/iVham/4vfdArNI= github.com/kr/text v0.2.0 h1:5Nx0Ya0ZqY2ygV366QzturHI13Jq95ApcVaJBhpS+AY= github.com/kr/text v0.2.0/go.mod h1:eLer722TekiGuMkidMxC/pM04lWEeraHUUmBw8l2grE= -github.com/kubernetes-csi/csi-lib-utils v0.9.1 h1:sGq6ifVujfMSkfTsMZip44Ttv8SDXvsBlFk9GdYl/b8= -github.com/kubernetes-csi/csi-lib-utils v0.9.1/go.mod h1:8E2jVUX9j3QgspwHXa6LwyN7IHQDjW9jX3kwoWnSC+M= +github.com/kubernetes-csi/csi-lib-utils v0.10.0 h1:Aqm8X81eCzzfH/bvIEqSWtcbK9HF9NbFk4d+le1snVA= +github.com/kubernetes-csi/csi-lib-utils v0.10.0/go.mod h1:BmGZZB16L18+9+Lgg9YWwBKfNEHIDdgGfAyuW6p2NV0= github.com/kubernetes-csi/external-snapshotter/client/v4 v4.0.0/go.mod h1:YBCo4DoEeDndqvAn6eeu0vWM7QdXmHEeI9cFWplmBys= github.com/kubernetes-csi/external-snapshotter/client/v4 v4.2.0 h1:nHHjmvjitIiyPlUHk/ofpgvBcNcawJLtf4PYHORLjAA= github.com/kubernetes-csi/external-snapshotter/client/v4 v4.2.0/go.mod h1:YBCo4DoEeDndqvAn6eeu0vWM7QdXmHEeI9cFWplmBys= diff --git a/vendor/github.com/kubernetes-csi/csi-lib-utils/connection/connection.go b/vendor/github.com/kubernetes-csi/csi-lib-utils/connection/connection.go index fbd8d37b5..ad37321e8 100644 --- a/vendor/github.com/kubernetes-csi/csi-lib-utils/connection/connection.go +++ b/vendor/github.com/kubernetes-csi/csi-lib-utils/connection/connection.go @@ -84,7 +84,8 @@ func ExitOnConnectionLoss() func() bool { if err := ioutil.WriteFile(terminationLogPath, []byte(terminationMsg), 0644); err != nil { klog.Errorf("%s: %s", terminationLogPath, err) } - klog.Fatalf(terminationMsg) + klog.Exit(terminationMsg) + // Not reached. return false } } diff --git a/vendor/modules.txt b/vendor/modules.txt index 56ce6d7d9..f0c1a2265 100644 --- a/vendor/modules.txt +++ b/vendor/modules.txt @@ -176,7 +176,7 @@ github.com/inconshreveable/mousetrap github.com/jmespath/go-jmespath # github.com/json-iterator/go v1.1.11 github.com/json-iterator/go -# github.com/kubernetes-csi/csi-lib-utils v0.9.1 +# github.com/kubernetes-csi/csi-lib-utils v0.10.0 ## explicit github.com/kubernetes-csi/csi-lib-utils/connection github.com/kubernetes-csi/csi-lib-utils/metrics From 763387c8e2f15abc14e86930093ab97e3f07b497 Mon Sep 17 00:00:00 2001 From: Humble Chirammal Date: Wed, 18 Aug 2021 09:33:49 +0530 Subject: [PATCH 02/21] rebase: update external-resizer to v1.3.0 release Signed-off-by: Humble Chirammal --- build.env | 2 +- charts/ceph-csi-cephfs/values.yaml | 2 +- charts/ceph-csi-rbd/values.yaml | 2 +- deploy/cephfs/kubernetes/csi-cephfsplugin-provisioner.yaml | 2 +- deploy/rbd/kubernetes/csi-rbdplugin-provisioner.yaml | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/build.env b/build.env index 98522367b..83c716e4b 100644 --- a/build.env +++ b/build.env @@ -52,7 +52,7 @@ ROOK_CEPH_CLUSTER_IMAGE=docker.io/ceph/ceph:v16 CSI_ATTACHER_VERSION=v3.3.0 CSI_SNAPSHOTTER_VERSION=v4.2.0 CSI_PROVISIONER_VERSION=v3.0.0 -CSI_RESIZER_VERSION=v1.2.0 +CSI_RESIZER_VERSION=v1.3.0 CSI_NODE_DRIVER_REGISTRAR_VERSION=v2.3.0 # e2e settings diff --git a/charts/ceph-csi-cephfs/values.yaml b/charts/ceph-csi-cephfs/values.yaml index 962bc6190..effca9b49 100644 --- a/charts/ceph-csi-cephfs/values.yaml +++ b/charts/ceph-csi-cephfs/values.yaml @@ -179,7 +179,7 @@ provisioner: enabled: true image: repository: k8s.gcr.io/sig-storage/csi-resizer - tag: v1.2.0 + tag: v1.3.0 pullPolicy: IfNotPresent resources: {} diff --git a/charts/ceph-csi-rbd/values.yaml b/charts/ceph-csi-rbd/values.yaml index 28fc9fd8a..965baff5a 100644 --- a/charts/ceph-csi-rbd/values.yaml +++ b/charts/ceph-csi-rbd/values.yaml @@ -216,7 +216,7 @@ provisioner: enabled: true image: repository: k8s.gcr.io/sig-storage/csi-resizer - tag: v1.2.0 + tag: v1.3.0 pullPolicy: IfNotPresent resources: {} diff --git a/deploy/cephfs/kubernetes/csi-cephfsplugin-provisioner.yaml b/deploy/cephfs/kubernetes/csi-cephfsplugin-provisioner.yaml index 68cfdf88b..3434f9d7e 100644 --- a/deploy/cephfs/kubernetes/csi-cephfsplugin-provisioner.yaml +++ b/deploy/cephfs/kubernetes/csi-cephfsplugin-provisioner.yaml @@ -60,7 +60,7 @@ spec: - name: socket-dir mountPath: /csi - name: csi-resizer - image: k8s.gcr.io/sig-storage/csi-resizer:v1.2.0 + image: k8s.gcr.io/sig-storage/csi-resizer:v1.3.0 args: - "--csi-address=$(ADDRESS)" - "--v=5" diff --git a/deploy/rbd/kubernetes/csi-rbdplugin-provisioner.yaml b/deploy/rbd/kubernetes/csi-rbdplugin-provisioner.yaml index 25980a214..e887e89b1 100644 --- a/deploy/rbd/kubernetes/csi-rbdplugin-provisioner.yaml +++ b/deploy/rbd/kubernetes/csi-rbdplugin-provisioner.yaml @@ -97,7 +97,7 @@ spec: - name: socket-dir mountPath: /csi - name: csi-resizer - image: k8s.gcr.io/sig-storage/csi-resizer:v1.2.0 + image: k8s.gcr.io/sig-storage/csi-resizer:v1.3.0 args: - "--csi-address=$(ADDRESS)" - "--v=5" From 9ac1391d0f07924714f03772722ac19eaba13022 Mon Sep 17 00:00:00 2001 From: Humble Chirammal Date: Wed, 18 Aug 2021 09:46:11 +0530 Subject: [PATCH 03/21] util: correct interface name and remove redundancy ContollerManager had a typo in it, and if we correct it, linter will fail and suggest not to use controller.ControllerManager as the interface name and package name is redundant, keeping manager as the interface name which is the practice and also address the linter issues. Signed-off-by: Humble Chirammal --- internal/controller/controller.go | 6 +++--- internal/controller/persistentvolume/persistentvolume.go | 4 ++-- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/internal/controller/controller.go b/internal/controller/controller.go index d557fef86..d5c78b1d2 100644 --- a/internal/controller/controller.go +++ b/internal/controller/controller.go @@ -25,10 +25,10 @@ import ( "sigs.k8s.io/controller-runtime/pkg/manager/signals" ) -// ContollerManager is the interface that will wrap Add function. +// Manager is the interface that will wrap Add function. // The New controllers which gets added, as to implement Add function to get // started by the manager. -type ContollerManager interface { +type Manager interface { Add(manager.Manager, Config) error } @@ -39,7 +39,7 @@ type Config struct { } // ControllerList holds the list of managers need to be started. -var ControllerList []ContollerManager +var ControllerList []Manager // addToManager calls the registered managers Add method. func addToManager(mgr manager.Manager, config Config) error { diff --git a/internal/controller/persistentvolume/persistentvolume.go b/internal/controller/persistentvolume/persistentvolume.go index 7e884e3cd..8fd039ffa 100644 --- a/internal/controller/persistentvolume/persistentvolume.go +++ b/internal/controller/persistentvolume/persistentvolume.go @@ -45,8 +45,8 @@ type ReconcilePersistentVolume struct { } var ( - _ reconcile.Reconciler = &ReconcilePersistentVolume{} - _ ctrl.ContollerManager = &ReconcilePersistentVolume{} + _ reconcile.Reconciler = &ReconcilePersistentVolume{} + _ ctrl.Manager = &ReconcilePersistentVolume{} ) // Init will add the ReconcilePersistentVolume to the list. From 8447a1feab344374c967f53de75ce88647616701 Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Thu, 19 Aug 2021 09:56:31 +0200 Subject: [PATCH 04/21] cleanup: address pylint "consider-using-with" in tracevol.py pylint started to report errors like the following: troubleshooting/tools/tracevol.py:97:10: R1732: Consider using 'with' for resource-allocating operations (consider-using-with) There probably has been an update of Pylint in the test-container that is more strict than previous versions. Signed-off-by: Niels de Vos --- troubleshooting/tools/tracevol.py | 64 ++++++++++++++++--------------- 1 file changed, 34 insertions(+), 30 deletions(-) diff --git a/troubleshooting/tools/tracevol.py b/troubleshooting/tools/tracevol.py index 022f34e98..84d8fcaa9 100755 --- a/troubleshooting/tools/tracevol.py +++ b/troubleshooting/tools/tracevol.py @@ -94,9 +94,10 @@ def list_pvc_vol_name_mapping(arg): # list all pvc and get mapping else: cmd += ['get', 'pvc', '-o', 'json'] - out = subprocess.Popen(cmd, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT) - stdout, stderr = out.communicate() + + with subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) as out: + stdout, stderr = out.communicate() + if stderr is not None: if arg.debug: print("failed to list pvc %s", stderr) @@ -194,10 +195,9 @@ def check_pv_name_in_rados(arg, image_id, pvc_name, pool_name, is_rbd): if arg.toolboxdeployed is True: kube = get_cmd_prefix(arg) cmd = kube + cmd - out = subprocess.Popen(cmd, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT) + with subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) as out: + stdout, stderr = out.communicate() - stdout, stderr = out.communicate() if stderr is not None: return False name = b'' @@ -229,10 +229,9 @@ def check_image_in_cluster(arg, image_uuid, pool_name, volname_prefix): kube = get_cmd_prefix(arg) cmd = kube + cmd - out = subprocess.Popen(cmd, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT) + with subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) as out: + stdout, stderr = out.communicate() - stdout, stderr = out.communicate() if stderr is not None: if arg.debug: print(b"failed to toolbox %s", stderr) @@ -256,10 +255,10 @@ def check_image_uuid_in_rados(arg, image_id, pvc_name, pool_name, is_rbd): if arg.toolboxdeployed is True: kube = get_cmd_prefix(arg) cmd = kube + cmd - out = subprocess.Popen(cmd, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT) - stdout, stderr = out.communicate() + with subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) as out: + stdout, stderr = out.communicate() + if stderr is not None: if arg.debug: print("failed to get toolbox %s", stderr) @@ -320,9 +319,10 @@ def get_volume_handler_from_pv(arg, pvname): cmd += ["--kubeconfig", arg.kubeconfig] cmd += ['get', 'pv', pvname, '-o', 'json'] - out = subprocess.Popen(cmd, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT) - stdout, stderr = out.communicate() + + with subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) as out: + stdout, stderr = out.communicate() + if stderr is not None: if arg.debug: print("failed to pv %s", stderr) @@ -347,10 +347,10 @@ def get_tool_box_pod_name(arg): cmd += ["--kubeconfig", arg.kubeconfig] cmd += ['get', 'po', '-l=app=rook-ceph-tools', '-n', arg.rooknamespace, '-o', 'json'] - out = subprocess.Popen(cmd, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT) - stdout, stderr = out.communicate() + with subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) as out: + stdout, stderr = out.communicate() + if stderr is not None: if arg.debug: print("failed to get toolbox pod name %s", stderr) @@ -377,10 +377,10 @@ def get_pool_name(arg, vol_id, is_rbd): if arg.toolboxdeployed is True: kube = get_cmd_prefix(arg) cmd = kube + cmd - out = subprocess.Popen(cmd, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT) - stdout, stderr = out.communicate() + with subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) as out: + stdout, stderr = out.communicate() + if stderr is not None: if arg.debug: print("failed to get the pool name %s", stderr) @@ -426,9 +426,10 @@ def check_subvol_path(arg, subvol_name, subvol_group, fsname): if arg.toolboxdeployed is True: kube = get_cmd_prefix(arg) cmd = kube + cmd - out = subprocess.Popen(cmd, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT) - stdout, stderr = out.communicate() + + with subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) as out: + stdout, stderr = out.communicate() + if stderr is not None: if arg.debug: print("failed to get toolbox %s", stderr) @@ -451,9 +452,10 @@ def get_subvol_group(arg): cmd += ["--kubeconfig", arg.kubeconfig] cmd += ['get', 'cm', arg.configmap, '-o', 'json'] cmd += ['--namespace', arg.configmapnamespace] - out = subprocess.Popen(cmd, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT) - stdout, stderr = out.communicate() + + with subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) as out: + stdout, stderr = out.communicate() + if stderr is not None: if arg.debug: print("failed to get configmap %s", stderr) @@ -463,6 +465,7 @@ def get_subvol_group(arg): except ValueError as err: print(err, stdout) sys.exit() + # default subvolumeGroup subvol_group = "csi" cm_data = config_map['data'].get('config.json') @@ -508,9 +511,10 @@ def get_pv_data(arg, pvname): cmd += ["--kubeconfig", arg.kubeconfig] cmd += ['get', 'pv', pvname, '-o', 'json'] - out = subprocess.Popen(cmd, stdout=subprocess.PIPE, - stderr=subprocess.STDOUT) - stdout, stderr = out.communicate() + + with subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT) as out: + stdout, stderr = out.communicate() + if stderr is not None: if arg.debug: print("failed to get pv %s", stderr) From 7d04cf4fe7e5c2914e774b1de1ace6341dae79cb Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Thu, 19 Aug 2021 10:44:34 +0200 Subject: [PATCH 05/21] doc: add tickgit TODO badge There are TODO and FIXME comments in the Ceph-CSI source code that need addressing at one point. Adding this TODO badge and link to tickgit to the main README makes it obvious that some cleanup is needed. This might invite new contributors to address reported TODOs. Signed-off-by: Niels de Vos --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 21a267989..8470d4202 100644 --- a/README.md +++ b/README.md @@ -2,6 +2,7 @@ [![Go Report Card](https://goreportcard.com/badge/github.com/ceph/ceph-csi)](https://goreportcard.com/report/github.com/ceph/ceph-csi) +[![TODOs](https://badgen.net/https/api.tickgit.com/badgen/github.com/ceph/ceph-csi/devel)](https://www.tickgit.com/browse?repo=github.com/ceph/ceph-csi&branch=devel) - [Ceph CSI](#ceph-csi) - [Overview](#overview) From 7da796dfc1c8ded3a5f76b6a509911b465beb5ad Mon Sep 17 00:00:00 2001 From: Rakshith R Date: Thu, 19 Aug 2021 16:02:48 +0530 Subject: [PATCH 06/21] doc: add Github release badge to README.md Signed-off-by: Rakshith R --- README.md | 1 + 1 file changed, 1 insertion(+) diff --git a/README.md b/README.md index 8470d4202..18f76d13a 100644 --- a/README.md +++ b/README.md @@ -1,5 +1,6 @@ # Ceph CSI +[![GitHub release](https://img.shields.io/github/release/ceph/ceph-csi/all.svg)](https://github.com/ceph/ceph-csi/releases) [![Go Report Card](https://goreportcard.com/badge/github.com/ceph/ceph-csi)](https://goreportcard.com/report/github.com/ceph/ceph-csi) [![TODOs](https://badgen.net/https/api.tickgit.com/badgen/github.com/ceph/ceph-csi/devel)](https://www.tickgit.com/browse?repo=github.com/ceph/ceph-csi&branch=devel) From 18f4a51a1569e6b8c154f6694d643641e8d242e1 Mon Sep 17 00:00:00 2001 From: Prasanna Kumar Kalever Date: Mon, 16 Aug 2021 15:49:25 +0530 Subject: [PATCH 07/21] e2e: improve the debug logs for rbd-nbd MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ceph’s logging levels operate on a scale of 1 to 20, where 1 is terse and 20 is verbose. Format: debug-{subsystem} = {log-level} Setting `rbd` loglevel to 20 at our e2e tests. Signed-off-by: Prasanna Kumar Kalever --- e2e/rbd.go | 27 ++++++++++++++++++++++----- 1 file changed, 22 insertions(+), 5 deletions(-) diff --git a/e2e/rbd.go b/e2e/rbd.go index ada2f2454..362f5d84c 100644 --- a/e2e/rbd.go +++ b/e2e/rbd.go @@ -410,7 +410,10 @@ var _ = Describe("RBD", func() { f, defaultSCName, nil, - map[string]string{"mounter": "rbd-nbd"}, + map[string]string{ + "mounter": "rbd-nbd", + "mapOptions": "debug-rbd=20", + }, deletePolicy) if err != nil { e2elog.Failf("failed to create storageclass with error %v", err) @@ -447,7 +450,10 @@ var _ = Describe("RBD", func() { f, defaultSCName, nil, - map[string]string{"mounter": "rbd-nbd"}, + map[string]string{ + "mounter": "rbd-nbd", + "mapOptions": "debug-rbd=20", + }, deletePolicy) if err != nil { e2elog.Failf("failed to create storageclass with error %v", err) @@ -489,7 +495,10 @@ var _ = Describe("RBD", func() { f, defaultSCName, nil, - map[string]string{"mounter": "rbd-nbd"}, + map[string]string{ + "mounter": "rbd-nbd", + "mapOptions": "debug-rbd=20", + }, deletePolicy) if err != nil { e2elog.Failf("failed to create storageclass with error %v", err) @@ -640,7 +649,11 @@ var _ = Describe("RBD", func() { f, defaultSCName, nil, - map[string]string{"mounter": "rbd-nbd", "encrypted": "true"}, + map[string]string{ + "mounter": "rbd-nbd", + "mapOptions": "debug-rbd=20", + "encrypted": "true", + }, deletePolicy) if err != nil { e2elog.Failf("failed to create storageclass with error %v", err) @@ -993,7 +1006,11 @@ var _ = Describe("RBD", func() { f, defaultSCName, nil, - map[string]string{"imageFeatures": "layering,journaling,exclusive-lock", "mounter": "rbd-nbd"}, + map[string]string{ + "imageFeatures": "layering,journaling,exclusive-lock", + "mounter": "rbd-nbd", + "mapOptions": "debug-rbd=20", + }, deletePolicy) if err != nil { e2elog.Failf("failed to create storageclass with error %v", err) From 0be702472635e35178081e3aeae373b650470716 Mon Sep 17 00:00:00 2001 From: Prasanna Kumar Kalever Date: Thu, 12 Aug 2021 18:37:54 +0530 Subject: [PATCH 08/21] rbd: provide host-path for rbd-nbd logging Problem: -------- 1. rbd-nbd by default logs to /var/log/ceph/ceph-client.admin.log, Unfortunately, container doesn't have /var/log/ceph directory hence rbd-nbd is not logging now. 2. Rbd-nbd logs are not persistent across nodeplugin restarts. Solution: -------- Provide a host path so that log directory is made available, and the logs persist on the hostnode across container restarts. Signed-off-by: Prasanna Kumar Kalever --- charts/ceph-csi-rbd/templates/nodeplugin-daemonset.yaml | 6 ++++++ charts/ceph-csi-rbd/templates/nodeplugin-psp.yaml | 2 ++ deploy/rbd/kubernetes/csi-nodeplugin-psp.yaml | 2 ++ deploy/rbd/kubernetes/csi-rbdplugin.yaml | 6 ++++++ 4 files changed, 16 insertions(+) diff --git a/charts/ceph-csi-rbd/templates/nodeplugin-daemonset.yaml b/charts/ceph-csi-rbd/templates/nodeplugin-daemonset.yaml index 09cde8939..bcc50a3e5 100644 --- a/charts/ceph-csi-rbd/templates/nodeplugin-daemonset.yaml +++ b/charts/ceph-csi-rbd/templates/nodeplugin-daemonset.yaml @@ -124,6 +124,8 @@ spec: mountPropagation: "Bidirectional" - name: keys-tmp-dir mountPath: /tmp/csi/keys + - name: ceph-logdir + mountPath: /var/log/ceph resources: {{ toYaml .Values.nodeplugin.plugin.resources | indent 12 }} {{- if .Values.nodeplugin.httpMetrics.enabled }} @@ -169,6 +171,10 @@ spec: hostPath: path: {{ .Values.kubeletDir }}/pods type: DirectoryOrCreate + - name: ceph-logdir + hostPath: + path: /var/log/ceph + type: DirectoryOrCreate - name: host-dev hostPath: path: /dev diff --git a/charts/ceph-csi-rbd/templates/nodeplugin-psp.yaml b/charts/ceph-csi-rbd/templates/nodeplugin-psp.yaml index fb9313c11..75665484f 100644 --- a/charts/ceph-csi-rbd/templates/nodeplugin-psp.yaml +++ b/charts/ceph-csi-rbd/templates/nodeplugin-psp.yaml @@ -42,6 +42,8 @@ spec: readOnly: true - pathPrefix: '/lib/modules' readOnly: true + - pathPrefix: '/var/log/ceph' + readOnly: false - pathPrefix: '{{ .Values.kubeletDir }}' readOnly: false {{- end }} diff --git a/deploy/rbd/kubernetes/csi-nodeplugin-psp.yaml b/deploy/rbd/kubernetes/csi-nodeplugin-psp.yaml index 2df3b6cc4..7d735fd86 100644 --- a/deploy/rbd/kubernetes/csi-nodeplugin-psp.yaml +++ b/deploy/rbd/kubernetes/csi-nodeplugin-psp.yaml @@ -38,6 +38,8 @@ spec: readOnly: true - pathPrefix: '/var/lib/kubelet/pods' readOnly: false + - pathPrefix: '/var/log/ceph' + readOnly: false - pathPrefix: '/var/lib/kubelet/plugins/rbd.csi.ceph.com' readOnly: false - pathPrefix: '/var/lib/kubelet/plugins_registry' diff --git a/deploy/rbd/kubernetes/csi-rbdplugin.yaml b/deploy/rbd/kubernetes/csi-rbdplugin.yaml index 9a424a7d6..7b7e7aa3f 100644 --- a/deploy/rbd/kubernetes/csi-rbdplugin.yaml +++ b/deploy/rbd/kubernetes/csi-rbdplugin.yaml @@ -111,6 +111,8 @@ spec: mountPropagation: "Bidirectional" - name: keys-tmp-dir mountPath: /tmp/csi/keys + - name: ceph-logdir + mountPath: /var/log/ceph - name: liveness-prometheus securityContext: privileged: true @@ -146,6 +148,10 @@ spec: hostPath: path: /var/lib/kubelet/pods type: DirectoryOrCreate + - name: ceph-logdir + hostPath: + path: /var/log/ceph + type: DirectoryOrCreate - name: registration-dir hostPath: path: /var/lib/kubelet/plugins_registry/ From 682b3a980bf2926cf8601d5146461378c925fdde Mon Sep 17 00:00:00 2001 From: Prasanna Kumar Kalever Date: Wed, 18 Aug 2021 12:51:23 +0530 Subject: [PATCH 09/21] rbd: rbd-nbd logging the ceph-CSI way - One logfile per device/volume - Add ability to customize the logdir, default: /var/log/ceph Note: if user customizes the hostpath to something else other than default /var/log/ceph, then it is his responsibility to update the `cephLogDir` in storageclass to reflect the same with daemon: ``` cephLogDir: "/var/log/mynewpath" ``` Signed-off-by: Prasanna Kumar Kalever --- examples/rbd/storageclass.yaml | 6 ++++++ internal/rbd/nodeserver.go | 1 + internal/rbd/rbd_attach.go | 8 ++++++++ internal/rbd/rbd_util.go | 15 +++++++++++++++ 4 files changed, 30 insertions(+) diff --git a/examples/rbd/storageclass.yaml b/examples/rbd/storageclass.yaml index ced2eaafe..80b421412 100644 --- a/examples/rbd/storageclass.yaml +++ b/examples/rbd/storageclass.yaml @@ -69,6 +69,12 @@ parameters: # on supported nodes # mounter: rbd-nbd + # (optional) ceph client log location, eg: rbd-nbd + # By default host-path /var/log/ceph of node is bind-mounted into + # csi-rbdplugin pod at /var/log/ceph mount path. See docs/rbd-nbd.md + # for available configuration options. + # cephLogDir: /var/log/ceph + # (optional) Prefix to use for naming RBD images. # If omitted, defaults to "csi-vol-". # volumeNamePrefix: "foo-bar-" diff --git a/internal/rbd/nodeserver.go b/internal/rbd/nodeserver.go index 6f6642028..6ba99034b 100644 --- a/internal/rbd/nodeserver.go +++ b/internal/rbd/nodeserver.go @@ -274,6 +274,7 @@ func (ns *NodeServer) NodeStageVolume( volOptions.MapOptions = req.GetVolumeContext()["mapOptions"] volOptions.UnmapOptions = req.GetVolumeContext()["unmapOptions"] volOptions.Mounter = req.GetVolumeContext()["mounter"] + volOptions.LogDir = req.GetVolumeContext()["cephLogDir"] err = volOptions.Connect(cr) if err != nil { diff --git a/internal/rbd/rbd_attach.go b/internal/rbd/rbd_attach.go index 81c7487a1..822782e2f 100644 --- a/internal/rbd/rbd_attach.go +++ b/internal/rbd/rbd_attach.go @@ -296,6 +296,14 @@ func createPath(ctx context.Context, volOpt *rbdVolume, device string, cr *util. util.WarningLog(ctx, "failed to detect if image %q is thick-provisioned: %v", volOpt, err) } + if isNbd { + if volOpt.LogDir == "" { + volOpt.LogDir = defaultLogDir + } + mapArgs = append(mapArgs, "--log-file", + getCephClientLogFileName(volOpt.VolID, volOpt.LogDir, "rbd-nbd")) + } + cli := rbd if device != "" { // TODO: use rbd cli for attach/detach in the future diff --git a/internal/rbd/rbd_util.go b/internal/rbd/rbd_util.go index 8a6a24352..f6bbea5f6 100644 --- a/internal/rbd/rbd_util.go +++ b/internal/rbd/rbd_util.go @@ -50,6 +50,7 @@ const ( rbdImageWatcherSteps = 10 rbdDefaultMounter = "rbd" rbdNbdMounter = "rbd-nbd" + defaultLogDir = "/var/log/ceph" // Output strings returned during invocation of "ceph rbd task add remove " when // command is not supported by ceph manager. Used to check errors and recover when the command @@ -136,6 +137,7 @@ type rbdVolume struct { ReservedID string MapOptions string UnmapOptions string + LogDir string VolName string `json:"volName"` MonValueFromSecret string `json:"monValueFromSecret"` VolSize int64 `json:"volSize"` @@ -2002,3 +2004,16 @@ func (ri *rbdImage) addSnapshotScheduling( return nil } + +// getCephClientLogFileName compiles the complete log file path based on inputs. +func getCephClientLogFileName(id, logDir, prefix string) string { + if prefix == "" { + prefix = "ceph" + } + + if logDir == "" { + logDir = defaultLogDir + } + + return fmt.Sprintf("%s/%s-%s.log", logDir, prefix, id) +} From 474100c1f1caf7583142fe8c2a6887d3b3014ee4 Mon Sep 17 00:00:00 2001 From: Prasanna Kumar Kalever Date: Mon, 23 Aug 2021 19:53:37 +0530 Subject: [PATCH 10/21] rbd: add a unit test for getCephClientLogFileName() Signed-off-by: Prasanna Kumar Kalever --- internal/rbd/rbd_util_test.go | 71 +++++++++++++++++++++++++++++++++++ 1 file changed, 71 insertions(+) diff --git a/internal/rbd/rbd_util_test.go b/internal/rbd/rbd_util_test.go index 1dfa35d7d..2a44682db 100644 --- a/internal/rbd/rbd_util_test.go +++ b/internal/rbd/rbd_util_test.go @@ -189,3 +189,74 @@ func TestGetMappedID(t *testing.T) { }) } } + +func TestGetCephClientLogFileName(t *testing.T) { + t.Parallel() + type args struct { + id string + logDir string + prefix string + } + volID := "0001-0024-fed5480a-f00f-417a-a51d-31d8a8144c03-0000000000000003-eba90b33-0156-11ec-a30b-4678a93686c2" + tests := []struct { + name string + args args + expected string + }{ + { + name: "test for empty id", + args: args{ + id: "", + logDir: "/var/log/ceph-csi", + prefix: "rbd-nbd", + }, + expected: "/var/log/ceph-csi/rbd-nbd-.log", + }, + { + name: "test for empty logDir", + args: args{ + id: volID, + logDir: "", + prefix: "rbd-nbd", + }, + expected: "/var/log/ceph/rbd-nbd-" + volID + ".log", + }, + { + name: "test for empty prefix", + args: args{ + id: volID, + logDir: "/var/log/ceph-csi", + prefix: "", + }, + expected: "/var/log/ceph-csi/ceph-" + volID + ".log", + }, + { + name: "test for all unavailable args", + args: args{ + id: "", + logDir: "", + prefix: "", + }, + expected: "/var/log/ceph/ceph-.log", + }, + { + name: "test for all available args", + args: args{ + id: volID, + logDir: "/var/log/ceph-csi", + prefix: "rbd-nbd", + }, + expected: "/var/log/ceph-csi/rbd-nbd-" + volID + ".log", + }, + } + for _, tt := range tests { + tt := tt + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + val := getCephClientLogFileName(tt.args.id, tt.args.logDir, tt.args.prefix) + if val != tt.expected { + t.Errorf("getCephClientLogFileName() got = %v, expected %v", val, tt.expected) + } + }) + } +} From d67e88ccd0dc042edaa64e4ec33fe6f8145b617b Mon Sep 17 00:00:00 2001 From: Prasanna Kumar Kalever Date: Mon, 23 Aug 2021 16:53:15 +0530 Subject: [PATCH 11/21] cleanup: embed args into struct and pass it to detachRBDImageOrDeviceSpec Signed-off-by: Prasanna Kumar Kalever --- internal/rbd/nodeserver.go | 17 ++++++---- internal/rbd/rbd_attach.go | 65 ++++++++++++++++++++++++-------------- 2 files changed, 51 insertions(+), 31 deletions(-) diff --git a/internal/rbd/nodeserver.go b/internal/rbd/nodeserver.go index 6ba99034b..3e61f6a6b 100644 --- a/internal/rbd/nodeserver.go +++ b/internal/rbd/nodeserver.go @@ -819,13 +819,16 @@ func (ns *NodeServer) NodeUnstageVolume( // Unmapping rbd device imageSpec := imgInfo.String() - if err = detachRBDImageOrDeviceSpec( - ctx, imageSpec, - true, - imgInfo.NbdAccess, - imgInfo.Encrypted, - req.GetVolumeId(), - imgInfo.UnmapOptions); err != nil { + + dArgs := detachRBDImageArgs{ + imageOrDeviceSpec: imageSpec, + isImageSpec: true, + isNbd: imgInfo.NbdAccess, + encrypted: imgInfo.Encrypted, + volumeID: req.GetVolumeId(), + unmapOptions: imgInfo.UnmapOptions, + } + if err = detachRBDImageOrDeviceSpec(ctx, dArgs); err != nil { util.ErrorLog( ctx, "error unmapping volume (%s) from staging path (%s): (%v)", diff --git a/internal/rbd/rbd_attach.go b/internal/rbd/rbd_attach.go index 822782e2f..28bb480a5 100644 --- a/internal/rbd/rbd_attach.go +++ b/internal/rbd/rbd_attach.go @@ -81,6 +81,15 @@ type nbdDeviceInfo struct { Device string `json:"device"` } +type detachRBDImageArgs struct { + imageOrDeviceSpec string + isImageSpec bool + isNbd bool + encrypted bool + volumeID string + unmapOptions string +} + // rbdGetDeviceList queries rbd about mapped devices and returns a list of rbdDeviceInfo // It will selectively list devices mapped using krbd or nbd as specified by accessType. func rbdGetDeviceList(ctx context.Context, accessType string) ([]rbdDeviceInfo, error) { @@ -325,14 +334,15 @@ func createPath(ctx context.Context, volOpt *rbdVolume, device string, cr *util. util.WarningLog(ctx, "rbd: map error %v, rbd output: %s", err, stderr) // unmap rbd image if connection timeout if strings.Contains(err.Error(), rbdMapConnectionTimeout) { - detErr := detachRBDImageOrDeviceSpec( - ctx, - imagePath, - true, - isNbd, - volOpt.isEncrypted(), - volOpt.VolID, - volOpt.UnmapOptions) + dArgs := detachRBDImageArgs{ + imageOrDeviceSpec: imagePath, + isImageSpec: true, + isNbd: isNbd, + encrypted: volOpt.isEncrypted(), + volumeID: volOpt.VolID, + unmapOptions: volOpt.UnmapOptions, + } + detErr := detachRBDImageOrDeviceSpec(ctx, dArgs) if detErr != nil { util.WarningLog(ctx, "rbd: %s unmap error %v", imagePath, detErr) } @@ -375,22 +385,29 @@ func detachRBDDevice(ctx context.Context, devicePath, volumeID, unmapOptions str nbdType = true } - return detachRBDImageOrDeviceSpec(ctx, devicePath, false, nbdType, encrypted, volumeID, unmapOptions) + dArgs := detachRBDImageArgs{ + imageOrDeviceSpec: devicePath, + isImageSpec: false, + isNbd: nbdType, + encrypted: encrypted, + volumeID: volumeID, + unmapOptions: unmapOptions, + } + + return detachRBDImageOrDeviceSpec(ctx, dArgs) } // detachRBDImageOrDeviceSpec detaches an rbd imageSpec or devicePath, with additional checking // when imageSpec is used to decide if image is already unmapped. func detachRBDImageOrDeviceSpec( ctx context.Context, - imageOrDeviceSpec string, - isImageSpec, isNbd, encrypted bool, - volumeID, unmapOptions string) error { - if encrypted { - mapperFile, mapperPath := util.VolumeMapper(volumeID) + dArgs detachRBDImageArgs) error { + if dArgs.encrypted { + mapperFile, mapperPath := util.VolumeMapper(dArgs.volumeID) mappedDevice, mapper, err := util.DeviceEncryptionStatus(ctx, mapperPath) if err != nil { util.ErrorLog(ctx, "error determining LUKS device on %s, %s: %s", - mapperPath, imageOrDeviceSpec, err) + mapperPath, dArgs.imageOrDeviceSpec, err) return err } @@ -399,31 +416,31 @@ func detachRBDImageOrDeviceSpec( err = util.CloseEncryptedVolume(ctx, mapperFile) if err != nil { util.ErrorLog(ctx, "error closing LUKS device on %s, %s: %s", - mapperPath, imageOrDeviceSpec, err) + mapperPath, dArgs.imageOrDeviceSpec, err) return err } - imageOrDeviceSpec = mappedDevice + dArgs.imageOrDeviceSpec = mappedDevice } } - unmapArgs := []string{"unmap", imageOrDeviceSpec} - unmapArgs = appendDeviceTypeAndOptions(unmapArgs, isNbd, false, unmapOptions) + unmapArgs := []string{"unmap", dArgs.imageOrDeviceSpec} + unmapArgs = appendDeviceTypeAndOptions(unmapArgs, dArgs.isNbd, false, dArgs.unmapOptions) _, stderr, err := util.ExecCommand(ctx, rbd, unmapArgs...) if err != nil { // Messages for krbd and nbd differ, hence checking either of them for missing mapping // This is not applicable when a device path is passed in - if isImageSpec && - (strings.Contains(stderr, fmt.Sprintf(rbdUnmapCmdkRbdMissingMap, imageOrDeviceSpec)) || - strings.Contains(stderr, fmt.Sprintf(rbdUnmapCmdNbdMissingMap, imageOrDeviceSpec))) { + if dArgs.isImageSpec && + (strings.Contains(stderr, fmt.Sprintf(rbdUnmapCmdkRbdMissingMap, dArgs.imageOrDeviceSpec)) || + strings.Contains(stderr, fmt.Sprintf(rbdUnmapCmdNbdMissingMap, dArgs.imageOrDeviceSpec))) { // Devices found not to be mapped are treated as a successful detach - util.TraceLog(ctx, "image or device spec (%s) not mapped", imageOrDeviceSpec) + util.TraceLog(ctx, "image or device spec (%s) not mapped", dArgs.imageOrDeviceSpec) return nil } - return fmt.Errorf("rbd: unmap for spec (%s) failed (%w): (%s)", imageOrDeviceSpec, err, stderr) + return fmt.Errorf("rbd: unmap for spec (%s) failed (%w): (%s)", dArgs.imageOrDeviceSpec, err, stderr) } return nil From ea3def0db2534a74a99b0dd67a2c35a06c5b8020 Mon Sep 17 00:00:00 2001 From: Prasanna Kumar Kalever Date: Fri, 20 Aug 2021 06:36:35 +0530 Subject: [PATCH 12/21] rbd: remove per volume rbd-nbd logfiles on detach - Update the meta stash with logDir details - Use the same to remove logfile on unstage/unmap to be space efficient Signed-off-by: Prasanna Kumar Kalever --- internal/rbd/nodeserver.go | 4 ++++ internal/rbd/rbd_attach.go | 12 +++++++++--- internal/rbd/rbd_util.go | 2 ++ 3 files changed, 15 insertions(+), 3 deletions(-) diff --git a/internal/rbd/nodeserver.go b/internal/rbd/nodeserver.go index 3e61f6a6b..03ddfee6f 100644 --- a/internal/rbd/nodeserver.go +++ b/internal/rbd/nodeserver.go @@ -275,6 +275,9 @@ func (ns *NodeServer) NodeStageVolume( volOptions.UnmapOptions = req.GetVolumeContext()["unmapOptions"] volOptions.Mounter = req.GetVolumeContext()["mounter"] volOptions.LogDir = req.GetVolumeContext()["cephLogDir"] + if volOptions.LogDir == "" { + volOptions.LogDir = defaultLogDir + } err = volOptions.Connect(cr) if err != nil { @@ -827,6 +830,7 @@ func (ns *NodeServer) NodeUnstageVolume( encrypted: imgInfo.Encrypted, volumeID: req.GetVolumeId(), unmapOptions: imgInfo.UnmapOptions, + logDir: imgInfo.LogDir, } if err = detachRBDImageOrDeviceSpec(ctx, dArgs); err != nil { util.ErrorLog( diff --git a/internal/rbd/rbd_attach.go b/internal/rbd/rbd_attach.go index 28bb480a5..98d16979a 100644 --- a/internal/rbd/rbd_attach.go +++ b/internal/rbd/rbd_attach.go @@ -88,6 +88,7 @@ type detachRBDImageArgs struct { encrypted bool volumeID string unmapOptions string + logDir string } // rbdGetDeviceList queries rbd about mapped devices and returns a list of rbdDeviceInfo @@ -306,9 +307,6 @@ func createPath(ctx context.Context, volOpt *rbdVolume, device string, cr *util. } if isNbd { - if volOpt.LogDir == "" { - volOpt.LogDir = defaultLogDir - } mapArgs = append(mapArgs, "--log-file", getCephClientLogFileName(volOpt.VolID, volOpt.LogDir, "rbd-nbd")) } @@ -341,6 +339,7 @@ func createPath(ctx context.Context, volOpt *rbdVolume, device string, cr *util. encrypted: volOpt.isEncrypted(), volumeID: volOpt.VolID, unmapOptions: volOpt.UnmapOptions, + logDir: volOpt.LogDir, } detErr := detachRBDImageOrDeviceSpec(ctx, dArgs) if detErr != nil { @@ -442,6 +441,13 @@ func detachRBDImageOrDeviceSpec( return fmt.Errorf("rbd: unmap for spec (%s) failed (%w): (%s)", dArgs.imageOrDeviceSpec, err, stderr) } + if dArgs.isNbd && dArgs.logDir != "" { + logFile := getCephClientLogFileName(dArgs.volumeID, dArgs.logDir, "rbd-nbd") + if err = os.Remove(logFile); err != nil { + util.WarningLog(ctx, "failed to remove logfile: %s, error: %v", + logFile, err) + } + } return nil } diff --git a/internal/rbd/rbd_util.go b/internal/rbd/rbd_util.go index f6bbea5f6..ad5727629 100644 --- a/internal/rbd/rbd_util.go +++ b/internal/rbd/rbd_util.go @@ -1525,6 +1525,7 @@ type rbdImageMetadataStash struct { NbdAccess bool `json:"accessType"` Encrypted bool `json:"encrypted"` DevicePath string `json:"device"` // holds NBD device path for now + LogDir string `json:"logDir"` // holds the client log path } // file name in which image metadata is stashed. @@ -1555,6 +1556,7 @@ func stashRBDImageMetadata(volOptions *rbdVolume, metaDataPath string) error { imgMeta.NbdAccess = false if volOptions.Mounter == rbdTonbd && hasNBD { imgMeta.NbdAccess = true + imgMeta.LogDir = volOptions.LogDir } encodedBytes, err := json.Marshal(imgMeta) From 473adf99fcb56c226e446699eea32dd8f2b9c813 Mon Sep 17 00:00:00 2001 From: Prasanna Kumar Kalever Date: Thu, 19 Aug 2021 16:33:39 +0530 Subject: [PATCH 13/21] deploy: provide variable to alter hostpath location for ceph clients Also update the documentation about the same. Signed-off-by: Prasanna Kumar Kalever --- charts/ceph-csi-rbd/README.md | 1 + charts/ceph-csi-rbd/templates/nodeplugin-daemonset.yaml | 2 +- charts/ceph-csi-rbd/templates/nodeplugin-psp.yaml | 2 +- charts/ceph-csi-rbd/values.yaml | 2 ++ 4 files changed, 5 insertions(+), 2 deletions(-) diff --git a/charts/ceph-csi-rbd/README.md b/charts/ceph-csi-rbd/README.md index 9f26025ed..ef2cc6d51 100644 --- a/charts/ceph-csi-rbd/README.md +++ b/charts/ceph-csi-rbd/README.md @@ -138,6 +138,7 @@ charts and their default values. | `provisionerSocketFile` | The filename of the provisioner socket | `csi-provisioner.sock` | | `pluginSocketFile` | The filename of the plugin socket | `csi.sock` | | `kubeletDir` | kubelet working directory | `/var/lib/kubelet` | +| `cephLogDir` | Host path location for ceph client processes logging, ex: rbd-nbd | `/var/log/ceph` | | `driverName` | Name of the csi-driver | `rbd.csi.ceph.com` | | `configMapName` | Name of the configmap which contains cluster configuration | `ceph-csi-config` | | `externallyManagedConfigmap` | Specifies the use of an externally provided configmap | `false` | diff --git a/charts/ceph-csi-rbd/templates/nodeplugin-daemonset.yaml b/charts/ceph-csi-rbd/templates/nodeplugin-daemonset.yaml index bcc50a3e5..003fbf5ec 100644 --- a/charts/ceph-csi-rbd/templates/nodeplugin-daemonset.yaml +++ b/charts/ceph-csi-rbd/templates/nodeplugin-daemonset.yaml @@ -173,7 +173,7 @@ spec: type: DirectoryOrCreate - name: ceph-logdir hostPath: - path: /var/log/ceph + path: {{ .Values.cephLogDir }} type: DirectoryOrCreate - name: host-dev hostPath: diff --git a/charts/ceph-csi-rbd/templates/nodeplugin-psp.yaml b/charts/ceph-csi-rbd/templates/nodeplugin-psp.yaml index 75665484f..fc10f134d 100644 --- a/charts/ceph-csi-rbd/templates/nodeplugin-psp.yaml +++ b/charts/ceph-csi-rbd/templates/nodeplugin-psp.yaml @@ -42,7 +42,7 @@ spec: readOnly: true - pathPrefix: '/lib/modules' readOnly: true - - pathPrefix: '/var/log/ceph' + - pathPrefix: '{{ .Values.cephLogDir }}' readOnly: false - pathPrefix: '{{ .Values.kubeletDir }}' readOnly: false diff --git a/charts/ceph-csi-rbd/values.yaml b/charts/ceph-csi-rbd/values.yaml index 965baff5a..8a00d96d9 100644 --- a/charts/ceph-csi-rbd/values.yaml +++ b/charts/ceph-csi-rbd/values.yaml @@ -382,6 +382,8 @@ provisionerSocketFile: csi-provisioner.sock pluginSocketFile: csi.sock # kubelet working directory,can be set using `--root-dir` when starting kubelet. kubeletDir: /var/lib/kubelet +# Host path location for ceph client processes logging, ex: rbd-nbd +cephLogDir: /var/log/ceph # Name of the csi-driver driverName: rbd.csi.ceph.com # Name of the configmap used for state From 7576bf400c9e66db20633c3c2791c0186ea281b9 Mon Sep 17 00:00:00 2001 From: Prasanna Kumar Kalever Date: Wed, 18 Aug 2021 14:21:19 +0530 Subject: [PATCH 14/21] doc: update rbd-nbd doc about log path details Document the changes needed for configuring custom logging path Signed-off-by: Prasanna Kumar Kalever --- docs/rbd-nbd.md | 36 ++++++++++++++++++++++++++++++++++++ 1 file changed, 36 insertions(+) diff --git a/docs/rbd-nbd.md b/docs/rbd-nbd.md index a24885536..cadf0c400 100644 --- a/docs/rbd-nbd.md +++ b/docs/rbd-nbd.md @@ -3,6 +3,7 @@ - [RBD NBD Mounter](#rbd-nbd-mounter) - [Overview](#overview) - [Configuration](#configuration) + - [Configuring logging path](#configuring-logging-path) - [Status](#status) - [Support Matrix](#support-matrix) - [CSI spec and Kubernetes version compatibility](#csi-spec-and-kubernetes-version-compatibility) @@ -28,6 +29,41 @@ client-side, which is inside the `csi-rbdplugin` node plugin. To use the rbd-nbd mounter for RBD-backed PVs, set `mounter` to `rbd-nbd` in the StorageClass. +### Configuring logging path + +If you are using the default rbd nodeplugin daemonset and StorageClass +templates then `cephLogDir` will be `/var/log/ceph`, this directory will be +a host-path and the default log file path will be +`/var/log/ceph/rbd-nbd-.log`. rbd-nbd creates a log file per volume +under the `cephLogDir` path on NodeStage(map) and removed the same on +the respective NodeUnstage(unmap). + +In case if you need a customized log path, you should do below: + +- Edit the daemonset templates to change the `cephLogDir` + - If you are using helm charts, then you can use key `cephLogDir` + + ``` + helm install --set cephLogDir=/var/log/ceph-csi/my-dir + ``` + + - For standard templates edit [csi-rbdplugin.yaml](../deploy/rbd/kubernetes/csi-rbdplugin.yaml) + to update `hostPath` for `ceph-logdir`, also edit psp [csi-nodeplugin-psp.yaml](../deploy/rbd/kubernetes/csi-nodeplugin-psp.yaml) + to update `pathPrefix` spec entries. +- Update the StorageClass with the customized log directory path + - Now update rbd StorageClass for `cephLogDir`, for example + + ``` + cephLogDir: "/var/log/prod-A-logs" + ``` + +`NOTE`: + +- On uninstall make sure to delete `cephLogDir` on host manually to freeup + some space just in case if there are any uncleaned log files. +- In case if you do not need the rbd-nbd logging to persistent, then just + update the StorageClass for `cephLogDir` to use a non-persistent path. + ## Status Rbd-nbd support status: **Alpha** From 3bf17ade7a21b41e80cc750c2d3b04078df73c07 Mon Sep 17 00:00:00 2001 From: Prasanna Kumar Kalever Date: Thu, 19 Aug 2021 11:53:15 +0530 Subject: [PATCH 15/21] doc: update code comments about available timeout options Adding some code comments to make them readable and easy to understand. Signed-off-by: Prasanna Kumar Kalever --- internal/rbd/rbd_attach.go | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/internal/rbd/rbd_attach.go b/internal/rbd/rbd_attach.go index 98d16979a..b855fa806 100644 --- a/internal/rbd/rbd_attach.go +++ b/internal/rbd/rbd_attach.go @@ -48,9 +48,16 @@ const ( rbdUnmapCmdNbdMissingMap = "rbd-nbd: %s is not mapped" rbdMapConnectionTimeout = "Connection timed out" - defaultNbdReAttachTimeout = 300 + defaultNbdReAttachTimeout = 300 /* in seconds */ - useNbdNetlink = "try-netlink" + // The default way of creating nbd devices via rbd-nbd is through the + // legacy ioctl interface, to take advantage of netlink features we + // should specify `try-netlink` flag explicitly. + useNbdNetlink = "try-netlink" + + // `reattach-timeout` of rbd-nbd is to tweak NBD_ATTR_DEAD_CONN_TIMEOUT. + // It specifies how long the device should be held waiting for the + // userspace process to come back to life. setNbdReattach = "reattach-timeout" ) From 4f40213d8e8f654e84626ff27fefd73395a8b99b Mon Sep 17 00:00:00 2001 From: Prasanna Kumar Kalever Date: Thu, 19 Aug 2021 11:55:21 +0530 Subject: [PATCH 16/21] rbd: fix rbd-nbd io-timeout to never abort With the tests at CI, it kind of looks like that the IO is timing out after 30 seconds (default with rbd-nbd). Since we have tweaked reattach-timeout to 300 seconds at ceph-csi, we need to explicitly set io-timeout on the device too, as it doesn't make any sense to keep io-timeout < reattach-timeout Hence we set io-timeout for rbd nbd to 0. Specifying io-timeout 0 tells the nbd driver to not abort the request and instead see if it can be restarted on another socket. Signed-off-by: Prasanna Kumar Kalever Suggested-by: Ilya Dryomov --- internal/rbd/rbd_attach.go | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/internal/rbd/rbd_attach.go b/internal/rbd/rbd_attach.go index b855fa806..42dcc0201 100644 --- a/internal/rbd/rbd_attach.go +++ b/internal/rbd/rbd_attach.go @@ -49,6 +49,7 @@ const ( rbdMapConnectionTimeout = "Connection timed out" defaultNbdReAttachTimeout = 300 /* in seconds */ + defaultNbdIOTimeout = 0 /* do not abort the requests */ // The default way of creating nbd devices via rbd-nbd is through the // legacy ioctl interface, to take advantage of netlink features we @@ -59,6 +60,10 @@ const ( // It specifies how long the device should be held waiting for the // userspace process to come back to life. setNbdReattach = "reattach-timeout" + + // `io-timeout` of rbd-nbd is to tweak NBD_ATTR_TIMEOUT. It specifies + // how long the IO should wait to get handled before bailing out. + setNbdIOTimeout = "io-timeout" ) var hasNBD = false @@ -256,6 +261,9 @@ func appendDeviceTypeAndOptions(cmdArgs []string, isNbd, isThick bool, userOptio if !strings.Contains(userOptions, setNbdReattach) { cmdArgs = append(cmdArgs, "--options", fmt.Sprintf("%s=%d", setNbdReattach, defaultNbdReAttachTimeout)) } + if !strings.Contains(userOptions, setNbdIOTimeout) { + cmdArgs = append(cmdArgs, "--options", fmt.Sprintf("%s=%d", setNbdIOTimeout, defaultNbdIOTimeout)) + } } if isThick { // When an image is thick-provisioned, any discard/unmap/trim @@ -280,6 +288,9 @@ func appendRbdNbdCliOptions(cmdArgs []string, userOptions string) []string { if !strings.Contains(userOptions, setNbdReattach) { cmdArgs = append(cmdArgs, fmt.Sprintf("--%s=%d", setNbdReattach, defaultNbdReAttachTimeout)) } + if !strings.Contains(userOptions, setNbdIOTimeout) { + cmdArgs = append(cmdArgs, fmt.Sprintf("--%s=%d", setNbdIOTimeout, defaultNbdIOTimeout)) + } if userOptions != "" { options := strings.Split(userOptions, ",") for _, opt := range options { From 1bd2d46cdb82a8ca27ed1bc56cd6aa4778983d2d Mon Sep 17 00:00:00 2001 From: Prasanna Kumar Kalever Date: Tue, 24 Aug 2021 14:00:24 +0530 Subject: [PATCH 17/21] e2e: add util to get kernel version from specified container Currently, we get the kernel version where the e2e (client) executable runs, not the kernel version that is used by the csi-rbdplugin pod. Add a function that run `uname -r` command from the specified container and returns the kernel version. Signed-off-by: Prasanna Kumar Kalever Suggested-by: Niels de Vos --- e2e/pod.go | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/e2e/pod.go b/e2e/pod.go index 940bdc261..5307781ed 100644 --- a/e2e/pod.go +++ b/e2e/pod.go @@ -406,3 +406,22 @@ func calculateSHA512sum(f *framework.Framework, app *v1.Pod, filePath string, op return checkSum, nil } + +// getKernelVersionFromDaemonset gets the kernel version from the specified container. +func getKernelVersionFromDaemonset(f *framework.Framework, ns, dsn, cn string) (string, error) { + selector, err := getDaemonSetLabelSelector(f, ns, dsn) + if err != nil { + return "", err + } + + opt := metav1.ListOptions{ + LabelSelector: selector, + } + + kernelRelease, stdErr, err := execCommandInContainer(f, "uname -r", ns, cn, &opt) + if err != nil || stdErr != "" { + return "", err + } + + return kernelRelease, nil +} From 55d3226d6bb5d07912c9af9d45a933bbf039a997 Mon Sep 17 00:00:00 2001 From: Prasanna Kumar Kalever Date: Mon, 23 Aug 2021 18:40:22 +0530 Subject: [PATCH 18/21] e2e: use io-timeout conditionally based on kernel version We need https://www.mail-archive.com/linux-block@vger.kernel.org/msg38060.html inorder to use `--io-timeout=0`. This patch is part of kernel 5.4 Since minikube doesn't have a v5.4 kernel yet, lets use io-timeout value conditionally based on kernel version at our e2e. Signed-off-by: Prasanna Kumar Kalever --- e2e/rbd.go | 52 ++++++++++++++++++++++++++--------------------- e2e/rbd_helper.go | 22 ++++++++++++++++++++ 2 files changed, 51 insertions(+), 23 deletions(-) diff --git a/e2e/rbd.go b/e2e/rbd.go index 362f5d84c..649d7ed0b 100644 --- a/e2e/rbd.go +++ b/e2e/rbd.go @@ -57,6 +57,8 @@ var ( appBlockSmartClonePath = rbdExamplePath + "block-pod-clone.yaml" snapshotPath = rbdExamplePath + "snapshot.yaml" defaultCloneCount = 10 + + nbdMapOptions = "debug-rbd=20" ) func deployRBDPlugin() { @@ -178,6 +180,7 @@ func validateRBDImageCount(f *framework.Framework, count int, pool string) { var _ = Describe("RBD", func() { f := framework.NewDefaultFramework("rbd") var c clientset.Interface + var kernelRelease string // deploy RBD CSI BeforeEach(func() { if !testRBD || upgradeTesting { @@ -232,6 +235,27 @@ var _ = Describe("RBD", func() { e2elog.Failf("failed to create node secret with error %v", err) } deployVault(f.ClientSet, deployTimeout) + + // wait for provisioner deployment + err = waitForDeploymentComplete(rbdDeploymentName, cephCSINamespace, f.ClientSet, deployTimeout) + if err != nil { + e2elog.Failf("timeout waiting for deployment %s with error %v", rbdDeploymentName, err) + } + + // wait for nodeplugin deamonset pods + err = waitForDaemonSets(rbdDaemonsetName, cephCSINamespace, f.ClientSet, deployTimeout) + if err != nil { + e2elog.Failf("timeout waiting for daemonset %s with error %v", rbdDaemonsetName, err) + } + + kernelRelease, err = getKernelVersionFromDaemonset(f, cephCSINamespace, rbdDaemonsetName, "csi-rbdplugin") + if err != nil { + e2elog.Failf("failed to get the kernel version with error %v", err) + } + // default io-timeout=0, needs kernel >= 5.4 + if !util.CheckKernelSupport(kernelRelease, nbdZeroIOtimeoutSupport) { + nbdMapOptions = "debug-rbd=20,io-timeout=330" + } }) AfterEach(func() { @@ -302,20 +326,6 @@ var _ = Describe("RBD", func() { Context("Test RBD CSI", func() { It("Test RBD CSI", func() { - By("checking provisioner deployment is running", func() { - err := waitForDeploymentComplete(rbdDeploymentName, cephCSINamespace, f.ClientSet, deployTimeout) - if err != nil { - e2elog.Failf("timeout waiting for deployment %s with error %v", rbdDeploymentName, err) - } - }) - - By("checking nodeplugin deamonset pods are running", func() { - err := waitForDaemonSets(rbdDaemonsetName, cephCSINamespace, f.ClientSet, deployTimeout) - if err != nil { - e2elog.Failf("timeout waiting for daemonset %s with error %v", rbdDaemonsetName, err) - } - }) - // test only if ceph-csi is deployed via helm if helmTest { By("verify PVC and app binding on helm installation", func() { @@ -412,7 +422,7 @@ var _ = Describe("RBD", func() { nil, map[string]string{ "mounter": "rbd-nbd", - "mapOptions": "debug-rbd=20", + "mapOptions": nbdMapOptions, }, deletePolicy) if err != nil { @@ -435,10 +445,6 @@ var _ = Describe("RBD", func() { }) By("Resize rbd-nbd PVC and check application directory size", func() { - kernelRelease, err := util.GetKernelVersion() - if err != nil { - e2elog.Failf("failed to get kernel version with error %v", err) - } if util.CheckKernelSupport(kernelRelease, nbdResizeSupport) { err := deleteResource(rbdExamplePath + "storageclass.yaml") if err != nil { @@ -452,7 +458,7 @@ var _ = Describe("RBD", func() { nil, map[string]string{ "mounter": "rbd-nbd", - "mapOptions": "debug-rbd=20", + "mapOptions": nbdMapOptions, }, deletePolicy) if err != nil { @@ -497,7 +503,7 @@ var _ = Describe("RBD", func() { nil, map[string]string{ "mounter": "rbd-nbd", - "mapOptions": "debug-rbd=20", + "mapOptions": nbdMapOptions, }, deletePolicy) if err != nil { @@ -651,7 +657,7 @@ var _ = Describe("RBD", func() { nil, map[string]string{ "mounter": "rbd-nbd", - "mapOptions": "debug-rbd=20", + "mapOptions": nbdMapOptions, "encrypted": "true", }, deletePolicy) @@ -1009,7 +1015,7 @@ var _ = Describe("RBD", func() { map[string]string{ "imageFeatures": "layering,journaling,exclusive-lock", "mounter": "rbd-nbd", - "mapOptions": "debug-rbd=20", + "mapOptions": nbdMapOptions, }, deletePolicy) if err != nil { diff --git a/e2e/rbd_helper.go b/e2e/rbd_helper.go index 3100fc996..ce695bb2c 100644 --- a/e2e/rbd_helper.go +++ b/e2e/rbd_helper.go @@ -40,6 +40,28 @@ var nbdResizeSupport = []util.KernelVersion{ }, // standard 5.3+ versions } +// To use `io-timeout=0` we need +// www.mail-archive.com/linux-block@vger.kernel.org/msg38060.html +// nolint:gomnd // numbers specify Kernel versions. +var nbdZeroIOtimeoutSupport = []util.KernelVersion{ + { + Version: 5, + PatchLevel: 4, + SubLevel: 0, + ExtraVersion: 0, + Distribution: "", + Backport: false, + }, // standard 5.4+ versions + { + Version: 4, + PatchLevel: 18, + SubLevel: 0, + ExtraVersion: 305, + Distribution: ".el8", + Backport: true, + }, // CentOS 8.4 +} + func imageSpec(pool, image string) string { if radosNamespace != "" { return pool + "/" + radosNamespace + "/" + image From 0a7a490496e0c0cb340e393b536b2c0d564e3b53 Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Wed, 25 Aug 2021 15:10:02 +0530 Subject: [PATCH 19/21] ci: update mergify rules to include teams updated mergify rules to consider the teams approval to merge a PR. more details at #2367 fixes #2367 Signed-off-by: Madhu Rajanna --- .mergify.yml | 36 +++++++++++++++++++++++++++--------- 1 file changed, 27 insertions(+), 9 deletions(-) diff --git a/.mergify.yml b/.mergify.yml index b0c223964..83fbb14d7 100644 --- a/.mergify.yml +++ b/.mergify.yml @@ -38,6 +38,8 @@ pull_request_rules: - base~=^(devel)|(release-.+)$ - "#approved-reviews-by>=2" - "#changes-requested-reviews-by=0" + - "approved-reviews-by=@ceph/ceph-csi-contributors" + - "approved-reviews-by=@ceph/ceph-csi-maintainers" - "status-success=codespell" - "status-success=multi-arch-build" - "status-success=go-test" @@ -64,7 +66,7 @@ pull_request_rules: - label!=DNM - label=ready-to-merge - base~=^(devel)|(release-.+)$ - - "#approved-reviews-by>=1" + - "approved-reviews-by=@ceph/ceph-csi-maintainers" - "status-success=codespell" - "status-success=multi-arch-build" - "status-success=go-test" @@ -122,7 +124,9 @@ pull_request_rules: - base=release-v2.0 - label!=DNM - "#changes-requested-reviews-by=0" - - "#approved-reviews-by>=1" + - "#approved-reviews-by>=2" + - "approved-reviews-by=@ceph/ceph-csi-contributors" + - "approved-reviews-by=@ceph/ceph-csi-maintainers" actions: merge: {} dismiss_reviews: {} @@ -142,7 +146,9 @@ pull_request_rules: - base=release-v2.1 - label!=DNM - "#changes-requested-reviews-by=0" - - "#approved-reviews-by>=1" + - "#approved-reviews-by>=2" + - "approved-reviews-by=@ceph/ceph-csi-contributors" + - "approved-reviews-by=@ceph/ceph-csi-maintainers" actions: merge: {} dismiss_reviews: {} @@ -162,7 +168,9 @@ pull_request_rules: - base=release-v3.0 - label!=DNM - "#changes-requested-reviews-by=0" - - "#approved-reviews-by>=1" + - "#approved-reviews-by>=2" + - "approved-reviews-by=@ceph/ceph-csi-contributors" + - "approved-reviews-by=@ceph/ceph-csi-maintainers" actions: merge: {} dismiss_reviews: {} @@ -182,7 +190,9 @@ pull_request_rules: - base=release-v3.1 - label!=DNM - "#changes-requested-reviews-by=0" - - "#approved-reviews-by>=1" + - "#approved-reviews-by>=2" + - "approved-reviews-by=@ceph/ceph-csi-contributors" + - "approved-reviews-by=@ceph/ceph-csi-maintainers" - "status-success=multi-arch-build" - "status-success=commitlint" - "status-success=ci/centos/mini-e2e-helm/k8s-1.20" @@ -212,7 +222,9 @@ pull_request_rules: - author=mergify[bot] - base=release-v3.2 - label!=DNM - - "#approved-reviews-by>=1" + - "#approved-reviews-by>=2" + - "approved-reviews-by=@ceph/ceph-csi-contributors" + - "approved-reviews-by=@ceph/ceph-csi-maintainers" - "status-success=codespell" - "status-success=multi-arch-build" - "status-success=go-test" @@ -249,7 +261,9 @@ pull_request_rules: - author=mergify[bot] - base=release-v3.3 - label!=DNM - - "#approved-reviews-by>=1" + - "#approved-reviews-by>=2" + - "approved-reviews-by=@ceph/ceph-csi-contributors" + - "approved-reviews-by=@ceph/ceph-csi-maintainers" - "status-success=codespell" - "status-success=multi-arch-build" - "status-success=go-test" @@ -286,7 +300,9 @@ pull_request_rules: - author=mergify[bot] - base=release-v3.4 - label!=DNM - - "#approved-reviews-by>=1" + - "#approved-reviews-by>=2" + - "approved-reviews-by=@ceph/ceph-csi-contributors" + - "approved-reviews-by=@ceph/ceph-csi-maintainers" - "status-success=codespell" - "status-success=multi-arch-build" - "status-success=go-test" @@ -321,6 +337,8 @@ pull_request_rules: - label!=DNM - base=ci/centos - "#approved-reviews-by>=2" + - "approved-reviews-by=@ceph/ceph-csi-contributors" + - "approved-reviews-by=@ceph/ceph-csi-maintainers" - "#changes-requested-reviews-by=0" - "status-success=ci/centos/job-validation" - "status-success=ci/centos/jjb-validate" @@ -334,7 +352,7 @@ pull_request_rules: - label!=DNM - label=ready-to-merge - base=ci/centos - - "#approved-reviews-by>=1" + - "approved-reviews-by=@ceph/ceph-csi-maintainers" - "#changes-requested-reviews-by=0" - "status-success=ci/centos/job-validation" - "status-success=ci/centos/jjb-validate" From b0b46680e3c61a94a491b8ca90203c113fe91726 Mon Sep 17 00:00:00 2001 From: Madhu Rajanna Date: Wed, 25 Aug 2021 15:56:27 +0530 Subject: [PATCH 20/21] doc: update development guide for new rules updated development guide requirement to have review from contributors and reviewers. Signed-off-by: Madhu Rajanna --- docs/development-guide.md | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/docs/development-guide.md b/docs/development-guide.md index 08c822616..c027e7978 100644 --- a/docs/development-guide.md +++ b/docs/development-guide.md @@ -257,6 +257,10 @@ Once your PR has been submitted for review the following criteria will need to be met before it will be merged: * Each PR needs reviews accepting the change from at least two developers for merging. +* Each PR needs approval from + [ceph-csi-contributors](https://github.com/orgs/ceph/teams/ceph-csi-contributors) + and + [ceph-csi-maintainers](https://github.com/orgs/ceph/teams/ceph-csi-maintainers). * It is common to request reviews from those reviewers automatically suggested by GitHub. * Each PR needs to have been open for at least 24 working hours to allow for From 68588dc7df8e0255116ec208c6a82dd95a1e3c8e Mon Sep 17 00:00:00 2001 From: Niels de Vos Date: Tue, 24 Aug 2021 17:23:51 +0200 Subject: [PATCH 21/21] util: fix unit-test for GetClusterMappingInfo() Unit-testing often fails due to a race condition while writing the clusterMappingConfigFile from multiple go-routines at the same time. Failures from `make containerized-test` look like this: === CONT TestGetClusterMappingInfo/site2-storage_cluster-id_mapping cluster_mapping_test.go:153: GetClusterMappingInfo() = , expected data &[{map[site1-storage:site2-storage] [map[1:3]] [map[11:5]]} {map[site3-storage:site2-storage] [map[8:3]] [map[10:5]]}] === CONT TestGetClusterMappingInfo/site3-storage_cluster-id_mapping cluster_mapping_test.go:153: GetClusterMappingInfo() = , expected data &[{map[site3-storage:site2-storage] [map[8:3]] [map[10:5]]}] --- FAIL: TestGetClusterMappingInfo (0.01s) --- PASS: TestGetClusterMappingInfo/mapping_file_not_found (0.00s) --- PASS: TestGetClusterMappingInfo/mapping_file_found_with_empty_data (0.00s) --- PASS: TestGetClusterMappingInfo/cluster-id_mapping_not_found (0.00s) --- FAIL: TestGetClusterMappingInfo/site2-storage_cluster-id_mapping (0.00s) --- FAIL: TestGetClusterMappingInfo/site3-storage_cluster-id_mapping (0.00s) --- PASS: TestGetClusterMappingInfo/site1-storage_cluster-id_mapping (0.00s) By splitting the public GetClusterMappingInfo() function into an internal getClusterMappingInfo() that takes a filename, unit-testing can use different files for each go-routine, and testing becomes more predictable. Signed-off-by: Niels de Vos --- internal/util/cluster_mapping.go | 18 ++++++++++++------ internal/util/cluster_mapping_test.go | 12 ++++++------ 2 files changed, 18 insertions(+), 12 deletions(-) diff --git a/internal/util/cluster_mapping.go b/internal/util/cluster_mapping.go index 51af34f9a..173f5126e 100644 --- a/internal/util/cluster_mapping.go +++ b/internal/util/cluster_mapping.go @@ -65,9 +65,9 @@ type ClusterMappingInfo struct { // ... // }] -func readClusterMappingInfo() (*[]ClusterMappingInfo, error) { +func readClusterMappingInfo(filename string) (*[]ClusterMappingInfo, error) { var info []ClusterMappingInfo - content, err := ioutil.ReadFile(clusterMappingConfigFile) + content, err := ioutil.ReadFile(filename) // #nosec:G304, file inclusion via variable. if err != nil { err = fmt.Errorf("error fetching clusterID mapping %w", err) @@ -83,11 +83,11 @@ func readClusterMappingInfo() (*[]ClusterMappingInfo, error) { return &info, nil } -// GetClusterMappingInfo returns corresponding cluster details like clusterID's -// poolID,fscID lists read from configfile. -func GetClusterMappingInfo(clusterID string) (*[]ClusterMappingInfo, error) { +// getClusterMappingInfo returns corresponding cluster details like clusterID's +// poolID,fscID lists read from 'filename'. +func getClusterMappingInfo(clusterID, filename string) (*[]ClusterMappingInfo, error) { var mappingInfo []ClusterMappingInfo - info, err := readClusterMappingInfo() + info, err := readClusterMappingInfo(filename) if err != nil { // discard not found error as this file is expected to be created by // the admin in case of failover. @@ -114,3 +114,9 @@ func GetClusterMappingInfo(clusterID string) (*[]ClusterMappingInfo, error) { return &mappingInfo, nil } + +// GetClusterMappingInfo returns corresponding cluster details like clusterID's +// poolID,fscID lists read from configfile. +func GetClusterMappingInfo(clusterID string) (*[]ClusterMappingInfo, error) { + return getClusterMappingInfo(clusterID, clusterMappingConfigFile) +} diff --git a/internal/util/cluster_mapping_test.go b/internal/util/cluster_mapping_test.go index 92aa2da5c..2a85ed9ef 100644 --- a/internal/util/cluster_mapping_test.go +++ b/internal/util/cluster_mapping_test.go @@ -138,19 +138,19 @@ func TestGetClusterMappingInfo(t *testing.T) { currentTT := tt t.Run(currentTT.name, func(t *testing.T) { t.Parallel() - clusterMappingConfigFile = fmt.Sprintf("%s/mapping-%d.json", mappingBasePath, currentI) + mappingConfigFile := fmt.Sprintf("%s/mapping-%d.json", mappingBasePath, currentI) if len(currentTT.mappingFilecontent) != 0 { - err = ioutil.WriteFile(clusterMappingConfigFile, currentTT.mappingFilecontent, 0o600) + err = ioutil.WriteFile(mappingConfigFile, currentTT.mappingFilecontent, 0o600) if err != nil { - t.Errorf("GetClusterMappingInfo() error = %v", err) + t.Errorf("failed to write to %q, error = %v", mappingConfigFile, err) } } - data, mErr := GetClusterMappingInfo(currentTT.clusterID) + data, mErr := getClusterMappingInfo(currentTT.clusterID, mappingConfigFile) if (mErr != nil) != currentTT.expectErr { - t.Errorf("GetClusterMappingInfo() error = %v, expected Error %v", mErr, currentTT.expectErr) + t.Errorf("getClusterMappingInfo() error = %v, expected Error %v", mErr, currentTT.expectErr) } if !reflect.DeepEqual(data, currentTT.expectedData) { - t.Errorf("GetClusterMappingInfo() = %v, expected data %v", data, currentTT.expectedData) + t.Errorf("getClusterMappingInfo() = %v, expected data %v", data, currentTT.expectedData) } }) }