diff mbox series

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

Message ID 20210120211002.2959466-1-mmichels@redhat.com
State Superseded
Headers show
Series [ovs-dev] Include OVS as a git submodule. | expand

Commit Message

Mark Michelson Jan. 20, 2021, 9:10 p.m. UTC 
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@redhat.com>
---
 .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

Comments

0-day Robot Jan. 20, 2021, 10:18 p.m. UTC | #1 
Bleep bloop.  Greetings Mark Michelson, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


checkpatch:
WARNING: Line has non-spaces leading whitespace
#40 FILE: .gitmodules:2:
	path = ovs

WARNING: Line has non-spaces leading whitespace
#41 FILE: .gitmodules:3:
	url = git@github.com:openvswitch/ovs.git

Lines checked: 190, Warnings: 2, Errors: 0


Please check this out.  If you feel there has been an error, please email aconole@redhat.com

Thanks,
0-day Robot
Dumitru Ceara Jan. 21, 2021, 1:08 p.m. UTC | #2 
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@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.

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

>   .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@github.com:openvswitch/ovs.git

Wouldn't it be better to use https://github.com/openvswitch/ovs.git instead?

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

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

Regards,
Dumitru
Ilya Maximets Jan. 21, 2021, 1:24 p.m. UTC | #3 
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@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@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
Mark Michelson Jan. 21, 2021, 6:15 p.m. UTC | #4 
Thanks for the feedback, guys.

On 1/21/21 8:24 AM, Ilya Maximets wrote:
> 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@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.
> 

Yes thanks for this. I'll update the CI jobs.

>>
>> 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.
I'll focus on correcting the CI jobs first and then focus on this 
improvement as a separate change.

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

This was the one thing I was least sure about when making this change. I 
use git protocol for everything mostly out of convenience. Using https 
requires having to enter github credentials all the time, and that's not 
something I wanted here.

I'll change to https. If git protocol is blocked in certain 
environments, that's enough of a reason to switch.

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

I'll go with "minimum recommended version for OVN's operation" (or 
something very similar)

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

Will do.

> 
>>
>> Regards,
>> Dumitru
>
Ilya Maximets Jan. 21, 2021, 8:34 p.m. UTC | #5 
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@redhat.com>
> ---
>  .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
> 

For some reason this patch broke OSX build:

  The following files are in git but not the distribution:
  ovs
  make[1]: *** [dist-hook-git] Error 1
  make: *** [all] Error 2

Caught by ovsrobot:
  https://github.com/ovsrobot/ovn/runs/1738128420?check_suite_focus=true#step:5:2998

Best regards, Ilya Maximets.
diff mbox series

Patch

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@github.com:openvswitch/ovs.git
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
+  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
+
+Then configure the package by running the configure script::
+
+    $ ./configure
+
+If desired, you may override the default ovs source code location by
+specifying the location using --with-ovs-source::
 
     $ ./configure --with-ovs-source=/path/to/ovs/source
 
diff --git a/Makefile.am b/Makefile.am
index 7ce3d27e4..c779328ec 100644
--- a/Makefile.am
+++ b/Makefile.am
@@ -157,6 +157,7 @@  noinst_HEADERS += $(EXTRA_DIST)
 
 ro_c = echo '/* -*- mode: c; buffer-read-only: t -*- */'
 ro_shell = printf '\043 Generated automatically -- do not modify!    -*- buffer-read-only: t -*-\n'
+submodules = $(shell grep 'path =' .gitmodules | sed -E 's/\s*path = (.*)/\1/')
 
 SUFFIXES += .in
 .in:
@@ -216,6 +217,8 @@  dist-hook-git: distfiles
 	@if test -e $(srcdir)/.git && (git --version) >/dev/null 2>&1; then \
 	  (cd $(srcdir) && git ls-files) | grep -v '\.gitignore$$' | \
 	    grep -v '\.gitattributes$$' | \
+	    grep -v '\.gitmodules$$' | \
+	    grep -v "$(submodules)" | \
 	    LC_ALL=C sort -u > all-gitfiles; \
 	  LC_ALL=C comm -1 -3 distfiles all-gitfiles > missing-distfiles; \
 	  if test -s missing-distfiles; then \
@@ -247,8 +250,8 @@  ALL_LOCAL += config-h-check
 config-h-check:
 	@cd $(srcdir); \
 	if test -e .git && (git --version) >/dev/null 2>&1 && \
-	  git --no-pager grep -L '#include <config\.h>' `git ls-files | grep '\.c$$' | \
-	    grep -vE '^ovs/datapath|^ovs/lib/sflow|^ovs/datapath-windows|^python|^ovs/python'`; \
+	  git --no-pager grep -L '#include <config\.h>' `git ls-files | grep -v $(submodules) | grep '\.c$$' | \
+	    grep -vE '^python'`; \
 	then \
 	  echo "See above for list of violations of the rule that"; \
 	  echo "every C source file must #include <config.h>."; \
@@ -261,8 +264,7 @@  ALL_LOCAL += printf-check
 printf-check:
 	@cd $(srcdir); \
 	if test -e .git && (git --version) >/dev/null 2>&1 && \
-	  git --no-pager grep -n -E -e '%[-+ #0-9.*]*([ztj]|hh)' --and --not -e 'ovs_scan' `git ls-files | grep '\.[ch]$$' | \
-	    grep -vE '^ovs/datapath|^ovs/lib/sflow'`; \
+	  git --no-pager grep -n -E -e '%[-+ #0-9.*]*([ztj]|hh)' --and --not -e 'ovs_scan' `git ls-files | grep -v $(submodules) | grep '\.[ch]$$'`; \
 	then \
 	  echo "See above for list of violations of the rule that"; \
 	  echo "'z', 't', 'j', 'hh' printf() type modifiers are"; \
@@ -288,7 +290,7 @@  ALL_LOCAL += check-assert-h-usage
 check-assert-h-usage:
 	@if test -e $(srcdir)/.git && (git --version) >/dev/null 2>&1 && \
 	  (cd $(srcdir) && git --no-pager grep -l -E '[<]assert.h[>]') | \
-	  $(EGREP) -v '^ovs/lib/(sflow_receiver|vlog).c$$|^ovs/tests/|^tests/'; \
+	  $(EGREP) -v '^tests/'; \
 	then \
 	  echo "Files listed above unexpectedly #include <""assert.h"">."; \
 	  echo "Please use ovs_assert (from util.h) instead of assert."; \
@@ -304,8 +306,7 @@  ALL_LOCAL += check-endian
 check-endian:
 	@if test -e $(srcdir)/.git && (git --version) >/dev/null 2>&1 && \
 	  (cd $(srcdir) && git --no-pager grep -l -E \
-	   -e 'BIG_ENDIAN|LITTLE_ENDIAN' --and --not -e 'BYTE_ORDER' | \
-	  $(EGREP) -v '^ovs/datapath/|^ovs/include/sparse/rte_'); \
+	   -e 'BIG_ENDIAN|LITTLE_ENDIAN' --and --not -e 'BYTE_ORDER'); \
 	then \
 	  echo "See above for list of files that misuse LITTLE""_ENDIAN"; \
 	  echo "or BIG""_ENDIAN.  Please use WORDS_BIGENDIAN instead."; \
@@ -329,7 +330,7 @@  check-tabs:
 	@cd $(srcdir); \
 	if test -e .git && (git --version) >/dev/null 2>&1 && \
 	  grep -ln "^	" \
-	    `git ls-files \
+	    `git ls-files | grep -v $(submodules) \
 	      | grep -v -f build-aux/initial-tab-whitelist` /dev/null \
 	      | $(EGREP) -v ':[ 	]*/?\*'; \
 	then \
@@ -344,8 +345,7 @@  thread-safety-check:
 	@cd $(srcdir); \
 	if test -e .git && (git --version) >/dev/null 2>&1 && \
 	  grep -n -f build-aux/thread-safety-blacklist \
-	    `git ls-files | grep '\.[ch]$$' \
-	      | $(EGREP) -v '^ovs/datapath|^ovs/lib/sflow'` /dev/null \
+	    `git ls-files | grep -v $(submodules) | grep '\.[ch]$$'` /dev/null \
 	      | $(EGREP) -v ':[ 	]*/?\*'; \
 	then \
 	  echo "See above for list of calls to functions that are"; \
diff --git a/acinclude.m4 b/acinclude.m4
index a797adc82..2f8755961 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -338,7 +338,7 @@  AC_DEFUN([OVN_CHECK_OVS], [
       AC_ERROR([$OVSDIR is not an OVS source directory])
     fi
   else
-    AC_ERROR([OVS source dir path needs to be specified (use --with-ovs-source)])
+    OVSDIR=`pwd`/ovs
   fi
 
   AC_MSG_RESULT([$OVSDIR])
diff --git a/build-aux/initial-tab-whitelist b/build-aux/initial-tab-whitelist
index 216cd2ed3..b2f5a0791 100644
--- a/build-aux/initial-tab-whitelist
+++ b/build-aux/initial-tab-whitelist
@@ -8,3 +8,4 @@ 
 ^xenserver/
 ^debian/rules.modules$
 ^debian/rules$
+^\.gitmodules$
diff --git a/ovs b/ovs
new file mode 160000
index 000000000..50e5523b9
--- /dev/null
+++ b/ovs
@@ -0,0 +1 @@ 
+Subproject commit 50e5523b9b2b154e5fafc5acdcdec85e9cc5a330