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

Bug 1796929: baremetal: make provisioning networks more configurable #2895

Merged

Conversation

stbenjam
Copy link
Member

@stbenjam stbenjam commented Jan 9, 2020

This change adds 4 new options to the baremetal platform:

  • ProvisioningNetworkInterface
  • ProvisioningNetworkCIDR
  • ProvisioningDHCPExternal
  • ProvisioningDHCPRange

This makes the provisioning network more configurable, by allowing users
to specify a specific network interface, which network, and settings for
whether the cluster should provide DHCP services on the provisioning
network, or if it will be managed externally. If we will provide DHCP
services on the network, the user may further customize which range to
use.

This change is a prerequisite to a number of additional high-priority
work items for the baremetal IPI platform, including single-stack IPv6
provisioning, external DHCP support, and end-to-end seamless deployment
without a manually created config map.

fixes #2091, among other issues.

Co-authored-by: Ian Main imain@redhat.com
Co-authored-by: Stephen Benjamin stephen@redhat.com

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 9, 2020
@stbenjam
Copy link
Member Author

stbenjam commented Jan 9, 2020

/cc @hardys @imain @sadasu

Since we're going with another approach for the CRD again, and openshift/api#540 won't be the solution, I'm breaking out the platform config changes separately so we can make progress on the other blocked tasks and deal with this asynchronously.

Once this lands and we have a CR somewhere, we can wire these values up with that.

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jan 10, 2020
@stbenjam
Copy link
Member Author

/label platform/baremetal

@openshift-ci-robot openshift-ci-robot added the platform/baremetal IPI bare metal hosts platform label Jan 10, 2020
@stbenjam stbenjam changed the title [WIP] baremetal: make provisioning networks more configurable baremetal: make provisioning networks more configurable Jan 10, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 10, 2020
@stbenjam stbenjam force-pushed the configurable-networks branch 2 times, most recently from 5f5e1a6 to 61cf2d8 Compare January 10, 2020 14:39
@sadasu
Copy link
Contributor

sadasu commented Jan 10, 2020

/test e2e-aws-upgrade

@metal3ci
Copy link

Build SUCCESS, see build http://10.8.144.11:8080/job/dev-tools/1410/

@stbenjam stbenjam force-pushed the configurable-networks branch 2 times, most recently from 7307e32 to a0e0981 Compare January 10, 2020 16:45
@metal3ci
Copy link

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1411/

@metal3ci
Copy link

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1412/

@metal3ci
Copy link

Build SUCCESS, see build http://10.8.144.11:8080/job/dev-tools/1413/

@stbenjam
Copy link
Member Author

/retest

@stbenjam
Copy link
Member Author

/test e2e-libvirt

@wking
Copy link
Member

wking commented Jan 15, 2020

Seems like metal docs on the install-config schema are here? Install-config settings aren't UPI-specific though, so I'd like to see metal with a customization.md (like this) to talk about the settings. Those metal docs would be linked from the platform-agnostic docs, e.g. here and here. But wherever metal-specific install-config docs are living, those docs should be bumped by this PR to keep up with the install-config type changes you're making.

@stbenjam
Copy link
Member Author

stbenjam commented Jan 15, 2020

Seems like metal docs on the install-config schema are here? Install-config settings aren't UPI-specific though, so I'd like to see metal with a customization.md (like this) to talk about the settings. Those metal docs would be linked from the platform-agnostic docs, e.g. here and here. But wherever metal-specific install-config docs are living, those docs should be bumped by this PR to keep up with the install-config type changes you're making.

This PR includes documentation changes to discuss the new settings: https://github.com/openshift/installer/pull/2895/files#diff-4fb874a2f3b6c60cebac5ba179481f94

I am happy to create a customization.md, would you like that included here? So far, we've mostly kept things in the install_ipi.md documents, as this is evolving. I wasn't sure we wanted to advertise the existence of the platform in other places in the installer docs until it's really something generally consumable.

There's a lot of work going on in https://github.com/openshift-kni/baremetal-deploy to write much more comprehensive docs, eventually those will be included here.

@wking
Copy link
Member

wking commented Jan 15, 2020

This PR includes documentation changes to discuss the new settings: https://github.com/openshift/installer/pull/2895/files#diff-4fb874a2f3b6c60cebac5ba179481f94

But that's up before the install-config section down on line R96? I guess the property docs are spread out and that install-config section is not exhaustive?

@stbenjam
Copy link
Member Author

This PR includes documentation changes to discuss the new settings: openshift/installer/pull/2895/files#diff-4fb874a2f3b6c60cebac5ba179481f94

But that's up before the install-config section down on line R96? I guess the property docs are spread out and that install-config section is not exhaustive?

We've had complaints in previous PR's when the install-config section had non-mandatory options, so I just enumerated them in the networking section, but it does seem like we need a customization.md. Can we handle that in a separate PR? I believe the baremetal docs in general do need a lot of work.

I opened #2931 as a rough draft.

@hardys
Copy link
Contributor

hardys commented Jan 16, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2020
This change adds 4 new options to the baremetal platform:

        - ProvisioningNetworkInterface
        - ProvisioningNetworkCIDR
        - ProvisioningDHCPExternal
        - ProvisioningDHCPRange

This makes the provisioning network more configurable, by allowing users
to specify a specific network interface, which network, and settings for
whether the cluster should provide DHCP services on the provisioning
network, or if it will be managed externally. If we will provide DHCP
services on the network, the user may further customize which range to
use.

This change is a prerequisite to a number of additional high-priority
work items for the baremetal IPI platform, including single-stack IPv6
provisioning, external DHCP support, and end-to-end seamless deployment
without a manually created config map.

Co-authored-by: Ian Main <imain@redhat.com>
Co-authored-by: Stephen Benjamin <stephen@redhat.com>
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2020
@hardys
Copy link
Contributor

hardys commented Jan 16, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 16, 2020
@metal3ci
Copy link

Build FAILURE, see build http://10.8.144.11:8080/job/dev-tools/1427/

@sdodson
Copy link
Member

sdodson commented Jan 16, 2020

All feedback appears to have been addressed and changes are isolated to baremetal templates now.
/approve

@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: sdodson

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-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 16, 2020
@openshift-bot
Copy link
Contributor

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit d9253bd into openshift:master Jan 16, 2020
@stbenjam stbenjam deleted the configurable-networks branch January 16, 2020 19:15
@openshift-ci-robot
Copy link
Contributor

@stbenjam: The following tests failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/e2e-openstack c6db602 link /test e2e-openstack
ci/prow/e2e-libvirt c6db602 link /test e2e-libvirt
ci/prow/e2e-aws-fips c6db602 link /test e2e-aws-fips
ci/prow/e2e-ovirt c6db602 link /test e2e-ovirt
ci/prow/e2e-aws-scaleup-rhel7 c6db602 link /test e2e-aws-scaleup-rhel7

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

@stbenjam stbenjam changed the title baremetal: make provisioning networks more configurable Bug 1796929: baremetal: make provisioning networks more configurable Jan 31, 2020
@openshift-ci-robot
Copy link
Contributor

@stbenjam: All pull requests linked via external trackers have merged. Bugzilla bug 1796929 has been moved to the MODIFIED state.

In response to this:

Bug 1796929: baremetal: make provisioning networks more configurable

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.

@stbenjam
Copy link
Member Author

/cherry-pick release-4.3

@openshift-cherrypick-robot

@stbenjam: #2895 failed to apply on top of branch "release-4.3":

error: Failed to merge in the changes.
Using index info to reconstruct a base tree...
M	docs/user/metal/install_ipi.md
M	pkg/asset/ignition/bootstrap/bootstrap.go
M	pkg/types/baremetal/validation/platform.go
M	pkg/types/baremetal/validation/platform_test.go
M	pkg/types/validation/installconfig_test.go
Falling back to patching base and 3-way merge...
Auto-merging pkg/types/validation/installconfig_test.go
Auto-merging pkg/types/baremetal/validation/platform_test.go
Auto-merging pkg/types/baremetal/validation/platform.go
Auto-merging pkg/asset/ignition/bootstrap/bootstrap.go
CONFLICT (content): Merge conflict in pkg/asset/ignition/bootstrap/bootstrap.go
Auto-merging docs/user/metal/install_ipi.md
Patch failed at 0001 baremetal: make provisioning networks more configurable

In response to this:

/cherry-pick release-4.3

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.

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. platform/baremetal IPI bare metal hosts platform size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

baremetal: Support alternate subnets for provisioning network