Skip to content
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

Check mode support for aci_rest module #436

Closed
adealdag opened this issue May 23, 2023 · 3 comments · Fixed by #470
Closed

Check mode support for aci_rest module #436

adealdag opened this issue May 23, 2023 · 3 comments · Fixed by #470
Assignees
Labels
enhancement New feature or request

Comments

@adealdag
Copy link

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

Description

Module aci_rest does not work correctly when check mode is used.

  • Task is skipped
  • Module returns message "remote module (cisco.aci.aci_rest) does not support check mode"
  • Nothing is saved in the output_path location

Affected Module Name(s):

  • aci_rest

APIC version and APIC Platform

  • V 5.2.4e on-prem

Collection versions

  • cisco.aci 2.6.0

Output/ Error message

ADEALDAG-M-69WL:aci-ansible-webinar adealdag$ ansible-playbook -i inventory.yaml --vault-pass-file vault.key test.yaml --check -v
Using /etc/ansible/ansible.cfg as config file

PLAY [Create Development Tenant] ***********************************************************************************************************

TASK [Add a tenant using an XML string] ****************************************************************************************************
Tuesday 23 May 2023  16:35:31 +0200 (0:00:00.064)       0:00:00.064 *********** 
skipping: [apic1-mlg1.cisco.com] => {
    "changed": false
}

MSG:

remote module (cisco.aci.aci_rest) does not support check mode

PLAY RECAP *********************************************************************************************************************************
apic1-mlg1.cisco.com       : ok=0    changed=0    unreachable=0    failed=0    skipped=1    rescued=0    ignored=0   

Tuesday 23 May 2023  16:35:32 +0200 (0:00:00.656)       0:00:00.720 *********** 
=============================================================================== 
Add a tenant using an XML string ---------------------------------------------------------------------------------------------------- 0.66s

Expected Behavior

  • aci_rest module runs in check-only mode (i.e. nothing pushed to the APIC) but continues saving payload to file, as it happens in other modules

Actual Behavior

  • Task is skipped
  • Module returns message "remote module (cisco.aci.aci_rest) does not support check mode"
  • Nothing is saved in the output_path location

Playbook tasks to Reproduce

---
- name: Create Sales Tenant
  hosts: apic
  gather_facts: no

  vars:
    aci_login: &aci_login
      host: "{{ ansible_host }}"
      username: "{{ aci_username }}"
      password: "{{ aci_password }}"
      validate_certs: "{{ aci_validate_certs }}"
      output_path: dryrun.json

  tasks:
  - name: Add a tenant using an XML string
    cisco.aci.aci_rest:
      <<: *aci_login
      path: /api/mo/uni.json
      method: post
      content:
        {
          "fvTenant": {
            "attributes": {
              "name": "Sales",
              "descr": "Sales department"
            }
          }
        }
    delegate_to: localhost

Important Factoids

References

  • #0000
@adealdag adealdag added the bug Something isn't working label May 23, 2023
@lhercot lhercot added enhancement New feature or request and removed bug Something isn't working labels Jun 30, 2023
@akinross akinross self-assigned this Aug 30, 2023
@dalamanster
Copy link

Sorry for discussing already closed issue.

Regarding the code part:
aci.result["changed"] = True

Is this hardcoded True? Every time I run the task - I get status changed ?

example:

  • The aci object is created when I run the task for the first time (via the aci_rest module)
  • Later I want to check the status of that object - therefor I run the same task in check mode
  • Nothing was changed on the object - but I get status changed ...

Can you add some logic? If my proposed payload is the same like a remote aci object (fetched with get ? ). I dont know what is the proper way. But check mode for all other modules works as expected.

Thanks

@lhercot
Copy link
Member

lhercot commented Nov 8, 2023

Hi @dalamanster, yes this is hardcoded to True as aci_rest never checks what exist and just push the content or src that it is provided.

Because aci_rest is build in such a way that you can provide a path that does not match the DN of the object and that the object can technically be down the tree from the path endpoint, it is difficult to query the correct object and calculate differences.

As we cannot tell without make the API call (which will go against check_mode reason of existence) if the call will make any changes, the safe option is to return changed=True.

If you are using aci_rest to fill in gaps in modules we have not yet released, please let us know which classes you are looking for or open enhancement requests so we can plan to deliver those. That is the preferred way so you can have all the benefit of check_mode and other features.

If this is an issue you think need more consideration, feel free to open a new issue and we can expand the discussion there. This will also help other in the community to chime in and maybe to propose solutions we have not thought about.

@dalamanster
Copy link

Thanks @lhercot for clear clarification. Now it makes sense for me. You are right - the path and real DN of the changed object can vary in every task. Therefore we cannot tell if the object was modified.

As a workaround we will skip the tasks (which are using aci-rest module) in checkmode and output some debug message to user.

Permanent fix would be to implement all the tasks in new dedicated modules. You are doing great work in this project - I will open new issue / feature request - as we are not able to do this by ourselves.

Just to mention some - we are using aci rest for tasks regarding :

  • bridge domain - host route advertisement
  • epg micro-segmentation - static leaf binding, add endpoint attributes (physical and also vmm domains)
  • service graph template apply
  • other SGT related objects - rhg, pbr, l4-l7 device, ip sla

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants