-
Notifications
You must be signed in to change notification settings - Fork 109
feat: Added aci_bulk_static_bindings_to_epgs module (DCNE-622) #822
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
base: master
Are you sure you want to change the base?
feat: Added aci_bulk_static_bindings_to_epgs module (DCNE-622) #822
Conversation
|
Hi @andreasgraber, Thank you for creation of the PR, we will have an internal discussion regarding your proposal. A few questions already from my end:
|
|
Hi @akinross, Thanks for the quick reply!
In the end it is an performance Issue. We have an IaC Stack in Front of the Playbook where we configure Servers in ACI. If we have a Server with a few Interfaces and each Interface have 300-400 Bindings the playbook takes over 30min with looping over the module aci_static_binding_to_epg. We also tried some performance improvements described in https://galaxy.ansible.com/ui/repo/published/cisco/aci/docs/optimizing/ but we could not get it faster.
I haven't tried it with parallel Execution. But I think than we need to handle Rate limiting Issues from the APIC as there are still hundreds of API Calls to configure one interface.
That's a good Point! My Experience with the APIC is that they can deal with very big payloads, but if somebody is pushing a big number of interfaces at once it could get problematic. To address this we could also limit the amount of interfaces under the attribute interface_configs or remove the list completely and allow just one interface at a time to be configured?
The current Bulk Module is addressing the usecase to push an EPG to many Interfaces. We need the use case to configure many EPGs to an interface at once, like a "switchport trunk allowed vlan" command on the catalyst switches. I think it could get hard to implement this logic in the current module? But maybe the naming of our module is confusing with the existing once?
I just saw that the /uni object gets the ansible annotation after running our bulk change module. This feels wrong to me as this object is managed by aci itself and we just want to manage the RsPathAtt Object. But we also could remove this part? Thanks for checking it internally and just let me know if You have any further question. Once we know if we can proceed with the PR I'm going to fix the the pipeline issues. Br, |
|
Hi @andreasgraber, Regarding your point 1, why would you refer to aci_static_binding_to_epg module and not the bulk equivalent, this would reduce the amount of tasks. In this case you would either loop per EPG or execute per EPG in parallel but having an inventory per EPG. What amount of EPGs are you configuring? Regarding your point 2, this would depend on the amount of EPG as asked in point 1. Regarding your point 4, the module is configuring a N amount of fvRsPathAtt MO to an EPG. I think this module could also take in a list of EPG to which you want to configure this same fvRsPathAtt MOs. As far as I am aware this module was made for performance optimisation of the single aci_static_binding_to_epg module. I personally think this module could be further adjusted, also to limit the amount of different modules which are sort of doing similar configuration. This would be a discussion with internal team. Regarding your point 5, this is wrong and happens automatically in the payload function because the assumption previously is made that there are no modules that are directly manipulation polUni. So likely in your situation each object that gets created should have this attribute added individually in my opinion. If you want to have some additional discussion on point 1 parallel execution we could set up a call to discuss your options there. Feel free to email me directly to schedule something. |
|
Hi @akinross We would be open to do some work on the current bulk_ module but I see an issue with the datastructure that would need some change. We would need:
|
|
Hi @dariomi, thanks for the clarifications. Will discuss with the team and get back to you on this. |
|
Hi @andreasgraber and @dariomi, I had an internal discussion and we have agreed that we will be willing to add this functionality. I will support you initially with writing the module, but after would need to follow the regular review process we have in our team. I would need some more time to do a proper review of the code and look at various options we have / choices we would need to consider. Are there any deadlines on your side for this? |
|
Hi @akinross, That are great news! Thx for the Information and Your support! You can also ping us on webex to proceed with the PR. Br, |
ae8f02d to
5d205bb
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are also quite a few sanity issues reported from the CI related to syntax. Unfortunately we still need to keep compatibility with older Python versions. Can you please look into those?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
5d205bb to
dcc87e7
Compare
| tenant: | ||
| description: | ||
| - Name of the tenant. | ||
| - Module level epg attributes are mutual exclusive to the epgs list. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Could you modify the sentence to
Module level EPG attributes are mutually exclusive with the EPGs list.? - Is there a reason for adding this under all the attributes instead of just epg and epgs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @shrsr, the sentence is fixed. Regarding Point 2; I just thought to make it clear that either the global or the EPGs Vars can be used. This will force the User to setup one or the other option, otherwise the usability of the module may can be misinterpreted. For ex. defining the Tenant globally and under the epgs list. The Global one would not have any relevance in this case. But I'm happy to change it?
| description: | ||
| - Determines the primary encapsulation ID associating the C(epg) | ||
| with the interface path when using micro-segmentation. | ||
| - Accepted values are any valid encap ID for specified encap, currently ranges between C(1) and C(4096) and C(unknown). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
->
- Accepted values are any valid encap ID for specified encap, currently ranges between C(1) and C(4096).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Primary Encap can also be unknown. I've copied this from the global primary_encap_id. Should I remove the unknown part?
1ed208c to
8e4c50a
Compare
| [ | ||
| "state", | ||
| "absent", | ||
| ["ap", "epg", "tenant", "epgs", "interface_configs"], | ||
| True, | ||
| ], | ||
| [ | ||
| "state", | ||
| "present", | ||
| ["ap", "epg", "tenant", "epgs", "interface_configs"], | ||
| True, | ||
| ], |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think this required if is correct, I think it should be split into more statement to accurately represent what is need:
- state absent [epg, epgs] true ( epg or epgs must be provided, where mutually_exclusive will catch that they are not provided together )
- state absent interface_configs ( true not needed since interface configs would always be required for delete )
same would apply for state present
perhaps required_by should be used to for complex scenarios, see https://docs.ansible.com/projects/ansible/latest/dev_guide/developing_program_flow_modules.html#argument-spec?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should be fixed as discussed.
| argument_spec = aci_argument_spec() | ||
| argument_spec.update(aci_annotation_spec()) | ||
| argument_spec.update( | ||
| def aci_epg_spec(): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is interface type not part of this spec and identified only at the root? can the same input precedence order not be used for interface type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The Module interface_type can now be used to specify a default value for all interfaces. I didn't put it into the epgs level because than the interface_type needs to be provided on each epg and different values are anyway not supported at this level.
tests/integration/targets/aci_bulk_static_binding_to_epg/tasks/main.yml
Outdated
Show resolved
Hide resolved
| ap_to_push = dict(fvAp=dict(attributes=dict(name=ap_inner), children=[epg_to_push])) | ||
| binding_tree_to_push = dict(fvTenant=dict(attributes=dict(name=tenant_inner), children=[ap_to_push])) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do all these things inside the static_bindings loop? i would have expected that you would create a payload object before the loop which you would populate with those objects required
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I simplified the logic a bit, but I would leave it at this position. Otherwise we have some new logic to define the whole Body and remove the unused part again which also brings additional complexity. TBD
82daa8c to
ff85cae
Compare
f9f62fe to
6b4d333
Compare
6b4d333 to
eecc400
Compare
This PR enables configuration of trunk ports in one run. It is based on the already existing module aci_bulk_static_binding_to_epg.py. But in addition to that, the Goal is to push a list of EPGs to a list of interfaces.
Some additional notes:
Thanks for a review!