[ovs-dev] [PATCH ovn] Include OVS as a git submodule.

[Ilya Maximets]

On 1/21/21 2:08 PM, Dumitru Ceara wrote:
> On 1/20/21 10:10 PM, Mark Michelson wrote:
>> OVN developers have had isssues with the current method by which OVS
>> source code is used by OVN.
>>
>> * There is no way to record the minimum commit/version of OVS to use
>>    when compiling OVN.
>> * When debugging issues, bisecting OVN commits may also requires
>>    simultaneously changing OVS commits. This makes for multiple moving
>>    targets to try to track.
>> * Performance improvements made to OVS libraries and OVSDB may benefit
>>    OVN. However, there's no way to encourage the use of the improved OVS
>>    source.
>>
>> By using a submodule, it allows for OVN to record a specific commit of
>> OVS that is expected to be used.
>>
>> Signed-off-by: Mark Michelson <mmichels at redhat.com>
>> ---
> 
> Hi Mark,
> 
> Many thanks for working on this!
> 
> Should we also modify the CI jobs in .github/workflows/test.yml to run OVN CI when compiled against the new OVS submodule?  Right now linux-build.sh always checks out upstream OVS master branch.

+1
We need to be sure that OVN builds with the current version of the submodule,
i.e. that we didn't forget to move the submodule while starting to use new
features from OVS.

> 
> I guess it's also interesting to run (maybe just one job, e.g., clang) against upstream OVS master branch.

Not sure if this should be part of the default run, but it might be
good to have a separate scheduled job that will test against master
once a week, for example.  Could be added later, separately from this
patch.

> 
>>   .gitmodules                             |  3 +++
>>   Documentation/intro/install/general.rst | 19 ++++++++++++++++---
>>   Makefile.am                             | 20 ++++++++++----------
>>   acinclude.m4                            |  2 +-
>>   build-aux/initial-tab-whitelist         |  1 +
>>   ovs                                     |  1 +
>>   6 files changed, 32 insertions(+), 14 deletions(-)
>>   create mode 100644 .gitmodules
>>   create mode 160000 ovs
>>
>> diff --git a/.gitmodules b/.gitmodules
>> new file mode 100644
>> index 000000000..5c36a018f
>> --- /dev/null
>> +++ b/.gitmodules
>> @@ -0,0 +1,3 @@
>> +[submodule "ovs"]
>> +    path = ovs
>> +    url = git at github.com:openvswitch/ovs.git
> 
> Wouldn't it be better to use https://github.com/openvswitch/ovs.git instead?

+1
git protocol is banned in many environments and corporate networks.

> 
>> diff --git a/Documentation/intro/install/general.rst b/Documentation/intro/install/general.rst
>> index 65b1f4a40..1f19baec4 100644
>> --- a/Documentation/intro/install/general.rst
>> +++ b/Documentation/intro/install/general.rst
>> @@ -66,6 +66,9 @@ To compile the userspace programs in the OVN distribution, you will
>>   need the following software:
>>     - Open vSwitch (https://docs.openvswitch.org/en/latest/intro/install/).
>> +  Open vSwitch is included as a submodule in the OVN source code. It is
>> +  kept at the minimum version required in order for OVN to compile. See
> 
> What if we rephrase this to "in order for OVN to work properly." or something similar?  I'm thinking for example of the case when, even though compilation works fine, OVN may crash because of a bug in the OVS submodule's version of the libraries.

The pphrase 'minimum required to compile' also conflicts with the statement
in a commit message about performance.  So, it's not a minimal, but recommended
version. 

> 
>> +  below for instructions about how to use a different OVS source location.
>>     - GNU make
>>   @@ -153,9 +156,19 @@ Before configuring OVN, clone, configure and build Open vSwitch.
>>   Configuring
>>   -----------
>>   -Configure the package by running the configure script. You need to
>> -invoke configure with atleast the argument --with-ovs-source.
>> -For example::
>> +OVN requires Open vSwitch source code to be present in order to compile.
>> +The easiest way to fulfill this requirement is to use the included ovs
>> +submodule. After cloning the OVN source, run the following to initialize
>> +the ovs submodule::
>> +
>> +    $ git submodule udpate --init
> 
> s/udpate/update
> 
>> +
>> +Then configure the package by running the configure script::
> 
> We should also mention that we need to build OVS inside the submodule.

+1

> 
> Regards,
> Dumitru