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

Jump to bottom

baremetal: make provisioningNetworkInterface optional #5015

Merged
merged 1 commit into from Jul 21, 2021
Merged

baremetal: make provisioningNetworkInterface optional #5015

merged 1 commit into from Jul 21, 2021

Conversation

asalkeld
Copy link
Contributor

@asalkeld asalkeld commented Jun 21, 2021 •
edited

No description provided.

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 21, 2021
@asalkeld
Copy link
Contributor Author

/retest

@asalkeld
Copy link
Contributor Author

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 24, 2021
@asalkeld
Copy link
Contributor Author

/retest

@asalkeld
Copy link
Contributor Author

/hold
requires this to merge openshift/cluster-baremetal-operator#132

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jun 27, 2021
@asalkeld
Copy link
Contributor Author

asalkeld commented Jul 2, 2021

/hold cancel
/retest
(dependent PR merged)

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 2, 2021
@asalkeld
Copy link
Contributor Author

asalkeld commented Jul 5, 2021

/retest

1 similar comment
@sadasu
Copy link
Contributor

sadasu commented Jul 6, 2021

/retest

@dhellmann
Copy link
Contributor

/uncc dhellmann

@openshift-ci openshift-ci bot removed the request for review from dhellmann July 6, 2021 21:58
hardys
hardys reviewed Jul 9, 2021
View reviewed changes
docs/user/metal/install_ipi.md
@@ -50,7 +50,7 @@ accessible over the external network which may not be desirable.

* **NIC #2 - Provisioning Network (optional) **
* A private network used for PXE based provisioning.
* You must specify `provisioningNetworkInterface` to indicate which
* You can specify `provisioningNetworkInterface` to indicate which
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we mention that if not specified the interface is derived from the bootMACAddress?

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also mention that when provisioningNetwrokInterface is specified, the user needs to make sure all the control plane hosts have an interface of that name?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks, updated. Apologies for the slow response.

@hardys
Copy link
Contributor

hardys commented Jul 9, 2021

This also depends on openshift/cluster-baremetal-operator#149

@hardys
Copy link
Contributor

hardys commented Jul 9, 2021

/retest

@hardys
Copy link
Contributor

hardys commented Jul 9, 2021

/approve
/cc @sadasu @kirankt

@openshift-ci openshift-ci bot requested review from kirankt and sadasu July 9, 2021 10:32
@hardys
Copy link
Contributor

hardys commented Jul 9, 2021

Ugh, the manifests file isn't covered by baremetal-approvers OWNERS, but

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 9, 2021
sadasu
sadasu reviewed Jul 12, 2021
View reviewed changes
Copy link
Contributor

@sadasu sadasu left a comment

Choose a reason for hiding this comment

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

/lgtm

@sadasu
Copy link
Contributor

sadasu commented Jul 13, 2021

/test e2e-metal-ipi-ovn-ipv6

1 similar comment
@sadasu
Copy link
Contributor

sadasu commented Jul 19, 2021

/test e2e-metal-ipi-ovn-ipv6

@stbenjam
Copy link
Member

/label platform/baremetal

Would you mind changing metal3 to baremetal in the commit and PR title? Metal3 doesn't mean much to the installer folks.

@openshift-ci openshift-ci bot added the platform/baremetal IPI bare metal hosts platform label Jul 20, 2021
@stbenjam
Copy link
Member

stbenjam commented Jul 20, 2021 •
edited

@asalkeld asalkeld changed the title metal3: make provisioningNetworkInterface optional baremetal: make provisioningNetworkInterface optional Jul 20, 2021
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 20, 2021
@stbenjam
Copy link
Member

/retest
/lgtm
/assign @patrickdillon

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 21, 2021 •
edited

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

Test name Commit Details Rerun command
ci/prow/e2e-aws-fips 334b3c9 link /test e2e-aws-fips
ci/prow/e2e-openstack-kuryr 334b3c9 link /test e2e-openstack-kuryr
ci/prow/e2e-metal-ipi-ovn-ipv6 334b3c9 link /test e2e-metal-ipi-ovn-ipv6
ci/prow/e2e-crc 334b3c9 link /test e2e-crc
ci/prow/e2e-libvirt 334b3c9 link /test e2e-libvirt
ci/prow/e2e-aws-workers-rhel7 334b3c9 link /test e2e-aws-workers-rhel7

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.

@patrickdillon
Copy link
Contributor

/approve

Also, please include more informative commit messages according to the contributing guidelines.

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 21, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hardys, patrickdillon

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2021
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

5 similar comments
@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest-required

Please review the full test history for this PR and help us cut down flakes.

@openshift-merge-robot openshift-merge-robot merged commit 735d664 into openshift:master Jul 21, 2021
hardys pushed a commit to hardys/installer that referenced this pull request Jul 22, 2021
baremetal: reinstate provisioningInterface for provisioning CR
29dddd0 
This was removed in openshift#5015 but we should still include it if a user
provides a value - since it's now optional we'll pass an empty
string if no value is provided, which will trigger the new logic
that detects the interface name by MAC.
@hardys
Copy link
Contributor

hardys commented Jul 22, 2021

@patrickdillon @sadasu @stbenjam FYI this caused a regression ref https://bugzilla.redhat.com/show_bug.cgi?id=1984576

Looking at the test history, I can see that pull-ci-openshift-installer-master-e2e-metal-ipi-ovn-ipv6 never passed, we need to be careful to ensure that passes before merging any baremetal IPI PRs

@hardys hardys mentioned this pull request Jul 22, 2021
Merged
@hardys
Copy link
Contributor

hardys commented Jul 22, 2021

#5100 should resolve the regression I think.

@stbenjam
Copy link
Member

Looking at the test history, I can see that pull-ci-openshift-installer-master-e2e-metal-ipi-ovn-ipv6 never passed, we need to be careful to ensure that passes before merging any baremetal IPI PRs

Yea sorry I missed this. I think the installer team normally relies on the platform teams to /lgtm before merging and take that as our word we think the change is good.

patrickdillon pushed a commit to patrickdillon/installer that referenced this pull request Jul 22, 2021
@patrickdillon
baremetal: reinstate provisioningInterface for provisioning CR
8d3ee93 
This was removed in openshift#5015 but we should still include it if a user
provides a value - since it's now optional we'll pass an empty
string if no value is provided, which will trigger the new logic
that detects the interface name by MAC.
AnnaZivkovic pushed a commit to AnnaZivkovic/installer that referenced this pull request Apr 1, 2022
baremetal: reinstate provisioningInterface for provisioning CR
9f49b01 
This was removed in openshift#5015 but we should still include it if a user
provides a value - since it's now optional we'll pass an empty
string if no value is provided, which will trigger the new logic
that detects the interface name by MAC.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@hardys hardys hardys left review comments

@sadasu sadasu sadasu left review comments

@celebdor celebdor Awaiting requested review from celebdor

@kirankt kirankt Awaiting requested review from kirankt

Assignees

@stbenjam stbenjam

@hardys hardys

@sadasu sadasu

@patrickdillon patrickdillon

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. platform/baremetal IPI bare metal hosts platform
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
8 participants
@asalkeld @sadasu @dhellmann @hardys @stbenjam @patrickdillon @openshift-bot @openshift-merge-robot