[quagga-dev 7918] Re: ospfd: connected routes not redistributed

Joakim Tjernlund joakim.tjernlund at transmode.se
Tue Apr 13 09:45:58 BST 2010


>
> Dear maintainers
>
> I found the following bug in quagga 0.99.16, as well as in the latest master
> repository quagga.git.
>
> When adding a connected route (using vtysh, without restart) to the
> redistribution access list of ospfd, while static routes already exist, the
> update timer ospf_distribute_list_update_timer() is being run for static
> routes only. That way, the connected route never appears in the OSPF database,
> until quagga is completely restarted.
>
> The update timer for connected routes is cancelled in ospfd/
> ospfd_zebra.c:ospf_distribute_list_update():976, were a new timer is scheduled
> for static routes, caused by the loop in ospf_filter_update().
>
> My fix replaces the global timer thread pointer with an array, that tracks a
> dedicated timer thread for each route type.
>
> How to reproduce:
> - have a static route for network A (192.168.88.0/24) in zebra.conf
> - have a connected route by adding a second IP for network B (192.168.99.0/24)
> to some interface
> - permit network A for ospf redistribution and deny network B
> - launch quagga (show ip ospf database contains network A only)
> - in vtysh remove the deny rule for B and add a permit rule for B
>
> The expected result would be that the connected route B appears in the OSPF
> database. But it does not.
> However, if you edit the config file instead of using vtysh and restart the
> daemons, A and B show up in the OSPF database as expected.
>
> I also experienced a significant delay when playing it the other way round
> (removing the access list entry for B):
> - have a static route for network A (192.168.88.0/24) in zebra.conf
> - have a connected route by adding a second IP for network B (192.168.99.0/24)
> to some interface
> - permit both network A and network B for ospf redistribution
> - launch quagga (show ip ospf database contains network A only)
> - in vtysh remove the permit rule for B
>
> It may take up to 20 minutes for B to actually being removed from the OSPF
> database. My patch seems to neutralize this effect too.
>
> Best Regards,
> Roman Hoog Antink


Hi Roman

I am not a maintainer but I looked into your patch. It is a good
catch. Cant believe the old code worked well enough to take this long for
this bug to be found :(

I think your patch is good, it creates a few extra entries in
ospf->t_distribute_update that won't trigger as not all types are
applicable to OSPF. Those will just waste a tiny about of memory though
so I don't mind too much.

However, you could add a list/array of route types instead and pass the list/array to the
thread. You would only stop the thread when the list becomes empty and start the
thread when the list becomes non empty. Thinking some more this is probably the
preferred solution as your current patch creates several threads that runs in parallel,
updating the dist list asynchronously.

    Jocke
>
> --- ./ospfd/ospfd.h.orig   2010-02-12 09:31:37.000000000 +0100
> +++ ./ospfd/ospfd.h   2010-04-12 11:48:11.000000000 +0200
> @@ -254,7 +254,7 @@ struct ospf
>    struct thread *t_router_lsa_update;   /* router-LSA update timer. */
>    struct thread *t_abr_task;            /* ABR task timer. */
>    struct thread *t_asbr_check;          /* ASBR check timer. */
> -  struct thread *t_distribute_update;   /* Distirbute list update timer. */
> +  struct thread *t_distribute_update[ZEBRA_ROUTE_MAX];   /* Distribute list
> update timer. */
>    struct thread *t_spf_calc;           /* SPF calculation timer. */
>    struct thread *t_ase_calc;      /* ASE calculation timer. */
>    struct thread *t_external_lsa;   /* AS-external-LSA origin timer. */
> --- ./ospfd/ospfd.c.orig   2010-02-12 09:31:37.000000000 +0100
> +++ ./ospfd/ospfd.c   2010-04-12 11:57:50.000000000 +0200
> @@ -480,7 +480,8 @@ ospf_finish_final (struct ospf *ospf)
>    OSPF_TIMER_OFF (ospf->t_maxage_walker);
>    OSPF_TIMER_OFF (ospf->t_abr_task);
>    OSPF_TIMER_OFF (ospf->t_asbr_check);
> -  OSPF_TIMER_OFF (ospf->t_distribute_update);
> +  for (i = 0; i < ZEBRA_ROUTE_MAX; i++)
> +    OSPF_TIMER_OFF (ospf->t_distribute_update[i]);
>    OSPF_TIMER_OFF (ospf->t_lsa_refresher);
>    OSPF_TIMER_OFF (ospf->t_read);
>    OSPF_TIMER_OFF (ospf->t_write);
> --- ./ospfd/ospf_zebra.c.orig   2010-02-12 09:31:37.000000000 +0100
> +++ ./ospfd/ospf_zebra.c   2010-04-12 11:55:13.000000000 +0200
> @@ -941,7 +941,7 @@ ospf_distribute_list_update_timer (struc
>    if (ospf == NULL)
>      return 0;
>
> -  ospf->t_distribute_update = NULL;
> +  ospf->t_distribute_update[type] = NULL;
>
>    zlog_info ("Zebra[Redistribute]: distribute-list update timer fired!");
>
> @@ -973,11 +973,11 @@ ospf_distribute_list_update (struct ospf
>      return;
>
>    /* If exists previously invoked thread, then cancel it. */
> -  if (ospf->t_distribute_update)
> -    OSPF_TIMER_OFF (ospf->t_distribute_update);
> +  if (ospf->t_distribute_update[type])
> +    OSPF_TIMER_OFF (ospf->t_distribute_update[type]);
>
>    /* Set timer. */
> -  ospf->t_distribute_update =
> +  ospf->t_distribute_update[type] =
>      thread_add_timer (master, ospf_distribute_list_update_timer,
>                        (void *) type, OSPF_DISTRIBUTE_UPDATE_DELAY);
>  }
>
> _______________________________________________
> Quagga-dev mailing list
> Quagga-dev at lists.quagga.net
> http://lists.quagga.net/mailman/listinfo/quagga-dev




More information about the Quagga-dev mailing list