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
