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

Fix hint annotation implementation in AntreaProxy #6607

Merged
merged 1 commit into from
Aug 14, 2024

Conversation

hongliangl
Copy link
Contributor

@hongliangl hongliangl commented Aug 13, 2024

This commit is to be consistent with kube-proxy's behavior for Topology
Aware Routing feature.

For service.kubernetes.io/topology-mode hint annotation, any non-empty
and non-disabled values for the annotation are acceptable to enable
Topology Aware Routing for a Service.

@hongliangl hongliangl added the action/backport Indicates a PR that requires backports. label Aug 13, 2024
@tnqn tnqn added the action/release-note Indicates a PR that should be included in release notes. label Aug 13, 2024
Copy link
Member

@tnqn tnqn left a comment

Choose a reason for hiding this comment

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

LGTM

Please add description that this is to be consistent with kube-proxy's behavior.

This commit is to be consistent with kube-proxy's behavior for Topology
Aware Routing feature.

For `service.kubernetes.io/topology-mode` hint annotation, any non-empty
and non-disabled values for the annotation are acceptable to enable
Topology Aware Routing for a Service.

Signed-off-by: Hongliang Liu <[email protected]>
@hongliangl
Copy link
Contributor Author

LGTM

Please add description that this is to be consistent with kube-proxy's behavior.

Done

@tnqn
Copy link
Member

tnqn commented Aug 14, 2024

I think setting the annotation value to other values wouldn't work because endpointslice controller only adds hints when the value is Auto or auto, so the PR is more to make code consistent with kube-proxy only. We don't need to backport and mention it in release note.

@tnqn tnqn removed action/backport Indicates a PR that requires backports. action/release-note Indicates a PR that should be included in release notes. labels Aug 14, 2024
@tnqn
Copy link
Member

tnqn commented Aug 14, 2024

I think setting the annotation value to other values wouldn't work because endpointslice controller only adds hints when the value is Auto or auto, so the PR is more to make code consistent with kube-proxy only. We don't need to backport and mention it in release note.

@hongliangl let me know if my understanding is correct.

@hongliangl
Copy link
Contributor Author

hongliangl commented Aug 14, 2024

I think setting the annotation value to other values wouldn't work because endpointslice controller only adds hints when the value is Auto or auto, so the PR is more to make code consistent with kube-proxy only. We don't need to backport and mention it in release note.

@tnqn You are right. Only Auto and auto are valid (https://github.com/kubernetes/kubernetes/blob/69dbf2eee96f1c95c097370ddcb1d5c30f86bec8/staging/src/k8s.io/endpointslice/utils.go#L360). This PR doesn't change the behavior.

@tnqn
Copy link
Member

tnqn commented Aug 14, 2024

/skip-all

1 similar comment
@tnqn
Copy link
Member

tnqn commented Aug 14, 2024

/skip-all

@tnqn tnqn merged commit 7d8d4b1 into antrea-io:main Aug 14, 2024
55 of 58 checks passed
@hongliangl hongliangl deleted the 20240813-topo-mod-annotation branch August 14, 2024 07:20
@luolanzone
Copy link
Contributor

@hongliangl are you going to back-port this to Antrea v2.0 and v2.1?

@hongliangl
Copy link
Contributor Author

@hongliangl are you going to back-port this to Antrea v2.0 and v2.1?

No. See #6607 (comment)

@luolanzone
Copy link
Contributor

@hongliangl are you going to back-port this to Antrea v2.0 and v2.1?

No. See #6607 (comment)

Ah, my bad, I didn't notice the label was already removed. Ignore me.

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.

3 participants