daemon: Create all global maps in cilium-agent #10626

pchaigno · 2020-03-18T21:33:08Z

The first 6 commits are preparatory work for the last 9 commits, which creates all global maps in the cilium-agent. The last 9 commits can be easily squashed if preferred as they're fairly similar (I initially broke them down to bisect a bug).

Fixes: #9276

/cc @tklauser to check I didn't revert any memory optimization effort :-)

jrajahalme · 2020-03-18T22:08:01Z

@pchaigno Do we still have bpf code that could potentially create maps? If so maybe remove so that we can be sure we are not depending on that code any more?

coveralls · 2020-03-18T22:12:11Z

Coverage decreased (-0.2%) to 45.393% when pulling f575116 on pr/pchaigno/open-maps-in-agent into 65f2fe2 on master.

pkg/maps/nat/nat.go

pkg/bpf/map_linux.go

pkg/maps/ctmap/ctmap.go

pchaigno · 2020-03-18T22:26:36Z

@pchaigno Do we still have bpf code that could potentially create maps? If so maybe remove so that we can be sure we are not depending on that code any more?

I think the existing code to create BPF maps is actually in tc (cf. iproute2/lib/bpf.c#L1719-L1734). We still rely on that code to recreate maps during the datapath regeneration if their attributes changed (cf. bpf/cilium-map-migrate.c).

tklauser

One small remark, didn't review thoroughly yet. Will also check regarding memory consumption tomorrow.

daemon/cmd/datapath.go

pkg/maps/ctmap/ctmap.go

pkg/maps/nat/nat.go

aanm · 2020-03-20T12:26:24Z

pkg/maps/policymap/policymap.go

-	CallMapName = "cilium_policy"
+	// PolicyCallMapName is the name of the map to do tail calls into policy
+	// enforcement programs.
+	PolicyCallMapName = "cilium_call_policy"


I assume this won't brake up and downgrade because we always create the map when we initialized the map?

My understanding is that we (tc) repopulate the map when we reload the datapath, so this won't be an issue for up/downgrades. I expect the old map to remain behind though; I don't know if we usually clean this sort of stale BPF objects (?)

My inclination is to agree with @pchaigno here, but I think it'd be worthwhile to run some tests on this. For instance set up a couple of chatty peers, perform upgrade, and check that they continue chatting. We could probably use one of the chaos monkeys for this, eg:
https://github.com/cilium/chaos-monkeys/tree/master/monkeys/clique

(Worth double-checking the behaviour by deploying the monkey, restarting an existing version of Cilium first to understand how it performs, then try upgrade and observe)

I don't know if we usually clean this sort of stale BPF objects (?)

Yes, we clean the old objects. I think there's proxymap references still in the code to help clean up from upgrade from older versions, maybe v1.5 or so. Of course, if we downgrade again then we'll leave the map with the new name on the filesystem... though I'm less concerned about that since we assume users will eventually get onto a stable version of the newer release.

pkg/bpf/map_linux.go

pkg/maps/lbmap/ipv6.go

pkg/maps/policymap/policymap.go

tklauser · 2020-03-23T15:57:44Z

When starting cilium-agent built from this PR (vs. the one built from current master) I get the following warnings which seem relevant:

level=warning msg="Flags mismatch for BPF map" file-path=/sys/fs/bpf/tc/globals/cilium_lxc new=0 old=1 subsys=bpf
level=warning msg="Removing map to allow for property upgrade (expect map data loss)" file-path=/sys/fs/bpf/tc/globals/cilium_lxc subsys=bpf
level=warning msg="Flags mismatch for BPF map" file-path=/sys/fs/bpf/tc/globals/cilium_metrics new=0 old=1 subsys=bpf
level=warning msg="Removing map to allow for property upgrade (expect map data loss)" file-path=/sys/fs/bpf/tc/globals/cilium_metrics subsys=bpf
level=warning msg="Flags mismatch for BPF map" file-path=/sys/fs/bpf/tc/globals/cilium_lb4_services_v2 new=0 old=1 subsys=bpf
level=warning msg="Removing map to allow for property upgrade (expect map data loss)" file-path=/sys/fs/bpf/tc/globals/cilium_lb4_services_v2 subsys=bpf
level=warning msg="Flags mismatch for BPF map" file-path=/sys/fs/bpf/tc/globals/cilium_lb4_backends new=0 old=1 subsys=bpf
level=warning msg="Removing map to allow for property upgrade (expect map data loss)" file-path=/sys/fs/bpf/tc/globals/cilium_lb4_backends subsys=bpf
level=warning msg="Flags mismatch for BPF map" file-path=/sys/fs/bpf/tc/globals/cilium_lb4_reverse_nat new=0 old=1 subsys=bpf
level=warning msg="Removing map to allow for property upgrade (expect map data loss)" file-path=/sys/fs/bpf/tc/globals/cilium_lb4_reverse_nat subsys=bpf

pchaigno · 2020-03-23T16:08:50Z

Woah, not sure how I missed that. Fix seems straightforward; I'll send it after community meeting. Thanks for spotting it @tklauser!

joestringer · 2020-03-23T19:55:26Z

I wonder if we should add the warnings from the above failure into the set of strings that the CI looks for to fail a build, so that we only ever break these things very intentionally.

cilium/test/helpers/cons.go

Lines 258 to 268 in cacf250

    
           // badLogMessages is a map which key is a part of a log message which indicates 
        
           // a failure if the message does not contain any part from value list. 
        
           var badLogMessages = map[string][]string{ 
        
           	panicMessage:      nil, 
        
           	deadLockHeader:    nil, 
        
           	segmentationFault: nil, 
        
           	NACKreceived:      nil, 
        
           	RunInitFailed:     {"signal: terminated", "signal: killed"}, 
        
           	sizeMismatch:      nil, 
        
           	emptyBPFInitArg:   nil, 
        
           }

pchaigno · 2020-03-23T21:00:22Z

The warnings @tklauser found look unrelated to this pull request. They concern maps that were already created by initMaps() before this pull request. I also tried to reproduce the warnings without success so far. Finally, I double checked all flags for maps created by this pull request; a few of them need the CONDITIONAL_PREALLOC flag, but that should already be handled by openOrCreate().

I wonder if we should add the warnings from the above failure into the set of strings that the CI looks for to fail a build, so that we only ever break these things very intentionally.

That sounds like a good idea. I'll prepare a pull request for that tomorrow morning and will see if it breaks any existing test.

common/utils.go

and use it to check that all Go-side keys and values for maps have the correct sizes. Signed-off-by: Paul Chaignon <paul@cilium.io>

getNumPossibleCPUs is needed for all per-cpu map and perf event arrays. Signed-off-by: Paul Chaignon <paul@cilium.io>

This commit declares the size and names of reverse NAT sock maps as constants on the Go side and writes them into the BPF program when generated. Signed-off-by: Paul Chaignon <paul@cilium.io>

Signed-off-by: Paul Chaignon <paul@cilium.io>

Create the signal map during daemon initialization. The map is closed as soon as it's created since cilium-agent never uses it. To generate the deepcopy file for the new map, I used: make generate-k8s-api Signed-off-by: Paul Chaignon <paul@cilium.io>

Create the policy call map during daemon initialization. The map is closed as soon as it's created since cilium-agent never uses it. To generate the deepcopy files for the new maps, I used: make generate-k8s-api Signed-off-by: Paul Chaignon <paul@cilium.io>

Create per-endpoint policy maps during daemon initialization. Signed-off-by: Paul Chaignon <paul@cilium.io>

Create global and per-endpoint ct maps during daemon initialization. Signed-off-by: Paul Chaignon <paul@cilium.io>

Create global NAT maps during daemon initialization. Signed-off-by: Paul Chaignon <paul@cilium.io>

Create neighbors maps used to store IP-MAC associations when NodePort services are implemented in BPF datapath. To generate the deepcopy file for the new maps, I used: make generate-k8s-api Signed-off-by: Paul Chaignon <paul@cilium.io>

pchaigno · 2020-03-24T12:04:56Z

test-me-please

tklauser

/cc @tklauser to check I didn't revert any memory optimization effort :-)

Checked the map size and agent memory usage (re. #10056) and I can see no difference compared to before this PR.

borkmann · 2020-03-26T08:58:08Z

I wonder if we should add the warnings from the above failure into the set of strings that the CI looks for to fail a build, so that we only ever break these things very intentionally.

cilium/test/helpers/cons.go

Lines 258 to 268 in cacf250

// badLogMessages is a map which key is a part of a log message which indicates

// a failure if the message does not contain any part from value list.

var badLogMessages = map[string][]string{

panicMessage: nil,

deadLockHeader: nil,

segmentationFault: nil,

NACKreceived: nil,

RunInitFailed: {"signal: terminated", "signal: killed"},

sizeMismatch: nil,

emptyBPFInitArg: nil,

}

@pchaigno @joestringer Hmm, looks like these don't trigger in our CI? :-(

In a later PR job, I've seen the following in the log (maybe due to padding or other size mismatch?):
https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/18238/

2020-03-25T23:07:55.245972761Z level=warning msg="Value-size mismatch for BPF map" file-path=/sys/fs/bpf/tc/globals/cilium_nodeport_neigh4 new=6 old=8 subsys=bpf
2020-03-25T23:07:55.245975572Z level=warning msg="Removing map to allow for property upgrade (expect map data loss)" file-path=/sys/fs/bpf/tc/globals/cilium_nodeport_neigh4 subsys=bpf
2020-03-25T23:07:55.256431723Z level=debug msg="Registered BPF map" path=/sys/fs/bpf/tc/globals/cilium_nodeport_neigh4 subsys=bpf
2020-03-25T23:07:55.256461108Z level=debug msg="Unregistered BPF map" path=/sys/fs/bpf/tc/globals/cilium_nodeport_neigh4 subsys=bpf
2020-03-25T23:07:55.2564645Z level=warning msg="Value-size mismatch for BPF map" file-path=/sys/fs/bpf/tc/globals/cilium_nodeport_neigh6 new=6 old=8 subsys=bpf
2020-03-25T23:07:55.256467416Z level=warning msg="Removing map to allow for property upgrade (expect map data loss)" file-path=/sys/fs/bpf/tc/globals/cilium_nodeport_neigh6 subsys=bpf
2020-03-25T23:07:55.266740598Z level=debug msg="Registered BPF map" path=/sys/fs/bpf/tc/globals/cilium_nodeport_neigh6 subsys=bpf
2020-03-25T23:07:55.266758628Z level=debug msg="Unregistered BPF map" path=/sys/fs/bpf/tc/globals/cilium_nodeport_neigh6 subsys=bpf

borkmann · 2020-03-26T09:09:43Z

5d6b669 also triggers the following warn:

2020-03-25T23:08:21.073661843Z level=warning msg="Skipping symbol substitution" subsys=elf symbol=cilium_call_policy
2020-03-25T23:08:21.074001157Z level=debug msg="Finished writing ELF" error="<nil>" new-elf-path=196_next/bpf_lxc.o subsys=elf template-path=/var/run/cilium/state/templates/e833cc141a8f3f0840ed0cff56ea71f9f458a747/bpf_lxc.o
2020-03-25T23:08:21.074114439Z level=warning msg="Skipping symbol substitution" subsys=elf symbol=cilium_call_policy
2020-03-25T23:08:21.074295574Z level=debug msg="Finished writing ELF" error="<nil>" new-elf-path=106_next/bpf_lxc.o subsys=elf template-path=/var/run/cilium/state/templates/e833cc141a8f3f0840ed0cff56ea71f9f458a747/bpf_lxc.o

This needs to update ignoredELFPrefixes after the rename with a cilium_call prefix or the like.

pchaigno · 2020-03-26T15:15:44Z

badLogMessages not triggering in CI: We actually fail to retrieve the logs in which to search for "bad messages". ~~The command to retrieve logs using kubectl seems to fail because of the --previous flag. I'm double checking this and will try to send a fix today.~~ Fixed in test: Fix kubectl log retrieval for badLogMessages #10717.
Property mismatch for NodePort neighbors map: This is indeed a padding issue (I guess the numbers in the error message are a bit of a giveaway). I'll send a pull request to fix that before end of day, with other followups to the present PR (map cleanup). Fixed in loader: Fixes for map creation from daemon #10728.
Symbol substitution warning: I have a fix for this one locally and we'll send before end of day. Fixed in loader: Fixes for map creation from daemon #10728.

Three new maps were added to pkg/maps/ in #10626. The corresponding checks in alignchecker are however missing. This commit adds them. Signed-off-by: Paul Chaignon <paul@cilium.io>

pchaigno added pending-review area/loader Impacts the loading of BPF programs into the kernel. area/daemon Impacts operation of the Cilium daemon. release-note/misc This PR makes changes that have no direct user impact. labels Mar 18, 2020

pchaigno requested review from a team March 18, 2020 21:33

pchaigno requested review from a team as code owners March 18, 2020 21:33

pchaigno requested a review from a team March 18, 2020 21:33

pchaigno commented Mar 18, 2020

View reviewed changes

pkg/maps/nat/nat.go Outdated Show resolved Hide resolved

pkg/bpf/map_linux.go Show resolved Hide resolved

pkg/maps/ctmap/ctmap.go Outdated Show resolved Hide resolved

jrajahalme approved these changes Mar 19, 2020

View reviewed changes

tklauser reviewed Mar 19, 2020

View reviewed changes

daemon/cmd/datapath.go Outdated Show resolved Hide resolved

aanm requested changes Mar 20, 2020

View reviewed changes

pchaigno force-pushed the pr/pchaigno/open-maps-in-agent branch 3 times, most recently from 2a7b89d to bcbb6fa Compare March 21, 2020 11:24

qmonnet approved these changes Mar 23, 2020

View reviewed changes

aanm requested changes Mar 24, 2020

View reviewed changes

common/utils.go Show resolved Hide resolved

pchaigno added 4 commits March 24, 2020 11:51

alignchecker: Support checking types' sizes only

e0aab71

and use it to check that all Go-side keys and values for maps have the correct sizes. Signed-off-by: Paul Chaignon <paul@cilium.io>

maps/metricsmap, common: Move getNumPossibleCPUs to common.util

2c129bb

getNumPossibleCPUs is needed for all per-cpu map and perf event arrays. Signed-off-by: Paul Chaignon <paul@cilium.io>

maps/lbmap, bpf: Declare rev. NAT sock map constants on Go side

8b89f8b

This commit declares the size and names of reverse NAT sock maps as constants on the Go side and writes them into the BPF program when generated. Signed-off-by: Paul Chaignon <paul@cilium.io>

maps/policymap: Rename policy call map to clarify intent

8f1b331

Signed-off-by: Paul Chaignon <paul@cilium.io>

pchaigno added 6 commits March 24, 2020 11:52

maps/policymap, daemon: Create policy maps from daemon

7ad93f2

Create per-endpoint policy maps during daemon initialization. Signed-off-by: Paul Chaignon <paul@cilium.io>

maps/ctmap, daemon: Create maps from daemon

2fb64e8

Create global and per-endpoint ct maps during daemon initialization. Signed-off-by: Paul Chaignon <paul@cilium.io>

maps/nat, daemon: Create maps from daemon

d2f3f75

Create global NAT maps during daemon initialization. Signed-off-by: Paul Chaignon <paul@cilium.io>

maps/neighborsmap, daemon: Create map from daemon

f575116

Create neighbors maps used to store IP-MAC associations when NodePort services are implemented in BPF datapath. To generate the deepcopy file for the new maps, I used: make generate-k8s-api Signed-off-by: Paul Chaignon <paul@cilium.io>

pchaigno force-pushed the pr/pchaigno/open-maps-in-agent branch from bcbb6fa to f575116 Compare March 24, 2020 10:52

pchaigno mentioned this pull request Mar 24, 2020

test: Fail in case of map property upgrade warning #10680

Merged

aanm approved these changes Mar 24, 2020

View reviewed changes

tklauser approved these changes Mar 24, 2020

View reviewed changes

brb approved these changes Mar 24, 2020

View reviewed changes

borkmann merged commit 8fd7415 into master Mar 25, 2020

borkmann deleted the pr/pchaigno/open-maps-in-agent branch March 25, 2020 09:02

pchaigno mentioned this pull request Mar 26, 2020

loader: Fixes for map creation from daemon #10728

Merged

This was referenced Mar 27, 2020

bpf: Improve compilation coverage #10712

Merged

test: Fix kubectl log retrieval for badLogMessages #10717

Merged

This was referenced Mar 31, 2020

Cleanup old policy call map #10781

Closed

daemon: Remove old policy call map #10845

Merged

vadorovsky mentioned this pull request Apr 29, 2020

policymap, fragmap: clean up doc, map size configuration #10964

Merged

pchaigno mentioned this pull request Jun 16, 2020

bpf: Use nproc --all for __NR_CPUS__ #12121

Merged

gandro mentioned this pull request Jul 1, 2020

v1.7 backports 2020-07-01 bpf NR_CPU fixes #12363

Merged

This was referenced Sep 2, 2020

daemon: Fix handling of policy call map on upgrades #13051

Merged

daemon: Fix handling of policy call map on downgrades #13052

Merged

daemon: Create all global maps in cilium-agent #10626

daemon: Create all global maps in cilium-agent #10626

Uh oh!

Conversation

pchaigno commented Mar 18, 2020

Uh oh!

jrajahalme commented Mar 18, 2020

Uh oh!

coveralls commented Mar 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pchaigno commented Mar 18, 2020

Uh oh!

tklauser left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

aanm Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

pchaigno Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

joestringer Mar 20, 2020

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

tklauser commented Mar 23, 2020

Uh oh!

pchaigno commented Mar 23, 2020

Uh oh!

joestringer commented Mar 23, 2020

Uh oh!

pchaigno commented Mar 23, 2020

Uh oh!

Uh oh!

pchaigno commented Mar 24, 2020

Uh oh!

tklauser left a comment

Choose a reason for hiding this comment

Uh oh!

borkmann commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

borkmann commented Mar 26, 2020

Uh oh!

pchaigno commented Mar 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

coveralls commented Mar 18, 2020 •

edited

Loading

borkmann commented Mar 26, 2020 •

edited

Loading

pchaigno commented Mar 26, 2020 •

edited

Loading