OPNET-268: Configure-ovs Alternative

[cybertron]

There are a number of limitations of the configure-ovs script. This is a proposed design to address them.

[openshift-ci-robot]

@cybertron: This pull request references OPNET-268 which is a valid jira issue.

In response to this:

There are a number of limitations of the configure-ovs script. This is a proposed design to address them.

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.

[cgoncalves]

Will an explicit configuration of br-ex be mandatory moving forward or optional? If mandatory, we need to evaluate upgrade paths.

[cgoncalves]

OK, I see below under the 'Proposal' section that this will be mandatory.

[cybertron]

It will be mandatory if you're using this new mechanism, but configure-ovs will still exist. I think manual configuration of br-ex is probably too big a hurdle for some deployers so I think we need to keep that easy path. I'm open to being convinced otherwise, but we'll need to discuss that with the OVNK folks.

[qinqon]

Well, we may need to document examples to do it manually so users that do not exactly like the configure-ovs version of br-ex do not start from scratch.

[cybertron]

Yep, we're definitely going to want to document what a br-ex config using this mechanism should look like, probably including some more advanced options too.

[cgoncalves]

Thinking about an upgrade path, it might be simpler than I had in mind. Deployers upgrading their clusters won't need to explicitly configure br-ex as its configuration already exists in each node. The exception is when scaling out or reinstalling nodes post-upgrade; deployers must create a valid br-ex configuration. If that is so, we will have to highlight such in the test plan and documentation.

[cgoncalves]

networkConfig YAML section in install-config.yaml for IPI and agent-config.yaml for agent-based installer.

[cybertron]

Do you mean a new networkConfig section in install-config? We can't use the existing one for this functionality because we don't have OVS in the ramdisk.

[cgoncalves]

You say we cannot use the existing one (referring to networkConfig, right?), yet the example below with br-ex is in the networkConfig block. I'm confused.

[cybertron]

Yeah, I reused the name but it's in a different section. I don't know if that's more or less confusing than coming up with a new name for a very similar feature.

[cgoncalves]

The name field uniquely identifies a given host. Giving an example of master-0 node in this enhancement proposal seems reasonable.

[cybertron]

The issue is that there's no guarantee the name specified in install-config matches the eventual hostname of the node. At least on baremetal, hostnames are controlled by the DHCP server which is outside our control. It's probably fair to say the names need to match if you're going to do this, but I'm open to other options that would reduce the opportunity for misconfiguration too.

[cgoncalves]

Good point on hostname change because of DHCP.

[qinqon]

Why is that different from what we have now ? I understand that we are already using the NMState version per node that generated the NM connection files.

[cybertron]

That's baremetal-specific, we're trying to make this more platform-agnostic. In baremetal we can know the hostnames ahead of time, but I don't know if that's true for all platforms. I suspect there may be some randomness to the hostnames in non-baremetal cases.

[cgoncalves]

Reads extraneous to this enhancement.

[cybertron]

I pre-emptively included this because it was a point of discussion on the baremetal networkConfig design. In that case we agreed to do common config across nodes as a followup, but since it turns out YAML can already handle that for us I thought I'd mention it in case anyone was wondering about that here too.

[cgoncalves]

Other downsides:

• Machine-configs only apply at a much later install stage, no?
• Additional node reboot when machine-config created at day-2
• No NMstate syntax/config validation, could render nodes inaccessible

[cybertron]

• Machine-configs are applied by ignition, the same as this.
• That's a good point. If you scaled out a machine config pool on day 2 you'd have to reboot all of the nodes in that pool because you'd have to add the new node configs to all of them. That's a pretty big limitation.
• True, although in this case I envision the machine-configs writing NMState YAML instead of raw nmconnection files, so you'd still have NMState validating them before it applies them and rolling back if they break things. You couldn't catch syntax errors in install-config though because the installer would just see opaque machine-configs, not their contents.

Actually, the scaleout case is something we need to consider. At install time we can handle mapping things in the installer, but on day 2 we'll need a manual process, much like baremetal has. Unfortunately I don't think we can use kubernetes-nmstate for scaleout either because it won't be running soon enough to apply the bridge config. :-/

[cybertron]

Note that scaleout works fine with my latest machine-config prototype. You just update the MC with the new node configs and it gets rolled out reboot-less so it will be there when the new nodes start. I discuss how this works below now.

[cgoncalves]

On the positive side, should the deployer apply a configuration that renders the node inaccessible, the trio (NetworkManager, nmstate and kubernetes-nmstate) are expected to recover by rolling back to the previous network config state.

[cybertron]

Good point. I'll mention that as a mitigation to the risk.

[cgoncalves]

Could you please share some references to both statements?

[cybertron]

I'll add a link here, but for my own reference it first came up in this thread: #1267 (comment)

There is an initial PR to smooth integration with OCP here: nmstate/nmstate#2338 It doesn't actually change any of the API yet, but I believe it was a first step in that direction.

[cgoncalves]

OVN-Kubernetes will not handle the replacement of the gateway interface (enp2s0 in the example above). This is an existing limitation where OVN-Kubernetes installs an OpenFlow rule in br-ex that states the gateway interface is the OVS port ID of enp2s0 (port 1) and where attaching a new interface, say enp3s0, its OVS port ID will be different hence no egress traffic forwarding.

Monitoring br-ex port attachments and recalculating the OpenFlow rule may help. This is outside the scope of this enhancement, though.

[cybertron]

Ack, thanks. I'm just going to include your comment in the enhancement.

[jcaamano]

I recall once trying this and even a noop change on br-ex through nmstate operator caused undesired changes in the underlying configuration. I am not 100% sure if I am recalling incorrectly or if this might have been fixed since then.

[cybertron]

I'm wondering if that's because configure-ovs uses different profile names from NMState. It's possible we ended up with multiple profiles doing the same thing.

Maybe I just need to test this. I can deploy my prototype and change the MTU or something like that and see what happens.

[jcaamano]

At the very least I recall nmstate changing the names of things, like ovs ports and such. Yeah, I am curious about testing this again.

[cgoncalves]

FYI, opened RFE: https://issues.redhat.com/browse/RFE-5401

[cgoncalves]

What are the cases where configure-ovs.sh would still be used?

[cybertron]

Probably most single-nic deployments where the logic for creating br-ex is simple. I don't anticipate this new mechanism being widely used in AWS, Azure, or GCP, for instance. In fact, I'm not sure how it will work in non-on-prem envs.

[cgoncalves]

Agreed. Can we add that as an example to this doc?

[cybertron]

Sure, done.

[cybertron]

/cc @jcaamano @trozet

[cybertron]

New revision with updates based on the discussion so far. I also left some more thoughts on the variation where this is mostly implemented with machine-configs. I'm actually liking that more and more as I think about it because it uses existing configuration mechanisms so we aren't going to have to invent something completely new.

[cybertron]

I've pushed an update based on the implementation I did in https:/cybertron/machine-config-operator/tree/configure-ovs-alt

It handles both initial deployment and scaling in a sane fashion and required minimal code changes. The main concern is whether we can use hostname-based mapping of configs on non-baremetal platforms since I think at least some of them add a random identifier into the node names. If we can't use hostnames, does anyone have suggestions on what we could use?

[phoracek]

I don't know if it is relevant for this design, but in case the configure-ovs script today also needs to configure ovn-bridge-mappings you may find this useful - we have recently introduced a nice API for it to nmstate: https://bugzilla.redhat.com/show_bug.cgi?id=2209690

With this, you could configure the mapping easily from this CR:

hostConfig:
  ovn:
    bridge-mappings:
      - localnet: FOO
        bridge: br-ex

[cybertron]

Yeah, that's definitely a nice fit for this. It would be a pain to introduce an OpenShift-specific API to do that, but with NMState we get it for "free". :-)

[qinqon]

Well, we may need to document examples to do it manually so users that do not exactly like the configure-ovs version of br-ex do not start from scratch.

[qinqon]

Can you add that configure-ovs will be skip if NMState is detected.

Also a question, is the NMState configured going to be "converted" automatically into a NNCP for kubernetes-nmstate so users can modify it or they will create the NNCP from scratch.

[cybertron]

Yep, will do.

Right now it would need to be done manually. There isn't really a way to convert it at install time because kubernetes-nmstate won't be installed yet. I suppose there may be some way to auto-create some manifests that could be applied later though.

[qinqon]

nmpolicies is already implemented at nmstatectl so we can "generalize" the the nmstate if there is a property to identify the interface.

What I mean if that at some scenarios maybe we can have a nmstate per cluster so maybe we can extend networking part and do it per cluster (it will be configured per node at the backend by MCO though).

[cybertron]

I suspect we would use the results of nodeip-configuration to generate the policy. I'm not sure exactly how that would work, but it's definitely something I'd like to look into. It's not my primary goal right now because I want to handle the advanced cases where configure-ovs struggles, but I do see nmpolicy as a potential way to completely replace configure-ovs.

On a more general note, a per-cluster config (maybe with the possibilty of overriding on a per-node basis) may be a solution to platforms where we don't have a way to map per-node configs ahead of time (like if the hostname is not predictable). I don't know how important this feature will be in those cases, but I have heard some discussion of needing to override the MTU at deploy time, so this might provide a solution to that. I think with my current machine-config prototype it would be pretty easy to extend to include a per-cluster config so maybe I'll give that a try and update the design if it works well.

[qinqon]

Why is that different from what we have now ? I understand that we are already using the NMState version per node that generated the NM connection files.

[qinqon]

Maybe it's better to add something new to the install-config syntax to configure network cluster-wide for this scenarios (also taking into account the nmpolicy comment above).

clusterConfig:
networkConfig:
      interfaces:
      - name: br-ex

Also with this it would be possible to generate an NNCP from it so you can say at day2 what was done by boot process, although we will have to be super careful if we introduce nmpolicies since captures haste to take value from the cache files.

[cybertron]

Agreed. I'm seriously tempted to drop the install-config part of this entirely for the initial implementation since we can do it all with machine-configs, but even if we do that I think a cluster-wide config makes a lot of sense.

I think day 2 would be handled similar to how it is today, which is the user would need to manually copy their config into an NNCP. I want to eliminate that step, but I think we'll need to get KNMState into the core payload first.

[qinqon]

The systemd service is already in place as part of nmstate installation, we just need to put yamls at /etc/nmstate/

[cybertron]

It is, but I wonder if that does what we need. I only want this to apply once, and only the configuration that applies to the specific host. If we implemented this with machine-configs, all of the configs for all of the nodes will be present in the directory. I also only want this to be applied at first boot, and then if changes are needed they should be made by kubernetes-nmstate. We could allow updating via machine-config, but then we throw out all of the benefits that kubernetes-nmstate gives us.

[qinqon]

Stuff under /etc/nmstate apply only once, then nmstate move the file so it's not apply at reboot, also I understand that machine config will dump there just the one for that node.

If we miss features for /etc/nmstate we can ask for them.

[cybertron]

Unfortunately there's no way to tell MCO to only deploy the config specific to the node. We have to write all of the configs to all of the nodes and have logic on the node itself to pick the right one. I've added some discussion of how we might integrate with the NMState service to do this in the Variation section.

[qinqon]

To reduce this risk we should document nmstate examples to create br-ex manually for differente scenarios (single-nic, multi-nic, bond + bridge + vlans, etc...)

We have to be super careful with MTUs though.

[cybertron]

Yep, probably similar to what we have for kubernetes-nmstate today. We'll include examples of some common configs.

[qinqon]

Well except for the static IPs stuff is possible that most of the config will be common across nodes, maybe we can intorduce at clusterConfig section at ignition to configure networking if it's the same at all the nodes but keep the hostConfig so if hostConfig will be fallback if there is no clusterConfig (let's not merge them).

[qinqon]

But we can change k-nmstate operator to read stuff under /etc/nmstate and convert it to NNCPs.

[cybertron]

That's an interesting idea. We couldn't do that with the baremetal feature because only the nmconnection files made it to the host, but in this case we'll have the full NMState config available. It would be very interesting to add a feature to discover the existing config for the operator.

[qinqon]

Just not that we can configure ovs_db per interface at nmstate, don't know if installing a openflow rule it's possible with that.

[cybertron]

Thanks for all the comments! I will update this and the prototype accordingly.

[cybertron]

Yep, we're definitely going to want to document what a br-ex config using this mechanism should look like, probably including some more advanced options too.

[cybertron]

Yep, will do.

Right now it would need to be done manually. There isn't really a way to convert it at install time because kubernetes-nmstate won't be installed yet. I suppose there may be some way to auto-create some manifests that could be applied later though.

[cybertron]

I suspect we would use the results of nodeip-configuration to generate the policy. I'm not sure exactly how that would work, but it's definitely something I'd like to look into. It's not my primary goal right now because I want to handle the advanced cases where configure-ovs struggles, but I do see nmpolicy as a potential way to completely replace configure-ovs.

On a more general note, a per-cluster config (maybe with the possibilty of overriding on a per-node basis) may be a solution to platforms where we don't have a way to map per-node configs ahead of time (like if the hostname is not predictable). I don't know how important this feature will be in those cases, but I have heard some discussion of needing to override the MTU at deploy time, so this might provide a solution to that. I think with my current machine-config prototype it would be pretty easy to extend to include a per-cluster config so maybe I'll give that a try and update the design if it works well.

[cybertron]

Yeah, that's definitely a nice fit for this. It would be a pain to introduce an OpenShift-specific API to do that, but with NMState we get it for "free". :-)

[cybertron]

That's baremetal-specific, we're trying to make this more platform-agnostic. In baremetal we can know the hostnames ahead of time, but I don't know if that's true for all platforms. I suspect there may be some randomness to the hostnames in non-baremetal cases.

[cybertron]

Oops, did I not reply to this before?

The answer is yes. configure-ovs will still run in the default case, but if you've provided custom br-ex config it will prevent configure-ovs from running.

[cybertron]

It is, but I wonder if that does what we need. I only want this to apply once, and only the configuration that applies to the specific host. If we implemented this with machine-configs, all of the configs for all of the nodes will be present in the directory. I also only want this to be applied at first boot, and then if changes are needed they should be made by kubernetes-nmstate. We could allow updating via machine-config, but then we throw out all of the benefits that kubernetes-nmstate gives us.

[cybertron]

Note that scaleout works fine with my latest machine-config prototype. You just update the MC with the new node configs and it gets rolled out reboot-less so it will be there when the new nodes start. I discuss how this works below now.

[cybertron]

Yep, probably similar to what we have for kubernetes-nmstate today. We'll include examples of some common configs.

[cybertron]

That's an interesting idea. We couldn't do that with the baremetal feature because only the nmconnection files made it to the host, but in this case we'll have the full NMState config available. It would be very interesting to add a feature to discover the existing config for the operator.

[cybertron]

I've updated the document to reflect the fact that I would like to implement the machine-config method as an MVP, and then work on improved integration and more features as followups. I think that will let us get a useful feature into the hands of users more quickly, and we can address any usability shortcomings based on real-world feedback.

[jcaamano]

There are some features that depend on day-2 configuration changes, like MTU migration. configure-ovs generates ephemeral configuration on each boot so it picks up any day-2 config changes to its source config. I think it needs to be considered within the scope of the enhancement.

[cybertron]

In the MTU case, is configure-ovs responsible for changing the MTU or does it just pick up the changed MTU of the underlying interface? I was thinking that you had to manually modify the interface as part of the process, which you would just do with Kubernetes-NMState here.

It would be good if we can come up with a list of day 2 features that might require bridge changes so we know what use cases to target.

[jcaamano]

It just picks up the changed MTU of the underlying interface that a user is responsible for configuring correctly.

which you would just do with Kubernetes-NMState here

If you mean changing br-ex and children interfaces, then we need to make sure this works with Kubernetes-NMState. That's what I meant that needs to be considered within the scope of the enhancement.

I am aware of any other features that depend directly on changing network configuration on day 2 (perhaps this could also help when enabling dualstack) but I do know some other features depend on the MTU migration itself, like enabling ipsec or sdn<->ovn migration.

[jcaamano]

I recall once trying this and even a noop change on br-ex through nmstate operator caused undesired changes in the underlying configuration. I am not 100% sure if I am recalling incorrectly or if this might have been fixed since then.

[jcaamano]

Is it worth mentioning how we would distinguish a first boot from subsequent boots?

[jcaamano]

I read above that the nmstate service moves the files away after they are applied. So we wouldn't need to worry about this if we used that so maybe worth mentioning within that option.

[cybertron]

I was looking at that as an implementation detail. My current implementation just writes a file after we process the configuration once (which we'll still need to do even using the default NMState service because we need to make sure it only applies the configuration for the current node).

The file is convenient though because configure-ovs can look for it to determine whether we're using this new method.

[jcaamano]

Understood. I guess I was being overzealous in crosschecking if that would be something a user might inadvertently revert but I guess there is no need to worry about that.

[jcaamano]

Also occurred to me that if we use the nmstate service and that moves the files around, MCO might complain.

[jcaamano]

In this case, would we be concerned with any configuration already present in /etc/nmstate, and its order with respect to the copied config if relevant?

[cybertron]

We generally don't support mixing network configuration mechanisms. Still, it would probably be safest to verify there is no conflict with files already in /etc/nmstate to avoid overwriting a file we shouldn't. In that case we should probably just fail the service.

[jcaamano]

Would nmstate generate persistent NetworkManager configuration in /etc/NetwrokManager/system-connections? If so, how does this persistent configuration interact with day-2 config changes from nmstate operator, is it overwritten?

[cybertron]

It would. To modify the config on day 2 you would copy the entire day 1 config into an NNCP and make your changes from that. That way the generated config from NMState should be identical.

There's some more discussion below about possibly automating the sync of day 1 config to day 2. Right now we have a similar situation with the baremetal day 1 feature and just require a manual copy, but for this we're in better shape because we have access to the original NMState config.

In a perfect world I'd love to have kubernetes-nmstate as part of the core payload and we'd configure everything day 1 or 2 with NNCPs, but based on past discussion that's going to be a much bigger hill to climb so I'd prefer not to in this first version.

[jcaamano]

So let's say that you copy the entire day 1 config into an NNCP and make changes to that NNCP on day 2. Would that result in nmstate operator writing back updated config to /etc/NetwrokManager/system-connections? Or would the node, upon reboot, use the original day 1 configuration in /etc/NetwrokManager/system-connections and then once the nmstate operator is running again would hte configuraiton be reconciled?

[jcaamano]

I guess we cannot remove any currently supported use case from configure-ovs if we want to keep backward compatibility. So I guess this is more about an new way of supporting new advanced use cases without complicating configure-ovs more than it already is.

[cybertron]

Good point. Until/unless we replace configure-ovs and provide a migration path we probably can't actually remove anything.

[cybertron]

Unfortunately it looks like applying an NNCP to br-ex, even if there are no changes from the day 1 configuration, still breaks connectivity. That's a pretty significant problem for this design, which means one of two things will need to happen:

1. We change the scope so that this only applies to day1 and you are still not allowed to change br-ex using the operator.
2. We find a way to make updates with the operator work. Unless there's some quick fix (which I doubt), this option likely means missing 4.15.

[jcaamano]

Unfortunately it looks like applying an NNCP to br-ex, even if there are no changes from the day 1 configuration, still breaks connectivity. That's a pretty significant problem for this design, which means one of two things will need to happen:

1. We change the scope so that this only applies to day1 and you are still not allowed to change br-ex using the operator.
2. We find a way to make updates with the operator work. Unless there's some quick fix (which I doubt), this option likely means missing 4.15.

I was just going to update here that I tried this today and I did not get very far. :/ I would like to touch base with @qinqon on this next week.

1. We change the scope so that this only applies to day1 and you are still not allowed to change br-ex using the operator.

You would still be allowed to make day-2 configuration changes on the resulting NM configuration that should live in /etc/NetworkManager/system-connections right? Which is probably not much more difficult than handling the source nmstate manifests.

[qinqon]

Unfortunately it looks like applying an NNCP to br-ex, even if there are no changes from the day 1 configuration, still breaks connectivity. That's a pretty significant problem for this design, which means one of two things will need to happen:

1. We change the scope so that this only applies to day1 and you are still not allowed to change br-ex using the operator.
2. We find a way to make updates with the operator work. Unless there's some quick fix (which I doubt), this option likely means missing 4.15.

@cybertron can you dump here the original nmstate from br-ex and what change did you try ? I hink nmstate team has fix some issues related to ovs/ovn like https://docs.rs/nmstate/latest/nmstate/struct.OvsBridgeConfig.html#structfield.allow_extra_patch_ports

[cybertron]

Here's the NMState YAML I used. This is the same both on day 1 and on day 2 in the NNCP.

- name: enp2s0
  type: ethernet
  state: up
  ipv4:
    enabled: false
  ipv6:
    enabled: false
- name: br-ex
  type: ovs-bridge
  state: up
  copy-mac-from: enp2s0
  ipv4:
    enabled: false
    dhcp: false
  ipv6:
    enabled: false
    dhcp: false
  bridge:
    port:
    - name: enp2s0
    - name: br-ex
- name: br-ex
  type: ovs-interface
  state: up
  ipv4:
    enabled: true
    address:
    - ip: "192.168.111.110"
      prefix-length: 24
  ipv6:
    enabled: false
    dhcp: false
dns-resolver:
  config:
    server:
    - 192.168.111.1
routes:
  config:
  - destination: 0.0.0.0/0
    next-hop-address: 192.168.111.1
    next-hop-interface: br-ex```

[jcaamano]

We tested day-2 configuration changes with Kubernetes NMstate and got them more-less working with this fix:
openshift/machine-config-operator#3998
More potential fixes might be needed in Kubernetes NMstate for smoother operation.

There is still the open question of how to quantify the possible disruption of making day-2 configuration changes with Kubernetes NMstate on the default network when compared to the procedure that could be done today which implies a rolling reboot.

I think that the only day-2 change documented today is the MTU migration procedure:
https://docs.openshift.com/container-platform/4.13/networking/changing-cluster-network-mtu.html

So we could try it out. Overall I think we need to cover that procedure for users using the mechanism described in this enhancement.

[cgoncalves]

FYI, we have 7 test cases for day-2 operations on the br-ex attached default gateway interface (kernel bond) and enslaved VLAN/SR-IOV VF interfaces: https://docs.google.com/document/d/1QSbSziss64vLnGiPhFbH2jL8gJY-26VdhSny0t_MNPE/

This is part of a 4.14.z epic: https://issues.redhat.com/browse/CNF-7461

[openshift-bot]

Inactive enhancement proposals go stale after 28d of inactivity.

See https:/openshift/enhancements#life-cycle for details.

Mark the proposal as fresh by commenting /remove-lifecycle stale.
Stale proposals rot after an additional 7d of inactivity and eventually close.
Exclude this proposal from closing by commenting /lifecycle frozen.

If this proposal is safe to close now please do so with /close.

/lifecycle stale

[openshift-bot]

Stale enhancement proposals rot after 7d of inactivity.

See https:/openshift/enhancements#life-cycle for details.

Mark the proposal as fresh by commenting /remove-lifecycle rotten.
Rotten proposals close after an additional 7d of inactivity.
Exclude this proposal from closing by commenting /lifecycle frozen.

If this proposal is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

[openshift-bot]

Rotten enhancement proposals close after 7d of inactivity.

See https:/openshift/enhancements#life-cycle for details.

Reopen the proposal by commenting /reopen.
Mark the proposal as fresh by commenting /remove-lifecycle rotten.
Exclude this proposal from closing again by commenting /lifecycle frozen.

/close

[openshift-ci]

@openshift-bot: Closed this PR.

In response to this:

Rotten enhancement proposals close after 7d of inactivity.

See https:/openshift/enhancements#life-cycle for details.

Reopen the proposal by commenting /reopen.
Mark the proposal as fresh by commenting /remove-lifecycle rotten.
Exclude this proposal from closing again by commenting /lifecycle frozen.

/close

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.

[dhellmann]

(automated message) This pull request is closed with lifecycle/rotten. The associated Jira ticket, OPNET-265, has status "In Progress". Should the PR be reopened, updated, and merged? If not, removing the lifecycle/rotten label will tell this bot to ignore it in the future.

[cybertron]

/remove-lifecycle rotten

I do still need to update this to reflect the new template though.

[cybertron]

Rebased and updated to pass lint. I filled out the TBDs for reviewers and approvers, but I'm happy to change those if someone else would be more appropriate.

[cgoncalves]

FYI, opened RFE: https://issues.redhat.com/browse/RFE-5401

[cgoncalves]

Suggested change

	> OVN-Kubernetes will not handle the replacement of the gateway interface (enp2s0 in the example above). This is an existing limitation where OVN-Kubernetes installs an OpenFlow rule in br-ex that states the gateway interface is the OVS port ID of enp2s0 (port 1) and where attaching a new interface, say enp3s0, its OVS port ID will be different hence no egress traffic forwarding.
	> OVN-Kubernetes will not handle the replacement of the gateway interface (e.g. enp2s0). This is an existing limitation where OVN-Kubernetes installs an OpenFlow rule in br-ex that states the gateway interface is the OVS port ID of enp2s0 (port 1) and where attaching a new interface, say enp3s0, its OVS port ID will be different hence no egress traffic forwarding.

[jcaamano]

I wonder if we should check just for a file of the same name or just any file:

• Do we consider the the method described here as the only supported mechanism to deploy nmstate config on the host?
• What if there was some configuration already there from a system on a previous version that was upgraded to this version and user wants to start using this mechanism?

What I mean here is that if there are other files we can't really be sure if there's any conflict. So I feel this should be a completely exlusive mechanism and warned as so in the docs.

[cybertron]

Agreed, and this is already our recommendation for kubernetes-nmstate because it is problematic to have multiple separate configurations attempting to configure the same host. I will add to this section to be explicit that when using the new mechanism for configuration we expect that to be the only network configuration used.

[jcaamano]

Two questions here:

• Do we prevent in any way processing day-2 changes on the MachineConfigs described in this procedure? If not, would that be an alternative mechanism to deploy day-2 changes if user is willing to put up with a reboot? I guess the answer is yes, to allow for the scale out scenario.
• Do we prevent day-2 with Kubernetes-NMState to deployments that are still relying on configure-ovs

[cybertron]

• A reboot won't happen on day 2 changes to these files because we are adding the /etc/nmstate/openshift path to the reboot-less update list in MCO. We won't otherwise prevent changes to these files, but changes to existing nodes will have no effect because these configs are only applied on first boot.
• We have a warning in the kubernetes-nmstate docs that modification of br-ex and the underlying interface is not allowed. We'll need to modify that to allow changes if this new feature is used instead of configure ovs.

[jcaamano]

There is a very little nit handling on configure-ovs tied to the MTU migration procedure:

https:/openshift/machine-config-operator/blob/204767253e30608b5b7fd70ad1ace02ba1d64b46/templates/common/_base/files/configure-ovs-network.yaml#L880-L884

[cybertron]

Thanks for the heads up. I will make sure this is handled in the implementation patch.

[cybertron]

I think this should address all of the comments so far. Let me know what you think.

I'll also be updating the implementation patch today based on what we've discussed here and in the sync meeting.

[cgoncalves]

LGTM. Thank you!

[jcaamano]

/lgtm

[qinqon]

/lgtm

[yuqi-zhang]

Functionally I think the proposal makes sense. Most of the MCO's concerns are around having a proper API workflow around this, which as I understand is in long-term design considerations. Just leaving some notes based on our discussions on slack so we keep those in mind

[yuqi-zhang]

As a note, "firstboot only" configuration is a pattern in the MCO (for e.g. disks) so it's fine to have it this way, but technically we'd like any machineconfig based changes to take effect, if possible

[yuqi-zhang]

this pattern should work but is somewhat exploiting the MCO to do a "per-machine" config, which is a pattern we don't have atm. We would like the ability to do so eventually so we can just keep that in mind

[yuqi-zhang]

Discussed on slack, but it feels like we should have a higher level API for this instead of using MachineConfigs as the configuration point

[yuqi-zhang]

Just a note that this isn't presently directly supported in the new API mentioned below, but we can add an exception for now

[knobunc]

/approve

[openshift-ci]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: knobunc

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

The pull request process is described here

Needs approval from an approver in each of these files:

• enhancements/network/OWNERS [knobunc]

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

[openshift-ci]

@cybertron: 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/test-infra repository. I understand the commands that are listed here.