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

cli: Rename kpr Protocols status field #14977

Merged
merged 1 commit into from
Mar 10, 2021
Merged

cli: Rename kpr Protocols status field #14977

merged 1 commit into from
Mar 10, 2021

Conversation

brb
Copy link
Member

@brb brb commented Feb 15, 2021

This commit renames the "Protocols" field to Socket LB Protocols" to better reflect the fact that the field indicates which protocols are handled by the host reachable svc (aka socket-lb).

The example output:

    $ cilium status --verbose
    [..]
    KubeProxyReplacement Details:
      Status:              Strict
      Socket LB Protocols: TCP, UDP
      Devices:             wlp3s0 192.168.178.29 (Direct Routing)
      Mode:                SNAT
      Backend Selection:   Random
      Session Affinity:    Enabled
      XDP Acceleration:    Disabled
      Services:
      - ClusterIP:      Enabled
      - NodePort:       Enabled (Range: 30000-32767)
      - LoadBalancer:   Enabled
      - externalIPs:    Enabled
      - HostPort:       Enabled

Also, I'm using socket-lb instead of host-reachable svc, as in v1.10 we are planning to rename the feature (it's used not only for reaching services from a host nents).

@brb brb added kind/feature This introduces new functionality. sig/loadbalancing labels Feb 15, 2021
@brb brb requested a review from a team as a code owner February 15, 2021 07:29
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 15, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Feb 15, 2021
protocols := ""
if hs := sr.KubeProxyReplacement.Features.HostReachableServices; hs.Enabled {
socketLB = "Enabled"
protocols = strings.Join(hs.Protocols, ", ")
}
Copy link
Member

Choose a reason for hiding this comment

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

But if you have kpr disabled, then Protocols: would not be shown as indicator, no?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, but it's not clear without reading the code / docs.

Copy link
Member

Choose a reason for hiding this comment

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

But if status says 'strict', then it's indicator that it has to be enabled. If it says 'disabled', then it has to be disabled as well. So only in case of partial this questions comes up, and there we show the protocols. Maybe this is just a matter of documentation? What does the user make out of 'Socket LB'? If it's just for us debugging isn't the code as-is or sysdump sufficient?

Copy link
Member

Choose a reason for hiding this comment

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

Should we just combine this with the "Protocols:" line with a clearer format like "Socket LB Protocols: "? Personally I was confused whether socket LB was enabled or not while debugging an issue, which is why I filed #14927. It doesn't make sense to me that you'd list a bunch of the feature aspects of the kube-proxy replacement explicitly but then for socket-lb you have to infer whether it's enabled or not based on this other line called "Protocols".

Copy link
Member

Choose a reason for hiding this comment

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

I think the Protocols: TCP, UDP line needs rework perhaps. For example we might still handle TCP, UDP when KPR is disabled and Cilium only xlated ClusterIP. So there should be some sort of indicator what is handled where. E.g. if we have partial and for sock LB we only have TCP xlation enabled, then cilium status --verbose should probably tell which is handled where. Maybe this somehow begs for an output like ...

Protocols:           
- North-South:      TCP, UDP
- East-West:        TCP, UDP
 - Syscall:         TCP
 - NAT:             UDP

... not sure if this can be made nicer / less complicated though.

Copy link
Member Author

Choose a reason for hiding this comment

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

The N-S and E-W terminology is not used in the k8s context, so it might confuse users. For now I'd just do as @joestringer suggested above.

Copy link
Member

Choose a reason for hiding this comment

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

Is that true? Some other components like traefik have been discussing east-west and north-south in k8s context:

https://traefik.io/blog/beyond-kubernetes-bringing-microservices-together-with-service-mesh/

Also, I think that anyone trying to interpret this status section probably needs the kube-proxy replacement docs anyway so we can always write a very short sentence explaining north/south and east/west.

🤷‍♂️ I'm OK with either approach.

Copy link
Member Author

Choose a reason for hiding this comment

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

Is that true?

Yeah, the source of truth doesn't have any single mention of N-S or E-W.

@brb brb added the release-note/misc This PR makes changes that have no direct user impact. label Feb 16, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Feb 16, 2021
@brb
Copy link
Member Author

brb commented Feb 16, 2021

test-me-please

@brb brb changed the title cli: Add socket-lb field cli: Rename kpr Protocols status field Mar 9, 2021
@brb
Copy link
Member Author

brb commented Mar 9, 2021

@joestringer @borkmann I've renamed the field Protocols to Socket LB Protocols. PTAL.

This commit renames the "Protocols" field to Socket LB Protocols" to
better reflect the fact that the field indicates which protocols are
handled by the host reachable svc (aka socket-lb).

The example output:

    $ cilium status --verbose
    [..]
    KubeProxyReplacement Details:
      Status:              Strict
      Socket LB Protocols: TCP, UDP
      Devices:             wlp3s0 192.168.178.29 (Direct Routing)
      Mode:                SNAT
      Backend Selection:   Random
      Session Affinity:    Enabled
      XDP Acceleration:    Disabled
      Services:
      - ClusterIP:      Enabled
      - NodePort:       Enabled (Range: 30000-32767)
      - LoadBalancer:   Enabled
      - externalIPs:    Enabled
      - HostPort:       Enabled

Also, I'm using socket-lb instead of host-reachable svc, as in v1.10
we are planning to rename the feature (it's used not only for reaching
services from a host nents).

Reported-by: Joe Stringer <joe@cilium.io>
Signed-off-by: Martynas Pumputis <m@lambda.lt>
@brb
Copy link
Member Author

brb commented Mar 10, 2021

test-me-please

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 10, 2021
@kkourt kkourt merged commit 991fd55 into master Mar 10, 2021
1.10.0 automation moved this from In progress to Done Mar 10, 2021
@kkourt kkourt deleted the pr/brb/kpr-cli-status branch March 10, 2021 08:46
@BurlyLuo
Copy link

KubeProxyReplacement Details:
Status: Strict
Socket LB: Enabled
Socket LB Protocols: TCP, UDP

root@bpf1:/home/cilium# cat /tmp/cilium/config-map/bpf-lb-sock
falseroot@bpf1:/home/cilium#

The above two feature is the same of different?

@BurlyLuo
Copy link

image

@brb
Copy link
Member Author

brb commented Nov 23, 2022

The above two feature is the same of different?

The same.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/feature This introduces new functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

7 participants