brocade / ansible

56 stars 33 forks source link

Add module for brocade-fibrechannel-logical-switch #85

Open db0 opened 3 years ago

db0 commented 3 years ago

The only way to move ports between two FIDs is to use the brocade-fibrechannel-logical-switch REST API module. While I see There's a brocade_interface_fibrechannel.py for port configuration and a brocade_fibrechannel_switch.py for switch configuration, the logical switch configuration capabilities are missing

daniel-chung-broadcom commented 3 years ago

It would be very helpful if you can clarify what use cases that you are referring to by "logical switch configuration capabilities"? Thanks.

db0 commented 3 years ago

I already mentioned one: Moving a port from one fid to another on the same switch.

daniel-chung-broadcom commented 3 years ago

I am sorry but I mistook the original request thinking that you were looking for some enhancements to rest api. I should have read it as an enhancement on ansible module. the request is to support brocade_fibrechannel_logical_switch.py module. Please let me know if I am still mistaken.

In the meanwhile, you should also be able to use brocade_list_obj module to make use of the backend rest api. Please let me know if you need some examples for reference.

db0 commented 3 years ago

Ah, I was not aware I could use brocade_list_obj to call the REST API directly. Until now I've been calling it directly using ansible's uri module. I'll check it out.

Otherwise, that is correct. I'd effectively like to have a brocade_fibrechannel_logical_switch module :)

daniel-chung-broadcom commented 3 years ago

We provide "named" modules to indicate additional testing, while _list_obj and _singleton_obj are available to provide coverage in the absence of named modules. Most named modules generally call into _list_obj and _singleon_obj. So, the code flow and functionalities should generally match, although we do have some module-specific handling code within these objects in some instances.

We'll look at the support for brocade_fibrechannel_logical_switch. We'll keep you posted on the progress. In the meanwhile, if you have any questions about the _list_obj usage or issues, please let us know.

db0 commented 3 years ago

I think I would need some help indeed as I'm not really a brocade specialist, but rather helping the experts with the automation.

To avoid turning this ticket into a support thread, do you have a slack channel or something where I can ask some questions on their use?

daniel-chung-broadcom commented 3 years ago

I made some changes to add brocade_fibrechannel_logical_switch_fibrechanel_logical_switch.py module. Can you try out the branch documentation_attribute_content_change? There are two playbooks for reference: facts_logical_switch.yml and logical_switch.yml. facts playbook will print out all the logical switches available, including 128. But when you are creating a playbook to set, you need to specify all the non-default switches only. I verified the following

1) create non-default logical switch with ports 2) update to remove ports from the non-default logical switch and return the ports to default switch 3) remove a non-default logical switch and return the ports to default switch

Please check it out and let me know what you think.

db0 commented 3 years ago

So if I understand correctly, if I want to move a port from one fabric to another, I need to specify both fabrics, and all ports that should be in each fabric? Can you clarify some points for me?

(Unfortunately we're not ready to use to a full IasC approach here just yet, so I am trying to tweak individual ports.)

Thanks

PS: The fact gathering playbook worked fine.

daniel-chung-broadcom commented 3 years ago

I just added 'all_entries" option to the module and pushed the changes. I think that should help with some scenarios but not entirely on others. Here are some examples below:

1) with all_entries set to True or not present (default by True) if you do not specify the fid in the list, the fid will be removed. As part of the process, the ports associated with the fid will moved to default switch if you do not specify the post in the listed fid, the port will be moved to default switch In other word, the list of fid and the ports are meant to reflect the final configuration on your switch at the end of the play

1) with all_entries set to False if you do not specify the fid in the list, the fid will NOT be touched if you do not specify the post in the listed fid, the port will be moved to default switch In other word, the list of fid and the ports are meant to reflect the subset of configuration on your switch at the end of the play

The best way, I think, to create the playbook is to run the facts gathering and use the output (minus the default switch) to add to your playbook to being with for either all_entries or not.

db0 commented 3 years ago

Great that helps. So If I want to move just one port to a new fid, I need to do three steps:

Sounds about right?

Would I need to also retrieve and specify these values to avoid getting the defaults (assuming they're changed)

base_switch_enabled
ficon_mode_enabled
logical_isl_enabled
daniel-chung-broadcom commented 3 years ago

I believe that sounds right. The basic idea here is that logical_switches attribute should contain a list of fid and all associated ports that you want the end result to be at the end of running the play. So, if you run the playbook again, no changes will be reported. That allows you to take advantage of dry-runs that tells us if anything has drifted or not.

all_entries being false, simply allow fids not listed in the logical_switches to remain present. But some of their ports may be moved into the fids being specified by the logical_switches attribute. For example, fid 1 had a,b,c and fid 2 had d,e,f and your logical_switches are set to fid 1 of a,b,c,d and all_entries false. That play will result in fid 1 of a,b,c,e and fid 2 of e,f.

Yes. those three fields should also be specified. One thing to note here is that these fields are associated with each other as to when they are valid. For example, logical-isl-enabled is only valid field when base-switch-enabled is 0. You can see the details in the rest yang files.

leaf logical-isl-enabled { when "../base-switch-enabled = 0";

The safest thing to do is just keep these values from the facts output as you mentioned in your first bullet.

db0 commented 3 years ago

Perfect! Thanks so much for the help. I'll run some tests and let you know.

db0 commented 3 years ago

So, I'm having some trouble testing this module. Because this repository is not in the form of an ansible collection, I can't use ansible galaxy to install it directly from the branch. As such, I had to manually copy library/brocade_fibrechannel_logical_switch_fibrechannel_logical_switch.py to my collection's modules folder, and this allowed the brocade_list_obj_facts to utilize it.

However when trying to use brocade_fibrechannel_logical_switch_fibrechannel_logical_switch directly, I get an error

fatal: [server]: FAILED! => {"msg": "Could not find imported module support code for ansible_collections.brocade.fos.plugins.modules.brocade_fibrechannel_logical_switch_fibrechannel_logical_switch.  Looked for (['ansible.module_utils.brocade_objects.list_helper', 'ansible.module_utils.brocade_objects'])"}

I tried to copy utils/brocade_objects.py into the collection's module_utils/ dir, but that uses what seems to be the wrong import structure and it breaks the whole collection. I tried to copy brocade_objects.py and replace the import structure with the same as the collection currently uses, and I still end up with the same error as before.

Any advice how I could test this module?

EDIT: Nevermind, managed it by editing brocade_fibrechannel_logical_switch_fibrechannel_logical_switch.py and changing its import to use the correct path for the collection as well

daniel-chung-broadcom commented 3 years ago

That is because the include paths are different between the two. I typically make the necessary changes on galaxy branch on this repo before generating a collection from it.

In the meanwhile, I think the change that you need would be to change the importing from

from ansible.module_utils.brocade_objects import list_helper

to

from ansible_collections.brocade.fos.plugins.module_utils.brocade_objects import list_helper

when you copy brocade_fibrechannel_logical_switch_fibrechannel_logical_switch.py to the new location under your existing collection.

Please try out the change and let me know if you run into additional issues.

db0 commented 3 years ago

I'm getting an error. Maybe I'm formatting the payload wrong? I copied the example. This is the verbose out

    "PATCH_resp_code": 404,
    "PATCH_resp_data": {
        "errors": {
            "@xmlns": "urn:ietf:params:xml:ns:yang:ietf-restconf",
            "error": {
                "error-app-tag": "Error",
                "error-info": {
                    "error-code": "16717060",
                    "error-module": "cal"
                },
                "error-message": "Invalid resource attribute in the request",
                "error-tag": "operation-failed",
                "error-type": "application"
            }
        }
    },
    "PATCH_resp_reason": "Not Found",
    "PATCH_url": "https://********/rest/running/brocade-fibrechannel-logical-switch/fibrechannel-logical-switch",
    "add_entries": [],
    "add_retcode": 0,
...
    "msg": "PATCH failed",
    "patch_str": "<fibrechannel-logical-switch>\n<fabric-id>25</fabric-id>\n<all-entries>false</all-entries>\n<port-member-list>\n<port-member>0/3</port-member>\n<port-member>0/95</port-member>\n<port-member>0/10</port-member>\n</port-member-list>\n</fibrechannel-logical-switch>\n",

This is my task

- name: test
  brocade_fibrechannel_logical_switch_fibrechannel_logical_switch:
    credential:
      fos_ip_addr: "{{ SWITCH }}"
      fos_user_name: "{{ HUSER }}"
      fos_password: "{{ HPASS }}"
      https: self
    vfid: -1
    logical_switches: 
    - fabric_id: 25
      all_entries: false
      base_switch_enabled: 0
      ficon_mode_enabled: 0
      logical_isl_enabled: 1
      port_member_list: 
        port_member: 
        - "0/3"
        - "0/95"
        - "0/10"
daniel-chung-broadcom commented 3 years ago

I think the problem is that "all_entries" attribute is in the wrong level. It should indicate if logical_switches list contains all entries or not and should be at the same level and not within each entry of the list. So, try the below and see how it works.

db0 commented 3 years ago

Yes, that was it! Thanks!

However it seems like it is having trouble with idempotency. Sending the request with the same values, fails instead of returning OK (see error-message below):

    "PATCH_resp_code": 400,
    "PATCH_resp_data": {
        "errors": {
            "@xmlns": "urn:ietf:params:xml:ns:yang:ietf-restconf",
            "error": {
                "error-app-tag": "Error",
                "error-info": {
                    "error-code": "16715778",
                    "error-module": "cal"
                },
                "error-message": "Port member list already set",
                "error-path": "/fibrechannel-logical-switch/fabric-id/",
                "error-tag": "Operation-failed",
                "error-type": "application"
            }
        }
    },
    "PATCH_resp_reason": "Bad Request",
    "PATCH_url": "https://********/rest/running/brocade-fibrechannel-logical-switch/fibrechannel-logical-switch",
    "add_entries": [],
    "add_retcode": 0,
daniel-chung-broadcom commented 3 years ago

that's weird. I verified that existing configurations return no change in my testing. Do you mind sending me the content of your playbook for me to try?

db0 commented 3 years ago

In the process of gathering the payload I'm sending to post here, I noticed what I did wrong. I am generating the payload automatically and after the first run, I ended up having the same port twice in the port_member list. This apparently caused the API to barf 😄

It works fine when you have each port only once in the port_member list 👍 (i.e. it returns OK as expected).

Do let me know when you publish these changes in ansible-galaxy btw and thanks for the good (and fast) work!

daniel-chung-broadcom commented 3 years ago

Great. Thanks for letting me know. Please let me know if you find any other issues along the way.

We usually gather a few changes before doing a sanity test to merge to master and also publish in the galaxy. I'll keep you posted on the progress.

Thanks.

prasad-valmeti-broadcom commented 2 years ago

This issue will get resolved with the merge of Ansible branches. Discussions are going on how to merge these branches. It will be resolved once the different branches are merged. This work is in discussions and is going on. Will let you once there is any concrete release is planned for it.

prasad-valmeti-broadcom commented 1 year ago

Ansible fos 1.3.2 addresses this issue. Currently, ansible fos 1.3.2 is scheduled to release in early June 20203. Will keep you posted.

prasad-valmeti-broadcom commented 1 year ago

The issue is already addressed in Ansible FOS 1.3.3. If there are no further issues, please suggest if we can close the issue.

prasad-valmeti-broadcom commented 1 year ago

Since there is no response and the issue is already addressed in Ansible FOS 1.3.3, planning to close the issue. Please let us know your comments/suggestions on this.

prasad-valmeti-broadcom commented 6 months ago

This issue is resolved with Ansible FOS 1.3.3. If there are no further issues, please suggest if we can close this issue.