Bug 1128458 - [vdsm] - refactor/improve getPid()
Summary: [vdsm] - refactor/improve getPid()
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Red Hat Enterprise Virtualization Manager
Classification: Red Hat
Component: vdsm
Version: 3.4.1-1
Hardware: x86_64
OS: Linux
low
low
Target Milestone: ovirt-3.6.0-rc
: 3.6.0
Assignee: Francesco Romani
QA Contact: Artyom
URL:
Whiteboard:
: 1128445 (view as bug list)
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2014-08-10 14:41 UTC by Eldad Marciano
Modified: 2016-03-09 19:24 UTC (History)
9 users (show)

Fixed In Version:
Doc Type: Bug Fix
Doc Text:
Clone Of:
Environment:
Last Closed: 2016-03-09 19:24:15 UTC
oVirt Team: Virt


Attachments (Terms of Use)


Links
System ID Priority Status Summary Last Updated
Red Hat Product Errata RHBA-2016:0362 normal SHIPPED_LIVE vdsm 3.6.0 bug fix and enhancement update 2016-03-09 23:49:32 UTC
oVirt gerrit 36129 master MERGED vm: abort vm start if can't read pid Never

Description Eldad Marciano 2014-08-10 14:41:36 UTC
Description of problem:
validation for pid required in  '/usr/share/vdsm/supervdsmServer'


    @logDecorator
    def getVmPid(self, vmName):
        pidFile = "/var/run/libvirt/qemu/%s.pid" % vmName
        return open(pidFile).read()

better to use like this:

pidFile = "/var/run/libvirt/qemu/%s.pid" % vmName
try:
o = open(pidFile)
pid = o.read().strip('\n')
if len(pid) > 3; # mininum 3 digits
   all_vms_pids.append( (pid, vmName) ) # mange private list in order to reduce the performance to not read files to much
    o.close() very important    
else:
#todo:// do something in case less than 3 digits 
except Exception as ex:
#todo:// do something if no file exist suhtdown vm
finally:
if o:
 o.close()
return pid 




Version-Release number of selected component (if applicable):
vdsm-4.14.11-1.el6ev.x86_64


Actual results:
function returns nothing, there is no validation for pid.  

Expected results:
-validation for pid
-close the file object reduce resources
-manage list of pids instead of read the file every time the function being called

Additional info:

Comment 1 Eldad Marciano 2014-08-11 11:21:43 UTC
set CACHED_PIDS={} in '_SuperVdsm'


    @logDecorator
    def getVmPid(self, vmName):
        pidFile = "/var/run/libvirt/qemu/%s.pid" % vmName
        file_reader = None
        pid = None
        try:
            #check if vm cached
            if vmName in str(self.CACHED_PIDS):
                pid = self.CACHED_PIDS[vmName]
            else:
                file_reader = open(pidFile)
                pid = file_reader.read().strip('\n')
                if pid.isdigit():
                    self.CACHED_PIDS.append((vmName, pid))
        except Exception as ex:
            self.log.error("getVmPid failed shutdown vm \'%s\' required" % vmName)
            pass
        finally:
            if file_reader:
                file_reader.close()
            else:
                self.log.error("no pid file exist for \'%s\'" % vmName)
            if pid:
                return pid

Comment 2 Michal Skrivanek 2014-08-11 14:04:05 UTC
I don't see a point of the above, caching is useless as this is called just once. 

however, we want to re-asses how do we handle 0 PIDs (proposal is to destroy() in domDependentInit if we can't read PID)

and we should fix the file read() to close the FD

Moving to Francesco to address that

Comment 3 Michal Skrivanek 2014-08-11 14:04:42 UTC
*** Bug 1128445 has been marked as a duplicate of this bug. ***

Comment 4 Artyom 2015-04-21 11:34:58 UTC
Hi can you please provide me the best way to verify this bug?

Comment 5 Francesco Romani 2015-04-22 09:56:50 UTC
(In reply to Artyom from comment #4)
> Hi can you please provide me the best way to verify this bug?

Hi,

it is not easy unless you can pause qemu in the middle of its startup.
Probably the best way to test this is on vdsm recovery:

1. make sure QEMU pid doesn't change across VDSM recoveries.
   Of course this assumes QEMU process is never restarted or harmed in any way
2. this is tricky anyway.
   2.a. stop VDSM
   2.b. manually tamper VDSM recovery file, removing the PID
   2.c. manually tamper the pid (/var/run/libvirt/qemu/*.pid) to include invalid values, like zero or anything < 0
   2.d. restart VDSM: vm should fail to recover

#1 should be quick and easy
#2 is pretty hackish, and we're dealing with extreme conditions here, not sure the test is worth the effort.

Comment 6 Artyom 2015-04-28 10:57:37 UTC
Thanks for explanation verifying on: vdsm-4.17.0-632.git19a83a2.el7.x86_64

Comment 9 errata-xmlrpc 2016-03-09 19:24:15 UTC
Since the problem described in this bug report should be
resolved in a recent advisory, it has been closed with a
resolution of ERRATA.

For information on the advisory, and where to find the updated
files, follow the link below.

If the solution does not work for you, open a new bug report.

https://rhn.redhat.com/errata/RHBA-2016-0362.html


Note You need to log in before you can comment on or make changes to this bug.