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

[Mark Wu]

On 04/12/2014 01:06 AM, 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.
I believe we need fix this issue in run_command,  which should
reflect the exception too.  Would you like to post a patch to improve it?
Thanks!
> 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().
Yes, it's fine for me to leave it as-is now.
> 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
>