Skip to content

Conversation

@bn222
Copy link

@bn222 bn222 commented Jan 20, 2022

Currently, the provision interface name is hardcoded to ens4. On my setup, this interface has a different name but there is no way to override this.

This patch adds a way to specify a different name.

@openshift-ci openshift-ci bot requested review from ardaguclu and sadasu January 20, 2022 14:23
@zshi-redhat
Copy link

@bn222 Some CI jobs are failing (e.g. verify-codegen), you may need to run the hack/verify-codegen.sh to generate the code and commit it to the same PR.

@zshi-redhat
Copy link

For other reviewers, this is an issue Balazs found on the ARM platform where it has a different interface name to ens4 (on x86 VMs). I think we can either provides an override interface as what this PR does or detect the platform to use the right interface name (assuming the interface name would persist across all ARM VMs).

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 24, 2022

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign jhixson74 after the PR has been reviewed.
You can assign the PR to them by writing /assign @jhixson74 in a comment when ready.

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

Details 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

@bn222
Copy link
Author

bn222 commented Jan 24, 2022

@zshi-redhat Thanks for adding the comment. I've run hack/verify-codegen.sh and added it to the PR.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 26, 2022

@bn222: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
ci/prow/okd-e2e-aws-upgrade 7297160 link false /test okd-e2e-aws-upgrade
ci/prow/e2e-metal-single-node-live-iso 7297160 link false /test e2e-metal-single-node-live-iso
ci/prow/e2e-aws 7297160 link true /test e2e-aws
ci/prow/e2e-ibmcloud 7297160 link false /test e2e-ibmcloud
ci/prow/e2e-metal-ipi-ovn-dualstack 7297160 link false /test e2e-metal-ipi-ovn-dualstack
ci/prow/e2e-aws-workers-rhel7 7297160 link false /test e2e-aws-workers-rhel7
ci/prow/e2e-metal-ipi-virtualmedia 7297160 link false /test e2e-metal-ipi-virtualmedia
ci/prow/e2e-alibaba 7297160 link true /test e2e-alibaba
ci/prow/e2e-aws-workers-rhel8 7297160 link false /test e2e-aws-workers-rhel8
ci/prow/e2e-metal-ipi-swapped-hosts 7297160 link false /test e2e-metal-ipi-swapped-hosts
ci/prow/okd-verify-codegen 7297160 link true /test okd-verify-codegen
ci/prow/e2e-metal-ipi 7297160 link false /test e2e-metal-ipi
ci/prow/okd-e2e-aws 7297160 link false /test okd-e2e-aws
ci/prow/e2e-metal-ipi-ovn-ipv6-required 7297160 link true /test e2e-metal-ipi-ovn-ipv6-required
ci/prow/e2e-metal-ipi-ovn-ipv6 7297160 link false /test e2e-metal-ipi-ovn-ipv6
ci/prow/e2e-azure-upi 7297160 link false /test e2e-azure-upi
ci/prow/okd-images 7297160 link true /test okd-images
ci/prow/e2e-crc 7297160 link false /test e2e-crc
ci/prow/verify-codegen 7297160 link true /test verify-codegen

Full PR test history. Your PR dashboard.

Details

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.

if config.BootstrapProvisioningInterfaceOverride == "" {
templateData.ProvisioningInterface = "ens3"
} else {
templateData.ProvisioningInterface = config.BootstrapProvisioningInterfaceOverride
Copy link

Choose a reason for hiding this comment

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

This nic is defined in the (installer created) bootstrap VM - so I'm not sure it's reasonable to expect the user to somehow know what the appropriate nic name is here?

I'm wondering why the VM nic name is different in your environment (which is ARM I believe) - is there some way we can make the nic name consistent between architectures instead, or else detect the required value based on the architecture?

@hardys
Copy link

hardys commented Feb 9, 2022

/label platform/baremetal

/cc @zaneb @sadasu @andfasano

@openshift-ci openshift-ci bot requested review from andfasano and zaneb February 9, 2022 10:31
@openshift-ci openshift-ci bot added the platform/baremetal IPI bare metal hosts platform label Feb 9, 2022
@hardys
Copy link

hardys commented Feb 10, 2022

For other reviewers, this is an issue Balazs found on the ARM platform where it has a different interface name to ens4 (on x86 VMs)

Do we know why the name is different? It seems weird that the nic number would increment if the VM definition is the same e.g we didn't add any nics?

I'm wondering if there's some way to make the name consistent, and if that's not possible then IMO we should set the value inside the installer and not expose this implementation detail to the user.

@jeffdyoung
Copy link
Contributor

@hardys For me the nics are named different between arm/x86 because the bootstrap vm created by libvirt uses pci vs pcie controllers:
x86: <controller type="pci" index="0" model="pci-root"/> results in ensX
arm64: <controller type="pci" index="0" model="pcie-root"/> results in enpXs0

More on nic naming in the rhel 8 network doc

I have time to look into what it would take to discover the right nic name in the bootstrap container based on mac address (or something we don't have to hardcode), if you all agree and think it's feasible.

@ardaguclu
Copy link
Member

/uncc

@openshift-ci openshift-ci bot removed the request for review from ardaguclu March 15, 2022 11:26
@bn222
Copy link
Author

bn222 commented Mar 21, 2022

Superseded by #5698

@bn222 bn222 closed this Mar 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

platform/baremetal IPI bare metal hosts platform

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants