[PATCH v2] Set virt_use_nfs when NFS pool is added.
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@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']) + if error or returncode: + 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): -- 1.9.0
Reviewed-by: Royce Lv<lvroyce@linux.vnet.ibm.com> On 2014年04月11日 06:56, 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@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']) + if error or returncode: + 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):
The tests are failing with this patch set: E..........EEEE......E.....EE ====================================================================== ERROR: test_exception (unittest.loader.ModuleImportFailure) ---------------------------------------------------------------------- ImportError: Failed to import test module: test_exception Traceback (most recent call last): File "/usr/lib/python2.7/unittest/loader.py", line 252, in _find_tests module = self._get_module_from_name(name) File "/usr/lib/python2.7/unittest/loader.py", line 230, in _get_module_from_name __import__(name) File "/home/alinefm/kimchi/tests/test_exception.py", line 25, in <module> import kimchi.mockmodel File "/home/alinefm/kimchi/src/kimchi/mockmodel.py", line 49, in <module> from kimchi.model.storagepools import ISO_POOL_NAME, STORAGE_SOURCES File "/home/alinefm/kimchi/src/kimchi/model/storagepools.py", line 133 kimchi_log.error('Unable to set virt_use_nfs=1. If you use ^ SyntaxError: EOL while scanning string literal ====================================================================== ERROR: test_mockmodel (unittest.loader.ModuleImportFailure) ---------------------------------------------------------------------- ImportError: Failed to import test module: test_mockmodel Traceback (most recent call last): File "/usr/lib/python2.7/unittest/loader.py", line 252, in _find_tests module = self._get_module_from_name(name) File "/usr/lib/python2.7/unittest/loader.py", line 230, in _get_module_from_name __import__(name) File "/home/alinefm/kimchi/tests/test_mockmodel.py", line 27, in <module> import kimchi.mockmodel File "/home/alinefm/kimchi/src/kimchi/mockmodel.py", line 49, in <module> from kimchi.model.storagepools import ISO_POOL_NAME, STORAGE_SOURCES File "/home/alinefm/kimchi/src/kimchi/model/storagepools.py", line 133 kimchi_log.error('Unable to set virt_use_nfs=1. If you use ^ SyntaxError: EOL while scanning string literal ====================================================================== ERROR: test_rest (unittest.loader.ModuleImportFailure) ---------------------------------------------------------------------- ImportError: Failed to import test module: test_rest Traceback (most recent call last): File "/usr/lib/python2.7/unittest/loader.py", line 252, in _find_tests module = self._get_module_from_name(name) File "/usr/lib/python2.7/unittest/loader.py", line 230, in _get_module_from_name __import__(name) File "/home/alinefm/kimchi/tests/test_rest.py", line 35, in <module> import kimchi.mockmodel File "/home/alinefm/kimchi/src/kimchi/mockmodel.py", line 49, in <module> from kimchi.model.storagepools import ISO_POOL_NAME, STORAGE_SOURCES File "/home/alinefm/kimchi/src/kimchi/model/storagepools.py", line 133 kimchi_log.error('Unable to set virt_use_nfs=1. If you use ^ SyntaxError: EOL while scanning string literal ====================================================================== ERROR: test_model (unittest.loader.ModuleImportFailure) ---------------------------------------------------------------------- ImportError: Failed to import test module: test_model Traceback (most recent call last): File "/usr/lib/python2.7/unittest/loader.py", line 252, in _find_tests module = self._get_module_from_name(name) File "/usr/lib/python2.7/unittest/loader.py", line 230, in _get_module_from_name __import__(name) File "/home/alinefm/kimchi/tests/test_model.py", line 34, in <module> import utils File "/home/alinefm/kimchi/tests/utils.py", line 35, in <module> import kimchi.server File "/home/alinefm/kimchi/src/kimchi/server.py", line 30, in <module> from kimchi import mockmodel File "/home/alinefm/kimchi/src/kimchi/mockmodel.py", line 49, in <module> from kimchi.model.storagepools import ISO_POOL_NAME, STORAGE_SOURCES File "/home/alinefm/kimchi/src/kimchi/model/storagepools.py", line 133 kimchi_log.error('Unable to set virt_use_nfs=1. If you use ^ SyntaxError: EOL while scanning string literal ====================================================================== ERROR: test_server (unittest.loader.ModuleImportFailure) ---------------------------------------------------------------------- ImportError: Failed to import test module: test_server Traceback (most recent call last): File "/usr/lib/python2.7/unittest/loader.py", line 252, in _find_tests module = self._get_module_from_name(name) File "/usr/lib/python2.7/unittest/loader.py", line 230, in _get_module_from_name __import__(name) File "/home/alinefm/kimchi/tests/test_server.py", line 24, in <module> import utils File "/home/alinefm/kimchi/tests/utils.py", line 35, in <module> import kimchi.server File "/home/alinefm/kimchi/src/kimchi/server.py", line 30, in <module> from kimchi import mockmodel File "/home/alinefm/kimchi/src/kimchi/mockmodel.py", line 49, in <module> from kimchi.model.storagepools import ISO_POOL_NAME, STORAGE_SOURCES File "/home/alinefm/kimchi/src/kimchi/model/storagepools.py", line 133 kimchi_log.error('Unable to set virt_use_nfs=1. If you use ^ SyntaxError: EOL while scanning string literal ====================================================================== ERROR: test_authorization (unittest.loader.ModuleImportFailure) ---------------------------------------------------------------------- ImportError: Failed to import test module: test_authorization Traceback (most recent call last): File "/usr/lib/python2.7/unittest/loader.py", line 252, in _find_tests module = self._get_module_from_name(name) File "/usr/lib/python2.7/unittest/loader.py", line 230, in _get_module_from_name __import__(name) File "/home/alinefm/kimchi/tests/test_authorization.py", line 28, in <module> import kimchi.mockmodel File "/home/alinefm/kimchi/src/kimchi/mockmodel.py", line 49, in <module> from kimchi.model.storagepools import ISO_POOL_NAME, STORAGE_SOURCES File "/home/alinefm/kimchi/src/kimchi/model/storagepools.py", line 133 kimchi_log.error('Unable to set virt_use_nfs=1. If you use ^ SyntaxError: EOL while scanning string literal ====================================================================== ERROR: test_networkxml (unittest.loader.ModuleImportFailure) ---------------------------------------------------------------------- ImportError: Failed to import test module: test_networkxml Traceback (most recent call last): File "/usr/lib/python2.7/unittest/loader.py", line 252, in _find_tests module = self._get_module_from_name(name) File "/usr/lib/python2.7/unittest/loader.py", line 230, in _get_module_from_name __import__(name) File "/home/alinefm/kimchi/tests/test_networkxml.py", line 25, in <module> import utils File "/home/alinefm/kimchi/tests/utils.py", line 35, in <module> import kimchi.server File "/home/alinefm/kimchi/src/kimchi/server.py", line 30, in <module> from kimchi import mockmodel File "/home/alinefm/kimchi/src/kimchi/mockmodel.py", line 49, in <module> from kimchi.model.storagepools import ISO_POOL_NAME, STORAGE_SOURCES File "/home/alinefm/kimchi/src/kimchi/model/storagepools.py", line 133 kimchi_log.error('Unable to set virt_use_nfs=1. If you use ^ SyntaxError: EOL while scanning string literal ====================================================================== ERROR: test_plugin (unittest.loader.ModuleImportFailure) ---------------------------------------------------------------------- ImportError: Failed to import test module: test_plugin Traceback (most recent call last): File "/usr/lib/python2.7/unittest/loader.py", line 252, in _find_tests module = self._get_module_from_name(name) File "/usr/lib/python2.7/unittest/loader.py", line 230, in _get_module_from_name __import__(name) File "/home/alinefm/kimchi/tests/test_plugin.py", line 28, in <module> import kimchi.mockmodel File "/home/alinefm/kimchi/src/kimchi/mockmodel.py", line 49, in <module> from kimchi.model.storagepools import ISO_POOL_NAME, STORAGE_SOURCES File "/home/alinefm/kimchi/src/kimchi/model/storagepools.py", line 133 kimchi_log.error('Unable to set virt_use_nfs=1. If you use ^ SyntaxError: EOL while scanning string literal ---------------------------------------------------------------------- Ran 29 tests in 0.011s FAILED (errors=8) make[3]: *** [check-local] Error 1 make[3]: Leaving directory `/home/alinefm/kimchi/tests' make[2]: *** [check-am] Error 2 make[2]: Leaving directory `/home/alinefm/kimchi/tests' make[1]: *** [check] Error 2 make[1]: Leaving directory `/home/alinefm/kimchi/tests' make: *** [check-recursive] Error 1 On 04/10/2014 07:56 PM, 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@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']) + if error or returncode: + 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):
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@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@localhost libvirt]# time setsebool -P virt_use_nfs=1
real 0m10.686s user 0m9.680s sys 0m0.337s [root@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):
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@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@localhost libvirt]# time setsebool -P virt_use_nfs=1
real 0m10.686s user 0m9.680s sys 0m0.337s [root@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
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@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@localhost libvirt]# time setsebool -P virt_use_nfs=1
real 0m10.686s user 0m9.680s sys 0m0.337s [root@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
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@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@localhost libvirt]# time setsebool -P virt_use_nfs=1
real 0m10.686s user 0m9.680s sys 0m0.337s [root@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
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@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@localhost libvirt]# time setsebool -P virt_use_nfs=1
real 0m10.686s user 0m9.680s sys 0m0.337s [root@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
participants (4)
-
Aline Manera -
Christy Perez -
Mark Wu -
Royce Lv