New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support for execution modules and states mount on AIX #48803

Merged
merged 9 commits into from Aug 9, 2018

Conversation

Projects
None yet
5 participants
@dmurphy18
Contributor

dmurphy18 commented Jul 27, 2018

What does this PR do?

Provides support for mount on AIX for execution modules and states

What issues does this PR fix or reference?

saltstack/zh#1407

Previous Behavior

mount.active functioned on AIX

New Behavior

Full support for mount functionality in execution modules and states on AIX

Tests written?

Yes

Commits signed with GPG?

No

Please review Salt's Contributing Guide for best practices.

See GitHub's page on GPG signing for more information about signing commits with GPG.

@@ -5,6 +5,7 @@
# Import python libs
from __future__ import absolute_import, print_function, unicode_literals
from collections import OrderedDict

This comment has been minimized.

@cachedout

cachedout Jul 27, 2018

Contributor

We generally prefer the use of salt.utils.odict instead.

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 27, 2018

Contributor

Will do, hang over from developing it.

@@ -432,6 +434,141 @@ def match(self, line):
return True
class _filesystems_entry(object):

This comment has been minimized.

@cachedout

cachedout Jul 27, 2018

Contributor

Class names are typically camel-case: https://www.python.org/dev/peps/pep-0008/#class-names

This comment has been minimized.

@terminalmage

terminalmage Jul 27, 2018

Member

Yeah, and they don't need to be private. It's a common misconception that it'll be picked up by Sphinx autodoc but it only picks up functions, not classes.

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 27, 2018

Contributor

@terminalmage @cachedout I was following the current code's implementation, _fstab_entry, _vfstab_entry. I can certainly change the name, however it would appear inconsistent with existing code. I prefer to maintain the style and usage already present in the file, but if preferred I can update my additions to be conformant with existing standards.
Let me know which is the preference moving forward so that I can be consistent in the future.

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 31, 2018

Contributor

Updated to CamelCase

{ '/dir' : { 'dev': '/dev/hd8', .... }}
False return dictionary keyed by 'name' value and dictionary with all keys, values
{ '/dir' : { 'name' : '/dir', 'dev': '/dev/hd8', .... }}

This comment has been minimized.

@cachedout

cachedout Jul 27, 2018

Contributor

What is the expected return of this function?

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 27, 2018

Contributor

OrderedDict, have to maintain the order of the entries found in the file /etc/filesystems as it is significant for correct operation. I shall update the doc string to better reflect this.

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 27, 2018

Contributor

done

'''
ret = {}
if 'AIX' not in __grains__['kernel']:
return ret

This comment has been minimized.

@cachedout

cachedout Jul 27, 2018

Contributor

We can't just silently do this. It's going to appear completely broken to nearly all users. It either needs to be implemented for the operating systems supported by this module or not implemented at all.

This comment has been minimized.

@terminalmage

terminalmage Jul 27, 2018

Member

I think that this is meant as an AIX complement to the fstab function used on other platforms. It should be explicitly documented as such to distinguish itself.

I don't understand the reasoning for putting all the work in a separate, private function, though.

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 27, 2018

Contributor

@terminalmage reason for separate functions for filesystems, set_filesystems, rm_filesystems is following established pattern, for example: automaster, set_automaster, rm_automaster and similar for vfstab, even if it basically call fstab equivalent with a different config file. In this case more so, since /etc/filesystems (multi-line entries), is not even close to /etc/fstab in form (single line entries).

@cachedout the quick exit is similar to that already implemented, for example def automaster checks for a file (/etc/auto_salt) which will probably not exist on Linux and hence quickly exit. I could do the same check for file, however I feel the OS check is more inherently accurate as to functionality exists or not. That said, I can change it to a file test similar to automaster.
Let me know your preference.

This comment has been minimized.

@terminalmage

terminalmage Jul 30, 2018

Member

No that's not what I was talking about. I was talking about the fact that all the actual work was in a _filesystems function.

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 31, 2018

Contributor

@terminalmage the use of _filesystems function is only to read in the contents /etc/filesystems, in order, and then allow manipulation as desired, here to write the new state to /etc/filesystems in the correct order. Order is significant on AIX and should not be changed, otherwise operations are affected.
Hence the use of _filesystems is to obtain the current /etc/filesystems into an instance where it can be manipulated. The function is used in plain output, setting and removing. A common internal function used, perhaps you could explain further your objection to its use.

Partial contents of /etc/filesystems for example:

/opt:
dev = /dev/hd10opt
vfs = jfs2
log = /dev/hd8
mount = true
check = true
vol = /opt
free = false
quota = no

/var/adm/ras/livedump:
dev = /dev/livedump
vfs = jfs2
log = /dev/hd8
mount = true
account = false
quota = no

This comment has been minimized.

@terminalmage

terminalmage Aug 3, 2018

Member

I did not catch the other calls to the function in my earlier review.

.. versionadded:: 2018.3.3
Verify that this mount is represented in the filesystems, change the mount
to match the data passed, or add the mount if it is not present on AIX

This comment has been minimized.

@cachedout

cachedout Jul 27, 2018

Contributor

I'm not sure how I follow that the functionality of this is different than the remount function

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 27, 2018

Contributor

@cachedout so what is the reason for set_automaster, set_fstab, set_vfstab, just being consistent with existing functionality which is used internally in states/mount.py, where set_fstab is called. Need to have AIX equivalents hence set_filesystems.

@@ -407,7 +407,10 @@ def flopen(*args, **kwargs):
with fopen(*args, **kwargs) as f_handle:
try:
if is_fcntl_available(check_sunos=True):
fcntl.flock(f_handle.fileno(), fcntl.LOCK_SH)
lock_type = fcntl.LOCK_SH
if salt.utils.platform.is_aix() and ('a' in args[1] or 'w' in args[1]):

This comment has been minimized.

@cachedout

cachedout Jul 27, 2018

Contributor

I am not a huge fan of putting this lookup overhead into 100% of read calls in Salt. Can't we make this an argument instead?

This comment has been minimized.

@terminalmage

terminalmage Jul 27, 2018

Member

Yeah, we use this function everywhere, that would impact performance. It's better if we introduce a lock_type argument with a default value of fcntl.LOCK_SH and then rely on AIX-specific code to pass lock_type=LOCK_EX when opening files.

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 27, 2018

Contributor

referring to saltstack/zh#1409,
Will attempt to leverage something at import time so that I can use that, but the issue being that Exclusive locks is only required if file opened for write, and I would not want to hurt performance for reads. But if worried about performance, why is that is_fcntl_available constantly being checked when only really applies if Solaris.

Could always rewrite such that AIX don't bother with is_fcntl_available and resultant minimized performance hit. Let me know your thoughts

@@ -407,7 +407,10 @@ def flopen(*args, **kwargs):
with fopen(*args, **kwargs) as f_handle:
try:
if is_fcntl_available(check_sunos=True):
fcntl.flock(f_handle.fileno(), fcntl.LOCK_SH)
lock_type = fcntl.LOCK_SH
if salt.utils.platform.is_aix() and ('a' in args[1] or 'w' in args[1]):

This comment has been minimized.

@terminalmage

terminalmage Jul 27, 2018

Member

Yeah, we use this function everywhere, that would impact performance. It's better if we introduce a lock_type argument with a default value of fcntl.LOCK_SH and then rely on AIX-specific code to pass lock_type=LOCK_EX when opening files.

class ParseError(ValueError):
'''
Error raised when a line isn't parsible as an fstab entry
'''

This comment has been minimized.

@terminalmage

terminalmage Jul 27, 2018

Member

I'm not sure why a separate exception class is needed here, specifically one that is private to this class.

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 27, 2018

Contributor

part of cut & paste from _fstab_entry and _vfstab_entry. Took those as starting point and adjusted equivalently, preferring to leave similar if no particular reason to remove it.

def _filesystems(config='/etc/filesystems', leading_key=True):
'''
.. versionadded:: 2018.3.3

This comment has been minimized.

@terminalmage

terminalmage Jul 27, 2018

Member

Sphinx directives aren't really useful in a private function.

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 27, 2018

Contributor

Will remove, hangover from originally written def filesystems before making internal version.

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 27, 2018

Contributor

done

'''
ret = {}
if 'AIX' not in __grains__['kernel']:
return ret

This comment has been minimized.

@terminalmage

terminalmage Jul 27, 2018

Member

I think that this is meant as an AIX complement to the fstab function used on other platforms. It should be explicitly documented as such to distinguish itself.

I don't understand the reasoning for putting all the work in a separate, private function, though.

invalid_keys = filter(filterFn, match_on)
msg = 'Unrecognized keys in match_on: "{0}"'.format(invalid_keys)
raise CommandExecutionError(msg)

This comment has been minimized.

@terminalmage

terminalmage Jul 27, 2018

Member

We don't need to assign this to a variable if all we're doing is raising an exception with it.

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 27, 2018

Contributor

@terminalmage but automaster was written as a complement to fstab for Darwin. As for msg, adjusted but similar usage throughout file

except (IOError, OSError) as exc:
msg = 'Couldn\'t read from {0}: {1}'
raise CommandExecutionError(msg.format(config, exc))

This comment has been minimized.

@terminalmage

terminalmage Jul 27, 2018

Member

We don't need to assign this to a variable if all we're doing is raising an exception with it.

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 27, 2018

Contributor

Due to cut & paste, as similar usage throughout existing file.

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 27, 2018

Contributor

done

ofile.writelines(salt.utils.data.encode(mystrg))
except (IOError, OSError):
msg = 'File not writable {0}'
raise CommandExecutionError(msg.format(config))

This comment has been minimized.

@terminalmage

terminalmage Jul 27, 2018

Member

We don't need to assign this to a variable if all we're doing is raising an exception with it.

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 27, 2018

Contributor

Due to cut & paste, as similar usage throughout existing file.

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 27, 2018

Contributor

done

except (IOError, OSError) as exc:
msg = "Couldn't read from {0}: {1}"
raise CommandExecutionError(msg.format(config, exc))

This comment has been minimized.

@terminalmage

terminalmage Jul 27, 2018

Member

We don't need to assign this to a variable if all we're doing is raising an exception with it.

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 27, 2018

Contributor

Due to cut & paste, as similar usage throughout existing file.

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 27, 2018

Contributor

done

ofile.writelines(salt.utils.data.encode(mystrg))
except (IOError, OSError) as exc:
msg = "Couldn't write to {0}: {1}"
raise CommandExecutionError(msg.format(config, exc))

This comment has been minimized.

@terminalmage

terminalmage Jul 27, 2018

Member

We don't need to assign this to a variable if all we're doing is raising an exception with it.

This comment has been minimized.

@dmurphy18

dmurphy18 Jul 27, 2018

Contributor

@terminalmage As a general review question, do we keep new code similar to existing code in existing file, or write in new style ?

@dmurphy18 dmurphy18 force-pushed the dmurphy18:aix_filesystems branch from 1290acd to d89d8ef Jul 31, 2018

@dmurphy18

This comment has been minimized.

Contributor

dmurphy18 commented Jul 31, 2018

Updated for review comments. Note the changes to salt/utils/files.py flock and use of LOCK_EX.
We should be opening files for write with Exclusive Locks, rather than Shared. Default on Linux OS is LOCK_EX, on AIX OS it will object if attempt to open a file for write and not use LOCK_EX.

The use of LOCK_SH has been present in Salt since 2013, so this change may cause unanticipated affects. Please review this section carefully.

@rallytime rallytime requested review from cachedout and terminalmage Jul 31, 2018

@cachedout

This comment has been minimized.

Contributor

cachedout commented Aug 2, 2018

Once the comments from @terminalmage are resolved, I'm OK with this. The test still needs to be fixed though.

@terminalmage

LGTM pending tests being resolved.

@terminalmage

LGTM pending tests being resolved.

@dmurphy18

This comment has been minimized.

Contributor

dmurphy18 commented Aug 3, 2018

@rallytime Not seeing test_mount failures with:
python runtests.py -m
python runtests.py -m unit.modules.test_mount

On both Centos 7 7.3 and update to latest 7.5.1804

Do see 2 failures on both versions of Centos 7
integration.modules.test_cmdmod.CMDModuleTest.test_cmd_run_whoami
integration.modules.test_linux_acl.LinuxAclModuleTest.test_getfacl_w_single_file_without_acl

Running with VirtualBox rather than Docker and Kitchen etc. Believe issues seen are related to environment than unit tests.

@rallytime

This comment has been minimized.

Contributor

rallytime commented Aug 5, 2018

@dmurphy18 I can reproduce that test failure with just runtests.py -n unit.modules.test_mount. I ran this using a docker container from barnacle.

If you still can't reproduce, you should be able to get a matching environment with kitchen-salt. @gtmanfred should be able to point you in the right direction.

@dmurphy18

This comment has been minimized.

Contributor

dmurphy18 commented Aug 6, 2018

@rallytime I am very suspect that we are looking at a docker issue of some sort given VM's are a better abstraction of a machine. However understand the need to get it working with testing and will check with Daniel to resolve the issue with automatic testing, noting that both Debian 8 and Redhat 7 (7.3 and 7.5) pass fine on a VM.

@gtmanfred

This comment has been minimized.

Contributor

gtmanfred commented Aug 6, 2018

We run all our tests in jenkins in VMs in aws. So it is not working in VMs.

@dmurphy18 dmurphy18 force-pushed the dmurphy18:aix_filesystems branch from 5d7d7db to 2bc4238 Aug 7, 2018

@dmurphy18 dmurphy18 force-pushed the dmurphy18:aix_filesystems branch from 2bc4238 to 92818f8 Aug 8, 2018

@dmurphy18

This comment has been minimized.

Contributor

dmurphy18 commented Aug 8, 2018

Problem with tests were due to Redhat/Arch having a /etc/filesystems file, Debian/Ubuntu do not.
Adjusted tests and fixed Python 3 issue

@rallytime rallytime merged commit 7d6b9ed into saltstack:2018.3 Aug 9, 2018

6 of 8 checks passed

jenkins/pr/py2-centos-7 The py2-centos-7 job has failed
Details
jenkins/pr/py2-ubuntu-1604 running py2-ubuntu-1604...
Details
WIP ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint The lint job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment