Skip to content

[14/n] [sync-switch] detect conflicting per-switch bgp configs#10655

Open
sunshowers wants to merge 1 commit into
sunshowers/spr/main.14n-sync-switch-detect-conflicting-per-switch-bgp-configsfrom
sunshowers/spr/14n-sync-switch-detect-conflicting-per-switch-bgp-configs
Open

[14/n] [sync-switch] detect conflicting per-switch bgp configs#10655
sunshowers wants to merge 1 commit into
sunshowers/spr/main.14n-sync-switch-detect-conflicting-per-switch-bgp-configsfrom
sunshowers/spr/14n-sync-switch-detect-conflicting-per-switch-bgp-configs

Conversation

@sunshowers

Copy link
Copy Markdown
Contributor

Previously, we'd silently pick one of the BGP configs. That seems wrong -- let's reject this situation outright.

This is a functional change similar to the earlier one in #10650, where a previously-invalid state was silently accepted and is now rejected.

Like in the earlier commits in the stack, I haven't touched the non-bootstore paths in this PR to avoid colliding with other work. But they have the same issue as well where they silently pick one.

Depends on:

Created using spr 1.3.6-beta.1

@jgallagher jgallagher left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I'm less sure about the safety of this one than #10650. I'm not sure we have anything in place that would prevent spurious configs from being set up, and I'm slightly worried that we might take a "working by accident" rack to "not working at all". This is definitely a better place to be though.

If we had a rack that had extra configs where we ended up picking the first, what would the net effect be? Every peer applies that config (even if it should've been applying a different one, which might be okay if all the fields that matter were the same)? Or only the peers that match that config get settings applied? I'm wondering if it would be worth it to try to explicitly encode one of those behaviors vs failing outright.

To be clear: I'd be happy to be talked into "just fail outright" if we can either convince ourselves that it's sufficiently unlikely or we're okay with dealing with the fallout.

@sunshowers

Copy link
Copy Markdown
Contributor Author

If we had a rack that had extra configs where we ended up picking the first, what would the net effect be? Every peer applies that config (even if it should've been applying a different one [...])? Or only the peers that match that config get settings applied?

Hmm I'm not sure how the second possibility could be represented in the bootstore settings. It is the first one, and the net effect of picking the first (current behavior) is that every peer only gets the first config's local ASN and announce set.

which might be okay if all the fields that matter were the same

Yeah that is the case that would regress. One option here, and this is a bit belt-and-suspenders, is to only complain if any of the fields that matter differs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants