Missing parameters

[dwlehman]

This changes the behavior of the role such that omitted parameters do not change settings on existing pools and volumes. Omitted parameters on new pools and volumes will be given the default value, while the same on existing pools and volumes will leave whatever the current setting is intact.

[dwlehman]

I see there are CI failures. I'll take a look on Monday.

[pcahyna]

@dwlehman thanks, but I would prefer to wait with this for further user feedback, as the discussion in #48 shows, it is not clear to what extent it is desirable or not.

[dwlehman]

I only looked over it briefly, but from what I gathered everyone there is in agreement that we should not change settings for existing volumes because of omitted parameters.

[dwlehman]

I just rebased this and added the commit allowing lookup of an existing pool by name only. I ran one random integration test and it passed, so I'm waiting to see what CI turns up.

[dwlehman]

To Do:

• type-specific parameters (lvm, md, luks)
• docs

[richm]

lgtm

[richm]

please use file: "{{ playbook_dir }}/roles/linux-system-roles.storage/defaults/main.yml"

[richm]

please use name: linux-system-roles.storage

[richm]

please use name: linux-system-roles.storage

[richm]

please use name: linux-system-roles.storage

[richm]

please use name: linux-system-roles.storage

[richm]

please use name: linux-system-roles.storage

[richm]

please use name: linux-system-roles.storage

[lgtm-com]

This pull request introduces 1 alert when merging 51edee7 into e990c1f - view on LGTM.com

new alerts:

• 1 for Syntax error

[richm]

@jharuda can this be now changed to just versions: all?

[richm]

[citest]

[dwlehman]

Note to self, since I'm taking Friday off:

There are two persistent CI failures that I have been unable to reproduce locally, both of which appear to be triggered by the CI infrastructure's slowness:

1. udevd is failing to pick up the label after the mkfs; e2label sequence, which might be resolved using flock in blivet (tests_existing_pool.yml on f32)
2. some devices/filesystems are active on disks, causing a parted commit failure (tests_luks_pool.yml on rhel-7)

I can investigate blivet mitigation for failure 1, but I'll have to figure out how to examine the blivet log from the actual ci run in order to diagnose failure 2.

[dwlehman]

[citest]

[dwlehman]

It's worth noting that this is a pretty big behavioral change, and probably merits a major version bump. @pcahyna @richm thoughts on that?

[richm]

It's worth noting that this is a pretty big behavioral change, and probably merits a major version bump. @pcahyna @richm thoughts on that?

Are there changes which will break the API e.g. if a customer has playbooks that work with the existing role, will those playbooks continue to work the exact same way if they update the role?
If there are breaking changes, we will need to bump the major version - and also figure out a way to communicate this to users of the role so that they can modify their playbooks and/or inventories, and justify such a change.

[richm]

Can you use service_facts instead? https://docs.ansible.com/ansible/2.9/modules/service_facts_module.html#service-facts-module

[dwlehman]

Changed to use service_facts in latest version. Bonus fun ensued related to json_query.

[richm]

If you want to check the result of the failure in tests, you'll have to "re-raise" the error - see https://richm.github.io/how-to-catch-and-reraise-errors-in-ansible

[dwlehman]

Fixed in latest version, although tests were passing as it was.

[richm]

Fixed in latest version, although tests were passing as it was.

Then you probably aren't trying to rescue and check the error in a calling context.

[dwlehman]

It's worth noting that this is a pretty big behavioral change, and probably merits a major version bump. @pcahyna @richm thoughts on that?

Are there changes which will break the API e.g. if a customer has playbooks that work with the existing role, will those playbooks continue to work the exact same way if they update the role?

No, the behavior will change when they run the role against pre-existing pools or volumes with any parameters omitted. It seems clear that the new behavior is "better" (don't change anything unless explicitly told to), but it's definitely different.

If there are breaking changes, we will need to bump the major version - and also figure out a way to communicate this to users of the role so that they can modify their playbooks and/or inventories, and justify such a change.

As I said above, the new behavior is less disruptive/destructive, but it's still different.

[richm]

As I said above, the new behavior is less disruptive/destructive, but it's still different.

Will a playbook run against the new role report any differences at all?

• run playbook against old role
• upgrade role
• run playbook again with new role and no other changes

Will the playbook run report changed? That is, will the upgrade break idempotency (assuming the role is currently idempotent)? If so, then this suggests to me a major version change.

[dwlehman]

As I said above, the new behavior is less disruptive/destructive, but it's still different.

Will a playbook run against the new role report any differences at all?

* run playbook against old role

* upgrade role

* run playbook again with new role and no other changes

Will the playbook run report changed? That is, will the upgrade break idempotency (assuming the role is currently idempotent)? If so, then this suggests to me a major version change.

No, it should not report 'changed' in that case.

[richm]

No, it should not report 'changed' in that case.

Then IMO a major version change is not needed, only a minor version change.

[richm]

Here is a way to do it without json_query:

    storage_cryptsetup_services: "{{
      ansible_facts.services | dict2items | selectattr('key', 'match', '^systemd-cryptsetup@') |
      map(attribute='value') | map(attribute='name') | list
    }}"

however, on fedora 33, this gives me a value like this: ['systemd-cryptsetup@luks\\x2d7fa23e85\\x2d4729\\x2d43b2\\x2dcc7a\\x2d9608d8cdb732.service']
not sure if that is the same result of using json_query

[dwlehman]

Someone came in a while back and switched all of our selectattr usage to json_query since selectattr doesn't work with Jinja versions < 2.8. You can see the comment, look at blame, etc. in storage/tests/test-verify-volume-mount.yml.

[richm]

Someone came in a while back and switched all of our selectattr usage to json_query since selectattr doesn't work with Jinja versions < 2.8. You can see the comment, look at blame, etc. in storage/tests/test-verify-volume-mount.yml.

Mea culpa. That was based on my incomplete understanding of the problems of selectattr and json_query. The problem with jinja27 is not selectattr in general, it is that certain filters are not available - specifically, the equalto, equal, == filters to test if a value in an object is equal to some given value.

We have just completed converting all of the system roles to work with jinja27, and we figured out how to get around this limitation, by using the match filter instead of equalto or == - linux-system-roles/logging@48d21ef#diff-fe77ca1acb10f971c92988a8aed5bb5ae631443b1a20668629ccc846c608205aL11-L12

In the case of this PR, we have to use match anyway to look for services beginning with systemd-cryptsetup@.

In addition, I have verified the code above works with ansible 2.8/jinja2.7/centos7 controller host.

[richm]

We have CI tests for ansible 2.8/jinja2.7/centos7 controller host - the statuses like */ansible-2.8 - so you can know if you make a change that breaks in this case.

[richm]

However, all of that being said, if you prefer to keep the json_query implementation, that's fine with me.

[dwlehman]

I'm happy with it as it is now if you are. No need to fiddle with it any more.

[dwlehman]

[citest bad]

[richm]

Don't worry about the (staging) failures. All of the required tests have passed.

[richm]

The rhel-x/ansible-2.9 fails because of this:

TASK [Read the /etc/crypttab file] *********************************************
task path: /tmp/tmpppms2nvo/tests/verify-role-results.yml:24
fatal: [/cache/rhel-x.qcow2]: FAILED! => {"changed": false, "cmd": ["cat", "/etc/crypttab"], "delta": "0:00:00.002482", "end": "2021-02-08 20:39:44.912470", "msg": "non-zero return code", "rc": 1, "start": "2021-02-08 20:39:44.909988", "stderr": "cat: /etc/crypttab: No such file or directory", "stderr_lines": ["cat: /etc/crypttab: No such file or directory"], "stdout": "", "stdout_lines": []}

rhel-x uses the internal rhel 8.4.0 snapshot 1 - which is quite old by now - we are in the process of upgrading it - is it possible that this test uses a component that had a bug in this older snapshot, or the test uses a component in rhel 8.4 that is in a later version of 8.4?

[richm]

[citest bad]

[richm]

lgtm

[pcahyna]

rhel-x uses the internal rhel 8.4.0 snapshot 1 - which is quite old by now - we are in the process of upgrading it - is it possible that this test uses a component that had a bug in this older snapshot, or the test uses a component in rhel 8.4 that is in a later version of 8.4?

No, crypttab is still missing when testing against the latest nightly snapshot.