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

@@ -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",

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
 	}

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"

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-<UUID>: (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