OVN: use config file via ConfigMap rather than environment variables by pecameron · Pull Request #217 · openshift/cluster-network-operator

pecameron · 2019-06-28T19:06:06Z

Create a new daemonset version that passes the ovn-config configMap
into the container as a file. Modify master and node daemonsets.

Create a ovnkube config file using a configMap.

This must merge after ovn-kubernetes/ovn-kubernetes#748
which sets up daemonset version 4

SDN-456
https://jira.coreos.com/browse/SDN-456

Signed-off-by: Phil Cameron pcameron@redhat.com

openshift-ci-robot · 2019-06-28T19:06:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: pecameron
To complete the pull request process, please assign dcbw
You can assign the PR to them by writing /assign @dcbw in a comment when ready.

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

Details

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

pecameron · 2019-07-16T20:29:18Z

@dcbw This has the changes we talked about. PTAL

…bles The ovn-config configMap can be mounted into the container and values taken from there rather than using the environment variables. The config-file for ovnkube can be passed by a configMap and mounted into the container. This limits the need for environment variables. This change in API results in OVN_DAEMONSET_VERSION 4 Version 3 is still supported. Eliminated v1 and v2 support. The environment variables in V3 are preserved for existing users. See also: openshift/cluster-network-operator#217 SDN-456 https://jira.coreos.com/browse/SDN-456 Signed-off-by: Phil Cameron <pcameron@redhat.com>

bindata/network/ovn-kubernetes/ovnkube-master.yaml

dcbw · 2019-07-18T14:30:08Z

bindata/network/ovn-kubernetes/ovnkube-master.yaml

Same as above, now passed in config file.

Same comment as above.

bindata/network/ovn-kubernetes/ovnkube-master.yaml

dcbw · 2019-07-18T14:31:37Z

bindata/network/ovn-kubernetes/006-ovnkube-config.yaml

@pecameron for OpenShift this value will always be "openshift-ovn-kubernetes" so I think we can just hardcode it here for the CNO. Upstream of course it needs to be configurable but will default to "ovn-kubernetes".

pecameron · 2019-07-18T19:01:12Z

@dcbw made the config file change. The yaml file change will need a change to ovn-config configmap to pass it namespace in. I'll get to that next.

…bles The ovn-config configMap can be mounted into the container and values taken from there rather than using the environment variables. The config-file for ovnkube can be passed by a configMap and mounted into the container. This limits the need for environment variables. This change in API results in OVN_DAEMONSET_VERSION 4 Version 3 is still supported. Eliminated v1 and v2 support. The environment variables in V3 are preserved for existing users. See also: openshift/cluster-network-operator#217 SDN-456 https://jira.coreos.com/browse/SDN-456 Signed-off-by: Phil Cameron <pcameron@redhat.com>

pecameron · 2019-07-18T19:21:31Z

@dcbw added namespace to the ovn-config configmap and and removed OVN_KUBERNETES_NAMESPACE from the daemonsets. PTAL

bindata/network/ovn-kubernetes/006-ovnkube-config.yaml

pkg/network/ovn_kubernetes.go

dcbw · 2019-07-19T19:25:03Z

bindata/network/ovn-kubernetes/006-ovnkube-config.yaml

For all the cert variables, let's just remove them from the config for now, since the default is empty anyway. When we figure out what needs to be done for SSL we can add them back.

Indeed, we should not be putting secrets in configmaps.

The typical solution is to write them on disk via Secrets, and just configure the path to them via config files.

@squeed client-privkey is a defined part of the config file.
@dcbw @squeed, eliminated [ovnnorth] and [ovnsouth] sections. When ssl is supported they can be added back.

dcbw · 2019-07-19T19:25:52Z

pkg/network/ovn_kubernetes.go

The config file hardcodes the loglevel, so we should be consistent. Either hardcode it in the config and don't bother templating, or template it all the way through and not hardcode it in the config file.

@dcbw Its now templated.

…bles The ovn-config configMap can be mounted into the container and values taken from there rather than using the environment variables. The config-file for ovnkube can be passed by a configMap and mounted into the container. This limits the need for environment variables. This change in API results in OVN_DAEMONSET_VERSION 4 Version 3 is still supported. Eliminated v1 and v2 support. The environment variables in V3 are preserved for existing users. See also: openshift/cluster-network-operator#217 SDN-456 https://jira.coreos.com/browse/SDN-456 Signed-off-by: Phil Cameron <pcameron@redhat.com>

The ovn-config configMap can be mounted into the container and values taken from there rather than using the environment variables. The config-file for ovnkube can be passed by a configMap and mounted into the container. This limits the need for environment variables. This change in API results in OVN_DAEMONSET_VERSION 4 Version 3 is still supported. Eliminated v1 and v2 support. The environment variables in V3 are preserved for existing users. See also: openshift/cluster-network-operator#217 SDN-456 https://jira.coreos.com/browse/SDN-456 Signed-off-by: Phil Cameron <pcameron@redhat.com>

pecameron · 2019-08-29T21:04:03Z

@dcbw @squeed This is now working. PTAL

The ovn-config configMap can be mounted into the container and values taken from there rather than using the environment variables. The config-file for ovnkube can be passed by a configMap and mounted into the container. This limits the need for environment variables. This change in API results in OVN_DAEMONSET_VERSION 4 Version 3 is still supported. Eliminated v1 and v2 support. The environment variables in V3 are preserved for existing users. See also: openshift/cluster-network-operator#217 SDN-456 https://jira.coreos.com/browse/SDN-456 Signed-off-by: Phil Cameron <pcameron@redhat.com>

danwinship · 2019-08-30T13:42:50Z

/hold
depends on ovn-kubernetes/ovn-kubernetes#748 being merged and then openshift/ovn-kubernetes being rebased

The ovn-config configMap can be mounted into the container and values taken from there rather than using the environment variables. The config-file for ovnkube can be passed by a configMap and mounted into the container. This limits the need for environment variables. This change in API results in OVN_DAEMONSET_VERSION 4 Version 3 is still supported. Eliminated v1 and v2 support. The environment variables in V3 are preserved for existing users. See also: openshift/cluster-network-operator#217 SDN-456 https://jira.coreos.com/browse/SDN-456 Signed-off-by: Phil Cameron <pcameron@redhat.com>

openshift-ci-robot · 2019-09-24T19:18:22Z

@pecameron: PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

The ovn-config configMap can be mounted into the container and values taken from there rather than using the environment variables. The config-file for ovnkube can be passed by a configMap and mounted into the container. This limits the need for environment variables. This change in API results in OVN_DAEMONSET_VERSION 4 Version 3 is still supported. Eliminated v1 and v2 support. The environment variables in V3 are preserved for existing users. See also: openshift/cluster-network-operator#217 SDN-456 https://jira.coreos.com/browse/SDN-456 Signed-off-by: Phil Cameron <pcameron@redhat.com>

Create a new daemonset version that passes the ovn-config configMap into the container as a file. Create the ovnkube-config configMap to create the ovnkube config-file. Modify master and node daemonsets. OVN_DAEMONSET_VERSION 4 This must merge after ovn-kubernetes/ovn-kubernetes#748 which sets up daemonset version 4 SDN-456 https://jira.coreos.com/browse/SDN-456 Signed-off-by: Phil Cameron <pcameron@redhat.com>

openshift-ci-robot · 2019-09-24T20:19:28Z

@pecameron: The following tests failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/unit	`64ebbe0`	link	`/test unit`
ci/prow/verify	`64ebbe0`	link	`/test verify`
ci/prow/e2e-aws	`64ebbe0`	link	`/test e2e-aws`
ci/prow/images	`64ebbe0`	link	`/test images`
ci/prow/e2e-aws-upgrade	`64ebbe0`	link	`/test e2e-aws-upgrade`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

The ovn-config configMap can be mounted into the container and values taken from there rather than using the environment variables. The config-file for ovnkube can be passed by a configMap and mounted into the container. This limits the need for environment variables. This change in API results in OVN_DAEMONSET_VERSION 4 Version 3 is still supported. Eliminated v1 and v2 support. The environment variables in V3 are preserved for existing users. See also: openshift/cluster-network-operator#217 SDN-456 https://jira.coreos.com/browse/SDN-456 Signed-off-by: Phil Cameron <pcameron@redhat.com>

pecameron · 2019-10-04T14:05:16Z

@squeed @dcbw replaced this with #333
Refactored after PR 329 merged.

openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 28, 2019

openshift-ci-robot requested review from JacobTanenbaum and knobunc June 28, 2019 19:06

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 3, 2019

pecameron force-pushed the sdn456 branch from 8de3a54 to 01cc332 Compare July 16, 2019 20:27

openshift-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 16, 2019

pecameron changed the title ~~[WIP]OVN: use config file via ConfigMap rather than environment varia…~~ [WIP]OVN: use config file via ConfigMap rather than environment variables Jul 17, 2019

pecameron force-pushed the sdn456 branch 2 times, most recently from abfafd1 to 7cd3578 Compare July 17, 2019 19:30

dcbw reviewed Jul 18, 2019

View reviewed changes

bindata/network/ovn-kubernetes/ovnkube-master.yaml Outdated Show resolved Hide resolved

dcbw reviewed Jul 18, 2019

View reviewed changes

bindata/network/ovn-kubernetes/ovnkube-master.yaml Outdated Show resolved Hide resolved

dcbw reviewed Jul 18, 2019

View reviewed changes

bindata/network/ovn-kubernetes/ovnkube-master.yaml Outdated Show resolved Hide resolved

dcbw reviewed Jul 18, 2019

View reviewed changes

pecameron force-pushed the sdn456 branch from 7cd3578 to e45d034 Compare July 18, 2019 19:19

pecameron force-pushed the sdn456 branch from e45d034 to d3d4683 Compare July 19, 2019 15:48

dcbw reviewed Jul 19, 2019

View reviewed changes

bindata/network/ovn-kubernetes/006-ovnkube-config.yaml Outdated Show resolved Hide resolved

dcbw reviewed Jul 19, 2019

View reviewed changes

pkg/network/ovn_kubernetes.go Outdated Show resolved Hide resolved

dcbw reviewed Jul 19, 2019

View reviewed changes

pkg/network/ovn_kubernetes.go Outdated Show resolved Hide resolved

dcbw reviewed Jul 19, 2019

View reviewed changes

pecameron force-pushed the sdn456 branch from 8e94d64 to 2bccb99 Compare August 23, 2019 20:25

pecameron force-pushed the sdn456 branch from 2bccb99 to 7a32b73 Compare August 29, 2019 20:08

pecameron force-pushed the sdn456 branch from 7a32b73 to abea893 Compare August 29, 2019 20:50

pecameron changed the title ~~[WIP]OVN: use config file via ConfigMap rather than environment variables~~ OVN: use config file via ConfigMap rather than environment variables Aug 29, 2019

openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 29, 2019

pecameron force-pushed the sdn456 branch from abea893 to 549d3d0 Compare August 30, 2019 13:20

openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 30, 2019

pecameron force-pushed the sdn456 branch from 549d3d0 to df7ea35 Compare September 24, 2019 19:18

openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 24, 2019

pecameron force-pushed the sdn456 branch from df7ea35 to 64ebbe0 Compare September 24, 2019 20:17

pecameron closed this Oct 4, 2019

Conversation

pecameron commented Jun 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci-robot commented Jun 28, 2019

Uh oh!

pecameron commented Jul 16, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pecameron commented Jul 18, 2019

Uh oh!

pecameron commented Jul 18, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pecameron commented Aug 29, 2019

Uh oh!

danwinship commented Aug 30, 2019

Uh oh!

openshift-ci-robot commented Sep 24, 2019

Uh oh!

openshift-ci-robot commented Sep 24, 2019

Uh oh!

pecameron commented Oct 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

pecameron commented Jun 28, 2019 •

edited

Loading