Antoni Segura Puimedon has uploaded a new change for review.
Change subject: sriov: Make the hook safe for concurrency ......................................................................
sriov: Make the hook safe for concurrency
The current model uses a cache that is not locked and could be overwritten by a second VM creation effectively preventing the return of the chown to root:root.
This patch solves that by creating with exclusivity a file for each virtual function that is attached (thus avoiding locking) but having concurrency safety due to the OS guarantee that the not more than a single processes can perform an open when the flags O_CREAT | O_EXCL are set.
Change-Id: I2c6ad7f2ee53911312700396000e0aca07a917e9 Signed-off-by: Antoni S. Puimedon asegurap@redhat.com --- M vdsm_hooks/sriov/after_vm_destroy.py M vdsm_hooks/sriov/before_vm_start.py 2 files changed, 40 insertions(+), 46 deletions(-)
git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/41/13641/1
diff --git a/vdsm_hooks/sriov/after_vm_destroy.py b/vdsm_hooks/sriov/after_vm_destroy.py index a5e25b5..e82dab0 100755 --- a/vdsm_hooks/sriov/after_vm_destroy.py +++ b/vdsm_hooks/sriov/after_vm_destroy.py @@ -6,15 +6,13 @@
import hooking
-SYS_NIC_PATH = '/sys/class/net/%s' -VDSM_VAR_HOOKS_DIR = '/var/run/vdsm/hooks' -SRIOV_CACHE_FILENAME = 'sriov.cache' +VDSM_VAR_HOOKS_DIR = '/var/run/vdsm/hooks/sriov'
-def restoreDevicePermissions(addr, devpath): +def restoreDevicePermissions(devpath): owner = 'root:root' for f in os.listdir(devpath): - if f.startswith('resource') or f == 'rom' or f == 'reset': + if f.startswith('resource') or f in ('rom', 'reset'): dev = os.path.join(devpath, f) command = ['/bin/chown', owner, dev] retcode, out, err = hooking.execCmd(command, sudo=True, raw=True) @@ -26,24 +24,16 @@ if 'sriov' in os.environ: try: lines = '' - nics = os.environ['sriov'].split(',') - path = VDSM_VAR_HOOKS_DIR + '/' + SRIOV_CACHE_FILENAME - - if os.path.exists(path): - with open(path, 'r') as f: - for line in f: - nicAddr = line.split('=') - if nicAddr[0] in nics: - restoreDevicePermissions(nicAddr[1], - nicAddr[2].strip('\n')) - else: - lines += line - - with open(path, 'w') as f: - f.writelines(lines) - else: - sys.stderr.write('sriov after_vm_destroy: cannot find sriov cache ' - 'file %s\n' % path) + for nic in os.environ['sriov'].split(','): + vfFilePath = os.path.join(VDSM_VAR_HOOKS_DIR, nic) + if os.path.exists(vfFilePath): + with open(vfFilePath, 'r') as vfFile: + restoreDevicePermissions(vfFile.read()) + os.unlink(vfFilePath) + else: + sys.stderr.write('sriov after_vm_destroy: cannot find the ' + 'virtual function reservation file of %s' + 'that should be at %s\n' % (nic, vfFilePath))
except: sys.stderr.write('sriov after_vm_destroy: [unexpected error]: %s\n' % diff --git a/vdsm_hooks/sriov/before_vm_start.py b/vdsm_hooks/sriov/before_vm_start.py index 42b79e9..c23797b 100755 --- a/vdsm_hooks/sriov/before_vm_start.py +++ b/vdsm_hooks/sriov/before_vm_start.py @@ -1,5 +1,6 @@ #!/usr/bin/python
+import errno import os import sys import grp @@ -11,8 +12,7 @@ from vdsm import libvirtconnection
SYS_NIC_PATH = '/sys/class/net/%s' -VDSM_VAR_HOOKS_DIR = '/var/run/vdsm/hooks' -SRIOV_CACHE_FILENAME = 'sriov.cache' +VDSM_VAR_HOOKS_DIR = '/var/run/vdsm/hooks/sriov'
''' sriov vdsm hook @@ -93,32 +93,36 @@ return 'pci_%s_%s_%s' % (tokens[0], tokens[1], tokens[2].replace('.', '_'))
-def writeSriovCache(name, addr, devpath): +def writeVFReservationFile(nic, devpath): if not os.path.exists(VDSM_VAR_HOOKS_DIR): os.makedirs(VDSM_VAR_HOOKS_DIR) - - f = open(VDSM_VAR_HOOKS_DIR + '/' + SRIOV_CACHE_FILENAME, 'a') - f.write(name + '=' + addr + '=' + devpath + '\n') - f.close() + try: + fd = os.open(os.path.join(VDSM_VAR_HOOKS_DIR, nic), + os.O_WRONLY | os.O_CREAT | os.O_EXCL) + with os.fdopen(fd, 'w') as f: + f.write(devpath) + except OSError as e: + if e.errno == errno.EEXIST: + sys.stderr.write('sriov: Error. The device %s is already attached ' + 'or in the process of attaching to a VM. Aborting' + '.\n' % nic) + sys.stderr.write('sriov: Unexpected error creating virtual function ' + 'reservation file for nic %s. Aborting.\n%s\n' % + (nic, traceback.format_exc())) + sys.exit(2)
-def chown(devpath): - group = grp.getgrnam('qemu') - gid = group.gr_gid - user = pwd.getpwnam('qemu') - uid = user.pw_uid - +def chown(nic, devpath): + '''Uses sudo and chown to change the sriov ownership.''' + owner = ''.join([str(pwd.getpwnam('qemu').pw_uid), ':', + str(grp.getgrnam('qemu').gr_gid)]) for f in os.listdir(devpath): - if f.startswith('resource') or f == 'rom' or f == 'reset': - dev = os.path.join(devpath, f) - - # we don't use os.chown because we need sudo - owner = str(uid) + ':' + str(gid) - command = ['/bin/chown', owner, dev] + if f.startswith('resource') or f in ('rom', 'reset'): + command = ['/bin/chown', owner, os.path.join(devpath, f)] retcode, out, err = hooking.execCmd(command, sudo=True, raw=True) if retcode != 0: - sys.stderr.write('sriov: error chown %s to %s, err = %s\n' % - (dev, owner, err)) + sys.stderr.write('sriov: Error %s changing ownership of %s to' + 'owner %s. Aborting.\n' % (err, nic, owner)) sys.exit(2)
@@ -141,9 +145,9 @@
sys.stderr.write('sriov: VF %s xml: %s\n' % (nic, interface.toxml())) - chown(devpath) + writeVFReservationFile(nic, devpath) + chown(nic, devpath) devices.appendChild(interface) - writeSriovCache(nic, addr, devpath) else: sys.stderr.write('sriov: cannot find nic "%s", aborting\n' % nic)
-- To view, visit http://gerrit.ovirt.org/13641 To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: newchange Gerrit-Change-Id: I2c6ad7f2ee53911312700396000e0aca07a917e9 Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Antoni Segura Puimedon asegurap@redhat.com