-
Notifications
You must be signed in to change notification settings - Fork 97
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
ACI L4-L7 Services (DCNE-155) #186
base: master
Are you sure you want to change the base?
Conversation
else: | ||
path_type = 'paths' | ||
|
||
path_dn = ('topology/pod-{0}/{1}-{2}/pathep-[{3}]'.format(pod_id, |
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 we include some form of validation of input to ensure valid path_dn is created?
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'm not sure how much validation we can add, since the pathep for PCs and vPCs is just an IPG name (so just a generic string). The pod ID is already typed as int, and I've added a regex check to make sure the node ID is either an integer or a hyphen separated pair of integers.
9be4a81
to
2c8ec69
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.
review part 1
-> needs changes
-> Might need to add another module for object vnsSvcVip (aci_l4l7_device_selection_if_context_vip) -> inside aci_l4l7_device_selection_if_context
threshold=dict(type="int"), | ||
traffic_class=dict(type="int"), | ||
http_version=dict(type="str", choices=["1.0", "1.1"]), | ||
http_uri=dict(type="str"), |
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.
Module :
- Missing destination_port and detect_multiplier arguments (available when sla_type = TCP).
- missing description
Tests
-> in test cases
-> missing test cases with traffic_class, type_of_service arguments
-> missing test case when sla_type = L2ping
health_group=dict(type="str"), | ||
) |
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.
missing description argument in module
device = module.params.get("device") | ||
|
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.
- Missing context_name argument.
- While testing device (aci_l4l7_device) is created and available on UI, but it is not attached to aci_l4l7_device_selection_policy. (required parameter)
argument_spec.update(aci_annotation_spec()) | ||
argument_spec.update( | ||
tenant=dict(type="str", aliases=["tenant_name"]), | ||
contract=dict(type="str", aliases=["contract_name"]), |
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 don't think contract is the right name.
logical_device=dict(type="str"), | ||
logical_interface=dict(type="str"), | ||
redirect_policy=dict(type="str"), | ||
) |
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.
-> contract might not be the right name.
-
In module custom_qos_policy, permit_logging and l3out arguments are missing
-
l3_dest and permit_log arguments missing in test cases
-
In Test :
- Modify L4-L7 Device Selection Logical Interface Context - fails
- Verify context query - fails
- Azure cloud fails at - Add a Bridge Domain in ansible_tenant
module_object=context, | ||
target_filter={"connNameOrLbl": context}, | ||
), | ||
child_classes=["vnsRsLIfCtxToBD", "vnsRsLIfCtxToLIf", "vnsRsLIfCtxToSvcRedirectPol"], |
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.
child_classes=["vnsRsLIfCtxToBD", "vnsRsLIfCtxToLIf", "vnsRsLIfCtxToSvcRedirectPol"] -> should be modified by adding one more classes -> vnsRsLIfCtxToCustQosPol
tenant=dict(type="str", aliases=["tenant_name"]), | ||
service_graph=dict(type="str"), | ||
state=dict(type="str", default="present", choices=["absent", "present", "query"]), | ||
ui_template_type=dict(type="str"), | ||
) |
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.
test case for ui_template_type missing
tenant=dict(type="str", aliases=["tenant_name"]), | ||
service_graph=dict(type="str"), | ||
state=dict(type="str", default="present", choices=["absent", "present", "query"]), | ||
connection_name=dict(type="str"), | ||
direct_connect=dict(type="bool"), | ||
unicast_route=dict(type="bool"), | ||
adjacency_type=dict(type="str", choices=["l2", "l3"]), | ||
) |
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.
Missing connType and connDir from modules.
DOCUMENTATION = r""" | ||
--- | ||
module: aci_l4l7_service_graph_template_abs_connection_conns | ||
short_description: Manage L4-L7 Service Graph Template Connections (vns:RsAbsConnectionConns) |
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.
Do we need a separate module for relationship object?
tenant=dict(type="str", aliases=["tenant_name"]), | ||
service_graph=dict(type="str"), | ||
node=dict(type="str"), | ||
state=dict(type="str", default="present", choices=["absent", "present", "query"]), | ||
func_template_type=dict(type="str", choices=["fw_trans", "fw_routed", "adc_one_arm", "adc_two_arm", "other"]), | ||
func_type=dict(type="str", choices=["go_to", "go_through", "l1", "l2"]), | ||
device=dict(type="str"), | ||
device_tenant=dict(type="str"), | ||
managed=dict(type="bool"), | ||
routing_mode=dict(type="str"), |
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.
missing isCopy argument in the module.
tenant=dict(type="str", aliases=["tenant_name"]), | ||
service_graph=dict(type="str"), | ||
state=dict(type="str", default="present", choices=["absent", "present", "query"]), | ||
node_name=dict(type="str", choices=["T1", "T2"]), |
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.
Looks like this module is built for 2 nodes, no example of what happens if there are more than 2 node, or when copy node is present.
svc_type=dict(type="str", choices=["adc", "fw", "others"]), | ||
trunking=dict(type="bool"), | ||
domain=dict(type="str"), | ||
) |
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.
fails on 6.2 v and azure cloud
prom_mode=dict(type="bool"), | ||
svc_type=dict(type="str", choices=["adc", "fw", "others"]), | ||
trunking=dict(type="bool"), | ||
domain=dict(type="str"), |
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.
rename arguments, no example for copy device
device=dict(type="str"), | ||
logical_interface=dict(type="str"), | ||
state=dict(type="str", default="present", choices=["absent", "present", "query"]), | ||
encap=dict(type="str"), |
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.
missing configIssues (configuration issues) parameter.
device=dict(type="str"), | ||
logical_interface=dict(type="str"), | ||
state=dict(type="str", default="present", choices=["absent", "present", "query"]), | ||
concrete_device=dict(type="str"), | ||
concrete_interface=dict(type="str"), |
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.
this is a relationship object.
This PR includes modules and integration tests for L4-L7 services. The modules allow configuration of the following ACI classes: