Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parameterize interface name #20

Merged

Conversation

mboukhalfa
Copy link
Member

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

@metal3-io-bot metal3-io-bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Sep 20, 2024
@mboukhalfa mboukhalfa force-pushed the Parameterize-interface-name/mboukhalfa branch from 8b04baf to 759060a Compare September 20, 2024 05:36
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 20, 2024
@mboukhalfa
Copy link
Member Author

/hold I will add loop over all interfaces

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2024
@mboukhalfa mboukhalfa force-pushed the Parameterize-interface-name/mboukhalfa branch from 759060a to 2ede099 Compare September 20, 2024 06:23
@metal3-io-bot metal3-io-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 20, 2024
@mboukhalfa mboukhalfa force-pushed the Parameterize-interface-name/mboukhalfa branch from 2ede099 to 091f03a Compare September 20, 2024 06:25
@metal3-io-bot metal3-io-bot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 20, 2024
@mboukhalfa
Copy link
Member Author

/hold cancel

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2024
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

q + indent issues

fake-ipa/fake_ipa/inspector.py Outdated Show resolved Hide resolved
fake-ipa/fake_ipa/inspector.py Outdated Show resolved Hide resolved
@mboukhalfa mboukhalfa force-pushed the Parameterize-interface-name/mboukhalfa branch from 091f03a to f4c1e79 Compare September 20, 2024 06:37
@metal3-io-bot metal3-io-bot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Sep 20, 2024
Copy link
Member

@tuminoid tuminoid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 20, 2024
@mboukhalfa
Copy link
Member Author

/assign @Rozzii

@mboukhalfa
Copy link
Member Author

/hold

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2024
Signed-off-by: Mohammed Boukhalfa <mohammed.boukhalfa@est.tech>
@mboukhalfa mboukhalfa force-pushed the Parameterize-interface-name/mboukhalfa branch from f4c1e79 to ea7810a Compare September 20, 2024 07:43
@metal3-io-bot metal3-io-bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 20, 2024
@mboukhalfa
Copy link
Member Author

/hold cancel

nic named enp1s0 enp2s0 ..

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 20, 2024
@kashifest
Copy link
Member

/lgtm

@metal3-io-bot metal3-io-bot added the lgtm Indicates that a PR is ready to be merged. label Sep 23, 2024
@Rozzii
Copy link
Member

Rozzii commented Sep 23, 2024

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

@metal3-io-bot metal3-io-bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 23, 2024
@mboukhalfa
Copy link
Member Author

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
Copy link
Member Author

@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
Copy link
Member

Rozzii commented Sep 25, 2024

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 metal3-io/cluster-api-provider-metal3#1998 there would be no need for this PR.

I am fine with removing the hold
/hold cancel

@metal3-io-bot metal3-io-bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 25, 2024
@Rozzii
Copy link
Member

Rozzii commented Sep 25, 2024

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
Copy link
Member Author

mboukhalfa commented Sep 25, 2024

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

Copy link
Member

@Rozzii Rozzii left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@metal3-io-bot
Copy link
Collaborator

[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:

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

@metal3-io-bot metal3-io-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 25, 2024
@metal3-io-bot metal3-io-bot merged commit 5603eea into metal3-io:main Sep 25, 2024
2 checks passed
@metal3-io-bot metal3-io-bot deleted the Parameterize-interface-name/mboukhalfa branch September 25, 2024 13:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/S Denotes a PR that changes 10-29 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants