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.
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
