USHIFT-536: add user-facing config for ovn-kubernetes CNI #956

zshi-redhat · 2022-09-16T04:33:14Z

Closes USHIFT-536

zshi-redhat · 2022-09-16T06:17:13Z

/test e2e-openshift-conformance-sig-api-machinery

zshi-redhat · 2022-09-16T06:17:52Z

cc @fzdarsky @mangelajo

mangelajo · 2022-09-29T14:57:50Z

/approve

dgrisonnet · 2022-09-29T18:08:14Z

Putting a hold for the API team to take some time to consider how to handle this new configuration parameter going forward. We are currently working on rewriting the existing configuration to be more in line with OCP so adding a dependency on the existing configuration of MicroShift will be problematic for us.

/hold

dhellmann · 2022-10-02T15:03:39Z

packaging/microshift/config.yaml

We found a way to have all of the configuration for ODF-LVM in a file outside of MicroShift's main config file. That will, eventually, allow us to provide a different CSI implementation that can also have its own separate file.

Some of the future use cases for MicroShift, especially upstream, make it desirable to be able to replace the CNI implementation as well, and following the same pattern with a similar clean separation for the configuration will make that more straightforward.

Let's talk about the approach we want to take this week.

@dhellmann ack+, I updated the PR to follow the CSI approach and have a separate config file for ovn-kubernetes.

That looks good.

I'm not entirely comfortable with parsing YAML from bash, but what you have looks like it will work and we can consider whether we need something more "robust" in the future if we run into problems.

I thought about using yq in the bash to parse yaml file, but it's not available in the existing repos. Adding the installation dependency sounds a bit heavy for this change in configure-ovs.sh since it is currently the only one I'm aware of that requires parsing yaml in bash in microshift project.
Another idea might be to use json config file and jq to parse it, jq is installed by default. The downside is that it is inconsistent with csi and microshift config files. I could write a small golang program to parse the yaml if we will have similar needs in microshift scripts. Or any other ideas?

Like I said, I think what you have is fine. We can consider other options as an improvement later if we have issues or establish a pattern based on other config.

zshi-redhat · 2022-10-09T03:16:53Z

Putting a hold for the API team to take some time to consider how to handle this new configuration parameter going forward. We are currently working on rewriting the existing configuration to be more in line with OCP so adding a dependency on the existing configuration of MicroShift will be problematic for us.

/hold

@dgrisonnet The PR is updated to use a separate config file for ovn-kubernetes.

zshi-redhat · 2022-10-31T07:18:42Z

/retest

zshi-redhat · 2022-10-31T07:36:35Z

/jira refresh

dgrisonnet · 2022-10-31T17:44:25Z

Unholding since the initial concerns were cleared in this thread: #1030 (comment)

/unhold

dhellmann

Let's work on this, but wait until we branch for 4.13 to merge it.

dhellmann · 2022-11-01T18:54:35Z

packaging/systemd/configure-ovs.sh

Can we use yq to parse the YAML? How big is it and is it OK as a dependency?

If not, would it make sense to use a file format that is friendly for parsing with bash?

I found it is not straight to install yq on rhel. It requires snapd pkg management.
yq on the fedora system is ~13M.
Would it be OK to use a config file in json format? jq is available in rhel.

I believe @zshi-redhat solution is OK, we avoid installing yq which based on his comments doesn't seem to be available in RHEL, and we keep yaml which is a more familiar config format in the k8s world, also more human friendly than json.

dhellmann · 2022-11-01T18:56:49Z

packaging/systemd/configure-ovs.sh

Is it likely that a systemd unit would find something in ~/.microshift? Should we just look in /etc?

Good catch!
agree, systemd may not find the config file in user root. I'll update to use /etc/ directly.

dhellmann · 2022-11-01T18:58:00Z

pkg/components/networking.go

What do you think about passing ovnConfig here so the template can access any future settings without us having to extend this parameter list?

Good idea, will update to pass ovnConfig.

mangelajo · 2022-11-02T09:47:10Z

Waiting for last comments to be handled, looks good otherwise.

ggiguash · 2022-11-02T11:05:31Z

/retest

mangelajo

/lgtm

openshift-ci · 2022-11-03T08:53:56Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mangelajo, zshi-redhat

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [mangelajo,zshi-redhat]

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

openshift-ci · 2022-11-03T09:39:46Z

@zshi-redhat: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/periodic-ocp-4.13-images	d5875a2d1ef76fa9076893bc8d7989ceed1542cb	link	true	`/test periodic-ocp-4.13-images`

Full PR test history. Your PR dashboard.

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.

zshi-redhat · 2022-11-03T09:53:30Z

/retest

openshift-ci bot requested review from benluddy and dhellmann September 16, 2022 04:33

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 16, 2022

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 29, 2022

dhellmann reviewed Oct 2, 2022

View reviewed changes

zshi-redhat force-pushed the configurable-ovs branch from 6a7ed3f to d5875a2 Compare October 9, 2022 03:13

dinhxuanvu mentioned this pull request Oct 27, 2022

USHIFT-607: Introduce a new config format for microshift config #1030

Merged

zshi-redhat force-pushed the configurable-ovs branch from d5875a2 to b148790 Compare October 31, 2022 02:34

zshi-redhat changed the title ~~add user-facing config for ovn-kubernetes CNI~~ USHIFT-536: add user-facing config for ovn-kubernetes CNI Oct 31, 2022

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 31, 2022

dhellmann reviewed Nov 1, 2022

View reviewed changes

openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2022

Add user-facing config for ovn-kubernetes CNI

e9f1144

zshi-redhat force-pushed the configurable-ovs branch from b148790 to e9f1144 Compare November 2, 2022 10:18

openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 2, 2022

mangelajo reviewed Nov 3, 2022

View reviewed changes

openshift-ci bot assigned mangelajo Nov 3, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Nov 3, 2022

openshift-merge-robot merged commit 3529aee into openshift:main Nov 3, 2022

USHIFT-536: add user-facing config for ovn-kubernetes CNI #956

USHIFT-536: add user-facing config for ovn-kubernetes CNI #956

Uh oh!

Conversation

zshi-redhat commented Sep 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zshi-redhat commented Sep 16, 2022

Uh oh!

zshi-redhat commented Sep 16, 2022

Uh oh!

mangelajo commented Sep 29, 2022

Uh oh!

dgrisonnet commented Sep 29, 2022

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!

zshi-redhat commented Oct 9, 2022

Uh oh!

zshi-redhat commented Oct 31, 2022

Uh oh!

zshi-redhat commented Oct 31, 2022

Uh oh!

dgrisonnet commented Oct 31, 2022

Uh oh!

dhellmann left a comment

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!

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!

mangelajo commented Nov 2, 2022

Uh oh!

ggiguash commented Nov 2, 2022

Uh oh!

mangelajo left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Nov 3, 2022

Uh oh!

openshift-ci bot commented Nov 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zshi-redhat commented Nov 3, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

zshi-redhat commented Sep 16, 2022 •

edited

Loading

openshift-ci bot commented Nov 3, 2022 •

edited

Loading