[Kimchi-devel] [PATCH v2] Set virt_use_nfs when NFS pool is added.

[Christy Perez]

On Fri, 2014-04-11 at 11:54 +0800, Mark Wu wrote:
> On 04/11/2014 06:56 AM, Christy Perez wrote:
> > selinux has a special boolean to make it easier for disk images
> > to be managedi by libvirt. Set this to true when a user
> > adds an NFS storage pool.
> >
> > Most virtualzation documentation recommends that this be set
> > to true. For example:
> > http://www.ovirt.org/Troubleshooting_NFS_Storage_Issues
> > http://fedoraproject.org/wiki/How_to_debug_Virtualization_problems
> >
> > This will leave it set to true, even if
> > the user removes NFS storage pools. It is not a security risk, and
> > we should not set it to False in case it had already been set by the
> > user for another non-kimchi use.
> >
> > Signed-off-by: Christy Perez <christy at linux.vnet.ibm.com>
> > ---
> >   src/kimchi/model/storagepools.py | 7 +++++++
> >   1 file changed, 7 insertions(+)
> >
> > diff --git a/src/kimchi/model/storagepools.py b/src/kimchi/model/storagepools.py
> > index 5af33b7..1ec6e99 100644
> > --- a/src/kimchi/model/storagepools.py
> > +++ b/src/kimchi/model/storagepools.py
> > @@ -126,6 +126,13 @@ class StoragePoolsModel(object):
> >               kimchi_log.error("Problem creating Storage Pool: %s", e)
> >               raise OperationFailed("KCHPOOL0007E",
> >                                     {'name': name, 'err': e.get_error_message()})
> > +        if params['type'] == 'netfs':
> > +            output, error, returncode = run_command(['setsebool', '-P',
> > +                                                    'virt_use_nfs=1'])
> The persistent change of sebool is a very time-consuming operation 
> compared with the runtime change.
> Please see:
> [root at localhost libvirt]# time setsebool -P virt_use_nfs=1
> 
> real    0m10.686s
> user    0m9.680s
> sys    0m0.337s
> [root at localhost libvirt]# time setsebool virt_use_nfs=1
> 
> real    0m0.035s
> user    0m0.001s
> sys    0m0.005s
> 
> time getsebool virt_use_nfs
> virt_use_nfs --> off
> 
> real    0m0.002s
> user    0m0.000s
> sys    0m0.001s
> 
> 
> So  I think we could have a little optimization here:  check the bool 
> value by getsebool before setting it.
> I am fine to leave it in a following up patch.
> > +            if error or returncode:
> Just checking returncode is enough,  isn't it?
> > +                kimchi_log.error('Unable to set virt_use_nfs=1. If you use
> > +                                 SELinux, this may prevent NFS pools from
> > +                                 being used.')
> >           return name
> >
> >       def _clean_scan(self, pool_name):
> 

Thanks for the comments, Mark. This has uncovered some interesting
behavior in trying to implement your suggestion.

The error situation I was trying to handle here is if (as Royce pointed
out), there is no selinux on a system. I assumed that if the command
wasn't found, there wouldn't be an rc, but that an error message would
come back. I did some experiments, and it turns out that the way
kimchi's run_command() works, if a command doesn't exist, there's an
error printed (e.g. Failed to run command: fakesebool -P virt_use_nfs=1.
[Errno 2] No such file or directory), but then run_command returns None,
None, None because of the exception. So, really, I'll get back no error
or returncode in this situation! I'm not sure this is the correct
behavior in case of an exception -- but that can be taken up in a
separate thread.

This also makes it more difficult to know if getsebool virt_use_nfs is
set or not. If getsebool causes that exception, it won't get back an rc,
or output, or an error message. 

Since this won't be called very often, I think it's okay to leave it
as-is (except fix the errors that Aline pointed out in my string), and
revisit after we decide how to deal with run_command().

If that sounds okay, Aline -- are you ok with throwing in some new-line
chars at the end of those strings, or do you want me to submit a new
patch with that?

Regards,


- Christy