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

[Christy Perez]

Thanks Aline!


On Mon, 2014-04-14 at 15:59 -0300, Aline Manera wrote:
> On 04/11/2014 02:06 PM, Christy Perez wrote:
> >
> >
> > 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?
> 
> I will fix it before applying so you don't need to resend the patch.
> 
> > Regards,
> >
> >
> > - Christy
> >
>