-
Notifications
You must be signed in to change notification settings - Fork 42
'fp.monitoring' fixed and expanded (Issue623) #267
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: main
Are you sure you want to change the base?
Conversation
I guess this example data is vague?
|
}] | ||
} | ||
|
||
module "aci_monitoring_policy" { |
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.
maybe lets call it aci_monitoring_policy_common and 2nd module just aci_monitoring_policy?
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.
and lets add there fault severity policies (for common object), to keep all modules supporting the same
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.
by the way, i guess there is not an option to configure new syslog destination or snmp destination without adding it to common object?
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.
maybe lets call it aci_monitoring_policy_common and 2nd module just aci_monitoring_policy?
We can check this with Justyna. I tried to keep it the way it is in order to ensure backwards compatibility.
and lets add there fault severity policies (for common object), to keep all modules supporting the same
As per my tests, common policy does not allow changes in Fault severities.
by the way, i guess there is not an option to configure new syslog destination or snmp destination without adding it to common object?
Not sure about this one. Hope you can elaborate it
modules/terraform-aci-monitoring-policy-user-defined/examples/complete/versions.tf
Outdated
Show resolved
Hide resolved
modules/terraform-aci-monitoring-policy-user-defined/variables.tf
Outdated
Show resolved
Hide resolved
modules/terraform-aci-monitoring-policy-user-defined/versions.tf
Outdated
Show resolved
Hide resolved
If we analyze common policy, i guess that there was 1 more broken thing:
If we consider that we create an SNMP destination named "trap1" and also it automatically creates under monitoring common policy reference, in our case it will generate reference to 'mon_snmp1_dst_group` which doesn't exists. Wouldn't it be there that we need to keep destination_group and name always the same to make it working? I guess maybe we should force users to keep it the same? destination_group would have to include name_suffix as well |
User should be in charge of make sure references are pointing to valid objects (e.g. when you define a BD under EPG, tool does not validate if the BD exists and no error is popped up). Also, an internal issue was raised where the engineers are not using the same name for both objetcs, instead, they create the SNMP destinations independently. Let's meet and discuss this further. Thanks! |
faults = try(policy.faults, local.defaults.apic.fabric_policies.monitoring.syslogs.faults) | ||
session = try(policy.session, local.defaults.apic.fabric_policies.monitoring.syslogs.session) | ||
minimum_severity = try(policy.minimum_severity, local.defaults.apic.fabric_policies.monitoring.syslogs.minimum_severity) | ||
destination_group = try("${policy.destination_group}${local.defaults.apic.fabric_policies.monitoring.syslogs.destination_group_suffix}", "") |
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 should read from:
---
defaults:
apic:
fabric_policies:
monitoring:
syslogs:
name_suffix: ""
count = local.modules.aci_monitoring_policy == true && var.manage_fabric_policies ? 1 : 0 | ||
snmp_trap_policies = [for policy in try(local.fabric_policies.monitoring.snmp_traps, []) : { | ||
name = "${policy.name}${local.defaults.apic.fabric_policies.monitoring.snmp_traps.name_suffix}" | ||
destination_group = try("${policy.destination_group}${local.defaults.apic.fabric_policies.monitoring.snmp_traps.destination_group_suffix}", "") |
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 should read from:
---
defaults:
apic:
fabric_policies:
monitoring:
snmp_traps:
name_suffix: ""
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 is no need to create new destination_group_suffix
Current Fabric Monitoring Data Model only accept the configuration of
name
. Support fordestination_group
was added and sopport of user-defined fabric monitoring policies.These are non-breaking changes.
Data tested: