Skip to content
This repository has been archived by the owner on Nov 9, 2020. It is now read-only.

Avoiding redundant mounts by using /proc/mount info #1224

Merged
merged 5 commits into from
May 9, 2017
Merged

Avoiding redundant mounts by using /proc/mount info #1224

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
be205c1
Avoiding redundant mounts by using /proc/mount info
May 5, 2017
e88907c
Addressing comments from Mark and Prashant
May 5, 2017
62782d9
Use entry from refmap and avoid get trip to esx
May 9, 2017
51468d6
Wrapper struct for GetVolumeInfo util, cosmetics and refcnt decr on g…
May 9, 2017
f7e3360
minor comments
May 9, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 14 additions & 11 deletions misc/scripts/refcnt_test.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -67,10 +67,11 @@ function check_files {
}

function check_recovery_record {
line=`tail -10 /var/log/docker-volume-vsphere.log | $GREP 'Volume name=' | $GREP 'mounted=true'`
expected="name=$vname count=$count mounted=true"
# log contains refcounting attempts and after success logs summary.
line=`tail -50 /var/log/docker-volume-vsphere.log | $GREP 'Volume name=' | $GREP 'mounted=true'`
expected="count=$count mounted=true"

echo $line | $GREP -q "$expected" ; if [ $? -ne 0 ] ; then
echo $line | $GREP "$vname" | $GREP -q "$expected" ; if [ $? -ne 0 ] ; then
echo Found: \"$line\"
echo Expected pattern: \"$expected\"
return 1
Expand All @@ -80,20 +81,21 @@ function check_recovery_record {

function test_crash_recovery {
timeout=$1
echo "Checking recovery for VMDK plugin kill -9"
kill -9 `pidof docker-volume-vsphere`
until pids=$(pidof docker-volume-vsphere)
echo "Checking recovery through docker kill"
# kill docker daemon forcefully
pkill -9 dockerd
until pids=$(pidof dockerd)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this impact the refcount module. Unless the plugin is restarted, the refcount module hasn't lost it's refcounts to do a recovery.

do
echo "Waiting for docker-volume-vsphere to restart"
echo "Waiting for docker to restart"
sleep 1
done

echo "Waiting for plugin init"
sleep 3
sleep 5
sync # give log the time to flush
wait_for check_recovery_record $timeout
if [ "$?" -ne 0 ] ; then
echo PLUGIN RESTART TEST FAILED. Did not find proper recovery record
echo DOCKER RESTART TEST FAILED. Did not find proper recovery record
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure this change is needed. The test is to see if the plugin restarts then the refcounts are restored. If the plugin restart isn't possible then this test should be made manual to execute with a standalone binary and deprecated from automated test run.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test will in turn test plugin restart (docker restart). Plugin restart through docker restart tests the docker api consumption, timeout and handling of parallel mounts/unmounts till refcounting is successful. Not sure why not have this as automated test?

exit 1
fi
}
Expand All @@ -115,8 +117,9 @@ fi
echo "$(docker volume ls)"
for i in `seq 1 $count`
do
$DOCKER run -d -v $vname:/v busybox sh -c "touch /v/file$i; sync ; \
sleep $timeout"
# run containers with restart flag so they restart after docker restart
$DOCKER run -d --restart=always -v $vname:/v busybox sh -c "touch /v/file$i; sync ; \
while true; do sleep $timeout; done"
done

echo "Checking the last refcount and mount record"
Expand Down
2 changes: 1 addition & 1 deletion vmdk_plugin/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,7 @@ VMDKOPS_MODULE_SRC = $(VMDKOPS_MODULE)/*.go $(VMCI_SRC)

# All sources. We rebuild if anything changes here
SRC = main.go log_formatter.go utils/refcount/refcnt.go \
utils/fs/fs.go utils/config/config.go utils/TestInputParamsUtil.go \
utils/fs/fs.go utils/config/config.go utils/TestInputParamsUtil.go utils/plugin_utils/plugin_utils.go\
drivers/photon/photon_driver.go drivers/vmdk/vmdk_driver.go

# Canned recipe
Expand Down
1 change: 1 addition & 0 deletions vmdk_plugin/drivers/driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,4 +20,5 @@ type VolumeDriver interface {
MountVolume(string, string, string, bool, bool) (string, error)
UnmountVolume(string) error
GetVolume(string) (map[string]interface{}, error)
VolumesInRefMap() []string
}
65 changes: 50 additions & 15 deletions vmdk_plugin/drivers/photon/photon_driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import (
log "github.com/Sirupsen/logrus"
"github.com/docker/go-plugins-helpers/volume"
"github.com/vmware/docker-volume-vsphere/vmdk_plugin/utils/fs"
"github.com/vmware/docker-volume-vsphere/vmdk_plugin/utils/plugin_utils"
"github.com/vmware/docker-volume-vsphere/vmdk_plugin/utils/refcount"
"github.com/vmware/photon-controller-go-sdk/photon"
)
Expand All @@ -56,12 +57,13 @@ const (

// VolumeDriver - Photon volume driver struct
type VolumeDriver struct {
client *photon.Client
hostID string
mountRoot string
project string
refCounts *refcount.RefCountsMap
target string
client *photon.Client
hostID string
mountRoot string
project string
refCounts *refcount.RefCountsMap
target string
mountIDtoName map[string]string // map of mountID -> full volume name
}

func (d *VolumeDriver) verifyTarget() error {
Expand Down Expand Up @@ -95,6 +97,7 @@ func NewVolumeDriver(targetURL string, projectID string, hostID string, mountDir
d.mountRoot = mountDir
d.refCounts = refcount.NewRefCountsMap()
d.refCounts.Init(d, mountDir, driverName)
d.mountIDtoName = make(map[string]string)

log.WithFields(log.Fields{
"version": version,
Expand Down Expand Up @@ -360,8 +363,22 @@ func (d *VolumeDriver) MountVolume(name string, fstype string, id string, isRead
return mountpoint, fs.MountWithID(mountpoint, fstype, id, isReadOnly)
}

// VolumesInRefMap - get list of volumes names from refmap
// names are in format volume@datastore
func (d *VolumeDriver) VolumesInRefMap() []string {
return d.refCounts.GetVolumeNames()
}

// private function that does the job of mounting volume in conjunction with refcounting
func (d *VolumeDriver) processMount(r volume.MountRequest) volume.Response {
volumeInfo, err := plugin_utils.GetVolumeInfo(r.Name, "", d)
if err != nil {
log.Errorf("Unable to get volume info for volume %s. err:%v", r.Name, err)
return volume.Response{Err: err.Error()}
}
r.Name = volumeInfo.VolumeName
d.mountIDtoName[r.ID] = r.Name

// If the volume is already mounted , just increase the refcount.
// Note: for new keys, GO maps return zero value, so no need for if_exists.
refcnt := d.incrRefCount(r.Name) // save map traversal
Expand All @@ -373,22 +390,28 @@ func (d *VolumeDriver) processMount(r volume.MountRequest) volume.Response {
return volume.Response{Mountpoint: d.getMountPoint(r.Name)}
}

// There can be redundant mounts till refcounts are properly initialized
// TODO: #1220
status, err := d.GetVolume(r.Name)
if err != nil {
d.decrRefCount(r.Name)
return volume.Response{Err: err.Error()}
if plugin_utils.AlreadyMounted(r.Name, d.mountRoot) {
log.WithFields(log.Fields{"name": r.Name}).Info("Already mounted, skipping mount. ")
return volume.Response{Mountpoint: d.getMountPoint(r.Name)}
}

// get volume metadata if required
volumeMeta := volumeInfo.VolumeMeta
if volumeMeta == nil {
if volumeMeta, err = d.GetVolume(r.Name); err != nil {
d.decrRefCount(r.Name)
return volume.Response{Err: err.Error()}
}
}

fstype, exists := status[fsTypeTag]
fstype, exists := volumeMeta[fsTypeTag]
if !exists {
fstype = fs.FstypeDefault
}

skipAttach := false
// If the volume is already attached to the VM, skip the attach.
if state, stateExists := status["State"]; stateExists {
if state, stateExists := volumeMeta["State"]; stateExists {
if strings.Compare(state.(string), "DETACHED") != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is not your change but please file an issue and let's revisit.
Generally we don't make decision based on data in KV file and definitely not inside guest. Disk may be attached to some other VM. If neither refCnt nor mount map suggest disk is attached to this VM then we must issue attach to ESX. If it is already attached, it is a no-op from ESX perspective anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#1230

skipAttach = true
}
Expand All @@ -398,7 +421,7 @@ func (d *VolumeDriver) processMount(r volume.MountRequest) volume.Response {
}

// Mount the volume and for now its always read-write.
mountpoint, err := d.MountVolume(r.Name, fstype.(string), status["ID"].(string), false, skipAttach)
mountpoint, err := d.MountVolume(r.Name, fstype.(string), volumeMeta["ID"].(string), false, skipAttach)
if err != nil {
log.WithFields(
log.Fields{"name": r.Name, "error": err.Error()},
Expand Down Expand Up @@ -618,6 +641,18 @@ func (d *VolumeDriver) Unmount(r volume.UnmountRequest) volume.Response {
return volume.Response{Err: ""}
}

if fullVolName, exist := d.mountIDtoName[r.ID]; exist {
r.Name = fullVolName
delete(d.mountIDtoName, r.ID) //cleanup the map
} else {
volumeInfo, err := plugin_utils.GetVolumeInfo(r.Name, "", d)
if err != nil {
log.Errorf("Unable to get volume info for volume %s. err:%v", r.Name, err)
return volume.Response{Err: err.Error()}
}
r.Name = volumeInfo.VolumeName
}

// if refcount has been succcessful, Normal flow.
// if the volume is still used by other containers, just return OK
refcnt, err := d.decrRefCount(r.Name)
Expand Down
55 changes: 47 additions & 8 deletions vmdk_plugin/drivers/vmdk/vmdk_driver.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ import (
"github.com/docker/go-plugins-helpers/volume"
"github.com/vmware/docker-volume-vsphere/vmdk_plugin/drivers/vmdk/vmdkops"
"github.com/vmware/docker-volume-vsphere/vmdk_plugin/utils/fs"
"github.com/vmware/docker-volume-vsphere/vmdk_plugin/utils/plugin_utils"
"github.com/vmware/docker-volume-vsphere/vmdk_plugin/utils/refcount"
)

Expand All @@ -46,9 +47,10 @@ const (

// VolumeDriver - VMDK driver struct
type VolumeDriver struct {
useMockEsx bool
ops vmdkops.VmdkOps
refCounts *refcount.RefCountsMap
useMockEsx bool
ops vmdkops.VmdkOps
refCounts *refcount.RefCountsMap
mountIDtoName map[string]string // map of mountID -> full volume name
}

var mountRoot string
Expand Down Expand Up @@ -78,6 +80,7 @@ func NewVolumeDriver(port int, useMockEsx bool, mountDir string, driverName stri
}
}

d.mountIDtoName = make(map[string]string)
d.refCounts.Init(d, mountDir, driverName)

log.WithFields(log.Fields{
Expand All @@ -89,6 +92,12 @@ func NewVolumeDriver(port int, useMockEsx bool, mountDir string, driverName stri
return d
}

// VolumesInRefMap - get list of volumes names from refmap
// names are in format volume@datastore
func (d *VolumeDriver) VolumesInRefMap() []string {
return d.refCounts.GetVolumeNames()
}

// In following three operations on refcount, if refcount
// map hasn't been initialized, return 1 to prevent detach and remove.

Expand Down Expand Up @@ -212,6 +221,14 @@ func (d *VolumeDriver) UnmountVolume(name string) error {

// private function that does the job of mounting volume in conjunction with refcounting
func (d *VolumeDriver) processMount(r volume.MountRequest) volume.Response {
volumeInfo, err := plugin_utils.GetVolumeInfo(r.Name, "", d)
if err != nil {
log.Errorf("Unable to get volume info for volume %s. err:%v", r.Name, err)
return volume.Response{Err: err.Error()}
}
r.Name = volumeInfo.VolumeName
d.mountIDtoName[r.ID] = r.Name

// If the volume is already mounted , just increase the refcount.
// Note: for new keys, GO maps return zero value, so no need for if_exists.
refcnt := d.incrRefCount(r.Name) // save map traversal
Expand All @@ -223,9 +240,19 @@ func (d *VolumeDriver) processMount(r volume.MountRequest) volume.Response {
return volume.Response{Mountpoint: getMountPoint(r.Name)}
}

// There can be redundant mounts till refcounts are properly initialized
// TODO: #1220
status, err := d.ops.Get(r.Name)
if plugin_utils.AlreadyMounted(r.Name, mountRoot) {
log.WithFields(log.Fields{"name": r.Name}).Info("Already mounted, skipping mount. ")
return volume.Response{Mountpoint: getMountPoint(r.Name)}
}

// get volume metadata if required
volumeMeta := volumeInfo.VolumeMeta
if volumeMeta == nil {
if volumeMeta, err = d.ops.Get(r.Name); err != nil {
d.decrRefCount(r.Name)
return volume.Response{Err: err.Error()}
}
}

fstype := fs.FstypeDefault
isReadOnly := false
Expand All @@ -234,7 +261,7 @@ func (d *VolumeDriver) processMount(r volume.MountRequest) volume.Response {
return volume.Response{Err: err.Error()}
}
// Check access type.
value, exists := status["access"].(string)
value, exists := volumeMeta["access"].(string)
if !exists {
msg := fmt.Sprintf("Invalid access type for %s, assuming read-write access.", r.Name)
log.WithFields(log.Fields{"name": r.Name, "error": msg}).Error("")
Expand All @@ -244,7 +271,7 @@ func (d *VolumeDriver) processMount(r volume.MountRequest) volume.Response {
}

// Check file system type.
value, exists = status["fstype"].(string)
value, exists = volumeMeta["fstype"].(string)
if !exists {
msg := fmt.Sprintf("Invalid filesystem type for %s, assuming type as %s.",
r.Name, fstype)
Expand Down Expand Up @@ -458,6 +485,18 @@ func (d *VolumeDriver) Unmount(r volume.UnmountRequest) volume.Response {
return volume.Response{Err: ""}
}

if fullVolName, exist := d.mountIDtoName[r.ID]; exist {
r.Name = fullVolName
delete(d.mountIDtoName, r.ID) //cleanup the map
} else {
volumeInfo, err := plugin_utils.GetVolumeInfo(r.Name, "", d)
if err != nil {
log.Errorf("Unable to get volume info for volume %s. err:%v", r.Name, err)
return volume.Response{Err: err.Error()}
}
r.Name = volumeInfo.VolumeName
}

// if refcount has been succcessful, Normal flow
// if the volume is still used by other containers, just return OK
refcnt, err := d.decrRefCount(r.Name)
Expand Down
Loading