carp: add a 'SUPPRESS' state

[kp]

Summary

Introduce a new state for carp interfaces where the link or interface
are down, or net.inet.carp.allow is 0.

The intent is to make it a bit easier for users to figure out why a
given carp address is not negotiating MASTER/BACKUP.

Suggested by: melifaro@
Sponsored by: Rubicon Communications, LLC ("Netgate")

[kp]

There's a rather annoying bug left in this. If we add a carp address on an interface that's down (i.e. IFF_UP is not set) the interface will come up during this, but remain in SUPPRESS state.

That's because the SIOCSIFADDR ioctl is passed to ether_ioctl(), which sets IFF_UP in ifp->if_flags directly, without any event handling. Ordinarily we'd call if_up(), which gives carp a chance to react to the interface coming up. That's not the case here, so it stays in SUPPRESS.

Initial attempts to fix this, by replacing ifp->if_flags |= IFF_UP in ether_ioctl() with if_up(ifp) resulted in ifconfig: ioctl (SIOCAIFADDR): File exists when trying to set an address on the interface. There's clearly an order of operations problem around this, but I don't fully understand it.

In other words: this patch cannot go in as-is (or at least not until the IFF_UP issues are addressed).

[zlei]

When I was reviewing the design of CARP, I intended to introduce a new state COMPETING to resolve an annoying bug https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=269908 .

I'm not sure it is right to do so, too little resources for CARP rather than VRRP.

I'd suggest suspend this new feature, until we have a clear state machine.

The intent is to make it a bit easier for users to figure out why a given carp address is not negotiating MASTER/BACKUP.

I think we may have some debug / trace logs for that, other than a new state SUPPRESS.

[melifaro]

In D40317#917957, @kp wrote:

There's a rather annoying bug left in this. If we add a carp address on an interface that's down (i.e. IFF_UP is not set) the interface will come up during this, but remain in SUPPRESS state.

That's because the SIOCSIFADDR ioctl is passed to ether_ioctl(), which sets IFF_UP in ifp->if_flags directly, without any event handling. Ordinarily we'd call if_up(), which gives carp a chance to react to the interface coming up. That's not the case here, so it stays in SUPPRESS.

Initial attempts to fix this, by replacing ifp->if_flags |= IFF_UP in ether_ioctl() with if_up(ifp) resulted in ifconfig: ioctl (SIOCAIFADDR): File exists when trying to set an address on the interface. There's clearly an order of operations problem around this, but I don't fully understand it.

In other words: this patch cannot go in as-is (or at least not until the IFF_UP issues are addressed).

I ran into that when using netlink to configure interfaces. I guess we need to issue an eventhandler contification if some of interface flags were changed. I’ll take a look at that

[melifaro]

In other words: this patch cannot go in as-is (or at least not until the IFF_UP issues are addressed).

I ran into that when using netlink to configure interfaces. I guess we need to issue an eventhandler contification if some of interface flags were changed. I’ll take a look at that

I've raised D40332 to address lack of notifications to carp and the rest of the system.

[melifaro]

In D40317#918251, @melifaro wrote:

In other words: this patch cannot go in as-is (or at least not until the IFF_UP issues are addressed).

I ran into that when using netlink to configure interfaces. I guess we need to issue an eventhandler contification if some of interface flags were changed. I’ll take a look at that

I've raised D40332 to address lack of notifications to carp and the rest of the system.

This one got committed.

[rcm_rcm.sh]

sys/netinet/ip_carp.h

131

Should this be bumped to 3?

[kp]

sys/netinet/ip_carp.h

131

It should, but we should probably also improve the parsing on the set state side of things, because I don't think we want users to manually set 'SUPPRESS' (or to allow users to manually leave that state, for that matter).

[melifaro]

sys/netinet/ip_carp.h

131

I don't have the domain knowledge on the carp implementation and I don't hold any opinion on whether the state machine should be extended or not.

Personally I'd consider two things here:

1. does the state machine change simplify (or help) kernel handling?
2. does the state machine change simplify (or help) user handling?

If the answer for the first question is "no" then, maybe, (2) may be achieved by the other means (e.g. just providing string hint for the userland) ?

[glebius]

This creates invariant (sc_suppress == 1 && sc_state == SUPPRESS || sc_suppress == 0 && sc_state != SUPPRESS). I'm not in favor of this change, but if it goes in, I would suggest to remove sc_suppress fields and use sc_state as a flag of suppressed state.