FRR: Adding support for interface shutdown command #6169
Conversation
|
This change is |
Codecov Report
@@ Coverage Diff @@
## master #6169 +/- ##
============================================
- Coverage 72.88% 72.81% -0.07%
+ Complexity 34859 34742 -117
============================================
Files 2818 2818
Lines 142254 141749 -505
Branches 17146 17004 -142
============================================
- Hits 103676 103216 -460
+ Misses 30404 30360 -44
+ Partials 8174 8173 -1 |
There was a problem hiding this comment.
Reviewed 7 of 7 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @kylehoferamzn)
projects/batfish/src/main/java/org/batfish/representation/cumulus/FrrInterface.java, line 19 at r1 (raw file):
private final @Nonnull String _name; private final @Nullable String _vrfName; private @Nonnull Boolean _shutdown = false;
Are you absolutely sure the behavior you describe (shutdown = false being the default) is always true? i.e., it doesn't vary based on properties of iface (e.g., physical/l3/switchport/portchannel/etc.) Reason I ask, is because we've seen that w/ nxos for example. 🙄
if yes, then ok to make this a primitive boolean
if no, then mark @Nullable and just do firstNonNull(iface.getShutdown(), false) in conversion. Later, we can replace false w/ dynamically-computed default value if needed.
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @nonnull)
projects/batfish/src/main/java/org/batfish/representation/cumulus/FrrInterface.java, line 19 at r1 (raw file):
Previously, progwriter (Victor Heorhiadi) wrote…
Are you absolutely sure the behavior you describe (shutdown = false being the default) is always true? i.e., it doesn't vary based on properties of iface (e.g., physical/l3/switchport/portchannel/etc.) Reason I ask, is because we've seen that w/ nxos for example. 🙄
if yes, then ok to make this a primitive
boolean
if no, then mark@Nullableand just dofirstNonNull(iface.getShutdown(), false)in conversion. Later, we can replacefalsew/ dynamically-computed default value if needed.
Well I'm not 100% sure of anything ;)
But I tested in in FRR 7.2 where "unconfigured" means admin up - for any case where the "shutdown" command isn't seen, Batfish's behavior will continue as-is. All this code does is shutdown interfaces that are configured as "shutdown" in Zebra. I tested no shutdown and all it does is remove the "shutdown" command (essentially brings it back to default)
I was gonna make it a primitive but then I can't annotate it with @nonnull. Figured it'd be useful for IDE inspection but on second thought, it'll probably catch the null anyway. I'll go ahead and fix that.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r2.
Reviewable status:complete! all files reviewed, all discussions resolved
Behavior tested is that if you issue shutdown in Zebra, the interface is admin down. If you configure "no shut" it just deletes the command - similar to unconfigured. For that reason I made it non-null and initialized to false as default. /etc/network/interfaces state parsing happens prior and also controls (hence I do not have a no-shut force admin up for Zebra)