This repository has been archived by the owner. It is now read-only.

zfs module doesn't support management of OpenZFS pools #2415

Closed
xen0l opened this Issue Jun 12, 2016 · 25 comments

Comments

Projects
None yet
6 participants
@xen0l
Contributor

xen0l commented Jun 12, 2016

ISSUE TYPE
  • Bug Report
COMPONENT NAME

zfs

ANSIBLE VERSION

ansible 2.1.0.0

CONFIGURATION

Nothing

OS / ENVIRONMENT

Freshly installed latest version of OpenZFS on OpenIndiana. This also affects every illumos-based operating systems (SmartOS, OmniOS etc)

SUMMARY

The problem is with zpool version. OpenZFS uses feature flags and sets zpool version to "-", which breaks compatibility with this module. It fails when it tries to compare OpenZFS zpool version with zpool version 34 on illumos-based system (

if int(version) >= 34:
)

This module should check for Solaris presence not by os.uname, but by ansible facts if possible.

STEPS TO REPRODUCE

ansible all -m zfs -a 'name=rpool/data state=present' -vvv

EXPECTED RESULTS

ZFS dataset rpool/data should be created.

ACTUAL RESULTS

rpool/data wasn't created.

An exception occurred during task execution. The full traceback is:
Traceback (most recent call last):
  File "/tmp/ansible_Y3bKol/ansible_module_zfs.py", line 245, in <module>
    main()
  File "/tmp/ansible_Y3bKol/ansible_module_zfs.py", line 227, in main
    zfs = Zfs(module, name, properties)
  File "/tmp/ansible_Y3bKol/ansible_module_zfs.py", line 90, in __init__
    self.enhanced_sharing = self.check_enhanced_sharing()
  File "/tmp/ansible_Y3bKol/ansible_module_zfs.py", line 99, in check_enhanced_sharing
    if int(version) >= 34:
ValueError: invalid literal for int() with base 10: '-'

@xen0l xen0l changed the title from zfs module fails to create datasets on non-Solaris distributions to zfs module fails to create datasets on illumos distributions Jun 12, 2016

@xen0l

This comment has been minimized.

Show comment
Hide comment
@xen0l

xen0l Jun 13, 2016

Contributor
Contributor

xen0l commented Jun 13, 2016

@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca Jun 13, 2016

Member

the module was working with BSD and linux, last I checked, so this is not only a Solaris issue

Member

bcoca commented Jun 13, 2016

the module was working with BSD and linux, last I checked, so this is not only a Solaris issue

@bcoca bcoca added the bug_report label Jun 13, 2016

@johanwiren

This comment has been minimized.

Show comment
Hide comment
@johanwiren

johanwiren Jun 14, 2016

Contributor

Yes, the OpenSolaris distributions have not been tested during development. Detecting them and handling them correctly should be easy to implement.

How to identify OpenSolaris distributions: https://gist.github.com/natefoo/7af6f3d47bb008669467

Contributor

johanwiren commented Jun 14, 2016

Yes, the OpenSolaris distributions have not been tested during development. Detecting them and handling them correctly should be easy to implement.

How to identify OpenSolaris distributions: https://gist.github.com/natefoo/7af6f3d47bb008669467

@xen0l

This comment has been minimized.

Show comment
Hide comment
@xen0l

xen0l Jun 14, 2016

Contributor

@bcoca: do you know which commit broke the BSD and Linux support?

Contributor

xen0l commented Jun 14, 2016

@bcoca: do you know which commit broke the BSD and Linux support?

@xen0l

This comment has been minimized.

Show comment
Hide comment
@xen0l

xen0l Jun 14, 2016

Contributor

@johanwiren: uname way doesn't apply to illumos distributions any longer. I am also thinking about adding a function, check_is_solaris, in which I open /etc/release and check for Oracle Solaris or Solaris (for Solaris 10) and set set.is_solaris. In the probmelatic fuction, check_enhanced_sharing, I shall decide based on self.is_solaris. What do you think?

Contributor

xen0l commented Jun 14, 2016

@johanwiren: uname way doesn't apply to illumos distributions any longer. I am also thinking about adding a function, check_is_solaris, in which I open /etc/release and check for Oracle Solaris or Solaris (for Solaris 10) and set set.is_solaris. In the probmelatic fuction, check_enhanced_sharing, I shall decide based on self.is_solaris. What do you think?

@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca Jun 14, 2016

Member

@xen0l, sorry I was unclear, I was not saying it is broken on those platforms, I was trying to indicate that a fix that changes version detection to 'Solaris' detection is not a viable solution.

Member

bcoca commented Jun 14, 2016

@xen0l, sorry I was unclear, I was not saying it is broken on those platforms, I was trying to indicate that a fix that changes version detection to 'Solaris' detection is not a viable solution.

@xen0l

This comment has been minimized.

Show comment
Hide comment
@xen0l

xen0l Jun 14, 2016

Contributor

@bcoca what's the preferred fix then?

Contributor

xen0l commented Jun 14, 2016

@bcoca what's the preferred fix then?

@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca Jun 14, 2016

Member

I'm not sure, I don't know what version detection is controlling nor how/what interactions would change with openzfs, just that detecting 'solaris' is not a fix.

Member

bcoca commented Jun 14, 2016

I'm not sure, I don't know what version detection is controlling nor how/what interactions would change with openzfs, just that detecting 'solaris' is not a fix.

@xen0l

This comment has been minimized.

Show comment
Hide comment
@xen0l

xen0l Jun 14, 2016

Contributor

Well, the Oracle ZFS has different feature set than OpenZFS. Oracle ZFS can run only on Oracle Solaris and detecting Oracle Solaris would help here. I'm afraid it's only viable solution. If the module stays general and doesn't harcode Oracle ZFS-centric features, we are good enough. Another option might be new module, but that will duplicate functionality. I'd take the solution I described earlier.

Contributor

xen0l commented Jun 14, 2016

Well, the Oracle ZFS has different feature set than OpenZFS. Oracle ZFS can run only on Oracle Solaris and detecting Oracle Solaris would help here. I'm afraid it's only viable solution. If the module stays general and doesn't harcode Oracle ZFS-centric features, we are good enough. Another option might be new module, but that will duplicate functionality. I'd take the solution I described earlier.

@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca Jun 15, 2016

Member

That does not seem to be a correct assumption, as this module successfully ran on Linux and BSDs which are not using the Oracle ZFS implementation nor OpenZFS.

Member

bcoca commented Jun 15, 2016

That does not seem to be a correct assumption, as this module successfully ran on Linux and BSDs which are not using the Oracle ZFS implementation nor OpenZFS.

@xen0l

This comment has been minimized.

Show comment
Hide comment
@xen0l

xen0l Jun 15, 2016

Contributor

@bcoca FreeBSD and Linux uses OpenZFS. Developers from those platforms collaborate on OpenZFS. OpenZFS just has illumos distributions as upstream. I found another solution and that is that OpenZFS reports different version "-" from Oracle Solaris. So, the question here is about not detecting Solaris, but detecting OpenZFS.

Contributor

xen0l commented Jun 15, 2016

@bcoca FreeBSD and Linux uses OpenZFS. Developers from those platforms collaborate on OpenZFS. OpenZFS just has illumos distributions as upstream. I found another solution and that is that OpenZFS reports different version "-" from Oracle Solaris. So, the question here is about not detecting Solaris, but detecting OpenZFS.

@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca Jun 15, 2016

Member

FreeBSD had it's own implementation until recently, not all FreeBSD users run the latest, also it is not the only BSD, the same goes for linux, it has MANY implementations and many of them are in production.

OpenZFS is a relatively new consolidation of the efforts, I'm all for supporting it, but not for sacrificing existing installed systems in the process.

Member

bcoca commented Jun 15, 2016

FreeBSD had it's own implementation until recently, not all FreeBSD users run the latest, also it is not the only BSD, the same goes for linux, it has MANY implementations and many of them are in production.

OpenZFS is a relatively new consolidation of the efforts, I'm all for supporting it, but not for sacrificing existing installed systems in the process.

@xen0l

This comment has been minimized.

Show comment
Hide comment
@xen0l

xen0l Jun 15, 2016

Contributor

The change I am talking about will not affect those installations. The problem here is with detecting enhanced sharing properties, which is available since version 34 and applies only to Oracle Solaris. So, if not detecting OpenZFS vs Oracle ZFS, then detecting Solaris is the only way how to enable support on illumos platforms. So, are you OK with such solution then? If not, can you advise better approach?

Contributor

xen0l commented Jun 15, 2016

The change I am talking about will not affect those installations. The problem here is with detecting enhanced sharing properties, which is available since version 34 and applies only to Oracle Solaris. So, if not detecting OpenZFS vs Oracle ZFS, then detecting Solaris is the only way how to enable support on illumos platforms. So, are you OK with such solution then? If not, can you advise better approach?

@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca Jun 15, 2016

Member

that does not sound right as the other implementations we discussed above would fall on the current behavior but you are detecting specifically against 'Solaris' to avoid it. I suggest detecing 'OpenZFS' instead as it seems to be the outlier in this case.

Member

bcoca commented Jun 15, 2016

that does not sound right as the other implementations we discussed above would fall on the current behavior but you are detecting specifically against 'Solaris' to avoid it. I suggest detecing 'OpenZFS' instead as it seems to be the outlier in this case.

@olbohlen

This comment has been minimized.

Show comment
Hide comment
@olbohlen

olbohlen Jul 26, 2016

a very quick hack of course is in zfs.py:

        version = out.splitlines()[-1].split()[2]
        if version == '-':
           return False
        if int(version) >= 34:
            return True
    return False

this will bail out if the pool version is matched to a string "-"...

a very quick hack of course is in zfs.py:

        version = out.splitlines()[-1].split()[2]
        if version == '-':
           return False
        if int(version) >= 34:
            return True
    return False

this will bail out if the pool version is matched to a string "-"...

@xen0l xen0l changed the title from zfs module fails to create datasets on illumos distributions to zfs module doesn't support management of OpenZFS pools Jul 31, 2016

@xen0l

This comment has been minimized.

Show comment
Hide comment
@xen0l

xen0l Jul 31, 2016

Contributor

The fix is located at #2642. OpenZFS used to use version 5000 and uses "-" now. I added both checks.

Contributor

xen0l commented Jul 31, 2016

The fix is located at #2642. OpenZFS used to use version 5000 and uses "-" now. I added both checks.

@xen0l

This comment has been minimized.

Show comment
Hide comment
@xen0l

xen0l Aug 4, 2016

Contributor
Contributor

xen0l commented Aug 4, 2016

@johanwiren

This comment has been minimized.

Show comment
Hide comment
@johanwiren

johanwiren Aug 4, 2016

Contributor

Sorry, vacation mode here. Will have a look this evening.

Contributor

johanwiren commented Aug 4, 2016

Sorry, vacation mode here. Will have a look this evening.

@johanwiren

This comment has been minimized.

Show comment
Hide comment
@johanwiren

johanwiren Aug 7, 2016

Contributor

#2642 looks OK to me. @bcoca, can you mark it ship_it if you approve of the change too.

Contributor

johanwiren commented Aug 7, 2016

#2642 looks OK to me. @bcoca, can you mark it ship_it if you approve of the change too.

@bcoca

This comment has been minimized.

Show comment
Hide comment
@bcoca

bcoca Aug 8, 2016

Member

closing via merged #2642

Member

bcoca commented Aug 8, 2016

closing via merged #2642

@bcoca bcoca closed this Aug 8, 2016

@dcj

This comment has been minimized.

Show comment
Hide comment
@dcj

dcj Sep 30, 2016

This doesn't seemed fixed to me in ansible 2.1.2.0

An exception occurred during task execution. The full traceback is:
Traceback (most recent call last):
  File "/tmp/ansible_9JcUwu/ansible_module_zfs.py", line 246, in <module>
    main()
  File "/tmp/ansible_9JcUwu/ansible_module_zfs.py", line 228, in main
    zfs = Zfs(module, name, properties)
  File "/tmp/ansible_9JcUwu/ansible_module_zfs.py", line 90, in __init__
    self.enhanced_sharing = self.check_enhanced_sharing()
  File "/tmp/ansible_9JcUwu/ansible_module_zfs.py", line 99, in check_enhanced_sharing
    if int(version) >= 34:
ValueError: invalid literal for int() with base 10: '-'

dcj commented Sep 30, 2016

This doesn't seemed fixed to me in ansible 2.1.2.0

An exception occurred during task execution. The full traceback is:
Traceback (most recent call last):
  File "/tmp/ansible_9JcUwu/ansible_module_zfs.py", line 246, in <module>
    main()
  File "/tmp/ansible_9JcUwu/ansible_module_zfs.py", line 228, in main
    zfs = Zfs(module, name, properties)
  File "/tmp/ansible_9JcUwu/ansible_module_zfs.py", line 90, in __init__
    self.enhanced_sharing = self.check_enhanced_sharing()
  File "/tmp/ansible_9JcUwu/ansible_module_zfs.py", line 99, in check_enhanced_sharing
    if int(version) >= 34:
ValueError: invalid literal for int() with base 10: '-'
@xen0l

This comment has been minimized.

Show comment
Hide comment
@xen0l

xen0l Sep 30, 2016

Contributor

@dcj, can you be more specific?

Contributor

xen0l commented Sep 30, 2016

@dcj, can you be more specific?

@dcj

This comment has been minimized.

Show comment
Hide comment
@dcj

dcj Sep 30, 2016

The target machine is SmartOS, build 20160929T025934Z

   - name: Create zfs datasets
      zfs:
        name: zones/images
        state: present

@xen0l, what else would you like to know?

dcj commented Sep 30, 2016

The target machine is SmartOS, build 20160929T025934Z

   - name: Create zfs datasets
      zfs:
        name: zones/images
        state: present

@xen0l, what else would you like to know?

@xen0l

This comment has been minimized.

Show comment
Hide comment
@xen0l

xen0l Sep 30, 2016

Contributor

Well, it seems that fix is not yet available in 2.1.2.

Contributor

xen0l commented Sep 30, 2016

Well, it seems that fix is not yet available in 2.1.2.

@dcj

This comment has been minimized.

Show comment
Hide comment
@dcj

dcj Sep 30, 2016

@xen0l OK, thanks for the info. I have hacked your fix info my module path for now. When I read that the fix was merged in early August, I thought it must have been included in a late-September release...

dcj commented Sep 30, 2016

@xen0l OK, thanks for the info. I have hacked your fix info my module path for now. When I read that the fix was merged in early August, I thought it must have been included in a late-September release...

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.