search

metal3-io / utility-images

Containerized utility images for the Metal3 project
Apache License 2.0
2 stars 2 forks source link

Parameterize interface name #20

Closed mboukhalfa closed 2 months ago

mboukhalfa commented 2 months ago

CAPM3 cluster template include an interface name which might be found on the VM thus we add option to get the interface name from the fake system

mboukhalfa commented 2 months ago

/hold I will add loop over all interfaces

mboukhalfa commented 2 months ago

/hold cancel

mboukhalfa commented 2 months ago

/assign @Rozzii

mboukhalfa commented 2 months ago

/hold

mboukhalfa commented 2 months ago

/hold cancel

nic named enp1s0 enp2s0 ..

kashifest commented 2 months ago

/lgtm

Rozzii commented 2 months ago

I am not sure that is the actual interface name, Cluster template and metal3data template indeed reference "interface names" but those are not real interface names. The interface info from our "template files" will end up as cloud-init OpenStack network data that will be consumed by cloud init during user space initialization.

AFAIK in cloud-init OpenStack network data the "interface names" are just internal references (internal to the network data file) the actual interface is identified by the MAC address.

I might have misunderstood your commit message, so let's discuss this. /hold

mboukhalfa commented 2 months ago

I am not sure that is the actual interface name, Cluster template and metal3data template indeed reference "interface names" but those are not real interface names. The interface info from our "template files" will end up as cloud-init OpenStack network data that will be consumed by cloud init during user space initialization.

AFAIK in cloud-init OpenStack network data the "interface names" are just internal references (internal to the network data file) the actual interface is identified by the MAC address.

I might have misunderstood your commit message, so let's discuss this. /hold

That what we use in our cluster templates https://github.com/metal3-io/cluster-api-provider-metal3/blob/348375cd3705ccad398d8f0b82895ccfc468aaab/test/e2e/data/infrastructure-metal3/bases/cluster/cluster-with-kcp.yaml#L99 So we expect the VM to have interface named enp1s0 enp2s0 and that what we get in the BMH after inspection how it got named in real I am not sure whether the virsh VM has prenamed nics or it get the names after what I am adding here is just defaulting the nic name later the user can specify when defining the fake VMs any name

mboukhalfa commented 2 months ago

@Rozzii I create an issue for that I think still that current PR is just defaulting the nic names or getting whatever the user specify in the fake node definition in sushytools. Is it fine to unhold this ?

Rozzii commented 2 months ago

Just for those who will read this later, it looks like both @mboukhalfa and me are right here. Indeed right now CAPM3 checks the interface names reported by IPA, even though IMO it should not.

I am okay with merging this pr although if we would fix https://github.com/metal3-io/cluster-api-provider-metal3/issues/1998 there would be no need for this PR.

I am fine with removing the hold /hold cancel

Rozzii commented 2 months ago

Eventually it would be desired IMO to be able to provide a config file where we could specify any number of interfaces thus "mock" different scenarios, instead of just reporting the interface config of the host.

mboukhalfa commented 2 months ago

I am okay with merging this pr although if we would fix metal3-io/cluster-api-provider-metal3#1998 there would be no need for this PR.

Probably the interface name is not important if the issue fixed on capm3 but still the PR provides improvement to fakeIPA in term of allowing adding multiple interfaces instead of only one and used .get(key) instead of [key] to avoid the issue with key error if the key does not exist

metal3-io-bot commented 2 months ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Rozzii

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: - ~~[OWNERS](https://github.com/metal3-io/utility-images/blob/main/OWNERS)~~ [Rozzii] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment