[WIP]OCPBUGS-42701-api: Updated the disableMultiNetwork parameter description #2143
Conversation
|
Hello @dfitzmau! Some important instructions when contributing to openshift/api: |
openshift-ci
bot
added
the
size/XS
|
The openapi schema that you have updated is generated from the Go API types, you will need to update the Go types and then generate the openapi schema (and other generations) to make sure everything is updated |
dfitzmau
force-pushed
the
OCPBUGS-42701-api
branch
4 times, most recently
from
3a639f2 to
b2aa868
Compare
openshift-ci
bot
added
size/M
dfitzmau
changed the title
openshift-ci
bot
added
the
do-not-merge/work-in-progress
dfitzmau
force-pushed
the
OCPBUGS-42701-api
branch
from
b2aa868 to
a183631
Compare
|
Code generation is outdated, please run |
operator/v1/types_network.go
Outdated
| // disableMultiNetwork enables multiple network support when the OVN-Kubernetes Container Network Interface (CNI) | ||
| // plugin sets the parameter to the default value of 'false'. During cluster installation only, you can use a | ||
| // third-party CNI, such as Multus CNI, to set the disableMultiNetwork parameter to 'true', and this setting disables | ||
| // multiple pod network support. Red Hat does not support third-party CNIs. |
There was a problem hiding this comment.
I think we need a few tweaks...
My I suggest...
// disableMultiNetwork when set to true at cluster install will disable the installation of components which enable pod multinetworking, especially utilizing Multus CNI and network-attachment-definition custom resources. The default value is false.
// This option may potentially be useful for installation of third party CNI plugins, however, this is not supported by Red Hat.
// Setting this option after cluster install has no effect.
Happy to take a mix of the two, but, we should definitely remove: you can use a third-party CNI, such as Multus CNI, to set the disableMultiNetwork parameter to 'true'. As setting this to true by the user in the install-config will install Multus, might be the confusion.
dfitzmau
force-pushed
the
OCPBUGS-42701-api
branch
2 times, most recently
from
efcec91 to
70a6e5d
Compare
dfitzmau
force-pushed
the
OCPBUGS-42701-api
branch
from
70a6e5d to
107acc7
Compare
|
@dfitzmau: all tests passed! 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-sigs/prow repository. I understand the commands that are listed here. |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: dfitzmau, dougbtv, weliang1 The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
Hi @JoelSpeed . Thanks for your help and patience. I received all the approvals on the PR. Is the PR OK to merge? |
| // disableMultiNetwork defaults to 'false' and this setting enables the pod multi-networking capability. | ||
| // disableMultiNetwork when set to 'true' at cluster install time does not install the components, typically the Multus CNI and the network-attachment-definition CRD, | ||
| // that enable the pod multi-networking capability. Setting the parameter to 'true' might be useful when you need install third-party CNI plugins, | ||
| // but these plugins are not supported by Red Hat. Changing the parameter value as a postinstallation cluster task has no effect. |
There was a problem hiding this comment.
Is postinstallation supposed to be one word?
There was a problem hiding this comment.
Yes, it matches the noun adopted in our official docs.
| // disableMultiNetwork defaults to 'false' and this setting enables the pod multi-networking capability. | ||
| // disableMultiNetwork when set to 'true' at cluster install time does not install the components, typically the Multus CNI and the network-attachment-definition CRD, | ||
| // that enable the pod multi-networking capability. Setting the parameter to 'true' might be useful when you need install third-party CNI plugins, | ||
| // but these plugins are not supported by Red Hat. Changing the parameter value as a postinstallation cluster task has no effect. | ||
| DisableMultiNetwork *bool `json:"disableMultiNetwork,omitempty"` |
There was a problem hiding this comment.
Should we make this immutable then given changing it has no effect? What does changing it from true to false mean here? And vice versa?
If it's false by default, some components get installed? If you later try to change it to true, the operator doesn't react to this? If it was true, and you change it to false, it doesn't then install the components?
Why would I want to disable this?
There was a problem hiding this comment.
It really boils down to using third-party CNIs that are unsupported. Maybe I could expand on what pod multi-networking does on how it might be unique to OCP?
There was a problem hiding this comment.
If there's a way to make it immutable post install, that's great.
Here's the thing -- we don't know who we'll break if it make it so you can't use this during install. It's possible that third parties have relied on it.
It was originally here as a fail-safe during 4.0.0, the original openshift 4 when this component was released. Honestly, kind of a feature gate in case we ran into some kind of failure, which didn't happen.
There was a problem hiding this comment.
You can use CEL to write something to make it immutable.
There are a couple of things to consider with that, the field being completely missing from the document (because it's a nil pointer) means that someone can add a value later, and that might be true, or false. We could add a transition rule to the parent to prevent this.
Once it's set, an easy self == oldSelf rule would prevent it from being changed later to another value.
If, as mentioned, changing the value does nothing, what would someone relying on being able to change this actually mean?
Updating the disableMultiNetwork parameter description based on feedback from
Issue:
OCPBUGS-42701
Key updates: