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

Promote ServiceExternalIP to beta #6903

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

xliuxu
Copy link
Contributor

@xliuxu xliuxu commented Jan 8, 2025

Fixes: #6743

@xliuxu xliuxu requested a review from luolanzone January 8, 2025 03:07
@@ -209,7 +210,7 @@ var (
Multicast: {Default: true, PreRelease: featuregate.Beta},
Multicluster: {Default: false, PreRelease: featuregate.Alpha},
SecondaryNetwork: {Default: false, PreRelease: featuregate.Alpha},
ServiceExternalIP: {Default: false, PreRelease: featuregate.Alpha},
ServiceExternalIP: {Default: true, PreRelease: featuregate.Beta},
Copy link
Contributor

Choose a reason for hiding this comment

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

Need to update the value of ServiceExternalIP feature in the antrea-agent.conf and antrea-controller.conf, and regenerate manitests

Copy link
Contributor

Choose a reason for hiding this comment

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

Unit test is failed.

docs/service-loadbalancer.md Outdated Show resolved Hide resolved
@luolanzone luolanzone requested a review from tnqn January 8, 2025 04:01
@xliuxu xliuxu force-pushed the xliuxu/promote-service-external-ip branch 2 times, most recently from 0e44ad4 to ba3daf2 Compare January 10, 2025 08:41
@xliuxu xliuxu force-pushed the xliuxu/promote-service-external-ip branch from ba3daf2 to 0043100 Compare January 10, 2025 10:04
Comment on lines 311 to +314
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
Copy link
Contributor

Choose a reason for hiding this comment

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

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:

if currentIPPool == "" {
klog.V(2).InfoS("Ignored Service as required annotation is not found", "service", key)

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Please promote ServiceExternalIP to enable by default
3 participants