Skip to content
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

Feat(eos_cli_config_gen): Generate sFlow egress commands #2767

Merged
merged 10 commits into from
Jun 20, 2023

Conversation

xaviramon
Copy link
Contributor

@xaviramon xaviramon commented Apr 28, 2023

Change Summary

Knob to enable unmodified egress sflow on the CLI.

Related Issue(s)

To be used in #2311

Component(s) name

arista.avd.eos_cli_config_gen

Proposed changes

Add a new keys to the data model:

sflow:
  interface:
    egress:
      unmodified_enable_default: <bool>
      enable_default: <bool>

How to test

Molecule scenario added

CLI testing for command:

ld005(config)#sh run | i sflow
sflow interface disable default
sflow interface egress unmodified enable default
sflow run

This is a platform-dependent command, but adding no check following the component philosophy.

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)

@xaviramon xaviramon requested a review from a team as a code owner April 28, 2023 09:48
@github-actions github-actions bot added role: eos_cli_config_gen issue related to eos_cli_config_gen role state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated labels Apr 28, 2023
@xaviramon xaviramon changed the title Feat(eos_cli_config_gen): Generate sFlow egress unmodified command Feat(eos_cli_config_gen): Generate sFlow egress commands May 1, 2023
Copy link
Contributor

@gmuloc gmuloc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very open comments here!

For reference:

Leaf1(config)#show cli commands | grep "sflow interface"
[no|default] sflow interface disable default
[no|default] sflow interface egress enable default
[no|default] sflow interface egress unmodified enable default

Behavior on EOS 4.27.3F

Leaf1(config)#default sflow interface egress enable default
Leaf1(config)#show run | i sflow
<EMPTY>
Leaf1(config)#sflow interface egress enable default
Leaf1(config)#show run | i sflow
sflow interface egress unmodified enable default

Leaf1(config)#default sflow interface egress enable default
Leaf1(config)#sflow interface egress un  enable default
Leaf1(config)#show run | i sflow
sflow interface egress unmodified enable default

So not sure if there is a difference between enable_default and unmodified_default

Wondering if this could impact the model?

Could this make sense? (though seems like always enable_default is needed)

          egress:
            type: dict
            keys:
              unmodified:
                type: bool
              enable_default:
                type: bool

@gmuloc gmuloc requested a review from a team May 3, 2023 08:12
@xaviramon
Copy link
Contributor Author

Egress sFlow is a platform dependent feature and for this reason and to distinguish different behaviours, the commands used to activate it are different.

For SAND platforms sflow will sample all defined packets:

lp032(config)#sflow interface egress ?
  enable  Set default for egress sFlow to Enabled on interfaces

lp032(config)#sflow interface egress un
  unmodified  not supported on this hardware platform

lp032#sh ver
Arista DCS-7280QR-C36-F

For STRATA platforms sflow will sample defined packets ingress of an interface wich are exiting a defined interface, for this the keywork "unmodified" is used:

ld003(config)#sflow interface egress ?
  unmodified  Unmodified egress sFlow

ld003(config)#sflow interface egress en
  enable  not supported on this hardware platform

ld003(config)#sflow interface egress
ld003#sh ver
Arista DCS-7050CX3-32S-F

I am also proposing a new data model:

sflow:
  interface:
    egress:
      unmodified_enable_default: <bool>
      enable_default: <bool>

Copy link
Contributor

@gmuloc gmuloc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

so the last thing I have as a comment is that it is possible to render both on the same device which is not valid.

@ClausHolbechArista Wondering if we should for now in the template enforce that only one should be set?

Otherwise the PR content is good for me and I will approve it

@ClausHolbechArista
Copy link
Contributor

so the last thing I have as a comment is that it is possible to render both on the same device which is not valid.

@ClausHolbechArista Wondering if we should for now in the template enforce that only one should be set?

Otherwise the PR content is good for me and I will approve it

We can implement them as mutually exclusive (use elif) instead, and then document that they are mutually exclusive and the correct one depends on the platform.

@ClausHolbechArista
Copy link
Contributor

so the last thing I have as a comment is that it is possible to render both on the same device which is not valid.
@ClausHolbechArista Wondering if we should for now in the template enforce that only one should be set?
Otherwise the PR content is good for me and I will approve it

We can implement them as mutually exclusive (use elif) instead, and then document that they are mutually exclusive and the correct one depends on the platform.

With that in mind, actually your suggestion @gmuloc of having unmodified: <bool> would be more solid.

@ClausHolbechArista ClausHolbechArista requested a review from a team May 17, 2023 05:27
@github-actions github-actions bot added the state: conflict PR with conflict label May 19, 2023
@github-actions
Copy link

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@carlbuchmann carlbuchmann added this to the v4.1.0 milestone May 30, 2023
@haylinmoore haylinmoore requested a review from a team as a code owner June 20, 2023 19:50
@github-actions github-actions bot removed the state: conflict PR with conflict label Jun 20, 2023
@github-actions
Copy link

Conflicts have been resolved. A maintainer will review the pull request shortly.

Copy link
Contributor

@haylinmoore haylinmoore left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@carlbuchmann carlbuchmann merged commit fcc90de into aristanetworks:devel Jun 20, 2023
@carlbuchmann carlbuchmann removed this from the v4.2.0 milestone Jul 11, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rn: Feat(eos_cli_config_gen) role: eos_cli_config_gen issue related to eos_cli_config_gen role state: CI Updated CI scenario have been updated in the PR state: Documentation role Updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants