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

[v1.8] bpf: unconditionally enable tail calls in bpf_lxc #16965

Merged
merged 1 commit into from
Jul 22, 2021

Conversation

kkourt
Copy link
Contributor

@kkourt kkourt commented Jul 21, 2021

The cilium agent configuration below leads to the health endpoint
lxc program to fail to load on 5.4 (tested using the dev VMs). The
configuration does not enable IPv6, which means that the tailcalls on
bpf_lxc are not enabled.

This patch fixes this issue by unconditionally enabling tailcalls. The
patch keeps the compile-time checks in case we want to modify this
behaviour at a later time.

Configuration:

 --enable-hubble
 --hubble-listen-address :4244
 --enable-k8s-event-handover
 --k8s-require-ipv4-pod-cidr
 --kube-proxy-replacement=partial
 --enable-remote-node-identity=false
 --enable-ipv6=false
 -t vxlan
 --k8s-kubeconfig-path/var/lib/cilium/cilium.kubeconfig
 --identity-allocation-mode=crd
 --enable-k8s-event-handover=false
 --enable-session-affinity
 --enable-node-port=false
 --enable-bpf-clock-probe=true
 --enable-bpf-masquerade=true
 --bpf-map-dynamic-size-ratio='0.0'
 --bpf-policy-map-max='65536'
 --disable-cnp-status-updates='true'
 --disable-endpoint-crd='true'
 --enable-api-rate-limit='true'
 --enable-external-ips='false'
 --enable-host-port='false'
 --enable-k8s-event-handover='true' --identity-allocation-mode=crd
 --enable-remote-node-identity='false'
 --enable-well-known-identities='false'
 --mtu=1500
 --preallocate-bpf-maps='false'
 --monitor-aggregation='medium'
 --monitor-aggregation-flags=all"

Signed-off-by: Kornilios Kourtis kornilios@isovalent.com

The following cilium agent configuration [1] leads to the health endpoint
lxc program to fail to load on 5.4 (tested using the dev VMs). The
configuration does not enable IPv6, which means that the tailcalls on
bpf_lxc are not enabled.

This patch fixes this issue by unconditionally enabling tailcalls.  The
patch keeps the compile-time checks in case we want to modify this
behaviour at a later time.

[1]:
 --enable-hubble
 --hubble-listen-address :4244
 --enable-k8s-event-handover
 --k8s-require-ipv4-pod-cidr
 --kube-proxy-replacement=partial
 --enable-remote-node-identity=false
 --enable-ipv6=false
 -t vxlan
 --k8s-kubeconfig-path/var/lib/cilium/cilium.kubeconfig
 --identity-allocation-mode=crd
 --enable-k8s-event-handover=false
 --enable-session-affinity
 --enable-node-port=false
 --enable-bpf-clock-probe=true
 --enable-bpf-masquerade=true
 --bpf-map-dynamic-size-ratio='0.0'
 --bpf-policy-map-max='65536'
 --disable-cnp-status-updates='true'
 --disable-endpoint-crd='true'
 --enable-api-rate-limit='true'
 --enable-external-ips='false'
 --enable-host-port='false'
 --enable-k8s-event-handover='true' --identity-allocation-mode=crd
 --enable-remote-node-identity='false'
 --enable-well-known-identities='false'
 --mtu=1500
 --preallocate-bpf-maps='false'
 --monitor-aggregation='medium'
 --monitor-aggregation-flags=all"

Signed-off-by: Kornilios Kourtis <kornilios@isovalent.com>
@maintainer-s-little-helper maintainer-s-little-helper bot added backport/1.8 kind/backports This PR provides functionality previously merged into master. labels Jul 21, 2021
@kkourt kkourt added release-note/bug This PR fixes an issue in a previous release of Cilium. and removed backport/1.8 kind/backports This PR provides functionality previously merged into master. labels Jul 21, 2021
@kkourt
Copy link
Contributor Author

kkourt commented Jul 21, 2021

test-backport-1.8

@christarazi christarazi changed the title bpf: unconditionally enable tail calls in bpf_lxc [v1.8] bpf: unconditionally enable tail calls in bpf_lxc Jul 21, 2021
@kkourt
Copy link
Contributor Author

kkourt commented Jul 22, 2021

Cilium-PR-K8s-Upstream seems to be hitting a vagrant ssh issue:

20:30:49 + cd /home/jenkins/workspace/Cilium-PR-K8s-Upstream/src/github.com/cilium/cilium/test
20:30:49 + vagrant ssh k8s1-1.18 -c cd /home/vagrant/go/src/github.com/cilium/cilium; ./test/kubernetes-test.sh
20:32:14 ssh_exchange_identification: read: Connection reset by peer

image

Will restart it.

@kkourt
Copy link
Contributor Author

kkourt commented Jul 22, 2021

test-upstream-k8s

@kkourt
Copy link
Contributor Author

kkourt commented Jul 22, 2021

For the k8s-1.18-kernel-4.9 (test-1.18-4.9) failure,

Failure log reports:

21:51:20  Summarizing 1 Failure:
21:51:20  
21:51:20  [Fail] K8sVerifier [It] Runs the kernel verifier against Cilium's BPF datapath 
21:51:20  /home/jenkins/workspace/Cilium-PR-K8s-1.18-kernel-4.9/src/github.com/cilium/cilium/test/k8sT/Verifier.go:88

From the corresponding build artifact:

err:
exit status 2
stderr:
go: RLock /cilium/go.mod: no locks available
make: *** [Makefile:14: maptool] Error 1
command terminated with exit code 2

FAIL: Expected compilation of maptool to succeed
Expected command: kubectl exec -n default test-verifier -- make -C /cilium/tools/maptool/ 
To succeed, but it failed:
Exitcode: 2 
Err: exit status 2
Stdout:

For which @nbusseneau pointed out that:

This is the NFS lock issue for which Joe was trying to backport the fix in 1.8 here, but ultimately was skipped because of other issues: https://cilium.slack.com/archives/CDD1S4BHC/p1626909362173900?thread_ts=1626905560.172600&cid=CDD1S4BHC, #16912

@kkourt kkourt marked this pull request as ready for review July 22, 2021 12:13
@kkourt kkourt requested a review from a team as a code owner July 22, 2021 12:13
@kkourt
Copy link
Contributor Author

kkourt commented Jul 22, 2021

After retry, test-upstream-k8s is now green.

The only remaining failure k8s-1.18-kernel-4.9 (test-1.18-4.9) which, as discussed above, is expected to be addressed once #16554 is backported to 1.8.

I think there is an argument to be made for merging this PR as is, since it addresses an existing user issue. @cilium/cilium-maintainers thoughts?

Copy link
Member

@brb brb left a comment

Choose a reason for hiding this comment

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

Minor non-blocking comment.

@@ -433,8 +435,7 @@ static __always_inline int handle_ipv6(struct __ctx_buff *ctx, __u32 *dstID)
return ipv6_l3_from_lxc(ctx, &tuple, ETH_HLEN, ip6, dstID);
}

declare_tailcall_if(__or(__and(is_defined(ENABLE_IPV4), is_defined(ENABLE_IPV6)),
is_defined(DEBUG)), CILIUM_CALL_IPV6_FROM_LXC)
declare_tailcall_if(ENABLE_LXC_TAILCALLS, CILIUM_CALL_IPV6_FROM_LXC)
Copy link
Member

Choose a reason for hiding this comment

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

I would probably have kept original conditions and just change to __or(ENABLE_LXC_TAILCALLS, $PREV_COND)).

Copy link
Member

Choose a reason for hiding this comment

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

I agree this would be nice documentation, but if the condition is going to be hardcoded to 1 then it doesn't make such a big difference. We can always look back into git or do a revert if we want to switch back to the old behaviour. This is also limited to v1.8 branch so the latest will continue to document the real intent here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants