On Mon, Oct 7, 2024 at 1:33 PM Gavin McKee via discuss
<ovs-discuss@openvswitch.org> wrote:
>
> Hi Numan,
>
> Yes, DNS entries can be added / moved / updated frequently in our
> setup . Our usebase can create or delete VMs in bulk using Terraform
> so it's likely that we should handle a scenario where these actions
> don't block DNS lookups.
>
> I'm in deeper water than usual here so would appreciate it if you
> could make a proposal for the dns_cache_mutex . I'm fully open to any
> approach that will ensure that we keep processing DNS requests
> (especially the external) as blocking these is a big problem.
I've some ideas. I'll see if I can spin up a patch.
Thanks
Numan
>
> Gav
>
> On Mon, 7 Oct 2024 at 10:07, Numan Siddique <num...@ovn.org> wrote:
> >
> > On Mon, Oct 7, 2024 at 12:13 PM Gavin McKee via discuss
> > <ovs-discuss@openvswitch.org> wrote:
> > >
> > > Numan,
> > >
> > > Hopefully I don't embarrass myself too much here, but here goes :) .
> > > If we implemented a separate thread for performing DNS lookups would
> > > that work ?
> >
> > I see a couple of problems here.
> >
> > 1. The new thread will be sharing the "swconn" connection with
> > the pinctrl thread. Not sure how much of any issue this may cause.
> > Looks like "rconn_send" has a mutex.
> >
> > 2. There is a possibility that when the new thread iterates over the
> > dns_cache shash, the main ovn-controller
> > thread can modify this from pinctrl_run().
> >
> > Before going this way, is it possible to explore adding another
> > mutex say - "dns_cache_mutex" (or any other appropriate name).
> >
> > pinctrl thread before calling pinctrl_handle_dns_lookup() will lock
> > this mutex instead of the
> > main "pinctrl_mutex" and sync_dns_cache() will lock this mutex when it
> > is adding or deleting the dns_cache.
> >
> > What do you think ? This way dns requests will not be blocked due to
> > ovn-controller waking up and calling pinctrl_run().
> > It will be blocked though when SB dns entries are updated. Or is
> > that the case in your setup ? Do OVN dns records
> > get updated very frequently ?
> >
> > Thanks
> > Numan
> >
> > >
> > > diff --git a/controller/pinctrl.c b/controller/pinctrl.c
> > > index c86b4f940..aaa316542 100644
> > > --- a/controller/pinctrl.c
> > > +++ b/controller/pinctrl.c
> > > @@ -189,6 +189,27 @@ static struct pinctrl pinctrl;
> > >
> > > static void init_buffered_packets_ctx(void);
> > > static void destroy_buffered_packets_ctx(void);
> > > +
> > > +/* DNS thread */
> > > +static struct latch dns_thread_exit;
> > > +static pthread_t dns_thread;
> > > +
> > > +/* DNS query queue */
> > > +struct dns_query {
> > > + struct dp_packet packet;
> > > + struct ofputil_packet_in pin;
> > > + struct ofpbuf userdata;
> > > + struct ofpbuf *continuation;
> > > + struct ovs_list node;
> > > +};
> > > +
> > > +static struct ovs_list dns_query_queue =
> > > OVS_LIST_INITIALIZER(&dns_query_queue);
> > > +static struct ovs_mutex dns_queue_mutex = OVS_MUTEX_INITIALIZER;
> > > +static struct seq *dns_seq;
> > > +
> > > +static void dns_queue_push(struct dns_query *query);
> > > +static struct dns_query *dns_queue_pop(void);
> > > +static void *dns_lookup_thread(void *arg OVS_UNUSED);
> > > static void
> > > run_buffered_binding(const struct sbrec_mac_binding_table
> > > *mac_binding_table,
> > > struct ovsdb_idl_index *sbrec_port_binding_by_key,
> > > @@ -572,6 +593,10 @@ pinctrl_init(void)
> > > latch_init(&pinctrl.pinctrl_thread_exit);
> > > pinctrl.pinctrl_thread = ovs_thread_create("ovn_pinctrl",
> > > pinctrl_handler,
> > > &pinctrl);
> > > + latch_init(&dns_thread_exit);
> > > + dns_seq = seq_create();
> > > + pinctrl.dns_thread = ovs_thread_create("dns_lookup",
> > > dns_lookup_thread, NULL);
> > > +
> > > }
> > >
> > > static ovs_be32
> > > @@ -3435,6 +3460,51 @@ dns_build_ptr_answer(
> > > #define DNS_RCODE_SERVER_REFUSE 0x5
> > > #define DNS_QUERY_TYPE_CLASS_LEN (2 * sizeof(ovs_be16))
> > >
> > > +static void *
> > > +dns_lookup_thread(void *arg OVS_UNUSED)
> > > +{
> > > + while (!latch_is_set(&dns_thread_exit)) {
> > > + struct dns_query *query = dns_queue_pop();
> > > + if (query) {
> > > + bool processed = pinctrl_handle_dns_lookup(swconn,
> > > &query->packet,
> > > + &query->pin,
> > > &query->userdata,
> > > +
> > > query->continuation);
> > > + if (!processed) {
> > > + queue_msg(swconn, ofputil_encode_resume(&query->pin,
> > > +
> > > query->continuation,
> > > +
> > > ofputil_protocol_from_ofp_version(
> > > +
> > > rconn_get_version(swconn))));
> > > + }
> > > + ofpbuf_delete(query->continuation);
> > > + free(query);
> > > + }
> > > + poll_block();
> > > + }
> > > + return NULL;
> > > +}
> > > +
> > > +static void
> > > +dns_queue_push(struct dns_query *query)
> > > +{
> > > + ovs_mutex_lock(&dns_queue_mutex);
> > > + ovs_list_push_back(&dns_query_queue, &query->node);
> > > + seq_change(dns_seq);
> > > + ovs_mutex_unlock(&dns_queue_mutex);
> > > +}
> > > +
> > > +static struct dns_query *
> > > +dns_queue_pop(void)
> > > +{
> > > + struct dns_query *query = NULL;
> > > + ovs_mutex_lock(&dns_queue_mutex);
> > > + if (!ovs_list_is_empty(&dns_query_queue)) {
> > > + query = CONTAINER_OF(ovs_list_pop_front(&dns_query_queue),
> > > struct dns_query, node);
> > > + }
> > > + ovs_mutex_unlock(&dns_queue_mutex);
> > > + return query;
> > > +}
> > > +
> > > +
> > > /* Called with in the pinctrl_handler thread context. */
> > > static void
> > > pinctrl_handle_dns_lookup(
> > > @@ -3804,11 +3874,16 @@ process_packet_in(struct rconn *swconn, const
> > > struct ofp_header *msg)
> > > break;
> > >
> > > case ACTION_OPCODE_DNS_LOOKUP:
> > > - ovs_mutex_lock(&pinctrl_mutex);
> > > - pinctrl_handle_dns_lookup(swconn, &packet, &pin, &userdata,
> > > - &continuation);
> > > - ovs_mutex_unlock(&pinctrl_mutex);
> > > - break;
> > > + {
> > > + struct dns_query *query = xmalloc(sizeof *query);
> > > + dp_packet_clone(&query->packet, &packet);
> > > + query->pin = pin;
> > > + ofpbuf_clone(&query->userdata, &userdata);
> > > + query->continuation = continuation ? ofpbuf_clone(continuation)
> > > : NULL;
> > > + dns_queue_push(query);
> > > + poll_immediate_wake();
> > > + }
> > > + break;
> > >
> > > case ACTION_OPCODE_LOG:
> > > handle_acl_log(&headers, &userdata,
> > >
> > > On Fri, 4 Oct 2024 at 15:59, Gavin McKee <gavmcke...@googlemail.com>
> > > wrote:
> > > >
> > > > Hi Numan,
> > > >
> > > > Yes , the DNS packets are sent by VMs . We have DNS records for these
> > > > VMs so the controller is catching all DNS requests and failing to
> > > > respond in a timely manner.
> > > >
> > > > I think this relates to the fact that there is no incremental handler
> > > > for SB DNS.
> > > >
> > > > In short the blocking thread causes us all sorts of problems .
> > > >
> > > > Hope that makes sense
> > > >
> > > > Gav
> > > >
> > > > On Fri, Oct 4, 2024 at 15:34 Numan Siddique <num...@ovn.org> wrote:
> > > >>
> > > >> On Fri, Oct 4, 2024 at 5:50 PM Gavin McKee via discuss
> > > >> <ovs-discuss@openvswitch.org> wrote:
> > > >> >
> > > >> > Hi,
> > > >> >
> > > >> > We currently experience DNS related timeouts when the ovn controller
> > > >> > is under high load.
> > > >> >
> > > >> > Will this commit ensure that DNS packets needing to be forwarded to
> > > >> > an
> > > >> > external resolver will be processed in a timely manner and not lead
> > > >> > to
> > > >> > timeouts?
> > > >> > https://github.com/ovn-org/ovn/commit/762ae66cd70efa149d91d35305fcef0040e9addd
> > > >> >
> > > >>
> > > >> This particular patch is useful when the ovn-controller calls
> > > >> dns_resolve() of ovs/lib/dns_resolve.c [1]
> > > >> to resolve a dns entry.
> > > >>
> > > >> In your case, what are these DNS packets? Are these sent by VMs and
> > > >> ovn-controller pinctrl thread handles
> > > >> these requests and its blocking due to pinctrl_mutex ?
> > > >>
> > > >> If so, I think we need to find a solution for this problem, but the
> > > >> above commit will definitely not help you.
> > > >> Maybe we can maintain a separate mutex to protect the dns_cache [2] so
> > > >> that even if ovn-controller main thread
> > > >> is busy (and not updating the internal dns cache), the DNS packets
> > > >> from the VMs can be handled without any delay
> > > >> in the pinctrl thread.
> > > >>
> > > >> [1] -
> > > >> https://github.com/openvswitch/ovs/blob/main/lib/dns-resolve.c#L148
> > > >> [2] -
> > > >> https://github.com/ovn-org/ovn/blob/main/controller/pinctrl.c#L3288
> > > >>
> > > >>
> > > >> Thanks
> > > >> Numan
> > > >>
> > > >> > We currently run OVN 23.09.04.
> > > >> >
> > > >> > Thanks
> > > >> >
> > > >> > Gav
> > > >> > _______________________________________________
> > > >> > discuss mailing list
> > > >> > disc...@openvswitch.org
> > > >> > https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
> > > >> >
> > > _______________________________________________
> > > discuss mailing list
> > > disc...@openvswitch.org
> > > https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
> _______________________________________________
> discuss mailing list
> disc...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
_______________________________________________
discuss mailing list
disc...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-discuss
