Change defaultNetworkType to ovn-kubernetes #6014
Conversation
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
|
Is this being done for https://issues.redhat.com/browse/CORS-2085 ? |
well, I suppose, but I didn't really know about this Epic. This PR is initially to see it work (running tests with it). I'm sure |
|
/test |
|
@stbenjam: The
The following commands are available to trigger optional jobs:
Use
In response to this:
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. |
|
/payload 4.11 nightly informing |
|
@DennisPeriquet: trigger 49 job(s) of type informing for the nightly release of OCP 4.11
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/82d0b530-edcc-11ec-95f4-2f5f1aca7ac8-0 |
|
/payload 4.11 nightly informing |
|
@jluhrsen: trigger 49 job(s) of type informing for the nightly release of OCP 4.11
See details on https://pr-payload-tests.ci.openshift.org/runs/ci/09b0d7f0-ee7d-11ec-8c08-bf541cc10a57-0 |
jluhrsen
force-pushed
the
ovnk-default-cni
branch
2 times, most recently
from
89c7e0e
to
41d4bcd
Compare
jluhrsen
force-pushed
the
ovnk-default-cni
branch
from
41d4bcd
to
7a67c34
Compare
|
/hold cancel |
openshift-ci
bot
removed
the
do-not-merge/hold
|
/retest |
|
Can you update the commit message(s) according to the format here: https://github.com/openshift/installer/blob/master/CONTRIBUTING.md#commit-message-format |
|
Also it looks like some of the unit tests are failing. We can help with troubleshooting that if they prove problematic |
jluhrsen
force-pushed
the
ovnk-default-cni
branch
from
7a67c34
to
f99922a
Compare
updated. LMK if it's not good enough and I'll clean it up further. |
should be good now. |
|
/lgtm From TRT perspective we've roughly matched or exceeded parity with SDN jobs across the handful we have been monitoring. |
|
/retest |
1 similar comment
|
/retest |
|
@sadasu @patrickdillon - does this PR look good to go? |
| @@ -24,7 +24,7 @@ var ( | |||
| defaultServiceNetwork = ipnet.MustParseCIDR("172.30.0.0/16") | |||
| defaultClusterNetwork = ipnet.MustParseCIDR("10.128.0.0/14") | |||
| defaultHostPrefix = 23 | |||
| defaultNetworkType = string(operv1.NetworkTypeOpenShiftSDN) | |||
| defaultNetworkType = string(operv1.NetworkTypeOVNKubernetes) | |||
There was a problem hiding this comment.
Logic around https://github.com/openshift/installer/blob/master/pkg/types/defaults/installconfig.go#L50-L56 also needs to be cleaned up.
There was a problem hiding this comment.
It's updated now @sadasu, I think it is much simpler now right, and just set to ovnk if ""
|
/refresh |
|
/skip |
| if strings.ToLower(netconf.NetworkType) == strings.ToLower(string(operv1.NetworkTypeOpenShiftSDN)) { | ||
| netconf.NetworkType = string(operv1.NetworkTypeOpenShiftSDN) | ||
| if strings.ToLower(netconf.NetworkType) == strings.ToLower(string(operv1.NetworkTypeOVNKubernetes)) { | ||
| netconf.NetworkType = string(operv1.NetworkTypeOVNKubernetes) |
There was a problem hiding this comment.
This should not be changed. Well, the comment should be changed to not say "default", but the purpose of this code is not "whatever the default networkType value is, we should recognize it case-insensitively", it's "the correct value of this field used to be OpenshiftSDN but then we fixed it to be OpenShiftSDN, but we need to continue matching both values for backward-compatibility with (very) old install-configs.
| @@ -149,25 +149,6 @@ func TestConvertInstallConfig(t *testing.T) { | |||
| }, | |||
| expectedError: "no version was provided", | |||
| }, | |||
| { | |||
| name: "deprecated OpenShiftSDN spelling", | |||
There was a problem hiding this comment.
and likewise this should be preserved
| } else { | ||
| c.Networking.NetworkType = defaultNetworkType | ||
| } | ||
| c.Networking.NetworkType = defaultNetworkType |
There was a problem hiding this comment.
is this even needed? with the kubebuilder:default, won't it always have gotten filled in if the user didn't specify it?
There was a problem hiding this comment.
is there no other way for c.Networking.NetworkType == ""
I don't know. this doesn't hurt, but if you know for certain then I can remove it.
There was a problem hiding this comment.
yeah, dunno. it doesn't hurt
jluhrsen
force-pushed
the
ovnk-default-cni
branch
from
796b986
to
3e070af
Compare
|
/retest-required |
In 4.12, the default CNI will be OVNKubernetes. This change will deploy ovnk by default as well as adjust tests, docs and comments to reflect the same. Signed-off-by: Jamo Luhrsen <jluhrsen@gmail.com>
jluhrsen
force-pushed
the
ovnk-default-cni
branch
from
3e070af
to
74486c1
Compare
|
/retest |
|
/test e2e-vsphere |
|
Looking at the history of e2e-vsphere runs, we have had several successful installs and they are all failing with:
So... not something that would be fixed by this PR (AFAIK) but probably needs to be looked into |
|
@patrickdillon I think 4.12.0-0.ci-2022-07-28-142749 included openshift/cluster-policy-controller#82 |
doh I see your earlier comment now. thanks for clarifying. |
|
@sdodson, @patrickdillon: are we ready to lgtm again or do we think the network policy audit logging failure on vsphere should be gating this PR? |
|
@jluhrsen: The following tests failed, say
Full PR test history. Your PR dashboard. 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. |
vsphere passed |
|
/lgtm |
Signed-off-by: Jamo Luhrsen jluhrsen@gmail.com