Skip to content

metallb: Update operator, CRD, and BGP stack plans #772

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.

Sign up for GitHub

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
Merged
openshift-merge-robot merged 1 commit into from
May 25, 2021
Merged

metallb: Update operator, CRD, and BGP stack plans #772

openshift-merge-robot merged 1 commit into from
May 25, 2021

Conversation

@russellb
Copy link
Contributor

@russellb russellb commented May 5, 2021 •
edited
Loading

This PR includes a few updates on the proposal for how to proceed with
integrating MetalLB with OpenShift

  • Clarify that an OLM based operator, starting with an upstream
    implementation, is the preferred approach for the operator.

  • Add a pointer to our proposal upstream to adopt FRR as an option for
    providing BGP for MetalLB instead of MetalLB's existing custom BGP
    implementation.

@openshift-ci-robot
Copy link

openshift-ci-robot commented May 5, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from russellb after the PR has been reviewed.

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

@russellb russellb mentioned this pull request May 5, 2021
Closed
msherif1234
msherif1234 reviewed May 5, 2021
View reviewed changes
Copy link
Contributor

@msherif1234 msherif1234 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

pls call out all metallb layer 2 conifg limitation like customer will be limited to one addresspool, the only exposed config the pool address rnanage

@squeed
Copy link
Contributor

squeed commented May 10, 2021

This is extremely clever, @russellb. Thumbs up!

@russellb russellb force-pushed the metallb-operator-and-layer2-config branch from e1a7b6b to 56ab820 Compare May 10, 2021 20:17
@russellb
Copy link
Contributor Author

russellb commented May 10, 2021

@squeed @knobunc would the two of you mind being the approvers on this PR?

@russellb russellb mentioned this pull request May 11, 2021
Closed
@russellb russellb force-pushed the metallb-operator-and-layer2-config branch from 56ab820 to 19a0226 Compare May 17, 2021 16:07
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 17, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please ask for approval from knobunc after the PR has been reviewed.

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

squeed
squeed reviewed May 17, 2021
View reviewed changes
@russellb russellb force-pushed the metallb-operator-and-layer2-config branch from 19a0226 to 277bcc3 Compare May 17, 2021 17:47
squeed
squeed reviewed May 17, 2021
View reviewed changes
squeed
squeed reviewed May 17, 2021
View reviewed changes
@squeed
Copy link
Contributor

squeed commented May 17, 2021

This seems reasonable to me. If @sttts (or a delegate) wants to review the API implications, that would be nice. Otherwise I think this is basically mergeable.

@russellb russellb force-pushed the metallb-operator-and-layer2-config branch from 277bcc3 to e8b8aad Compare May 17, 2021 18:35
@russellb
Copy link
Contributor Author

russellb commented May 17, 2021

This seems reasonable to me. If @sttts (or a delegate) wants to review the API implications, that would be nice. Otherwise I think this is basically mergeable.

Summary of the API impact for @sttts or a delegate:

We have an existing AutoAssignCIDRs option in the openshift network config for providing a range of IP addresses where OpenShift will set these addresses in the ExternalIPs field in the spec for Services of type: LoadBalancer. We have a very similar use case here for having OpenShift manage a pool of addresses for the LoadBalancerIP field in the spec for Services of type: LoadBalancer. They would be handled almost identically, except they set the allocated IP address in a different field. There's more details in the PR text.

dcbw
dcbw reviewed May 17, 2021
View reviewed changes
enhancements/network/on-prem-service-load-balancers.md
there is an item to "Front the API servers and other master services with
service load balancers". If this functionality is required at install time,
the details on management of this operator may be revisited.
the details on management of this operator may be revisited, since MetalLB may
Copy link

@dcbw dcbw May 17, 2021

Choose a reason for hiding this comment

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

@russellb FYI @aojea has plans to make this not required, at least for Go-based things, by making client-go able to accept multiple API servers and balance/fail-over when required, instead of funneling into a single point of failure like the current keepalived/haproxy metal model or the slow-as-molasses-to-update cloud LB model, both of which we've had tons of problems with.

Copy link
Contributor Author

@russellb russellb May 17, 2021

Choose a reason for hiding this comment

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

thanks! that's good to know.

I think the impact of that would mean it's less likely that MetalLB would ever be a required component.

dcbw
dcbw reviewed May 17, 2021
View reviewed changes
abhat
abhat reviewed May 18, 2021
View reviewed changes
@russellb russellb force-pushed the metallb-operator-and-layer2-config branch from e8b8aad to a57349b Compare May 18, 2021 13:40
danwinship
danwinship reviewed May 18, 2021
View reviewed changes
@russellb
Copy link
Contributor Author

russellb commented May 21, 2021

Based on feedback from @dcbw and @danwinship in particular, I'm going to revise this to suggest that the new configuration option be done as MetalLB specific in the MetalLB operator instead of as a general OpenShift feature in the network config. We're not convinced enough that it'd ever be useful outside of this context.

@russellb russellb force-pushed the metallb-operator-and-layer2-config branch from a57349b to 9fd5cfb Compare May 21, 2021 18:55
@russellb
Copy link
Contributor Author

russellb commented May 21, 2021

@markdgray @msherif1234 @squeed @danwinship @dcbw This update reflects the change to creating a minimal CRD managed by the MetalLB operator which will cover layer2 mode only at first.

@russellb
metallb: Update operator, CRD, and BGP stack plans
f93c4ef 
This PR includes a few updates on the proposal for how to proceed with
integrating MetalLB with OpenShift

* Clarify that an OLM based operator, starting with an upstream
  implementation, is the preferred approach for the operator.

* Note that we're working on an upstream CRD based configuration
  interface, but describe a backup plan for layer2-only support using
  a minimal CRD in the MetalLB operator.

* Add a pointer to our proposal upstream to adopt FRR as an option for
  providing BGP for MetalLB instead of MetalLB's existing custom BGP
  implementation.
@russellb russellb force-pushed the metallb-operator-and-layer2-config branch from 9fd5cfb to f93c4ef Compare May 21, 2021 19:57
@knobunc
Copy link
Contributor

knobunc commented May 24, 2021

/lgtm

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

squeed commented May 25, 2021

Agreed, /lgtm

@squeed
Copy link
Contributor

squeed commented May 25, 2021

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 25, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: squeed

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

The pull request process is described 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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 25, 2021
@openshift-merge-robot openshift-merge-robot merged commit 2fc2b6e into openshift:master May 25, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

6 more reviewers

@abhat abhat abhat left review comments

@danwinship danwinship danwinship left review comments

@squeed squeed squeed left review comments

@markdgray markdgray markdgray left review comments

@dcbw dcbw dcbw left review comments

@msherif1234 msherif1234 msherif1234 left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@squeed squeed

@knobunc knobunc

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.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@russellb @openshift-ci-robot @squeed @knobunc @abhat @danwinship @markdgray @dcbw @msherif1234 @openshift-merge-robot