Feat(eos_cli_config_gen): Add Monitor Session support

[ccsnw]

Change Summary

• Add option to create monitor sessions

Related Issue(s)

Related to ##1412

Component(s) name

arista.avd.eos_cli_config_gen

Proposed changes

• Proposal for the datamodel below, please review and let me know if that ok

monitor_sessions:
  - name: < session_name_1 >
    sources:
      - name: < interface_name or range >
        direction: < rx | tx | both >
        access_group:
          type: < ip | ipv6 | mac >
          name: < acl_name >
          priority: < priority >
    destinations:
      - < interface(s) | cpu >
    encapsulation_gre_metadata_tx: < true | false >
    header_remove_size: < bytes >
    access_group:
      type: < ip | ipv6 | mac >
      name: < acl_name >
    rate_limit_per_ingress_chip: < "<int> bps" | "<int> kbps" | "<int> mbps" >
    rate_limit_per_egress_chip: < "<int> bps" | "<int> kbps" | "<int> mbps" >
    sample: < int >
    truncate:
      enabled: < true | false >
      size: < bytes >

How to test

added molecule test scenario

Checklist

User Checklist

• N/A

Repository Checklist

• My code has been rebased from devel before I start
• I have read the CONTRIBUTING document.
• My change requires a change to the documentation and documentation have been updated accordingly.
• I have updated molecule CI testing accordingly. (check the box if not applicable)

[ccsnw]

Hi guys, would appreciate a review of the datamodel before implementing the rest.
We should be able to use this in conjunction with a modification in eos_designs to solve: #1412

thanks

[ClausHolbechArista]

monitor_sessions:
  - name: < session_name_1 >
    sources:
      - name: < interface_name or range >
        direction: < rx | tx | both >
        ip_access_group: < acl_name >
        ip_access_group_priority: < priority >
        ipv6_access_group: < acl_name >
        ipv6_access_group_priority: < priority >
        mac_access_group: < acl_name >
        mac_access_group_priority: < priority >
    destinations:
      - < interface(s) | cpu >
    encapsulation_gre_metadata_tx: < true | false >
    header_remove_size: < bytes >
    ip_access_group: < acl_name >
    ip_access_group_priority: < priority >
    ipv6_access_group: < acl_name >
    ipv6_access_group_priority: < priority >
    mac_access_group: < acl_name >
    mac_access_group_priority: < priority >
    rate_limit_per_egress_chip: < "<int> bps" | "<int> kbps" | "<int> mbps" >
    sample: < int >
    truncate:
      enabled: < true | false >
      size: < bytes >

This CLI area is highly platform dependent. The supported values and features differ between chips, so I have removed all the specific numbers.
I have also adjusted a bit to cater for all the knobs, but you don't have to implement all of them. A lof of these are very rarely used, and ex. the source ACLs can be worked around by just appending the commands to the source interface.

[ccsnw]

Slight adjustment:

monitor_sessions:
  - name: < session_name_1 >
    sources:
      - name: < interface_name or range >
        direction: < rx | tx | both >
        ip_access_group: < acl_name >
        ip_access_group_priority: < priority >
        ipv6_access_group: < acl_name >
        ipv6_access_group_priority: < priority >
        mac_access_group: < acl_name >
        mac_access_group_priority: < priority >
    destinations:
      - < interface(s) | cpu >
    encapsulation_gre_metadata_tx: < true | false >
    header_remove_size: < bytes >
    ip_access_group: < acl_name >
    ipv6_access_group: < acl_name >
    mac_access_group: < acl_name >
    rate_limit_per_ingress_chip: < "<int> bps" | "<int> kbps" | "<int> mbps" >
    rate_limit_per_egress_chip: < "<int> bps" | "<int> kbps" | "<int> mbps" >
    sample: < int >
    truncate:
      enabled: < true | false >
      size: < bytes >

added: rate_limit_per_ingress_chip
removed: prio values for acls in general config, as far as I can see the only exist per source interface

[ccsnw]

@ClausHolbechArista & @tgodaA

Does it make sense to you to change to sth like:

monitor_sessions:
  - name: < session_name_1 >
    sources:
      - name: < interface_name or range >
        direction: < rx | tx | both >
        access_group:
          type: < ip | ipv6 | mac >
          priority: < priority >
    destinations:
      - < interface(s) | cpu >
    encapsulation_gre_metadata_tx: < true | false >
    header_remove_size: < bytes >
    access_group:
      type: < ip | ipv6 | mac >
    rate_limit_per_ingress_chip: < "<int> bps" | "<int> kbps" | "<int> mbps" >
    rate_limit_per_egress_chip: < "<int> bps" | "<int> kbps" | "<int> mbps" >
    sample: < int >
    truncate:
      enabled: < true | false >
      size: < bytes >

This might prevent users to input multiple ACLs of different types.. vs. slightly different to acutal CLI

[ClausHolbechArista]

@ClausHolbechArista & @tgodaA

Does it make sense to you to change to sth like:

monitor_sessions:
  - name: < session_name_1 >
    sources:
      - name: < interface_name or range >
        direction: < rx | tx | both >
        access_group:
          type: < ip | ipv6 | mac >
          priority: < priority >
    destinations:
      - < interface(s) | cpu >
    encapsulation_gre_metadata_tx: < true | false >
    header_remove_size: < bytes >
    access_group:
      type: < ip | ipv6 | mac >
    rate_limit_per_ingress_chip: < "<int> bps" | "<int> kbps" | "<int> mbps" >
    rate_limit_per_egress_chip: < "<int> bps" | "<int> kbps" | "<int> mbps" >
    sample: < int >
    truncate:
      enabled: < true | false >
      size: < bytes >

This might prevent users to input multiple ACLs of different types.. vs. slightly different to acutal CLI

Good idea. Just remember to also add name under access_group

[ccsnw]

Good idea. Just remember to also add name under access_group

@ClausHolbechArista & @tgodaA

Implemented as discussed and added a first try for the documentation.

My thoughts on it:

• Did not add general session settings explicitly. They are covered by the snippet. Adding them explicitly would most likely be somewhat redundant. Whats your take on it?
• Regarding the table: Decided to but "Access Group" Settings in one column, using "Not defined" in case nothing is defined at all, or priority is not defined. Think replacing these cases with "No Access Group defined" and "No Priority defined" might make more sense. Splitting the three values in a column each seems overkill?

[ClausHolbechArista]

• Did not add general session settings explicitly. They are covered by the snippet. Adding them explicitly would most likely be somewhat redundant. Whats your take on it?

You could do a "settings" table like we have under other features, where the columns are "setting" and "value" or similar. Check peer-groups under BGP.

• Regarding the table: Decided to but "Access Group" Settings in one column, using "Not defined" in case nothing is defined at all, or priority is not defined. Think replacing these cases with "No Access Group defined" and "No Priority defined" might make more sense. Splitting the three values in a column each seems overkill?

I don't think you should build comma-separated fields inside a single table column. IMO it is not making it easier to read.
I suggest that you either create table columns for each value and use - as the default value, or write out the cli in the field like "ip access-list blah priority 123" and if priority is not set, just don't print it. Again - if no access-list is set.

[ccsnw]

• Did not add general session settings explicitly. They are covered by the snippet. Adding them explicitly would most likely be somewhat redundant. Whats your take on it?

You could do a "settings" table like we have under other features, where the columns are "setting" and "value" or similar. Check peer-groups under BGP.

• Regarding the table: Decided to but "Access Group" Settings in one column, using "Not defined" in case nothing is defined at all, or priority is not defined. Think replacing these cases with "No Access Group defined" and "No Priority defined" might make more sense. Splitting the three values in a column each seems overkill?

I don't think you should build comma-separated fields inside a single table column. IMO it is not making it easier to read. I suggest that you either create table columns for each value and use - as the default value, or write out the cli in the field like "ip access-list blah priority 123" and if priority is not set, just don't print it. Again - if no access-list is set.

Thanks Claus.

Let me split the "Access Group" part in columns, it really isn't readable the way it is now.

Regarding the general settings: Still have my doubts about it. We would basically end up with rows like:

| Setting | Value |
| Encapsulation Gre Metadata Tx | True |

Is that really helpful? If so, should we adjust the model to

monitor_sessions:
  - name: < session_name_1 >
    sources:
      - name: < interface_name or range >
        direction: < rx | tx | both >
        access_group:
          type: < ip | ipv6 | mac >
          name: < acl_name >
          priority: < priority >
    destinations:
      - < interface(s) | cpu >
    session_settings:
      encapsulation_gre_metadata_tx: < true | false >
      header_remove_size: < bytes >
      access_group:
        type: < ip | ipv6 | mac >
        name: < acl_name >
      rate_limit_per_ingress_chip: < "<int> bps" | "<int> kbps" | "<int> mbps" >
      rate_limit_per_egress_chip: < "<int> bps" | "<int> kbps" | "<int> mbps" >
      sample: < int >
      truncate:
        enabled: < true | false >
      size: < bytes >

This would allow us to easily check if something under "session_settings" is defined and only render this table if needed? Otherwise we would need to check if anything besides "sources" and "destinations" and "name" exists or always render everything with default settings?

[ClausHolbechArista]

Regarding the general settings: Still have my doubts about it. We would basically end up with rows like:

| Setting | Value |
| Encapsulation Gre Metadata Tx | True |

Is that really helpful? If so, should we adjust the model to

Creating this table will at least show everything configured. It follows the same style as the rest of the documentation, but I agree it has limited value. Please do the table and we can get this merged :)

This would allow us to easily check if something under "session_settings" is defined and only render this table if needed? Otherwise we would need to check if anything besides "sources" and "destinations" and "name" exists or always render everything with default settings?

I don't think we should change the model. If you move destination up to the settings table you also avoid repeating the destination in each line in the source table, then the settings table would always be relevant, even with the minimal config.

[ccsnw]

If you move destination up to the settings table you also avoid repeating the destination in each line in the source table, then the settings table would always be relevant, even with the minimal config.

This sounds good, haven't thought about it this way. Let me adjust it accordingly and get back to you!
Thanks!

@ClausHolbechArista changes added. Let me know if it needs further tweaking :-)

[ClausHolbechArista]

LGTM

[tgodaA]

LGTM!