On Mon, Aug 30, 2021 at 3:46 PM Mark Gray <[email protected]> wrote: > > On 30/08/2021 20:27, Mark Michelson wrote: > > On 8/30/21 6:14 AM, Mark Gray wrote: > >> On 27/08/2021 18:56, Mark Michelson wrote: > >>> Hi Mark, > >>> > >>> I had a look at this series, but I'm not 100% sure what the intent is. > >>> In patch 2, you mentioned modularity and the ability to include northd > >>> as a library. But I'm not sure where that allows us to go. Can you > >>> elaborate a bit? > >> > >> Thanks for the review Mark! > >> > >> I think that describing my future intentions may, perhaps, be confusing > >> matters. Regardless of those intentions, I think that splitting out this > >> code makes sense in order simplify the code. ovn-northd.c is >15k lines > >> of code and any steps we can take to slowly break this down into > >> separate files should help make the code more manageable. Also, I am not > >> functionally changing anything? Therefore, I think that this series can > >> stand on its own. > >> > >> However, I also think my commit message could be misinterpreted as I am > >> not trying to make a northd "library" in the lib.a sense. I am exploring > >> the possibility of integrating I-P into northd and, rather than adding > >> the I-P code directly into ovn-northd.c, I am trying to add it in a way > >> that should be slightly less invasive and hopefully a bit easier to > >> manage. My basic idea is to organize the code so that each I-P node > >> consists of a corresponding .c and .h file (with all the associated > >> benefits - testing, etc). I would then run the IP framework from > >> ovn-northd.c. This would create dependencies something like this. > >> > >> ovn-northd.c ────► inc-proc.c/.h ───┬───► node1.c/.h > >> │ > >> ├───► node2.c/.h > >> │ > >> └───► node3.c/.h > >> > >> My first goal is to implement all of northd.c into one node and then > >> try to split out some elements of it into other nodes. I am trying to do > >> all this in order to incrementally(!) add I-P to northd rather than just > >> try to rewrite the whole thing in one fell swoop. Regardless of these > >> plans, which may amount to nothing, I think this series is independent. > >> However, if you don't like the idea, maybe I could try to add all the > >> I-P code directly into ovn-northd.c - i just think it will be messier. > > > > Thanks for the explanation, Mark. For the record, I usually am in favor > > of breaking large source files into smaller, more manageable, more > > testable units, and this is no exception. > > > > I'm fine with the change, but I wanted to double-check about the intent > > here, because > > Thanks for that. If the approach is generally good, then I can, at > least, proceed with my changes locally and then do a rebase before merge. > > > 1) It wasn't 100% clear to me just from the code changes themselves > > 2) I was trying to figure out if this is something that we should try to > > get into OVN 21.09.> > > For 1) your explanation here has made things more clear. > > > > For 2) I'll need to know what else you have up your sleeve. For > > instance, if you have follow-up changes ready to go, then this may be a > > candidate for 21.09. If, however, follow-up changes are not ready and > > they will take weeks to write, then I think even with an ACK this change > > should wait until 21.12. That way, it and additional followups can all > > be part of 21.12. > >
I see one slight advantage of having these 2 patches considered for 21.09 even if other patches are not ready provided these patches do not change any functionality. It will be easier to backport any bug fixes to 21.09 branch if the future bug fix patches touch the refactored/moved-to-other-file code. Numan > I'll see how I progress this week before soft-freeze. If I can make > soft-freeze, then we can consider it. > > Thanks again. > > > > >> > >>> > >>> Thanks, > >>> Mark > >>> > >>> On 8/26/21 3:04 PM, Mark Gray wrote: > >>>> Please note that this commit mainly involves moving code around with > >>>> minimal > >>>> code changes. However, due to tight coupling between ovn-northd.c and > >>>> northd.c, > >>>> some minor changes were needed. For reference, and to help reviews, > >>>> please > >>>> examine the following at a minimum: > >>>> > >>>> * Configuration of the probe interval in northd.c > >>>> (ovsdb_idl_set_probe_interval()) > >>>> * Passing of "use_parallel_build" and "lflow_locks" from ovn-northd.c and > >>>> northd.c. > >>>> * Update of "struct northd_context": additopn of fields and move to h > >>>> file.>> > >>>> The commits were (hopefully) structured in a way to make the review > >>>> easier. As > >>>> this change touches all of ovn-northd, any change to "master" will make > >>>> a rebase > >>>> necessary and probably difficult. Therefore, if the general ideas is OK, > >>>> then > >>>> it would be great if this series could be expedited to prevent many > >>>> rebases! > >>>> > >>>> Thanks > >>>> > >>>> Mark Gray (2): > >>>> ovn-northd: Rename ovn-northd.c to northd.c > >>>> northd: Split northd.c > >>>> > >>>> Documentation/tutorials/ovn-openstack.rst | 154 +- > >>>> northd/automake.mk | 2 + > >>>> northd/lrouter.dl | 2 +- > >>>> northd/northd.c | 14418 +++++++++++++++++++ > >>>> northd/northd.h | 42 + > >>>> northd/ovn-northd.c | 14717 +------------------- > >>>> northd/ovn.rs | 2 +- > >>>> northd/ovn_northd.dl | 2 +- > >>>> tests/ovn-northd.at | 2 +- > >>>> 9 files changed, 14709 insertions(+), 14632 deletions(-) > >>>> create mode 100644 northd/northd.c > >>>> create mode 100644 northd/northd.h > >>>> > >>> > >> > > > > _______________________________________________ > dev mailing list > [email protected] > https://mail.openvswitch.org/mailman/listinfo/ovs-dev _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
