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

lbmap: Correct issue that port info display error #13244

Merged
merged 2 commits into from
Oct 19, 2020

Conversation

Jianlin-lv
Copy link
Contributor

The port info is stored in the bpf map in network byte order;

bpftool map dump pinned /sys/fs/bpf/tc/globals/cilium_lb4_backends
key: 02 00 value: 0a 00 00 55 00 35 00 00
key: 01 00 value: 0a a9 d0 d6 19 2b 00 00
key: 04 00 value: 0a 00 00 64 00 35 00 00
key: 08 00 value: 0a 00 01 db 00 50 00 00
key: 03 00 value: 0a 00 00 55 23 c1 00 00
key: 05 00 value: 0a 00 00 64 23 c1 00 00
key: 07 00 value: 0a 00 01 e6 00 50 00 00
key: 06 00 value: 0a 00 00 59 00 50 00 00

cilium map get cilium_lb4_backends
Key Value State Error
6 ANY://10.0.0.89:20480 sync
7 ANY://10.0.1.230:20480 sync
8 ANY://10.0.1.219:20480 sync

When displaying the given lbmap content, the port needs to be
converted to host byte order.

cilium map get cilium_lb4_backends
Key Value State Error
8 ANY://10.0.0.64:80 sync
6 ANY://10.0.1.158:80 sync
7 ANY://10.0.1.66:80 sync

Signed-off-by: Jianlin Lv Jianlin.Lv@arm.com

Fixes: #13243

@Jianlin-lv Jianlin-lv requested a review from a team September 22, 2020 09:02
@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 Sep 22, 2020
@qmonnet
Copy link
Member

qmonnet commented Sep 22, 2020

test-me-please

@pchaigno pchaigno added the release-note/misc This PR makes changes that have no direct user impact. label Sep 22, 2020
@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 Sep 22, 2020
@qmonnet qmonnet added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 22, 2020
@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 Sep 22, 2020
@qmonnet qmonnet added area/cli Impacts the command line interface of any command in the repository. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. labels Sep 22, 2020
Copy link
Member

@pchaigno pchaigno left a comment

Choose a reason for hiding this comment

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

Isn't the same issue present for the three IPv6 maps?

@Jianlin-lv
Copy link
Contributor Author

Isn't the same issue present for the three IPv6 maps?

Thanks for reminding, I will check it.

@pchaigno
Copy link
Member

test-me-please

@Jianlin-lv
Copy link
Contributor Author

Isn't the same issue present for the three IPv6 maps?

@pchaigno I want to honestly let you know that I have not verified this patch in my test environment;
Because I don’t have an ipv6 experimental environment, today I spent a long time building k8s cluster that supports IPv4/IPv6 dual-stack networking, but the cilium agent only received ipv4 service information; I don’t know what is wrong, nor how to set up a pure ipv6 network in the cluster.
In short, I did not successfully verify the patch.

@pchaigno
Copy link
Member

No problem, I was expecting this; IPv6-only clusters are a bit painful to setup. I think that's fine since we both tested on IPv4 and the change is fairly trivial. I'll ask on Slack how folks usually setup their IPv6-only clusters, but I don't think we need to block on that.

@qmonnet
Copy link
Member

qmonnet commented Sep 23, 2020

test-me-please

@Jianlin-lv
Copy link
Contributor Author

Please note, I will append a new patch to this RP to fix a newly discovered issue tomorrow.

@borkmann
Copy link
Member

Please also add some CI tests, verifying that the output of cilium service list, cilium bpf lb list and cilium map get cilium_lb*_backends works as expected.

I just looked at one of the sysdumps from the CI and see the following:

The cilium service list looks okay:

ID   Frontend             Service Type   Backend                   
1    10.106.162.7:3000    ClusterIP                                
2    10.111.80.118:9090   ClusterIP      1 => 10.0.0.83:9090       
3    10.96.0.1:443        ClusterIP      1 => 192.168.36.11:6443   
4    10.96.0.10:53        ClusterIP      1 => 10.0.1.62:53         

But the cilium bpf lb list doesn't (though it was correct before the PR):

SERVICE ADDRESS       BACKEND ADDRESS
10.106.162.7:47115    0.0.0.0:0 (1) [ClusterIP]   
10.96.0.1:47873       192.168.36.11:6443 (3)      
                      0.0.0.0:0 (3) [ClusterIP]   
10.96.0.10:13568      0.0.0.0:0 (4) [ClusterIP]   
                      10.0.1.62:53 (4)            
10.111.80.118:33315   10.0.0.83:9090 (2)          
                      0.0.0.0:0 (2) [ClusterIP]   

@Jianlin-lv
Copy link
Contributor Author

Jianlin-lv commented Sep 24, 2020

Please also add some CI tests, verifying that the output of cilium service list, cilium bpf lb list and cilium map get cilium_lb*_backends works as expected.

I just looked at one of the sysdumps from the CI and see the following:

The cilium service list looks okay:

ID   Frontend             Service Type   Backend                   
1    10.106.162.7:3000    ClusterIP                                
2    10.111.80.118:9090   ClusterIP      1 => 10.0.0.83:9090       
3    10.96.0.1:443        ClusterIP      1 => 192.168.36.11:6443   
4    10.96.0.10:53        ClusterIP      1 => 10.0.1.62:53         

But the cilium bpf lb list doesn't (though it was correct before the PR):

SERVICE ADDRESS       BACKEND ADDRESS
10.106.162.7:47115    0.0.0.0:0 (1) [ClusterIP]   
10.96.0.1:47873       192.168.36.11:6443 (3)      
                      0.0.0.0:0 (3) [ClusterIP]   
10.96.0.10:13568      0.0.0.0:0 (4) [ClusterIP]   
                      10.0.1.62:53 (4)            
10.111.80.118:33315   10.0.0.83:9090 (2)          
                      0.0.0.0:0 (2) [ClusterIP]   

What you found is accurate, I have fixed this issue today and resolved the RevNatKey display issue.
After applying this patch, the execution result of lb cmd as follows:

root@jianlin:~# kubectl exec -it -n kube-system cilium-mzrbn -- cilium service list
ID   Frontend               Service Type   Backend
1    172.16.1.10:53         ClusterIP      1 => 10.0.0.199:53
                                           2 => 10.0.0.111:53
2    172.16.1.10:9153       ClusterIP      1 => 10.0.0.199:9153
                                           2 => 10.0.0.111:9153
3    172.16.1.1:443         ClusterIP      1 => 10.169.208.214:6443
4    172.16.1.13:80         ClusterIP      1 => 10.0.1.144:80
                                           2 => 10.0.0.236:80
                                           3 => 10.0.1.78:80
5    10.169.208.214:31427   NodePort       1 => 10.0.1.144:80
                                           2 => 10.0.0.236:80
                                           3 => 10.0.1.78:80
6    0.0.0.0:31427          NodePort       1 => 10.0.1.144:80
                                           2 => 10.0.0.236:80
                                           3 => 10.0.1.78:80
root@jianlin:~# kubectl exec -it -n kube-system cilium-mzrbn --  cilium bpf lb list
SERVICE ADDRESS        BACKEND ADDRESS
172.16.1.10:9153       10.0.0.111:9153 (2)
                       10.0.0.199:9153 (2)
                       0.0.0.0:0 (2) [ClusterIP]
172.16.1.10:53         10.0.0.111:53 (1)
                       0.0.0.0:0 (1) [ClusterIP]
                       10.0.0.199:53 (1)
0.0.0.0:31427          10.0.1.144:80 (6)
                       10.0.0.236:80 (6)
                       0.0.0.0:0 (6) [NodePort, non-routable]
                       10.0.1.78:80 (6)
172.16.1.1:443         10.169.208.214:6443 (3)
                       0.0.0.0:0 (3) [ClusterIP]
172.16.1.13:80         10.0.1.78:80 (4)
                       10.0.0.236:80 (4)
                       0.0.0.0:0 (4) [ClusterIP]
                       10.0.1.144:80 (4)
10.169.208.214:31427   0.0.0.0:0 (5) [NodePort]
                       10.0.1.144:80 (5)
                       10.0.1.78:80 (5)
                       10.0.0.236:80 (5)
root@jianlin:~# kubectl exec -it -n kube-system cilium-mzrbn -- cilium map get cilium_lb4_services_v2
Key                    Value                 State   Error
172.16.1.10:53         0 (1) [FLAGS: 0x40]   sync
172.16.1.10:9153       0 (2) [FLAGS: 0x40]   sync
172.16.1.1:443         0 (3) [FLAGS: 0x40]   sync
172.16.1.13:80         0 (4) [FLAGS: 0x40]   sync
10.169.208.214:31427   0 (5) [FLAGS: 0x42]   sync
0.0.0.0:31427          0 (6) [FLAGS: 0x2]    sync
root@jianlin:~# kubectl exec -it -n kube-system cilium-mzrbn -- cilium map get cilium_lb4_backends
Key   Value                       State   Error
5     ANY://10.0.0.236:80         sync
6     ANY://10.0.0.111:9153       sync
7     ANY://10.0.0.111:53         sync
8     ANY://10.0.1.78:80          sync
1     ANY://10.169.208.214:6443   sync
2     ANY://10.0.0.199:9153       sync
3     ANY://10.0.0.199:53         sync
4     ANY://10.0.1.144:80         sync
root@jianlin:~# kubectl exec -it -n kube-system cilium-mzrbn -- cilium map get cilium_lb4_reverse_nat
Key   Value                  State   Error
4     172.16.1.13:80         sync
5     10.169.208.214:31427   sync
6     0.0.0.0:31427          sync
1     172.16.1.10:53         sync
2     172.16.1.10:9153       sync
3     172.16.1.1:443         sync

@aanm aanm requested a review from borkmann September 25, 2020 11:36
pkg/maps/lbmap/affinity.go Show resolved Hide resolved
pkg/maps/lbmap/ipv4.go Outdated Show resolved Hide resolved
@Jianlin-lv
Copy link
Contributor Author

Fixed an issue that is when restarting cilium, Service`s port does not match;
Report by: https://jenkins.cilium.io/job/Cilium-PR-Runtime-4.9/1939/execution/node/100/log/

@Jianlin-lv
Copy link
Contributor Author

test-me-please

@Jianlin-lv
Copy link
Contributor Author

Squash commits into one.

@Jianlin-lv Jianlin-lv force-pushed the pr-fix-lbmap-endian branch 2 times, most recently from 2525d93 to b0ee847 Compare October 12, 2020 02:54
@pchaigno pchaigno requested a review from brb October 12, 2020 06:40
@tklauser tklauser merged commit 682f682 into cilium:master Oct 19, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.8 in 1.8.5 Oct 22, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.7 in 1.7.11 Oct 23, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.7 to Backport done to v1.7 in 1.7.11 Oct 23, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.8 to Backport done to v1.8 in 1.8.5 Oct 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/cli Impacts the command line interface of any command in the repository. kind/bug This is a bug in the Cilium logic. release-note/bug This PR fixes an issue in a previous release of Cilium. release-priority/best-effort The project for target version is not a hard requirement. sig/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages.
Projects
No open projects
1.7.11
Backport done to v1.7
1.8.5
Backport done to v1.8
Development

Successfully merging this pull request may close these issues.

lbmap:Port information display error