Hi Alexandra, I wanted to share (me, not the AI) that I like the fact that you're documenting this interesting use case! Looking forward to v2.
Regards, Dumitru On 9/22/26 1:56 PM, Dumitru Ceara via dev wrote: > NOTE: This review was produced with the help of an AI assistant. > A human reviewer has examined the feedback below and considered it > worth sharing. > --- > > Git SHA: 9d8a8aaa3a201f2bdde3b076fdc80068516e1817 > Author: Alexandra Rukomoinikova <[email protected]> > Subject: tests: Document the virtual gateway setup. > > This patch documents the "virtual gateway" setup where several > ovn-controller instances share a single chassis name and a single > anycast encapsulation IP reachable over the underlay through ECMP. > It adds an RST topic document, a NEWS entry, a note in the > ovn-controller man page, and a multinode test that deploys two > gateway nodes as one virtual chassis and verifies north-south > connectivity and that the controllers do not fight over the > shared records. > >> diff --git a/Documentation/topics/virtual-gateway.rst >> b/Documentation/topics/virtual-gateway.rst >> new file mode 100644 >> index 000000000..7d39bfbe2 >> --- /dev/null >> +++ b/Documentation/topics/virtual-gateway.rst >> [ ... ] >> +writes that keeps them from fighting each other. >> +Two consequences follow from this. The columns that OVN derives from the >> +local node rather than from the configuration must be forced to the same >> value >> +on every node, More detailed information on this is provided in the >> +``Identical OVS and OVN configuration and features`` section below. > > A few issues in this paragraph: > > 1. There is no blank line between the preceding paragraph (ending > with "fighting each other.") and "Two consequences follow from > this." RST will merge them into one paragraph, which is probably > not intended. > > 2. "Two consequences follow from this" promises two items, but only > one is stated (the columns must be forced to the same value). > The second consequence is never mentioned. > > 3. "on every node, More detailed information" -- the comma should > be a period (or the sentence should be restructured). > >> [ ... ] >> +For the same reason, every chassis scoped configuration option has to be >> +identical on all the nodes, including , for example: >> ``external_ids:hostname``, >> +``external_ids:ovn-bridge-mappings`` and all others. > > Nit: there is a spurious space before the comma: "including , for > example" should be "including, for example". > >> [ ... ] >> +Example >> +------- >> + >> +The ``ovn multinode virtual gateway - anycast encap ip with underlay ECMP`` >> +test in ``tests/multinode.at`` sets up a complete example of a two node > > The test name referenced here does not match the actual AT_SETUP. > The test is registered as "ovn multinode virtual gateway" (without > the "- anycast encap ip with underlay ECMP" suffix). > >> diff --git a/tests/multinode-macros.at b/tests/multinode-macros.at >> index 1c85f79f8..208af60a1 100644 >> --- a/tests/multinode-macros.at >> +++ b/tests/multinode-macros.at >> [ ... ] >> -# multinode_setup_controller NODE ENCAP_IP REMOTE_IP [ENCAP_TYPE] >> +# multinode_setup_controller NODE SYSTEM_ID ENCAP_IP REMOTE_IP [ENCAP_TYPE] >> # >> -# Sets up controller on specified node. >> +# Sets up controller on specified node. SYSTEM_ID defaults to NODE; passing >> a >> +# different value (or the same value on more than one node) allows a test to >> +# control the chassis name the node registers with. > > The function now also takes a 6th parameter (start_controller) > which is used by the new test (passing "no"), but the usage > comment does not document it. Should be something like: > > # multinode_setup_controller NODE SYSTEM_ID ENCAP_IP REMOTE_IP > # [ENCAP_TYPE] [START_CONTROLLER] > >> [ ... ] >> m_as $c ovs-vsctl set open . >> external-ids:ovn-bridge-datapath-type=system >> >> + >> # Add back br-ex which was removed when removing ovs conf.db > > Nit: spurious blank line added (two blank lines between the > external-ids block and the comment). > >> diff --git a/tests/multinode.at b/tests/multinode.at >> index 0c277f5e8..eb6948d5a 100644 >> --- a/tests/multinode.at >> +++ b/tests/multinode.at >> @@ -9,7 +9,7 @@ check_fake_multinode_setup >> cleanup_multinode_resources >> >> # Test East-West switching >> -check multinode_nbctl ls-add sw0 >> + > > This removes the "ls-add sw0" from the first test ("ovn multinode > basic test"), but the very next line is still "lsp-add sw0 > sw0-port1" which requires sw0 to already exist. Was this deletion > intentional? It looks like it will break the first test. > >> [ ... ] >> +# On ovn-chassis-2 (aka client) we have client port in public switch for >> checking >> +# internal connectivity, public switch is connected to dgp port that >> claimed by >> +# virtual chassis. Compute and client node both have underlay route for >> virtul gw ip (1.1.1.1) > > Nit: "virtul" should be "virtual". > >> [ ... ] >> +# The distributed gateway port is claimed by the virtual gateway chassis. >> +m_wait_row_count Port_Binding 1 logical_port=cr-lr0-public >> +vgw_uuid=$(m_central_as ovn-sbctl --bare --columns _uuid find Chassis >> name=$vgw_name) >> +m_wait_column "$vgw_uuid" Port_Binding chassis logical_port=cr-lr0-public >> + >> +check multinode_nbctl --wait=hv sync >> +m_wait_for_ports_up sw0-port1 >> + >> +# The distributed gateway port is claimed by the virtual gateway chassis. >> +m_wait_row_count Port_Binding 1 logical_port=cr-lr0-public >> +vgw_uuid=$(m_central_as ovn-sbctl --bare --columns _uuid find Chassis >> name=$vgw_name) >> +m_wait_column "$vgw_uuid" Port_Binding chassis logical_port=cr-lr0-public > > This block is duplicated. The "distributed gateway port is claimed" > check, the sync, the wait-for-ports-up and the second identical > block look like an accidental copy-paste. Should one of these be > removed? > >> [ ... ] >> +for c in $vgw1 $vgw2; do >> + multinode_setup_controller $c $vgw_name $vgw_encap_ip $central_ip >> geneve no >> + on_exit "m_as $c ip addr del $vgw_encap_ip/32 dev lo" >> + check m_as $c ip addr add $vgw_encap_ip/32 dev lo >> + check m_as $c ovs-vsctl set open . external-ids:hostname=$vgw_name >> + check m_as $c ovs-vsctl set open . >> external-ids:ovn-bridge-mappings=public:br-ex >> +done >> + >> +for c in $vgw1 $vgw2; do >> + m_as $c /usr/share/ovn/scripts/ovn-ctl start_controller >> ${CONTROLLER_SSL_ARGS} >> +done > > The multinode_setup_controller call with start_controller="no" > sets up OVS and all the external-ids but does not start > ovn-controller. The controller is then started in a second loop > after all external-ids (hostname, bridge-mappings) are set. This > is a good pattern -- it avoids a race where one controller starts > and writes the Chassis record before the other has finished > configuring its bridge-mappings. > >> [ ... ] >> +# Restore the original chassis configuration of the nodes used by this test. >> +# Connect the chassis back to the original northd and remove northd per >> chassis. >> +for i in 1 2; do >> + chassis="ovn-chassis-$i" >> + ip=$(m_as $chassis ip -4 addr show eth1 | grep inet | awk '{print $2}' >> | cut -d'/' -f1) >> + >> + multinode_cleanup_ic $chassis >> + multinode_setup_controller $chassis $chassis $ip "170.168.0.2" >> + multinode_cleanup_northd $chassis >> +done > > The cleanup restores ovn-chassis-1 and ovn-chassis-2, but the > two gateway nodes (ovn-gw-1 and ovn-gw-2) that were reconfigured > with the anycast system-id are not restored to their original > configuration. Subsequent tests in the same file that rely on the > gw nodes having their default system-id might be affected. Is > AT_CLEANUP sufficient to handle this, or should the gw nodes be > restored here as well? > > > Supplementary findings from full review of 9d8a8aaa3. > > The preliminary review noted the deletion of "check multinode_nbctl > ls-add sw0" from the first test ("ovn multinode basic test") as > suspicious. Build and closer inspection confirm this is a real bug. > > The hunk at the top of tests/multinode.at removes the only ls-add > for sw0: > > -check multinode_nbctl ls-add sw0 > + > > The very next line is: > > check multinode_nbctl lsp-add sw0 sw0-port1 > > Since cleanup_multinode_resources() deletes the NB database and > restarts northd, no switch named sw0 exists at this point. The > lsp-add call requires the switch to already exist and will fail. > This breaks the first multinode test ("ovn multinode basic test"). > > This deletion appears to be an accidental part of the patch; the > ls-add was not related to the virtual gateway feature and should be > restored. > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
