Promote ServiceExternalIP to beta #6903
Conversation
xliuxu
force-pushed
the
xliuxu/promote-service-external-ip
branch
3 times, most recently
from
ba3daf2 to
0043100
Compare
docs/service-loadbalancer.md
Outdated
| As MetalLB will allocate external IPs for all Services of type LoadBalancer, | ||
| once it is running, the Service external IP management feature of Antrea should | ||
| not be enabled to avoid conflicts with MetalLB. You can deploy Antrea with the | ||
| default configuration (in which the `ServiceExternalIP` feature gate of | ||
| ServiceExternalIP feature disabled (in which the `ServiceExternalIP` feature gate of |
There was a problem hiding this comment.
The phrasing is a bit strange. I would expect users to be able to keep using MetalLB with Antrea without having to make any specific configuration change, even after we graduate the feature to Beta.
My understanding is that despite the above warning about "conflicts with MetalLB", as long as the Services are not annotated with service.antrea.io/external-ip-pool, Antrea will not try to allocate LB IPs for the Services and there will be no conflict. However, the "check" kind of happens late IMO, which means we are doing work for nothing and some logs may create confusion for users:
antrea/pkg/controller/serviceexternalip/controller.go
Lines 501 to 502 in afb9dfc
I'd like to hear @tnqn's opinion on this. Do we feel like the annotation is enough to "gate" that feature once the FeatureGate itself is promoted to Beta (keep in mind that the long term goal after promoting a feature to Beta should to go to GA, at which point the FeatureGate stops being available altogether in theory). Or do we need a boolean configuration parameter, as a way to explicitly enable / disable the feature (as we have done in other places). Typically we do not need the boolean when the feature is API-driven, but this case is slightly different.
There was a problem hiding this comment.
Adding a boolean may be a bit surprising to users who asked to promote this feature and have it run by default. Maybe we could improve the event handlers enqueueService and drop unrelated events earlier to avoid some unnecessary handling and confusing logs? Ideally the queue should always empty if user never annotate any Service. It may still cost some cycles but that's the case of many event handlers too.
There was a problem hiding this comment.
That works for me if there is a good way to do that. We can improve the controller implementation as you suggested (as a separate PR, that we can merge prior to this one), and improve the documentation as part of this PR as suggested above.
There was a problem hiding this comment.
I checked for this part and found that enqueue services without ippool annotations are necessary for the unassigned case. I think it could be error-prone to add the filter to the enqueueService as it could be simpler to keep all reconciliation logic in a single syncService function.
How about just removing the log klog.V(2).InfoS("Ignored Service as required annotation is not found", "service", key) or changing it to klog.V(2).InfoS("Ignored Service as required annotation no longer exists", "service", key) and printing it only for IP release cases?
There was a problem hiding this comment.
@xliuxu why do we need to enqueue services without ippool annotation? My understanding is:
For Add, if the Service has no required annotation, we ignore it.
For Update, if neither the old Service nor the new Service has the required annotation, we ignore it.
For Delete, if the Service has no required annotation, we ignore it.
When user never annotates any Service, nothing will be enqueued.
When user annotates a Service, it will be enqueued.
When user unannotates a Service, it will be enqueued.
There was a problem hiding this comment.
thought over, the above approach cannot get rid of the log Finished syncing Service for %s. Created PR #6903 per Quan's suggestion.
xliuxu
force-pushed
the
xliuxu/promote-service-external-ip
branch
from
0043100 to
5ef24a4
Compare
|
@antoninbas @tnqn I have merged PR #6950 since all tests are passed. Could you take another look at this PR? Thanks. @xliuxu can you do a rebase to the latest main? then we can trigger all tests. |
xliuxu
force-pushed
the
xliuxu/promote-service-external-ip
branch
from
5ef24a4 to
ff05d42
Compare
|
/test-all |
docs/service-loadbalancer.md
Outdated
| Antrea. The `ServiceExternalIP` feature gate of `antrea-agent` and | ||
| `antrea-controller` must be enabled for the feature to work. You can enable | ||
| The `ServiceExternalIP` feature is enabled by default since Antrea 2.3. If you are | ||
| using previous versions, the `ServiceExternalIP` feature gate of `antrea-agent` |
There was a problem hiding this comment.
If you are using an earlier version
| @@ -306,14 +306,8 @@ kubectl apply -f https://raw.githubusercontent.com/metallb/metallb/v0.13.11/conf | |||
| The commands will deploy MetalLB version 0.13.11 into Namespace | |||
| `metallb-system`. You can also refer to this [MetalLB installation | |||
| guide](https://metallb.universe.tf/installation) for other ways of installing | |||
| MetalLB. | |||
|
|
|||
| As MetalLB will allocate external IPs for all Services of type LoadBalancer, | |||
There was a problem hiding this comment.
Even though it is pretty obvious, I would still keep a sentence like this one here:
To avoid conflicts, make sure not to use the
service.antrea.io/external-ip-poolannotation (AntreaServiceExternalIPfeature) when using MetallLB to allocate LoadBalancer IPs.
Fixes: antrea-io#6743 Signed-off-by: Xu Liu <[email protected]>
xliuxu
force-pushed
the
xliuxu/promote-service-external-ip
branch
from
ff05d42 to
5908eb2
Compare
antoninbas
added
the
action/release-note
|
/test-all |
Fixes: #6743