| Message ID | 20211018121403.842185-4-mark.d.gray@redhat.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | northd: Introduce incremental processing framework | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
| ovsrobot/github-robot-_ovn-kubernetes | fail | github build: failed |
On Mon, Oct 18, 2021 at 5:14 AM Mark Gray <mark.d.gray@redhat.com> wrote: > > destroy_nat_entries() iterates over nat_entries using 'n_nat' > as the number of NAT entries from the NB database. This behaviour can be > incorrect as it assumes that there are 'n_nat' 'nat_entries'. 'struct > ovn_datapath' should maintain a count of 'nat_entries' in 'struct > ovn_datapath' rather than read the value from NBDB IDL to properly > account for the number of 'nat_entries'. > > This issue becomes appatent when using destroy_nat_entries() > as part of an incremental processing loop: > > Consider an example in which we have completed iteration x, and > started iteration x+1. If we add a NAT entry to NBDB between iteration x > and iteration x+1, od->nbr->n_nat will contain the updated value of > n_nat as the northbound record will have been updated. However, if > we do not (re)initialized the nat entries in od, od->n_nat_entries > could be equal to the previous value. This can cause destroy_nat_entries() > to loop over the wrong number of 'nat_entries' causing unexpected > behaviour. > > Signed-off-by: Mark Gray <mark.d.gray@redhat.com> > --- > northd/northd.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/northd/northd.c b/northd/northd.c > index 1321e26faa9d..9e84ee6310a9 100644 > --- a/northd/northd.c > +++ b/northd/northd.c > @@ -603,6 +603,8 @@ struct ovn_datapath { > > /* NAT entries configured on the router. */ > struct ovn_nat *nat_entries; > + size_t n_nat_entries; > + > bool has_distributed_nat; > > /* Set of nat external ips on the router. */ > @@ -787,6 +789,7 @@ init_nat_entries(struct ovn_datapath *od) > od->has_distributed_nat = true; > } > } > + od->n_nat_entries = od->nbr->n_nat; > } > > static void > @@ -800,7 +803,7 @@ destroy_nat_entries(struct ovn_datapath *od) > destroy_lport_addresses(&od->dnat_force_snat_addrs); > destroy_lport_addresses(&od->lb_force_snat_addrs); > > - for (size_t i = 0; i < od->nbr->n_nat; i++) { > + for (size_t i = 0; i < od->n_nat_entries; i++) { > destroy_lport_addresses(&od->nat_entries[i].ext_addrs); > } > } > -- > 2.27.0 > Acked-by: Han Zhou <hzhou@ovn.org>
diff --git a/northd/northd.c b/northd/northd.c index 1321e26faa9d..9e84ee6310a9 100644 --- a/northd/northd.c +++ b/northd/northd.c @@ -603,6 +603,8 @@ struct ovn_datapath { /* NAT entries configured on the router. */ struct ovn_nat *nat_entries; + size_t n_nat_entries; + bool has_distributed_nat; /* Set of nat external ips on the router. */ @@ -787,6 +789,7 @@ init_nat_entries(struct ovn_datapath *od) od->has_distributed_nat = true; } } + od->n_nat_entries = od->nbr->n_nat; } static void @@ -800,7 +803,7 @@ destroy_nat_entries(struct ovn_datapath *od) destroy_lport_addresses(&od->dnat_force_snat_addrs); destroy_lport_addresses(&od->lb_force_snat_addrs); - for (size_t i = 0; i < od->nbr->n_nat; i++) { + for (size_t i = 0; i < od->n_nat_entries; i++) { destroy_lport_addresses(&od->nat_entries[i].ext_addrs); } }
destroy_nat_entries() iterates over nat_entries using 'n_nat' as the number of NAT entries from the NB database. This behaviour can be incorrect as it assumes that there are 'n_nat' 'nat_entries'. 'struct ovn_datapath' should maintain a count of 'nat_entries' in 'struct ovn_datapath' rather than read the value from NBDB IDL to properly account for the number of 'nat_entries'. This issue becomes appatent when using destroy_nat_entries() as part of an incremental processing loop: Consider an example in which we have completed iteration x, and started iteration x+1. If we add a NAT entry to NBDB between iteration x and iteration x+1, od->nbr->n_nat will contain the updated value of n_nat as the northbound record will have been updated. However, if we do not (re)initialized the nat entries in od, od->n_nat_entries could be equal to the previous value. This can cause destroy_nat_entries() to loop over the wrong number of 'nat_entries' causing unexpected behaviour. Signed-off-by: Mark Gray <mark.d.gray@redhat.com> --- northd/northd.c | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-)