Allow "isd loops" by default. #3184
Conversation
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @kormat)
a discussion (no related file):
I disagree with allowing ISD loops by default. It just further adds to the explosion of number of beacons and the core of the network should be well enough connected to not rely on these types of paths (by default). Can you give a reasoning why they are generally beneficial?
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @shitz)
a discussion (no related file):
Previously, shitz wrote…
I disagree with allowing ISD loops by default. It just further adds to the explosion of number of beacons and the core of the network should be well enough connected to not rely on these types of paths (by default). Can you give a reasoning why they are generally beneficial?
The beacon explosion is not something that will hurt us in the short-term, and is something we need to fix regardless of this. On the other hand, even our default testing topo will break if the single link between the core ASes in ISD2 fails.
They are "beneficial" in the sense that you're not arbitrarily throwing away perfectly good connectivity.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @shitz)
a discussion (no related file):
Previously, kormat (Stephen Shirley) wrote…
The beacon explosion is not something that will hurt us in the short-term, and is something we need to fix regardless of this. On the other hand, even our default testing topo will break if the single link between the core ASes in ISD2 fails.
They are "beneficial" in the sense that you're not arbitrarily throwing away perfectly good connectivity.
(with the pull model for core beaconing the whole question goes away, too)
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @kormat)
a discussion (no related file):
Previously, kormat (Stephen Shirley) wrote…
(with the pull model for core beaconing the whole question goes away, too)
OTOH, SCIONLab will be hurt by this. It's arguable whether this should be default, since connectivity within an ISD should be more reliable than across ISDs. Disallowing it would also filter out more degenerate cases where a single ISD is crossed three or more times. The point might be moot with the core beaconing redesign, but I'd rather see it as "exceptionally allow ISD loops" rather than "exceptionally disallow them".
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @shitz)
a discussion (no related file):
Previously, shitz wrote…
OTOH, SCIONLab will be hurt by this. It's arguable whether this should be default, since connectivity within an ISD should be more reliable than across ISDs. Disallowing it would also filter out more degenerate cases where a single ISD is crossed three or more times. The point might be moot with the core beaconing redesign, but I'd rather see it as "exceptionally allow ISD loops" rather than "exceptionally disallow them".
It's a config option - scionlab can turn it to the desired setting. If you don't know that you need to disable isd loops, you shouldn't be disabling them - as again, you are throwing away perfectly good connectivity. If we look at our production + scionlab + our CI, they should be enabled for prod+CI, and not for scionlab. That doesn't seem like a convincing reason to have them disable by default.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @shitz)
a discussion (no related file):
since connectivity within an ISD should be more reliable than across ISDs
This is a non-argument. "more reliable" != "can't fail". And it can fail.
There was a problem hiding this comment.
Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @shitz)
a discussion (no related file):
Code wise
I let you two battle out, whether this change should go in.
There was a problem hiding this comment.
Reviewable status:
complete! all files reviewed, all discussions resolved
Disallowing them is something you'd only want to do in very specific circumstances.
Disallowing them is something you'd only want to do in very specific
circumstances.
This change is