ec2_vol: backport fix incorrectly returned changed result #486

lowlydba · 2021-08-31T18:36:19Z

SUMMARY

Backport version of #483

When modify_volume is used but no new disk is being attached, the module incorrectly reports that no change has occurred even when disks have been modified (iops, throughput, type, etc.). This is due to the attach function overwriting the changed variable even if no disks are attached.

Fixes #482

The integration test has been fixed so that when the gp3 modifications are tested, the volume is already in an attached state (previously detached) when reporting back changed. The detach tests are moved further down now, allowing this case to be properly covered by the existing assert:

amazon.aws/tests/integration/targets/ec2_vol/tasks/tests.yml

Lines 384 to 387 in e8df917

    
           - name: check that volume_type has changed 
        
             assert: 
        
               that: 
        
                 - changed_gp3_volume.changed

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

ec2_vol

Depends-On: #491

ansibullbot · 2021-08-31T18:38:33Z

cc @jillr @lwade @s-hertel @tremble @wimnat
click here for bot help

lowlydba · 2021-08-31T19:14:20Z

build-ansible-collection : ERROR Project github.com/ansible-collections/community.general does not have the default branch master in 16s

tremble

While testing #485 I discovered that the tests were disabled and there's a slight bug here I've added the fix I used.

plugins/modules/ec2_vol.py

tremble · 2021-09-02T07:54:54Z

recheck

lowlydba · 2021-09-02T14:11:52Z

recheck

lowlydba · 2021-09-02T15:31:15Z

1 failed pylint check, all on files not touched in this PR:

ERROR: Found 5 pylint issue(s) which need to be resolved:
ERROR: plugins/inventory/aws_ec2.py:474:23: use-a-generator: Use a generator instead 'any(f['Name'] == 'instance-state-name' for f in filters)'
ERROR: plugins/modules/ec2_group.py:1316:23: use-a-generator: Use a generator instead 'any(rule_cmp(p, b) for b in named_tuple_ingress_list)'
ERROR: plugins/modules/ec2_group.py:1329:27: use-a-generator: Use a generator instead 'any(rule_cmp(p, b) for b in named_tuple_egress_list)'
ERROR: tests/unit/mock/loader.py:49:4: arguments-renamed: Parameter 'file_name' has been renamed to 'path' in overridden 'DictDataLoader._get_file_contents' method
ERROR: tests/unit/module_utils/test_elbv2.py:10:0: consider-using-from-import: Use 'from ansible_collections.amazon.aws.plugins.module_utils import elbv2' instead

alinabuzachis

@lowlydba Thank you for taking time to work on this.

tremble · 2021-09-06T19:07:39Z

recheck

lowlydba · 2021-09-07T17:50:19Z

recheck

briantist · 2021-09-07T18:04:26Z

@lowlydba I didn't see what error was there before but FYI there's a widespread issue with 2.9 sanity checks after a setuptools release today: ansible/ansible#75647

lowlydba · 2021-09-07T21:23:35Z

recheck

lowlydba · 2021-09-08T13:29:03Z

recheck

Co-authored-by: Mark Chappell <[email protected]>

lowlydba · 2021-09-08T19:02:21Z

recheck

Only error was a timeout in network-ee-build-container-image-stable-2.11

lowlydba · 2021-09-08T21:10:59Z

@tremble should be ready to be gated! 🎉

ansible-zuul

LGTM!

jillr · 2021-09-08T22:42:02Z

Thanks for your patience, and this PR @lowlydba!

alinabuzachis · 2021-09-09T10:34:07Z

The integrations tests seems failing:

TASK [ec2_vol : create another volume (override module defaults)] **************
task path: /home/zuul/.ansible/collections/ansible_collections/amazon/aws/tests/integration/targets/ec2_vol/tasks/tests.yml:83
<testhost> ESTABLISH LOCAL CONNECTION FOR USER: zuul
<testhost> EXEC /bin/sh -c 'echo ~zuul && sleep 0'
<testhost> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /home/zuul/.ansible/tmp `"&& mkdir "` echo /home/zuul/.ansible/tmp/ansible-tmp-1631142317.5069387-6003-177609821038826 `" && echo ansible-tmp-1631142317.5069387-6003-177609821038826="` echo /home/zuul/.ansible/tmp/ansible-tmp-1631142317.5069387-6003-177609821038826 `" ) && sleep 0'
Using module file /home/zuul/.ansible/collections/ansible_collections/amazon/aws/plugins/modules/ec2_vol.py
<testhost> PUT /home/zuul/.ansible/tmp/ansible-local-5667j9lfiknj/tmpwseinjhj TO /home/zuul/.ansible/tmp/ansible-tmp-1631142317.5069387-6003-177609821038826/AnsiballZ_ec2_vol.py
<testhost> EXEC /bin/sh -c 'chmod u+x /home/zuul/.ansible/tmp/ansible-tmp-1631142317.5069387-6003-177609821038826/ /home/zuul/.ansible/tmp/ansible-tmp-1631142317.5069387-6003-177609821038826/AnsiballZ_ec2_vol.py && sleep 0'
<testhost> EXEC /bin/sh -c 'ANSIBLE_DEBUG_BOTOCORE_LOGS=True /tmp/ansible.asunum72.test/virtualenv/bin/python /home/zuul/.ansible/tmp/ansible-tmp-1631142317.5069387-6003-177609821038826/AnsiballZ_ec2_vol.py && sleep 0'
<testhost> EXEC /bin/sh -c 'rm -f -r /home/zuul/.ansible/tmp/ansible-tmp-1631142317.5069387-6003-177609821038826/ > /dev/null 2>&1 && sleep 0'
ok: [testhost] => {
    "changed": false,
    "device": null,
    "invocation": {
        "module_args": {
            "aws_access_key": "ASIA6CCDWXDOJMOBWF46",
            "aws_ca_bundle": null,
            "aws_config": null,
            "aws_secret_key": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER",
            "debug_botocore_endpoint_logs": true,
            "delete_on_termination": false,
            "device_name": null,
            "ec2_url": null,
            "encrypted": true,
            "id": null,
            "instance": null,
            "iops": 101,
            "kms_key_id": null,
            "modify_volume": false,
            "name": "ansible-test-72282102-host-74-63-204-98",
            "profile": null,
            "purge_tags": false,
            "region": "us-east-1",
            "security_token": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER",
            "snapshot": null,
            "state": "present",
            "tags": {
                "Name": "ansible-test-72282102-host-74-63-204-98",
                "ResourcePrefix": "ansible-test-72282102-host-74-63-204-98"
            },
            "throughput": null,
            "validate_certs": true,
            "volume_size": 4,
            "volume_type": "io1",
            "zone": "us-east-1a"
        }
    },
    "resource_actions": [
        "ec2:DescribeTags",
        "ec2:DescribeVolumes"
    ],
    "volume": {
        "attachment_set": {
            "attach_time": null,
            "deleteOnTermination": null,
            "device": null,
            "instance_id": null,
            "status": null
        },
        "create_time": "2021-09-08T22:59:20.412000+00:00",
        "encrypted": true,
        "id": "vol-05f99b3a216a999f1",
        "iops": 101,
        "size": 4,
        "snapshot_id": "",
        "status": "available",
        "tags": {
            "Name": "ansible-test-72282102-host-74-63-204-98",
            "ResourcePrefix": "ansible-test-72282102-host-74-63-204-98"
        },
        "throughput": null,
        "type": "io1",
        "zone": "us-east-1a"
    },
    "volume_id": "vol-05f99b3a216a999f1",
    "volume_type": "io1"
}

TASK [ec2_vol : check task return attributes] **********************************
task path: /home/zuul/.ansible/collections/ansible_collections/amazon/aws/tests/integration/targets/ec2_vol/tasks/tests.yml:95
fatal: [testhost]: FAILED! => {
    "assertion": "volume2.changed",
    "changed": false,
    "evaluated_to": false,
    "msg": "Assertion failed"
}

lowlydba · 2021-09-09T12:39:48Z

Some of the previous check runs also seemed a bit flaky w/r/t timeouts or slow attachments of volumes (the volume being in "attaching" state instead of "attached", for example). I think the root cause is actually this one:

4678 | 2021-09-08 23:03:19.172143 \| fedora-34 \| task path: /home/zuul/.ansible/collections/ansible_collections/amazon/aws/tests/integration/targets/ec2_vol/tasks/tests.yml:194
4679 | 2021-09-08 23:03:19.253039 \| fedora-34 \| <testhost> ESTABLISH LOCAL CONNECTION FOR USER: zuul
4680 | 2021-09-08 23:03:19.254495 \| fedora-34 \| <testhost> EXEC /bin/sh -c 'echo ~zuul && sleep 0'
4681 | 2021-09-08 23:03:19.268196 \| fedora-34 \| <testhost> EXEC /bin/sh -c '( umask 77 && mkdir -p "` echo /home/zuul/.ansible/tmp `"&& mkdir "` echo /home/zuul/.ansible/tmp/ansible-tmp-1631142199.2675009-5506-252597966489249 `" && echo ansible-tmp-1631142199.2675009-5506-252597966489249="` echo /home/zuul/.ansible/tmp/ansible-tmp-1631142199.2675009-5506-252597966489249 `" ) && sleep 0'
4682 | 2021-09-08 23:03:19.296749 \| fedora-34 \| Using module file /home/zuul/.ansible/collections/ansible_collections/amazon/aws/plugins/modules/ec2_vol.py
4683 | 2021-09-08 23:03:19.299033 \| fedora-34 \| <testhost> PUT /home/zuul/.ansible/tmp/ansible-local-4728f1f7jgd0/tmp4gi7h_r0 TO /home/zuul/.ansible/tmp/ansible-tmp-1631142199.2675009-5506-252597966489249/AnsiballZ_ec2_vol.py
4684 | 2021-09-08 23:03:19.300835 \| fedora-34 \| <testhost> EXEC /bin/sh -c 'chmod u+x /home/zuul/.ansible/tmp/ansible-tmp-1631142199.2675009-5506-252597966489249/ /home/zuul/.ansible/tmp/ansible-tmp-1631142199.2675009-5506-252597966489249/AnsiballZ_ec2_vol.py && sleep 0'
4685 | 2021-09-08 23:03:19.314609 \| fedora-34 \| <testhost> EXEC /bin/sh -c 'ANSIBLE_DEBUG_BOTOCORE_LOGS=True /tmp/ansible.gvj6noxo.test/virtualenv/bin/python /home/zuul/.ansible/tmp/ansible-tmp-1631142199.2675009-5506-252597966489249/AnsiballZ_ec2_vol.py && sleep 0'
4686 | 2021-09-08 23:03:21.209369 \| fedora-34 \| <testhost> EXEC /bin/sh -c 'rm -f -r /home/zuul/.ansible/tmp/ansible-tmp-1631142199.2675009-5506-252597966489249/ > /dev/null 2>&1 && sleep 0'
4687 | 2021-09-08 23:03:21.231787 \| fedora-34 \| ok: [testhost] => {
4688 | 2021-09-08 23:03:21.231860 \| fedora-34 \|     "changed": false,
4689 | 2021-09-08 23:03:21.231876 \| fedora-34 \|     "device": "/dev/sdg",
4690 | 2021-09-08 23:03:21.231889 \| fedora-34 \|     "invocation": {
4691 | 2021-09-08 23:03:21.231911 \| fedora-34 \|         "module_args": {
4692 | 2021-09-08 23:03:21.231939 \| fedora-34 \|             "aws_access_key": "ASIA6CCDWXDOJMOBWF46",
4693 | 2021-09-08 23:03:21.231958 \| fedora-34 \|             "aws_ca_bundle": null,
4694 | 2021-09-08 23:03:21.231972 \| fedora-34 \|             "aws_config": null,
4695 | 2021-09-08 23:03:21.231985 \| fedora-34 \|             "aws_secret_key": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER",
4696 | 2021-09-08 23:03:21.231999 \| fedora-34 \|             "debug_botocore_endpoint_logs": true,
4697 | 2021-09-08 23:03:21.232011 \| fedora-34 \|             "delete_on_termination": false,
4698 | 2021-09-08 23:03:21.232024 \| fedora-34 \|             "device_name": "/dev/sdg",
4699 | 2021-09-08 23:03:21.232037 \| fedora-34 \|             "ec2_url": null,
4700 | 2021-09-08 23:03:21.232049 \| fedora-34 \|             "encrypted": false,
4701 | 2021-09-08 23:03:21.232062 \| fedora-34 \|             "id": "vol-0a970c967879e6c06",
4702 | 2021-09-08 23:03:21.232075 \| fedora-34 \|             "instance": "i-0209f36a3290950f5",
4703 | 2021-09-08 23:03:21.232087 \| fedora-34 \|             "iops": null,
4704 | 2021-09-08 23:03:21.232100 \| fedora-34 \|             "kms_key_id": null,
4705 | 2021-09-08 23:03:21.232113 \| fedora-34 \|             "modify_volume": false,
4706 | 2021-09-08 23:03:21.232125 \| fedora-34 \|             "name": null,
4707 | 2021-09-08 23:03:21.232138 \| fedora-34 \|             "profile": null,
4708 | 2021-09-08 23:03:21.232150 \| fedora-34 \|             "purge_tags": false,
4709 | 2021-09-08 23:03:21.232181 \| fedora-34 \|             "region": "us-east-1",
4710 | 2021-09-08 23:03:21.232196 \| fedora-34 \|             "security_token": "VALUE_SPECIFIED_IN_NO_LOG_PARAMETER",
4711 | 2021-09-08 23:03:21.232209 \| fedora-34 \|             "snapshot": null,
4712 | 2021-09-08 23:03:21.232221 \| fedora-34 \|             "state": "present",
4713 | 2021-09-08 23:03:21.232234 \| fedora-34 \|             "tags": {},
4714 | 2021-09-08 23:03:21.232247 \| fedora-34 \|             "throughput": null,
4715 | 2021-09-08 23:03:21.232260 \| fedora-34 \|             "validate_certs": true,
4716 | 2021-09-08 23:03:21.232273 \| fedora-34 \|             "volume_size": null,
4717 | 2021-09-08 23:03:21.232285 \| fedora-34 \|             "volume_type": "standard",
4718 | 2021-09-08 23:03:21.232298 \| fedora-34 \|             "zone": null
4719 | 2021-09-08 23:03:21.232311 \| fedora-34 \|         }
4720 | 2021-09-08 23:03:21.232323 \| fedora-34 \|     },
4721 | 2021-09-08 23:03:21.232336 \| fedora-34 \|     "resource_actions": [
4722 | 2021-09-08 23:03:21.232371 \| fedora-34 \|         "ec2:DescribeVolumes",
4723 | 2021-09-08 23:03:21.232385 \| fedora-34 \|         "ec2:DescribeTags",
4724 | 2021-09-08 23:03:21.232398 \| fedora-34 \|         "ec2:DescribeInstances"
4725 | 2021-09-08 23:03:21.232410 \| fedora-34 \|     ],
4726 | 2021-09-08 23:03:21.232423 \| fedora-34 \|     "volume": {
4727 | 2021-09-08 23:03:21.232435 \| fedora-34 \|         "attachment_set": {
4728 | 2021-09-08 23:03:21.232447 \| fedora-34 \|             "attach_time": "2021-09-08T23:03:18+00:00",
4729 | 2021-09-08 23:03:21.232460 \| fedora-34 \|             "deleteOnTermination": false,
4730 | 2021-09-08 23:03:21.232473 \| fedora-34 \|             "device": "/dev/sdg",
4731 | 2021-09-08 23:03:21.232485 \| fedora-34 \|             "instance_id": "i-0209f36a3290950f5",
4732 | 2021-09-08 23:03:21.232497 \| fedora-34 \|             "status": "attaching"
4733 | 2021-09-08 23:03:21.232510 \| fedora-34 \|         },
4734 | 2021-09-08 23:03:21.232522 \| fedora-34 \|         "create_time": "2021-09-08T22:59:02.093000+00:00",
4735 | 2021-09-08 23:03:21.232535 \| fedora-34 \|         "encrypted": false,
4736 | 2021-09-08 23:03:21.232547 \| fedora-34 \|         "id": "vol-0a970c967879e6c06",
4737 | 2021-09-08 23:03:21.232560 \| fedora-34 \|         "iops": null,
4738 | 2021-09-08 23:03:21.232572 \| fedora-34 \|         "size": 1,
4739 | 2021-09-08 23:03:21.232585 \| fedora-34 \|         "snapshot_id": "",
4740 | 2021-09-08 23:03:21.232597 \| fedora-34 \|         "status": "in-use",
4741 | 2021-09-08 23:03:21.232610 \| fedora-34 \|         "tags": {
4742 | 2021-09-08 23:03:21.232622 \| fedora-34 \|             "ResourcePrefix": "ansible-test-72282102-host-74-63-204-98"
4743 | 2021-09-08 23:03:21.232635 \| fedora-34 \|         },
4744 | 2021-09-08 23:03:21.232647 \| fedora-34 \|         "throughput": null,
4745 | 2021-09-08 23:03:21.232659 \| fedora-34 \|         "type": "standard",
4746 | 2021-09-08 23:03:21.232672 \| fedora-34 \|         "zone": "us-east-1a"
4747 | 2021-09-08 23:03:21.232684 \| fedora-34 \|     },
4748 | 2021-09-08 23:03:21.232697 \| fedora-34 \|     "volume_id": "vol-0a970c967879e6c06",
4749 | 2021-09-08 23:03:21.232709 \| fedora-34 \|     "volume_type": "standard"
4750 | 2021-09-08 23:03:21.232722 \| fedora-34 \| }
4751 | 2021-09-08 23:03:21.252323 \| fedora-34 \|
4752 | 2021-09-08 23:03:21.252425 \| fedora-34 \| TASK [ec2_vol : check task return attributes] **********************************
4753 | 2021-09-08 23:03:21.252761 \| fedora-34 \| task path: /home/zuul/.ansible/collections/ansible_collections/amazon/aws/tests/integration/targets/ec2_vol/tasks/tests.yml:202
4754 | 2021-09-08 23:03:21.342738 \| fedora-34 \| fatal: [testhost]: FAILED! => {
4755 | 2021-09-08 23:03:21.342817 \| fedora-34 \|     "assertion": "vol_attach_result.volume.attachment_set.status == 'attached'",
4756 | 2021-09-08 23:03:21.342834 \| fedora-34 \|     "changed": false,
4757 | 2021-09-08 23:03:21.342848 \| fedora-34 \|     "evaluated_to": false,
4758 | 2021-09-08 23:03:21.342861 \| fedora-34 \|     "msg": "Assertion failed"

tremble · 2021-09-09T12:46:18Z

Gate and Check are the same tests, the difference is that check runs when you submit the code, so can suffer from bit-rot, gate checks that your code still works against the latest code base and then immediately merges the code, so no further changes happen before your code is merged.

The ec2_vol and ec2_instance checks have historically been flakey (race conditions etc).

lowlydba · 2021-09-09T13:11:01Z

Another flake for the same test I mentioned above. Would adding a pause/retries in between the attaching task and assertion be an OK fix for this flaky piece of the testing? Happy to do it, but don't want to err too far outside of the original scope on this.

I noticed the "Attach volume" test was already set to accept "attaching", "attached" as valid results, so it seems fair to update the "Attach volume (idempotent)" to have the same assertion criteria. I believe this will greatly reduce the flakiness of this particular test.

tremble · 2021-09-09T17:59:26Z

@jillr FYI the tests changes are partial backports from #483 and #371

lowlydba changed the title ~~ec2_vol: backport fix overwritten changed var~~ ec2_vol: backport fix incorrectly returned changed result Aug 31, 2021

ansibullbot added bug This issue/PR relates to a bug community_review integration tests/integration module module needs_triage plugins plugin (any type) tests tests labels Aug 31, 2021

tremble requested changes Sep 1, 2021

View reviewed changes

plugins/modules/ec2_vol.py Outdated Show resolved Hide resolved

plugins/modules/ec2_vol.py Outdated Show resolved Hide resolved

plugins/modules/ec2_vol.py Outdated Show resolved Hide resolved

ansibullbot added needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR community_review and removed community_review needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Sep 1, 2021

lowlydba requested a review from tremble September 1, 2021 22:05

tremble approved these changes Sep 2, 2021

View reviewed changes

ansibullbot removed the needs_triage label Sep 2, 2021

lowlydba requested a review from tremble September 2, 2021 16:06

alinabuzachis reviewed Sep 6, 2021

View reviewed changes

alinabuzachis approved these changes Sep 6, 2021

View reviewed changes

tremble approved these changes Sep 6, 2021

View reviewed changes

lowlydba and others added 6 commits September 8, 2021 13:01

ec2_vol: backport fix overwritten changed var

bf096a3

Update plugins/modules/ec2_vol.py

a7e1810

Co-authored-by: Mark Chappell <[email protected]>

Update plugins/modules/ec2_vol.py

93ae833

Co-authored-by: Mark Chappell <[email protected]>

Update plugins/modules/ec2_vol.py

78bc6b7

Co-authored-by: Mark Chappell <[email protected]>

wait for full attachment

310ce8b

Update ec2_vol.py

59a6f14

jillr added the gate label Sep 8, 2021

ansible-zuul bot approved these changes Sep 8, 2021

View reviewed changes

tremble added gate and removed gate labels Sep 9, 2021

set idempotent assert criteria to match previous assertion

695315a

ansible-zuul bot removed the gate label Sep 9, 2021

tremble added the gate label Sep 9, 2021

ansible-zuul bot merged commit 7f2dc17 into ansible-collections:stable-1.5 Sep 9, 2021

lowlydba deleted the patch-4 branch September 9, 2021 18:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ec2_vol: backport fix incorrectly returned changed result #486

ec2_vol: backport fix incorrectly returned changed result #486

lowlydba commented Aug 31, 2021 •

edited by tremble

Loading

ansibullbot commented Aug 31, 2021

lowlydba commented Aug 31, 2021

tremble left a comment

tremble commented Sep 2, 2021

lowlydba commented Sep 2, 2021

lowlydba commented Sep 2, 2021

alinabuzachis left a comment

tremble commented Sep 6, 2021

lowlydba commented Sep 7, 2021

briantist commented Sep 7, 2021

lowlydba commented Sep 7, 2021

lowlydba commented Sep 8, 2021

lowlydba commented Sep 8, 2021 •

edited

Loading

lowlydba commented Sep 8, 2021

ansible-zuul bot left a comment

jillr commented Sep 8, 2021

alinabuzachis commented Sep 9, 2021

lowlydba commented Sep 9, 2021 •

edited

Loading

tremble commented Sep 9, 2021 •

edited

Loading

lowlydba commented Sep 9, 2021 •

edited

Loading

tremble commented Sep 9, 2021

	- name: check that volume_type has changed
	assert:
	that:
	- changed_gp3_volume.changed

ec2_vol: backport fix incorrectly returned changed result #486

ec2_vol: backport fix incorrectly returned changed result #486

Conversation

lowlydba commented Aug 31, 2021 • edited by tremble Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ansibullbot commented Aug 31, 2021

lowlydba commented Aug 31, 2021

tremble left a comment

Choose a reason for hiding this comment

tremble commented Sep 2, 2021

lowlydba commented Sep 2, 2021

lowlydba commented Sep 2, 2021

alinabuzachis left a comment

Choose a reason for hiding this comment

tremble commented Sep 6, 2021

lowlydba commented Sep 7, 2021

briantist commented Sep 7, 2021

lowlydba commented Sep 7, 2021

lowlydba commented Sep 8, 2021

lowlydba commented Sep 8, 2021 • edited Loading

lowlydba commented Sep 8, 2021

ansible-zuul bot left a comment

Choose a reason for hiding this comment

jillr commented Sep 8, 2021

alinabuzachis commented Sep 9, 2021

lowlydba commented Sep 9, 2021 • edited Loading

tremble commented Sep 9, 2021 • edited Loading

lowlydba commented Sep 9, 2021 • edited Loading

tremble commented Sep 9, 2021

lowlydba commented Aug 31, 2021 •

edited by tremble

Loading

lowlydba commented Sep 8, 2021 •

edited

Loading

lowlydba commented Sep 9, 2021 •

edited

Loading

tremble commented Sep 9, 2021 •

edited

Loading

lowlydba commented Sep 9, 2021 •

edited

Loading