-
Notifications
You must be signed in to change notification settings - Fork 387
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
Add document for Service of type LoadBalancer #3322
Conversation
Codecov Report
@@ Coverage Diff @@
## main #3322 +/- ##
===========================================
- Coverage 60.95% 41.83% -19.12%
===========================================
Files 266 201 -65
Lines 26508 24142 -2366
===========================================
- Hits 16159 10101 -6058
- Misses 8597 12991 +4394
+ Partials 1752 1050 -702
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for working on this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should add a link to this document to https://github.com/antrea-io/antrea/blob/main/docs/feature-gates.md#serviceexternalip, like we did for other features.
2693a7f
to
d3f2990
Compare
0e51287
to
a238e46
Compare
Thanks for the review. I have addressed most of the comments. Please take another look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM overall
Node network. The simplest way to achieve this is to reserve a range of IPs | ||
from the Node network subnet, and define Service ExternalIPPools with the | ||
reserved IPs, when the Nodes are connected to a layer 2 subnet. Or, another | ||
possible way might be to manually configure Node network routing (e.g. by |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
might be => is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I feel that way is not very convenient, even theoretically work. So, "might be" should be good.
0c0e740
to
aa6fbb4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a couple nits, LGTM
docs/service-loadbalancer.md
Outdated
configures the cloud load balancers for the Services. However, the load | ||
balancer support is not available on all platforms, or in some cases, it is | ||
complex or has extra cost to deploy external load balancers. This document | ||
describes two options of supporting Services of type LoadBalancer with Antrea, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/of supporting/for supporting
docs/service-loadbalancer.md
Outdated
#### Create an ExternalIPPool custom resource | ||
|
||
Service external IPs are allocated from an ExternalIPPool, which defines a pool | ||
of external IPs and the set of nodes to which the external IPs can be assigned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
s/nodes/Nodes for consistency
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok
docs/service-loadbalancer.md
Outdated
For Antrea to manage the externalIP for a Service of type LoadBalancer, the | ||
Service should be created with the `service.antrea.io/external-ip-pool` | ||
annotation. For example: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does it work if the Service is created first without the annotation and then updated to include the correct annotation?
if yes, maybe rephrase to: ..., the Service should be annotated with service.antrea.io/external-ip-pool
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated.
docs/service-loadbalancer.md
Outdated
|
||
The BGP mode of MetalLB requires more configuration parameters to establish BGP | ||
peering to the router. The example below configures MetalLB using AS number | ||
64500 to peer router 10.0.0.1 with AS number 64501: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should it be "to peer with router"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how about "connected to peer router"?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe: "connect to peer router"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. thanks.
aa6fbb4
to
a45860d
Compare
a45860d
to
8f91781
Compare
docs/service-loadbalancer.md
Outdated
- [Create a Service of type LoadBalancer](#create-a-service-of-type-loadbalancer) | ||
- [Validate Service external IP](#validate-service-external-ip) | ||
- [Limitations](#limitations) | ||
- [Using MetalLB with Antrea](#using-metalb-with-antrea) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missed a 'l' in "metallb" in "using-metalb-with-antrea"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
added. thanks!
8f91781
to
75f1fa1
Compare
75f1fa1
to
4e0e6c4
Compare
Signed-off-by: Tianyi Huang <hty690@126.com>
4e0e6c4
to
070fcef
Compare
/skip-all |
Signed-off-by: Tianyi Huang <hty690@126.com>
Signed-off-by: Tianyi Huang hty690@126.com
Closes #2020