On Mon, Feb 12, 2024 at 5:12 PM Xavier Simonart <[email protected]> wrote:
> Prefix delegation suffered from potential issues: > - An unicast SOLICIT message was sometimes sent instead of a RENEW. > - Pînctrl was sometimes not waking up as expected when timers such > as T1 or T2 expired. > - If reply was not received for a REQUEST, no new SOLICIT was sent. > - State update to RENEW, done by ipv6_prefixd_should_inject, was > sometimes done before send_ipv6_prefixd (as part of may_inject), > and sometimes not, depending for instance of garp. This influenced > how T1 and T2 were calculated. State change was also influenced > by other prefixes (e.g. another prefix in SOLICIT might skip an > update to RENEW). > > This patch fixes those issue by: > [1] Do not try to send RENEW in PENDING state as this resulted in > SOLICIT sent unicast. > [2] Make sure to wake up in DONE, PENDING, RENEW and REBIND states. > [3] Make sure to wake up and send REQUEST in REQUEST state in case > no reply received. > [4] Ensure that RENEW and REBIND states are updated indeoendently of > other network elements such as garps or other prefixes. > > The patch also modifies the "IPv6 prefix delegation" system test to > test scenarios 1 and 2, and reduces renewal-time to reduce test > duration. > > This issue was highlighted by frequent failures of "IPv6 prefix > delegation" test in Build_and_Test ovsrobot. > > Signed-off-by: Xavier Simonart <[email protected]> > --- > controller/pinctrl.c | 38 +++++++++++++++++++++++++++++------ > tests/system-common-macros.at | 38 ++++++++++++++++++++++------------- > 2 files changed, 56 insertions(+), 20 deletions(-) > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c > index 98b29de9f..3f041b295 100644 > --- a/controller/pinctrl.c > +++ b/controller/pinctrl.c > @@ -1171,7 +1171,7 @@ ipv6_prefixd_send(struct rconn *swconn, struct > ipv6_prefixd_state *pfd) > return pfd->next_announce; > } > > - if (pfd->state == PREFIX_DONE) { > + if ((pfd->state == PREFIX_PENDING) || (pfd->state == PREFIX_DONE)) { > goto out; > } > > @@ -1222,33 +1222,58 @@ static bool ipv6_prefixd_should_inject(void) > struct ipv6_prefixd_state *pfd = iter->data; > long long int cur_time = time_msec(); > > - if (pfd->state == PREFIX_SOLICIT) { > + if (pfd->state == PREFIX_SOLICIT || pfd->state == PREFIX_REQUEST) > { > return true; > } > if (pfd->state == PREFIX_DONE && > cur_time > pfd->last_complete + pfd->t1) { > - pfd->state = PREFIX_RENEW; > return true; > } > if (pfd->state == PREFIX_RENEW && > cur_time > pfd->last_complete + pfd->t2) { > - pfd->state = PREFIX_REBIND; > pfd->uuid.len = 0; > return true; > } > if (pfd->state == PREFIX_REBIND && > cur_time > pfd->last_complete + pfd->vlife_time) { > - pfd->state = PREFIX_SOLICIT; > return true; > } > } > return false; > } > > +static void ipv6_prefixd_update_state(struct ipv6_prefixd_state *pfd) > +{ > + long long int cur_time = time_msec(); > + > + if (pfd->state == PREFIX_DONE && > + cur_time > pfd->last_complete + pfd->t1) { > + pfd->state = PREFIX_RENEW; > + return; > + } > + if (pfd->state == PREFIX_RENEW && > + cur_time > pfd->last_complete + pfd->t2) { > + pfd->state = PREFIX_REBIND; > + pfd->uuid.len = 0; > + return; > + } > + if (pfd->state == PREFIX_REBIND && > + cur_time > pfd->last_complete + pfd->vlife_time) { > + pfd->state = PREFIX_SOLICIT; > + return; > + } > +} > + > static void > ipv6_prefixd_wait(long long int timeout) > { > - if (ipv6_prefixd_should_inject()) { > + /* We need to wake up in all states : > + * - In SOLICIT and REQUEST states we need to wakeup to handle > + * next_announce timer. > + * - In DONE, PENDING, RENEW and REBIND states, we need to wake up to > + * handle T1, T2 timers. > + */ > + if (!shash_is_empty(&ipv6_prefixd)) { > poll_timer_wait_until(timeout); > } > } > @@ -1266,6 +1291,7 @@ send_ipv6_prefixd(struct rconn *swconn, long long > int *send_prefixd_time) > if (*send_prefixd_time > next_msg) { > *send_prefixd_time = next_msg; > } > + ipv6_prefixd_update_state(pfd); > } > } > > diff --git a/tests/system-common-macros.at b/tests/system-common-macros.at > index 4bfc74582..9e7f40176 100644 > --- a/tests/system-common-macros.at > +++ b/tests/system-common-macros.at > @@ -418,15 +418,14 @@ ovn-nbctl lsp-add public public1 \ > ovn-nbctl set logical_router_port rp-public options:prefix_delegation=true > ovn-nbctl set logical_router_port rp-public options:prefix=true > ovn-nbctl set logical_router_port rp-sw0 options:prefix=true > -ovn-nbctl set logical_router_port rp-sw1 options:prefix=true > > OVN_POPULATE_ARP > > ovn-nbctl --wait=hv sync > > cat > /etc/dhcp/dhcpd.conf <<EOF > -option dhcp-rebinding-time 15; > -option dhcp-renewal-time 10; > +option dhcp-rebinding-time 10; > +option dhcp-renewal-time 5; > option dhcp6.unicast 2001:1db8:3333::1; > subnet6 2001:1db8:3333::/64 { > prefix6 2001:1db8:3333:100:: 2001:1db8:3333:111:: /96; > @@ -445,7 +444,6 @@ ovn-nbctl --wait=hv sync > > OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-public > ipv6_prefix | cut -c4-15)" = ""]) > OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-sw0 > ipv6_prefix | cut -c4-15)" = ""]) > -OVS_WAIT_WHILE([test "$(ovn-nbctl get logical_router_port rp-sw1 > ipv6_prefix | cut -c4-15)" = ""]) > > AT_CHECK([ovn-nbctl get logical_router_port rp-public ipv6_prefix | cut > -c3-16], [0], [dnl > [2001:1db8:3333] > @@ -453,27 +451,39 @@ AT_CHECK([ovn-nbctl get logical_router_port > rp-public ipv6_prefix | cut -c3-16], > AT_CHECK([ovn-nbctl get logical_router_port rp-sw0 ipv6_prefix | cut > -c3-16], [0], [dnl > [2001:1db8:3333] > ]) > -AT_CHECK([ovn-nbctl get logical_router_port rp-sw1 ipv6_prefix | cut > -c3-16], [0], [dnl > -[2001:1db8:3333] > -]) > > prefix=$(ovn-nbctl list logical_router_port rp-public | awk -F/ > '/ipv6_prefix/{print substr($ 1,25,9)}' | sed 's/://g') > ovn-nbctl list logical_router_port rp-public > /tmp/rp-public > -ovn-nbctl set logical_router_port rp-sw0 options:prefix=false > -ovn-nbctl set logical_router_port rp-sw1 options:prefix=false > -# Renew message > -NS_CHECK_EXEC([server], [tcpdump -c 1 -nni s1 ip6[[48:1]]=0x05 and > ip6[[113:4]]=0x${prefix} > renew.pcap &]) > + > +# Wait for 2 renew on each port. > +NS_CHECK_EXEC([server], [tcpdump -c 4 -nni s1 ip6[[48:1]]=0x05 and > ip6[[113:4]]=0x${prefix} > renew.pcap &]) > # Reply message with Status OK > -NS_CHECK_EXEC([server], [tcpdump -c 1 -nni s1 ip6[[48:1]]=0x07 and > ip6[[81:4]]=0x${prefix} > reply.pcap &]) > +NS_CHECK_EXEC([server], [tcpdump -c 4 -nni s1 ip6[[48:1]]=0x07 and > ip6[[81:4]]=0x${prefix} > reply.pcap &]) > > OVS_WAIT_UNTIL([ > total_pkts=$(cat renew.pcap | wc -l) > - test "${total_pkts}" = "1" > + test "${total_pkts}" = "4" > ]) > > OVS_WAIT_UNTIL([ > total_pkts=$(cat reply.pcap | wc -l) > - test "${total_pkts}" = "1" > + test "${total_pkts}" = "4" > +]) > + > +ovn-nbctl set logical_router_port rp-public options:prefix=false > +ovn-nbctl set logical_router_port rp-sw0 options:prefix=false > +ovn-nbctl --wait=hv set logical_router_port rp-sw1 options:prefix=true > +sleep_sb > +NS_CHECK_EXEC([server], [tcpdump -c 2 -nni s1 ip6[[48:1]]=0x05 and > ip6[[113:4]]=0x${prefix} > renew.pcap &]) > + > +# Sleep enough to have solicit and renew being sent, then wait for 2 > renew. > +# The reply to the request will usually be received as sb is sleeping. > +# Hence, the reply to the first renew will be received when sb is ro. > +sleep 10 > +wake_up_sb > +OVS_WAIT_UNTIL([ > + total_pkts=$(cat renew.pcap | wc -l) > + test "${total_pkts}" = "2" > ]) > > kill $(pidof tcpdump) > -- > 2.41.0 > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev > Looks good to me, thanks. Acked-by: Ales Musil <[email protected]> -- Ales Musil Senior Software Engineer - OVN Core Red Hat EMEA <https://www.redhat.com> [email protected] <https://red.ht/sig> _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
