[ovs-dev] [PATCH ovn 0/2] northd: Split northd

[Mark Gray]

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