Enhance NetworkInterface Address and State handling

[maxmoehl]

Summary

• Support reporting the allocated IP addresses, prefixes, and interface state via IRI.
• Make the machinepoollet the controller of the network interface associated with its machine pool, including writing back the status from the IRI response.
• Allow creation of network interfaces without IP addresses or IP families.

Motivation

Opening up the contract and handing control over the network interface to the machinepoollet allows creating novel network implementations that don't assume a top-down approach for IP addresses. It also lifts the requirement of implementing a dedicated control-plane for managing the status of the network interface like it is required today.

[maxmoehl]

Looking in more detail at the current IRI API surface and how it's used I believe we should not adjust anything except the Machine message type. The pollet anyway uses that as its source of truth and with asynchronous providers there is nothing to return on the initial attach call anyway. The question then becomes how we keep IRI relatively simple (the network interface status always lives inside the machine status) while also allowing the poollet to report the effective values at the right place (machine status vs. network interface status).

In that case, the change is almost trivial, we just add the new fields to the NetworkInterfaceStatus and extend the NetworkInterfaceState so that it can cover the lifecycle of both states, the one inside the machine and the one in the network interface:

diff --git i/iri/apis/machine/v1alpha1/api.proto w/iri/apis/machine/v1alpha1/api.proto
index 2879ba4a..364e9129 100644
--- i/iri/apis/machine/v1alpha1/api.proto
+++ w/iri/apis/machine/v1alpha1/api.proto
@@ -134,11 +134,16 @@ message NetworkInterfaceStatus {
   string name = 1;
   string handle = 2;
   NetworkInterfaceState state = 3;
+  repeated string ips = 4;
+  repeated string prefixes = 5;
+  string virtual_ip = 6;
 }
 
 enum NetworkInterfaceState {
   NETWORK_INTERFACE_PENDING = 0;
   NETWORK_INTERFACE_ATTACHED = 1;
+  NETWORK_INTERFACE_READY = 2;
+  NETWORK_INTERFACE_ERROR = 3;
 }
 
 enum MachineState {

For the poollet the state becomes a threshold function:

• pending: both pending.
• ready: the network interface is ready and it's data (IPs, prefixes, virtual IP) are final, machine side stays pending.
• error: network interface creation failed which is mirrored into the network interface state, machine side stays pending
• attached: has been attached to the VM, network interface automatically becomes ready, machine side becomes attached.

If a provider does not report the additional data poollet will initially not fill it in, leaving whatever component currently manages the NIC state to do that. For the current ironcore-net use-case the NIC is most likely already set to ready when the poollet gets word of it, that should also not be an issue. The poollet should initially ignore if someone else populates the ip / prefix / vip fields, but eventually we might want to give it full ownership and have it remove anything it isn't aware of.

As for breaking vs. non-breaking change: the change in itself is compatible thanks to protobuf. The only thing to be aware of is that a provider reporting a new state to an old poollet would trigger an error as it doesn't know how to handle that. Since poollet and provider are anyway usually deployed as one this should not be an issue. Pairing a newer poollet with an old provider should also work without any issues.

On the Kubernetes API side we only need to mark the IP families and IP addresses field as optional to allow creation without those.

[maxmoehl]

@adracus and I discussed the API changes and agreed to the following adjustments:

1. Drop the virtual IP from the status. Virtual IPs are usually attached in some way and not an inherent property of the network interface. We currently don't see in which case a network plugin would report a VIP. If that ever comes up, it's cheap to add at a later point.
2. Add the network interface metadata to the IRI network interface. This makes it clear that the poollet is operating on the actual network interface and allows for future logic to group networks based on namespaces or other metadata, should that be desired.

The updated patch is:

diff --git a/iri/apis/machine/v1alpha1/api.proto b/iri/apis/machine/v1alpha1/api.proto
index 2879ba4a..eb690bd7 100644
--- a/iri/apis/machine/v1alpha1/api.proto
+++ b/iri/apis/machine/v1alpha1/api.proto
@@ -85,6 +85,7 @@ message NetworkInterface {
   string network_id = 2;
   repeated string ips = 3;
   map<string, string> attributes = 4;
+  meta.v1alpha1.ObjectMetadata metadata = 5;
 }
 
 enum Power {
@@ -134,11 +135,15 @@ message NetworkInterfaceStatus {
   string name = 1;
   string handle = 2;
   NetworkInterfaceState state = 3;
+  repeated string ips = 4;
+  repeated string prefixes = 5;
 }
 
 enum NetworkInterfaceState {
   NETWORK_INTERFACE_PENDING = 0;
   NETWORK_INTERFACE_ATTACHED = 1;
+  NETWORK_INTERFACE_READY = 2;
+  NETWORK_INTERFACE_ERROR = 3;
 }
 
 enum MachineState {

[maxmoehl]

With the addition of metadata we get another interesting behavior: the only way for the machinepoollet to update a network interface is through re-attaching, there is no update RPC it could call. If you are managing your resources via Flux or some other component does bookkeeping in annotations every change would cause your NIC to be re-attached which seems wrong. We have two options:

1. Never update metadata on its own, only pass the metadata at attach.
2. Introduce a new RPC to update the NIC without re-attaching it.

[maxmoehl]

While testing a rough implementation to see if this fits our use-case for a network plugin which propagates the IP address up it became clear that running two networks in parallel is dangerous. With apinetlet assuming responsibility over all network interfaces it reverts the status to pending after the machinepoollet sets it to ready. A workaround is to annotate it with common.ironcore.dev/managed-by, maybe that will be the transition plan then, have the poollet annotate every network interface it takes over that way to have a smooth transition.

[johamster]

On the added states to NetworkInterfaceState, also copied below:

 enum NetworkInterfaceState {
   NETWORK_INTERFACE_PENDING = 0;
   NETWORK_INTERFACE_ATTACHED = 1;
+  NETWORK_INTERFACE_READY = 2;
+  NETWORK_INTERFACE_ERROR = 3;
 }

If I'm understanding correctly the new state transition would be PENDING -> READY -> ATTACHED. I guess that should be documented in the protobuf code. Potentially, it would make sense to swap READY and ATTACHED in the declaration. Then, the normal transition flow would be in the declaration. From protobuf POV that should not be a real change if the IDs are retained.

And on the naming: Is the READY -> ATTACHED transition obvious for everyone? If not, renaming ATTACHED to READ_AND_ATTACHED would make it explicit also for code using the enum.

Btw, do we need to care about backward compatibility already? If not, can't we actually renumber the enum variants still? Or would breaking changes require a version bump? In that case, it could be postponed.

[maxmoehl]

On the added states to NetworkInterfaceState, also copied below:

 enum NetworkInterfaceState {
   NETWORK_INTERFACE_PENDING = 0;
   NETWORK_INTERFACE_ATTACHED = 1;
+  NETWORK_INTERFACE_READY = 2;
+  NETWORK_INTERFACE_ERROR = 3;
 }

If I'm understanding correctly the new state transition would be PENDING -> READY -> ATTACHED. I guess that should be documented in the protobuf code. Potentially, it would make sense to swap READY and ATTACHED in the declaration. Then, the normal transition flow would be in the declaration. From protobuf POV that should not be a real change if the IDs are retained.

Yes, your understanding is correct. The code block in my comment was primarily there to make clear what would change on the API level, the PR will contain some doc comments and I can also re-order the lines to match the actual flow.

And on the naming: Is the READY -> ATTACHED transition obvious for everyone? If not, renaming ATTACHED to READ_AND_ATTACHED would make it explicit also for code using the enum.

I'd prefer to keep the distinct states with their own names and document the semantics accordingly. State transitions are something we commonly deal with and later states usually don't include all of the states that had to be transitioned before them.

Btw, do we need to care about backward compatibility already? If not, can't we actually renumber the enum variants still? Or would breaking changes require a version bump? In that case, it could be postponed.

Renumbering or not is something I defer to @afritzler and @adracus.

[adracus]

IMO we can still renumber, just make sure we add a breaking notice to our release. For a consumer, this means they have to bump their IRI-version in accordance with their implementation (e.g. provider-libvirt).