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_designs): Single uplink to mlag pair, mlag_on_orphan_port_channel_downlink #3495

Conversation

jrecchia1029
Copy link
Contributor

Change Summary

Fixes an existing bug in designs where there is a standalone switch with just a single link to just one switch of an mlag pair where AVD will unnecessarily make the port-channel created on the mlag pair towards the standalone switch an MLAG port-channel which prevents LACP on the link between the standalone switch and the single switch of the mlag pair from forming.

Related Issue(s)

Fixes #3459

Component(s) name

arista.avd.eos_designs

Proposed changes

Add the logic to make a fabric port-channel an MLAG port-channel only if the switch where we are configuring the port-channel or its mlag peer connect to a single logical switch.

How to test

Added a molecule test with a topology that triggers this condition

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)

@jrecchia1029 jrecchia1029 requested review from a team as code owners January 15, 2024 15:14
@github-actions github-actions bot added state: CI Updated CI scenario have been updated in the PR role: eos_designs issue related to eos_designs role labels Jan 15, 2024
@jrecchia1029 jrecchia1029 changed the title Single uplink to mlag pair Feat(eos_designs): Single uplink to mlag pair Jan 15, 2024
Copy link
Contributor

@ClausHolbechArista ClausHolbechArista left a comment

Choose a reason for hiding this comment

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

Reviewed configs and they seem correct. The change to an existing test case is also correct, but that shows that existing deployments may be impacted by this. I have tested in lab, and single connected port-channels with mlag configured still function. The mlag command will not do anything and can be removed without impact. But since this is changing existing config - causing concern, I think we need to put this behavior behind a knob single_uplink_switch_mlag: <bool; default=True> which we can then flip to default False in AVD 5.0

Copy link

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

@github-actions github-actions bot added the state: conflict PR with conflict label Jan 26, 2024
@github-actions github-actions bot removed the state: conflict PR with conflict label Jan 29, 2024
Copy link

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

Copy link
Contributor Author

@jrecchia1029 jrecchia1029 left a comment

Choose a reason for hiding this comment

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

LGTM

@ClausHolbechArista ClausHolbechArista requested a review from a team January 29, 2024 18:53
@ClausHolbechArista ClausHolbechArista added the one approval This PR has one approval and is only missing one more. label Jan 29, 2024
@ClausHolbechArista ClausHolbechArista changed the title Feat(eos_designs): Single uplink to mlag pair Feat(eos_designs): Single uplink to mlag pair, mlag_on_orphan_port_channel_downlink Jan 29, 2024
@github-actions github-actions bot added the state: conflict PR with conflict label Jan 29, 2024
Copy link

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

Copy link

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

@github-actions github-actions bot removed the state: conflict PR with conflict label Jan 30, 2024

type: leaf

mlag_on_orphan_port_channel_downlink: false
Copy link
Contributor

Choose a reason for hiding this comment

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

is the behavior withtrue being tested anywhere? (I mean explicitly tested to check the difference between true and false)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think so

Copy link
Contributor

Choose a reason for hiding this comment

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

True is the default and before adding this we saw a change on another device, where our changes here removed mlag. After adding this knob, we no longer change that other leaf. So effectively yes, we are testing it there. I need to check commit history to figure out where it was.

Copy link
Contributor

Choose a reason for hiding this comment

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

image
This was the other switch being changed before we added the toggle for this behavioral change.

@github-actions github-actions bot added the state: conflict PR with conflict label Jan 31, 2024
Copy link

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

Copy link

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

@github-actions github-actions bot removed the state: conflict PR with conflict label Jan 31, 2024
@github-actions github-actions bot removed the state: conflict PR with conflict label Feb 2, 2024
Copy link

github-actions bot commented Feb 2, 2024

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

@github-actions github-actions bot added the state: conflict PR with conflict label Feb 2, 2024
Copy link

github-actions bot commented Feb 2, 2024

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

Copy link
Contributor

@emilarista emilarista left a comment

Choose a reason for hiding this comment

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

Tested with double and single uplink to same parent and, LGTM!

@ClausHolbechArista
Copy link
Contributor

Approved. I will resolve the conflict once we have merged #3584

@github-actions github-actions bot removed the state: conflict PR with conflict label Feb 6, 2024
Copy link

github-actions bot commented Feb 6, 2024

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

@ClausHolbechArista ClausHolbechArista merged commit 70f2956 into aristanetworks:devel Feb 6, 2024
38 checks passed
@jrecchia1029 jrecchia1029 deleted the single_uplink_to_mlag_pair branch March 22, 2024 13:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
one approval This PR has one approval and is only missing one more. rn: Feat(eos_designs) role: eos_designs issue related to eos_designs 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.

Configure Non MLAG Port Channel to Downstream Single Homed L2 Switch
4 participants