Write MTU migration configuration #2856

jcaamano · 2021-12-03T11:24:29Z

Add support for MTU migration.

MCO updates the controller spec with MTU migration configuration obtained from network status. This MTU migration configuration is only there while a MTU migration is taking place. Only then, as a result, MCO renders a configuration file from a template included in this PR. This configuration file will be sourced from an upcoming NM dispatcher script that will also be rendered from an MCO template. Both configuration file and NM dispatcher script only need to be rendered while an MTU migration is in progress.

Enhancement: https://github.com/openshift/enhancements/blob/master/enhancements/network/allow-mtu-changes.md
Vendored in needed openshift-api changes: openshift/api/pull/1078

/assign @yuqi-zhang
/hold

jcaamano · 2021-12-03T18:48:31Z

/hold cancel

yuqi-zhang

Just some questions from an initial pass. Is this required for 4.10? Also would prefer some one from the Networking team to take a look

templates/common/_base/files/mtu-migration-conf.yaml

manifests/controllerconfig.crd.yaml

jcaamano · 2021-12-06T11:05:21Z

Just some questions from an initial pass. Is this required for 4.10? Also would prefer some one from the Networking team to take a look

Yes, this is required for 4.10.

jcaamano · 2021-12-06T11:06:40Z

/assign @trozet

jcaamano · 2021-12-07T09:27:24Z

/retest

yuqi-zhang

Here's some more context to the PR now that I understand it better:

This PR is mostly safe from the MCO's perspective, since it doesn't affect anything in the core code path. Nothing happens if you don't define migration which is set via the CNO (which we read already)
This PR itself is a no-op, and requires a follow up PR (which we may squash) that drops in a NM dispatcher script to do the actual operations
This is required in 4.10

So with that in mind, I am mostly confident this is an ok approach given the tight timelines. This does introduce the concept of "empty file templates" which only renders if a condition is met. For future consideration, I think either:

have the CNO be the one generating the MC in the first place, such that we don't have to read it in the MCO, or
have the MCO core logic have better understanding of conditional templates
Is the better way to go.

Some more comments below:

pkg/controller/template/render.go

jcaamano · 2021-12-09T20:07:17Z

PTAL please @trozet

Also your opinion on:

Can or should we, and how, deprecate the current ControllerConfigSpec.NetworkType in favor of a new ControllerConfigSpec.Network.NetworkType? Meaning:

We leave things as is, so we have the existing ControllerConfigSpec.NetworkType and the new ControllerConfigSpec.Network.MTUMigration
We remove ControllerConfigSpec.NetworkType in favor of a new ControllerConfigSpec.Network.NetworkType
We keep ControllerConfigSpec.NetworkType but deprecate it, and add ControllerConfigSpec.Network.NetworkType and we consider both in the templates until ControllerConfigSpec.NetworkType can be removed if ever.

@yuqi-zhang thoughts on this is that ControllerConfigSpec is not really a public API and that general way of proceeding is option 3 above. This could be worked on in a follow up PR as it is not needed for 4.10

trozet

In general lgtm.

trozet · 2021-12-10T16:32:03Z

templates/common/_base/files/mtu-migration-conf.yaml

maybe just comment that this is only for a use case where the host MTUs are not modified and just the CNI MTU is modified?

trozet · 2021-12-10T16:32:23Z

templates/common/_base/files/mtu-migration-conf.yaml

if its possible, it would make it more readable to indent this logic. The logic looks OK to me though.

jcaamano · 2021-12-13T09:49:58Z

/retest

Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

jcaamano · 2021-12-14T11:44:22Z

/retest-required

jcaamano · 2021-12-14T11:47:18Z

/retest

trozet

/lgtm

trozet · 2021-12-14T14:06:49Z

@yuqi-zhang can you PTAL for approval?

openshift-ci · 2021-12-14T14:25:11Z

@jcaamano: The following tests 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/e2e-aws-disruptive	`2c75679`	link	false	`/test e2e-aws-disruptive`
ci/prow/e2e-metal-ipi	`2c75679`	link	false	`/test e2e-metal-ipi`
ci/prow/e2e-aws-upgrade-single-node	`2c75679`	link	false	`/test e2e-aws-upgrade-single-node`

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.

yuqi-zhang

New changes lgtm

Also linking #2871 as the second part of this migration work

openshift-ci · 2021-12-14T23:48:45Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jcaamano, trozet, yuqi-zhang

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~~ [yuqi-zhang]

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

openshift-ci bot assigned yuqi-zhang Dec 3, 2021

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

openshift-ci bot requested review from cheesesashimi and yuqi-zhang December 3, 2021 11:24

jcaamano force-pushed the mtu-migration branch 3 times, most recently from 7864225 to eb0095f Compare December 3, 2021 14:33

jcaamano changed the title ~~Add support for MTU migration~~ Write MTU migration configuration Dec 3, 2021

jcaamano force-pushed the mtu-migration branch 5 times, most recently from 1f2268f to 40ae07b Compare December 3, 2021 18:47

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

yuqi-zhang reviewed Dec 3, 2021

View reviewed changes

templates/common/_base/files/mtu-migration-conf.yaml Outdated Show resolved Hide resolved

templates/common/_base/files/mtu-migration-conf.yaml Outdated Show resolved Hide resolved

manifests/controllerconfig.crd.yaml Outdated Show resolved Hide resolved

openshift-ci bot assigned trozet Dec 6, 2021

yuqi-zhang reviewed Dec 7, 2021

View reviewed changes

pkg/controller/template/render.go Outdated Show resolved Hide resolved

jcaamano force-pushed the mtu-migration branch 2 times, most recently from 59cc4ed to c016bd0 Compare December 9, 2021 19:43

jcaamano force-pushed the mtu-migration branch 3 times, most recently from 81e201d to 1992908 Compare December 10, 2021 14:07

trozet reviewed Dec 10, 2021

View reviewed changes

jcaamano force-pushed the mtu-migration branch 2 times, most recently from ab552aa to 2937399 Compare December 13, 2021 09:40

jcaamano force-pushed the mtu-migration branch from 2937399 to 359f95c Compare December 13, 2021 09:41

jcaamano added 2 commits December 14, 2021 08:53

Vendor in openshift/api MTU migration changes

77035da

Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

Write MTU migration configuration

2c75679

Signed-off-by: Jaime Caamaño Ruiz <[email protected]>

jcaamano force-pushed the mtu-migration branch from 359f95c to 2c75679 Compare December 14, 2021 08:53

trozet approved these changes Dec 14, 2021

View reviewed changes

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 14, 2021

yuqi-zhang approved these changes Dec 14, 2021

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 14, 2021

openshift-merge-robot merged commit aaa600e into openshift:master Dec 14, 2021

Write MTU migration configuration #2856

Write MTU migration configuration #2856

Uh oh!

Conversation

jcaamano commented Dec 3, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jcaamano commented Dec 3, 2021

Uh oh!

yuqi-zhang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jcaamano commented Dec 6, 2021

Uh oh!

jcaamano commented Dec 6, 2021

Uh oh!

jcaamano commented Dec 7, 2021

Uh oh!

yuqi-zhang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

jcaamano commented Dec 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

trozet left a comment

Choose a reason for hiding this comment

Uh oh!

trozet Dec 10, 2021

Choose a reason for hiding this comment

Uh oh!

trozet Dec 10, 2021

Choose a reason for hiding this comment

Uh oh!

jcaamano commented Dec 13, 2021

Uh oh!

jcaamano commented Dec 14, 2021

Uh oh!

jcaamano commented Dec 14, 2021

Uh oh!

trozet left a comment

Choose a reason for hiding this comment

Uh oh!

trozet commented Dec 14, 2021

Uh oh!

openshift-ci bot commented Dec 14, 2021

Uh oh!

yuqi-zhang left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Dec 14, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jcaamano commented Dec 3, 2021 •

edited

Loading

jcaamano commented Dec 9, 2021 •

edited

Loading