dind: add support for ovn-kubernetes network plugin #15756

dcbw · 2017-08-12T13:04:46Z

@rajatchopra here's the Origin part...

Operation:

get a git checkout of ovn-kubernetes with my changes: go-controller: move scripts to Go code and add SSL support ovn-kubernetes/ovn-kubernetes#141
rebuild your DIND images hack/dind-cluster.sh build-images
OVN_ROOT=/path/to/ovn-kubernetes hack/dind-cluster.sh start -o

dcbw · 2017-08-31T20:42:20Z

@rajatchopra updated; we actually get pod IPs now even

dcbw · 2017-09-01T15:40:41Z

@stevekuznetsov @danwinship review from you guys would be useful too, thanks!

stevekuznetsov

Most comments apply generally to all of the bash scripts

stevekuznetsov · 2017-09-01T16:00:20Z

hack/dind-cluster.sh

@@ -607,6 +642,17 @@ case "${1:-""}" in
      esac
    done

+   # OVN requires CNI network plugin and OVN_ROOT to be set
+   if [[ -n "${USE_OVN}" ]]; then


With set -o nounset these will fail, use "${USE_OVN:-}" etc for them

stevekuznetsov · 2017-09-01T16:00:30Z

hack/dind-cluster.sh

@@ -94,6 +95,15 @@ function start() {
  echo "OPENSHIFT_ADDITIONAL_ARGS='${additional_args}'" > "${config_root}/additional-args"
  copy-runtime "${origin_root}" "${config_root}/"

+  ovn_kubernetes=
+  if [ -d "${ovn_root}" ]; then


Prefer [[ over [

stevekuznetsov · 2017-09-01T16:00:55Z

hack/dind-cluster.sh

+       exit 1
+     fi
+   elif [[ -n "${OVN_ROOT}" ]]; then
+     OVN_ROOT=


Do you want this empty or unset?

@stevekuznetsov empty

stevekuznetsov · 2017-09-01T16:01:47Z

images/dind/master/ovn-kubernetes-master-setup.sh

+function is-api-running() {
+  local config=$1
+
+  /usr/local/bin/oc --config="${kube_config}" get nodes &> /dev/null


/usr/local/bin/oc --config="${kube_config}" get --raw /healthz/ready

?

stevekuznetsov · 2017-09-01T16:09:26Z

images/dind/master/ovn-kubernetes-master-setup.sh

+    /usr/local/bin/oadm --config="${kube_config}" policy add-scc-to-user anyuid -z ovn
+  fi
+
+  ovnsecret=$(/usr/local/bin/oc --config="${kube_config}" get secrets | grep ovn | tail -1 | awk '{ print $1 }')


oc sa get-token instead of these two lines

stevekuznetsov · 2017-09-01T16:10:03Z

images/dind/master/ovn-kubernetes-master.sh

+  local config_dir=$1
+  local kube_config="${config_dir}/admin.kubeconfig"
+
+  ovnsecret=$(/usr/local/bin/oc --config="${kube_config}" get secrets | grep ovn | tail -1 | awk '{ print $1 }')


oc sa get-token

stevekuznetsov · 2017-09-01T16:10:58Z

images/dind/master/ovn-kubernetes-master.sh

+  token=$(/usr/local/bin/oc --config="${kube_config}" describe secret $ovnsecret | grep "token:" | awk '{ print $2 }')
+
+  local master_config="${config_dir}/master-config.yaml"
+  cluster_cidr=$(grep clusterNetworkCIDR ${master_config} | cut -f 4 -d' ')


do you have python? might be better to use yaml.load() here -- no guarantee that the value is on the same line

done, does that look any better?

danwinship · 2017-09-01T17:17:23Z

OVN.md

@@ -0,0 +1,16 @@
+ovn-kubernetes DIND
+====================


if this is intended to stick around it should probably go in docs/

danwinship · 2017-09-01T17:25:38Z

hack/dind-cluster.sh

@@ -765,6 +811,7 @@ start accepts the following options:
 -i                build container images before starting the cluster
 -r                remove an existing cluster
 -s                skip waiting for nodes to become ready
+ -o                enable the OVN network plugin; implies "-n cni" and valid OVN_ROOT


Can you just make this be -n ovn? And while you're there, make -n subnet/multitenant/networkpolicy work too? There's no reason we have to make the user write out the full name of the plugin since we restrict it to a limited set of supported plugins anyway.

done, new commit for the plugin arg renames too

danwinship · 2017-09-01T17:35:07Z

images/dind/master/ovn-kubernetes-master.sh

+	--ovn-south-db "tcp://${ovn_master_ip}:6642" \
+	--init-master `hostname` \
+	--net-controller
+#	--ovn-north-db "ssl://${ovn_nb_ip}:6641" \


what's all the commented-out stuff?

danwinship · 2017-09-01T17:39:06Z

images/dind/node/Dockerfile

@@ -25,8 +25,12 @@ RUN dnf -y update && dnf -y install\

 # Upgrade to a newer OVS. (This can go away when the base image is upgraded to F26.)
 RUN dnf -y install dnf-plugins-core &&\
- dnf -y copr enable danw/origin-dind-ovs &&\
- dnf -y update openvswitch
+ dnf -y copr enable leifmadsen/ovs-master &&\


Even though this is developer-only I feel like it would be better to use a COPR we maintain. I can update the packages in my COPR...

@danwinship if you could update the packages, that would be awesome.

OK, try danw/origin-dind-ovs-master.

danwinship · 2017-09-01T17:42:03Z

images/dind/node/Dockerfile

@@ -25,8 +25,12 @@ RUN dnf -y update && dnf -y install\

 # Upgrade to a newer OVS. (This can go away when the base image is upgraded to F26.)


"F26" there probably has to be "F27" now?

Actually, the original idea was that the previous RUN installs everything (including an out-of-date version of openvswitch), and then this RUN is just to handle the fact that Fedora's OVS is too old. But the patch below changes things so this RUN would still be necessary even if Fedora had the latest OVN (because we're not even installing OVN in the previous RUN). So either the installs should be rearranged to keep the old behavior, or else the comment should be rewritten to not imply that this section will eventually go away.

rajatchopra · 2017-09-01T18:36:18Z

OVN_ROOT=/path/to/ovn-kubernetes hack/dind-cluster.sh start -n cni

Needs a '-o' or OVN will not run.

dcbw · 2017-09-02T03:12:25Z

@stevekuznetsov @danwinship I believe I've addressed all your comments, PTAL thanks!

stevekuznetsov

Bash bits LGTM, one nit

stevekuznetsov · 2017-09-04T20:17:30Z

images/dind/master/ovn-kubernetes-master-setup.sh

+  fi
+
+  token=$(/usr/local/bin/oc --config="${kube_config}" sa get-token ovn)
+  echo "${token}" > ${config_dir}/ovn.token


Could collapse this

/usr/local/bin/oc --config="${kube_config}" sa get-token ovn > ${config_dir}/ovn.token

stevekuznetsov · 2017-09-04T20:17:44Z

hack/dind-cluster.sh

+    # OVN requires CNI network plugin and OVN_ROOT to be set
+    if [[ "${NETWORK_PLUGIN}" = "ovn" ]]; then
+      NETWORK_PLUGIN="cni"
+      if [[ -z "${OVN_ROOT}" ]]; then


If it can be unset use :- defaulting

@stevekuznetsov I defaulted it above:

ADDITIONAL_ARGS="" OVN_ROOT="${OVN_ROOT:-}" case "${1:-""}" in start) BUILD=

is that sufficent?

Yes, sorry -- didn't catch that

danwinship

Changes look good except for a bit of cruft.

Commit message still refers to OVN.md

danwinship · 2017-09-06T15:20:26Z

hack/dind-cluster.sh

+    copy-ovn-runtime "${ovn_root}" "${config_root}/"
+    ovn_kubernetes=1
+  else
+    ovn_kubernetes=


redundant, you already set it before the if

danwinship · 2017-09-06T15:21:30Z

hack/dind-cluster.sh

@@ -576,7 +609,7 @@ case "${1:-""}" in
    NETWORK_PLUGIN=
    REMOVE_EXISTING_CLUSTER=
    OPTIND=2
-    while getopts ":bin:rsN:" opt; do
+    while getopts ":boin:rsN:" opt; do


no more -o

danwinship · 2017-09-06T15:23:01Z

hack/dind-cluster.sh

@@ -765,6 +810,7 @@ start accepts the following options:
 -i                build container images before starting the cluster
 -r                remove an existing cluster
 -s                skip waiting for nodes to become ready
+ -o                enable the OVN network plugin; implies "-n cni" and valid OVN_ROOT


See OVN.md for more details.

-n now takes one of [ subnet | multitenant | networkpolicy | ovn | cni ]

dcbw · 2017-09-06T18:52:53Z

@danwinship @stevekuznetsov one more PTAL, thanks!

stevekuznetsov

LGTM from a Bash perspective

rajatchopra

I tested it locally and the cluster comes up fine.
/lgtm

rajatchopra · 2017-09-06T23:39:22Z

/test end_to_end

dcbw · 2017-09-07T15:12:55Z

@liggitt could we get an approve on this one? thanks!

dcbw · 2017-09-07T18:01:45Z

@stevekuznetsov can you /approve?

stevekuznetsov · 2017-09-07T18:17:28Z

/approve

openshift-merge-robot · 2017-09-07T18:17:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dcbw, rajatchopra, stevekuznetsov

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~hack/OWNERS~~ [stevekuznetsov]
~~images/dind/OWNERS~~ [dcbw,stevekuznetsov]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

openshift-merge-robot · 2017-09-07T20:16:28Z

Automatic merge from submit-queue (batch tested with PRs 14825, 15756, 16178, 16188, 16189)

openshift-merge-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Aug 12, 2017

openshift-merge-robot assigned tdawson and liggitt Aug 12, 2017

dcbw force-pushed the dind-ovn branch 2 times, most recently from 99dccf3 to 2b87ad4 Compare August 13, 2017 02:01

tdawson removed their assignment Aug 14, 2017

dcbw force-pushed the dind-ovn branch 2 times, most recently from 08b0549 to d19ce7f Compare August 31, 2017 20:38

dcbw requested review from danwinship, marun and stevekuznetsov September 1, 2017 15:40

dcbw changed the title ~~[RFC] dind: add support for ovn-kubernetes network plugin~~ dind: add support for ovn-kubernetes network plugin Sep 1, 2017

stevekuznetsov suggested changes Sep 1, 2017

View reviewed changes

danwinship reviewed Sep 1, 2017

View reviewed changes

dcbw force-pushed the dind-ovn branch 2 times, most recently from 8111eba to fa767e5 Compare September 2, 2017 03:11

dcbw force-pushed the dind-ovn branch from fa767e5 to cdf1031 Compare September 2, 2017 04:06

stevekuznetsov reviewed Sep 4, 2017

View reviewed changes

danwinship suggested changes Sep 6, 2017

View reviewed changes

dcbw added 2 commits September 6, 2017 13:52

dind: add support for ovn-kubernetes network plugin

da183b2

See OVN.md for more details.

dind: simplify network plugin argument

d489df1

-n now takes one of [ subnet | multitenant | networkpolicy | ovn | cni ]

dcbw force-pushed the dind-ovn branch from cdf1031 to d489df1 Compare September 6, 2017 18:52

stevekuznetsov approved these changes Sep 6, 2017

View reviewed changes

danwinship approved these changes Sep 6, 2017

View reviewed changes

openshift-ci-robot assigned rajatchopra Sep 6, 2017

rajatchopra approved these changes Sep 6, 2017

View reviewed changes

openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2017

openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 7, 2017

openshift-merge-robot merged commit 2fee53c into openshift:master Sep 7, 2017

		@@ -25,8 +25,12 @@ RUN dnf -y update && dnf -y install\

		# Upgrade to a newer OVS. (This can go away when the base image is upgraded to F26.)

dind: add support for ovn-kubernetes network plugin #15756

dind: add support for ovn-kubernetes network plugin #15756

Conversation

dcbw commented Aug 12, 2017 • edited Loading

dcbw commented Aug 31, 2017

dcbw commented Sep 1, 2017

stevekuznetsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajatchopra commented Sep 1, 2017

dcbw commented Sep 2, 2017

stevekuznetsov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

danwinship left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcbw commented Sep 6, 2017

stevekuznetsov left a comment

Choose a reason for hiding this comment

rajatchopra left a comment

Choose a reason for hiding this comment

rajatchopra commented Sep 6, 2017

dcbw commented Sep 7, 2017

dcbw commented Sep 7, 2017

stevekuznetsov commented Sep 7, 2017

openshift-merge-robot commented Sep 7, 2017

openshift-merge-robot commented Sep 7, 2017

dcbw commented Aug 12, 2017 •

edited

Loading