From f2af1af82fceacddf6958b31b1946d204c313574 Mon Sep 17 00:00:00 2001 From: Yecheng Fu Date: Tue, 24 Oct 2017 12:16:12 +0800 Subject: [PATCH] RBD Plugin: No need to acquire advisory lock any more! With central attachdetach controller, we don't need to lock the image any more. But for backward compatibility, we should: 1) Check if the image is still used by nodes running old kubelet in attaching. 2) Clean old rbd.json file and remove lock if found in detaching. --- pkg/volume/rbd/BUILD | 2 +- pkg/volume/rbd/attacher.go | 2 +- pkg/volume/rbd/rbd_test.go | 84 --------------------------- pkg/volume/rbd/rbd_util.go | 114 +++++++++++++++++++------------------ 4 files changed, 61 insertions(+), 141 deletions(-) diff --git a/pkg/volume/rbd/BUILD b/pkg/volume/rbd/BUILD index 5185d960231..aef8b366b4e 100644 --- a/pkg/volume/rbd/BUILD +++ b/pkg/volume/rbd/BUILD @@ -17,6 +17,7 @@ go_library( ], importpath = "k8s.io/kubernetes/pkg/volume/rbd", deps = [ + "//pkg/util/file:go_default_library", "//pkg/util/mount:go_default_library", "//pkg/util/node:go_default_library", "//pkg/util/strings:go_default_library", @@ -43,7 +44,6 @@ go_test( "//pkg/util/mount:go_default_library", "//pkg/volume:go_default_library", "//pkg/volume/testing:go_default_library", - "//vendor/github.com/stretchr/testify/assert:go_default_library", "//vendor/k8s.io/api/core/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library", "//vendor/k8s.io/apimachinery/pkg/types:go_default_library", diff --git a/pkg/volume/rbd/attacher.go b/pkg/volume/rbd/attacher.go index f08f499ec7f..7d2fab2bbb9 100644 --- a/pkg/volume/rbd/attacher.go +++ b/pkg/volume/rbd/attacher.go @@ -206,7 +206,7 @@ func (detacher *rbdDetacher) UnmountDevice(deviceMountPath string) error { return err } glog.V(3).Infof("rbd: successfully detach device %s", devicePath) - err = os.RemoveAll(deviceMountPath) + err = os.Remove(deviceMountPath) if err != nil { return err } diff --git a/pkg/volume/rbd/rbd_test.go b/pkg/volume/rbd/rbd_test.go index 99192a2cb37..4f67311918b 100644 --- a/pkg/volume/rbd/rbd_test.go +++ b/pkg/volume/rbd/rbd_test.go @@ -18,16 +18,13 @@ package rbd import ( "fmt" - "io/ioutil" "os" - "path/filepath" "reflect" "strings" "sync" "testing" "time" - "github.com/stretchr/testify/assert" "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" @@ -397,87 +394,6 @@ func TestPersistentClaimReadOnlyFlag(t *testing.T) { } } -func TestPersistAndLoadRBD(t *testing.T) { - tmpDir, err := utiltesting.MkTmpdir("rbd_test") - if err != nil { - t.Fatalf("error creating temp dir: %v", err) - } - defer os.RemoveAll(tmpDir) - - testcases := []struct { - rbdMounter rbdMounter - expectedJSONStr string - expectedLoadedRBDMounter rbdMounter - }{ - { - rbdMounter{}, - `{"Mon":null,"Id":"","Keyring":"","Secret":""}`, - rbdMounter{}, - }, - { - rbdMounter{ - rbd: &rbd{ - podUID: "poduid", - Pool: "kube", - Image: "some-test-image", - ReadOnly: false, - MetricsProvider: volume.NewMetricsStatFS("/tmp"), - }, - Mon: []string{"127.0.0.1"}, - Id: "kube", - Keyring: "", - Secret: "QVFEcTdKdFp4SmhtTFJBQUNwNDI3UnhGRzBvQ1Y0SUJwLy9pRUE9PQ==", - }, - ` -{ - "Pool": "kube", - "Image": "some-test-image", - "ReadOnly": false, - "Mon": ["127.0.0.1"], - "Id": "kube", - "Keyring": "", - "Secret": "QVFEcTdKdFp4SmhtTFJBQUNwNDI3UnhGRzBvQ1Y0SUJwLy9pRUE9PQ==" -} - `, - rbdMounter{ - rbd: &rbd{ - Pool: "kube", - Image: "some-test-image", - ReadOnly: false, - }, - Mon: []string{"127.0.0.1"}, - Id: "kube", - Keyring: "", - Secret: "QVFEcTdKdFp4SmhtTFJBQUNwNDI3UnhGRzBvQ1Y0SUJwLy9pRUE9PQ==", - }, - }, - } - - util := &RBDUtil{} - for _, c := range testcases { - err = util.persistRBD(c.rbdMounter, tmpDir) - if err != nil { - t.Errorf("failed to persist rbd: %v, err: %v", c.rbdMounter, err) - } - jsonFile := filepath.Join(tmpDir, "rbd.json") - jsonData, err := ioutil.ReadFile(jsonFile) - if err != nil { - t.Errorf("failed to read json file %s: %v", jsonFile, err) - } - if !assert.JSONEq(t, c.expectedJSONStr, string(jsonData)) { - t.Errorf("json file does not match expected one: %s, should be %s", string(jsonData), c.expectedJSONStr) - } - tmpRBDMounter := rbdMounter{} - err = util.loadRBD(&tmpRBDMounter, tmpDir) - if err != nil { - t.Errorf("faild to load rbd: %v", err) - } - if !reflect.DeepEqual(tmpRBDMounter, c.expectedLoadedRBDMounter) { - t.Errorf("loaded rbd does not equal to expected one: %v, should be %v", tmpRBDMounter, c.rbdMounter) - } - } -} - func TestGetSecretNameAndNamespace(t *testing.T) { secretName := "test-secret-name" secretNamespace := "test-secret-namespace" diff --git a/pkg/volume/rbd/rbd_util.go b/pkg/volume/rbd/rbd_util.go index 2e598a99f73..edf0efbb290 100644 --- a/pkg/volume/rbd/rbd_util.go +++ b/pkg/volume/rbd/rbd_util.go @@ -35,6 +35,7 @@ import ( "github.com/golang/glog" "k8s.io/api/core/v1" + fileutil "k8s.io/kubernetes/pkg/util/file" "k8s.io/kubernetes/pkg/util/node" "k8s.io/kubernetes/pkg/volume" volutil "k8s.io/kubernetes/pkg/volume/util" @@ -225,46 +226,6 @@ func (util *RBDUtil) rbdLock(b rbdMounter, lock bool) error { return err } -func (util *RBDUtil) persistRBD(rbd rbdMounter, mnt string) error { - file := path.Join(mnt, "rbd.json") - fp, err := os.Create(file) - if err != nil { - return fmt.Errorf("rbd: create err %s/%s", file, err) - } - defer fp.Close() - - encoder := json.NewEncoder(fp) - if err = encoder.Encode(rbd); err != nil { - return fmt.Errorf("rbd: encode err: %v.", err) - } - - return nil -} - -func (util *RBDUtil) loadRBD(mounter *rbdMounter, mnt string) error { - file := path.Join(mnt, "rbd.json") - fp, err := os.Open(file) - if err != nil { - return fmt.Errorf("rbd: open err %s/%s", file, err) - } - defer fp.Close() - - decoder := json.NewDecoder(fp) - if err = decoder.Decode(mounter); err != nil { - return fmt.Errorf("rbd: decode err: %v.", err) - } - - return nil -} - -func (util *RBDUtil) fencing(b rbdMounter) error { - // no need to fence readOnly - if (&b).GetAttributes().ReadOnly { - return nil - } - return util.rbdLock(b, true) -} - // AttachDisk attaches the disk on the node. // If Volume is not read-only, acquire a lock on image first. func (util *RBDUtil) AttachDisk(b rbdMounter) (string, error) { @@ -287,16 +248,16 @@ func (util *RBDUtil) AttachDisk(b rbdMounter) (string, error) { glog.Warningf("rbd: failed to load rbd kernel module:%v", err) } - // fence off other mappers - if err = util.fencing(b); err != nil { - return "", rbdErrors(err, fmt.Errorf("rbd: failed to lock image %s (maybe locked by other nodes), error %v", b.Image, err)) + // Currently, we don't acquire advisory lock on image, but for backward + // compatibility, we need to check if the image is being used by nodes running old kubelet. + found, err := util.rbdStatus(&b) + if err != nil { + return "", err + } + if found { + glog.Info("rbd is still being used ", b.Image) + return "", fmt.Errorf("rbd %s is still being used", b.Image) } - // rbd lock remove needs ceph and image config - // but kubelet doesn't get them from apiserver during teardown - // so persit rbd config so upon disk detach, rbd lock can be removed - // since rbd json is persisted in the same local directory that is used as rbd mountpoint later, - // the json file remains invisible during rbd mount and thus won't be removed accidentally. - util.persistRBD(b, globalPDPath) // rbd map l := len(b.Mon) @@ -342,20 +303,55 @@ func (util *RBDUtil) DetachDisk(plugin *rbdPlugin, deviceMountPath string, devic } glog.V(3).Infof("rbd: successfully unmap device %s", device) } - // load ceph and image/pool info to remove fencing + // Currently, we don't persist rbd info on the disk, but for backward + // compatbility, we need to clean it if found. + rbdFile := path.Join(deviceMountPath, "rbd.json") + exists, err := fileutil.FileExists(rbdFile) + if err != nil { + return err + } + if exists { + glog.V(3).Infof("rbd: old rbd.json is found under %s, cleaning it", deviceMountPath) + err = util.cleanOldRBDFile(plugin, rbdFile) + if err != nil { + glog.Errorf("rbd: failed to clean %s", rbdFile) + return err + } + glog.V(3).Infof("rbd: successfully remove %s", rbdFile) + } + return nil +} + +// cleanOldRBDFile read rbd info from rbd.json file and removes lock if found. +// At last, it removes rbd.json file. +func (util *RBDUtil) cleanOldRBDFile(plugin *rbdPlugin, rbdFile string) error { mounter := &rbdMounter{ // util.rbdLock needs it to run command. rbd: newRBD("", "", "", "", false, plugin, util), } - err = util.loadRBD(mounter, deviceMountPath) + fp, err := os.Open(rbdFile) if err != nil { - glog.Errorf("failed to load rbd info from %s: %v", deviceMountPath, err) + return fmt.Errorf("rbd: open err %s/%s", rbdFile, err) + } + defer fp.Close() + + decoder := json.NewDecoder(fp) + if err = decoder.Decode(mounter); err != nil { + return fmt.Errorf("rbd: decode err: %v.", err) + } + + if err != nil { + glog.Errorf("failed to load rbd info from %s: %v", rbdFile, err) return err } // remove rbd lock if found // the disk is not attached to this node anymore, so the lock on image // for this node can be removed safely - return util.rbdLock(*mounter, false) + err = util.rbdLock(*mounter, false) + if err == nil { + os.Remove(rbdFile) + } + return err } func (util *RBDUtil) CreateImage(p *rbdVolumeProvisioner) (r *v1.RBDPersistentVolumeSource, size int, err error) { @@ -438,6 +434,14 @@ func (util *RBDUtil) rbdStatus(b *rbdMounter) (bool, error) { var output string var cmd []byte + // If we don't have admin id/secret (e.g. attaching), fallback to user id/secret. + id := b.adminId + secret := b.adminSecret + if id == "" { + id = b.Id + secret = b.Secret + } + l := len(b.Mon) start := rand.Int() % l // iterate all hosts until rbd command succeeds. @@ -457,9 +461,9 @@ func (util *RBDUtil) rbdStatus(b *rbdMounter) (bool, error) { // # image does not exist (exit=2) // rbd: error opening image kubernetes-dynamic-pvc-: (2) No such file or directory // - glog.V(4).Infof("rbd: status %s using mon %s, pool %s id %s key %s", b.Image, mon, b.Pool, b.adminId, b.adminSecret) + glog.V(4).Infof("rbd: status %s using mon %s, pool %s id %s key %s", b.Image, mon, b.Pool, id, secret) cmd, err = b.exec.Run("rbd", - "status", b.Image, "--pool", b.Pool, "-m", mon, "--id", b.adminId, "--key="+b.adminSecret) + "status", b.Image, "--pool", b.Pool, "-m", mon, "--id", id, "--key="+secret) output = string(cmd) // break if command succeeds