Details

Reviewers

mav
emaste
melifaro
asomers

Group Reviewers

network

Commits

rG64bb05e04fc3: carp tests: Basic functionality test
rS359828: carp tests: Basic functionality test

Summary

Set up three vnet jails, bridged together. Run carp between two of them.
Attempt to provoke locking / epoch issues.

Diff Detail

Repository

rS FreeBSD src repository - subversion

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

kp created this revision.Apr 5 2020, 5:01 PM

Herald added subscribers: melifaro, ae. · View Herald TranscriptApr 5 2020, 5:01 PM

Harbormaster completed remote builds in B30301: Diff 70230.Apr 5 2020, 5:01 PM

Remove incorrect sponsorship lines.

Harbormaster completed remote builds in B30307: Diff 70243.Apr 6 2020, 7:44 AM

LGTM, please see comment inline.

tests/sys/net/if_carp.sh
48 ↗	(On Diff #70243)	Would it be possible to prefix jail names with the the test name to reduce the possibility of jail name clashes?

This revision is now accepted and ready to land.Apr 6 2020, 10:45 AM

melifaro added inline comments.Apr 6 2020, 10:48 AM

tests/sys/net/if_carp.sh
57 ↗	(On Diff #70243)	Maybe worth moving this test to netinet/, as it doesn't share any ipv4/ipv6 code?

Would it be possible to add IPv6 counterpart to this test, so both IPv4 and IPv6 control plane parts gets covered?

Generally looks good to me, especially if we just test locking. For genera-purpose testing would be good to test master election, failover, etc.

tests/sys/net/if_carp.sh
58 ↗	(On Diff #70243)	Shouldn't this have /32 mask? I am curios how it works for routing table with another IP from the same subnet with /29, if it works.

In D24303#534728, @melifaro wrote:

Would it be possible to add IPv6 counterpart to this test, so both IPv4 and IPv6 control plane parts gets covered?

That'll hopefully be pretty straightforward. I'll give that a try.

tests/sys/net/if_carp.sh
48 ↗	(On Diff #70243)	Possibly, but I'm not sure it'll help. Many of these tests mess with the host system routing table (by keeping epair interfaces in the host vnet, and assigning addresses). So we'd have to do more than just change the jail names. We'd also want to do that manually (or at least deterministically), because debugging tests with random jail names is painful.
57 ↗	(On Diff #70243)	Yeah, that makes sense. I'd put it here because it's somewhat similar in nature to the bridge and epair tests, but that code does live in net/. I'll update the test and move it.
58 ↗	(On Diff #70243)	I have no idea. I don't pretend to understand carp. I mostly transposed the test script I initially used to trigger the panics this test tries to provoke. It might make sense to use a different subnet for the real IPs from the carp IP.

Move the test, add IPv6 test

This revision now requires review to proceed.Apr 6 2020, 5:39 PM

Herald added a subscriber: asomers. · View Herald TranscriptApr 6 2020, 5:39 PM

Harbormaster completed remote builds in B30319: Diff 70265.Apr 6 2020, 5:39 PM

asomers added inline comments.Apr 6 2020, 6:24 PM

tests/sys/netinet/carp.sh
52 ↗	(On Diff #70265)	Better to use RFC5737 addresses: 192.0.2.0/24
63 ↗	(On Diff #70265)	Why the hard-coded sleep?

kp added inline comments.Apr 6 2020, 9:41 PM

tests/sys/netinet/carp.sh
63 ↗	(On Diff #70265)	That gives carp a bit of time to come up. Otherwise we risk trying to ping an address that's not there yet.

Fix IP addresses and general cleanup

Harbormaster completed remote builds in B30330: Diff 70277.Apr 6 2020, 9:41 PM

asomers added inline comments.Apr 6 2020, 9:58 PM

tests/sys/netinet/carp.sh
63 ↗	(On Diff #70265)	It's unnecessarily slow on fast systems, and could potentially cause false negatives on slow systems. Please change it to a polling loop.

melifaro added inline comments.Apr 7 2020, 8:54 PM

tests/sys/net/if_carp.sh
48 ↗	(On Diff #70243)	Indeed, there are a bunch of tests that changes routing in the host system. Ideally we'd want to reduce the number of such tests or at least not increase it :-) I've changed a bunch of mine tests to use dual jail configuration to avoid changes in the host system and I plan to convert more. I totally agree that random jail names complicates debugging a lot. I'm proposing prefixing each jail with a test name, so they're unique and one can easily understand relationship between the jail name and pariticular test piece.
57 ↗	(On Diff #70243)	Thank you! One note - my understanding is that we split af-specific tests into netinet/ and netinet6/ , so one can easily test for specific address family.

Replace 'sleep 3' by a polling loop for carp readiness

Harbormaster completed remote builds in B30360: Diff 70336.Apr 8 2020, 11:34 AM

Rename jails

Harbormaster completed remote builds in B30362: Diff 70338.Apr 8 2020, 11:56 AM

Wait until one of the two jails has become carp master

Waiting until one of them leaves INIT state is insufficient. There's a
transition period where both carp jails are in BACKUP state, and the carp
address is unreachable.

Harbormaster completed remote builds in B30363: Diff 70339.Apr 8 2020, 12:52 PM

asomers accepted this revision.Apr 8 2020, 2:03 PM

This revision is now accepted and ready to land.Apr 8 2020, 2:03 PM

melifaro accepted this revision as: melifaro.Apr 8 2020, 2:07 PM

Closed by commit rS359828: carp tests: Basic functionality test (authored by kp). · Explain WhyApr 12 2020, 4:13 PM

This revision was automatically updated to reflect the committed changes.

kp added a commit: rS359828: carp tests: Basic functionality test.

Herald added a subscriber: imp. · View Herald TranscriptApr 12 2020, 4:13 PM

kp added a commit: rG64bb05e04fc3: carp tests: Basic functionality test.Nov 22 2021, 5:16 AM

Herald added a subscriber: glebius. · View Herald TranscriptNov 22 2021, 5:16 AM