pkg/maps/egressmap: refactor into a cell #24865

lmb · 2023-04-13T12:12:30Z

pkg/maps/egressmap: refactor policy map into a cell

Rework the egressmap from a global variable into a Cell. The map still has
singleton behaviour due to map pinning, but at least for unit tests we can
opt out of pinning and isolate the map from global changes.

Stop writing out EGRESS_POLICY_MAP_SIZE into node_config.h since the map
definition in C is never actually used. The map size is configured via a
command line parameter instead. Hardcode the previously used default value
to avoid errors when running BPF unit tests. This is simpler than wiring the
new PolicyConfig into HeaderfileWriter.

Reuse the MapOut strategy from configmap and authmap to ensure that the
egressmap is initialized by the agent before the loader is invoked. 
Otherwise map creation might use the (incorrect) MaxElems from the compiled
C code.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>

hivetest: add Lifecycle to make testing easier

hive requires splitting up object lifecycle into New, Start, Stop. It does
this by injecting a hive.Lifecycle into constructors. This in turn means
that we need a lifecycle to create objects during testing.

Implement a minimal hive.Lifecycle for use in testing, which doesn't 
distinguish between New and Start phases and instead immediately executes
any start hooks. Stop hooks are invoked at test cleanup time.

Signed-off-by: Lorenz Bauer <lmb@isovalent.com>

Refactor egressgateway specific maps into a cell

Updates: #23782

lmb · 2023-04-13T12:16:03Z

/test-runtime

lmb · 2023-04-13T13:29:20Z

/test-runtime

lmb · 2023-04-13T16:18:51Z

/test-runtime

lmb · 2023-04-17T09:03:09Z

/test

pkg/egressgateway/policy.go

pkg/maps/egressmap/policy.go

cilium/cmd/bpf_egress_list.go

pkg/maps/egressmap/policy.go

lmb · 2023-04-19T13:26:14Z

bpf/lib/maps.h

@@ -213,7 +213,7 @@ struct {
 	__type(key, struct egress_gw_policy_key);
 	__type(value, struct egress_gw_policy_entry);
 	__uint(pinning, LIBBPF_PIN_BY_NAME);
-	__uint(max_entries, EGRESS_POLICY_MAP_SIZE);
+	__uint(max_entries, 16384);


This is to keep the BPF unit tests happy.

So, value must be hardcoded and constant is not allowed?

I removed the define for EGRESS_POLICY_MAP_SIZE so that I can get rid of the entry in node_config.h which is written out at runtime. It's difficult to plumb the size requested via config / command line into node_config.h without relying on option.Config global which I wanted to avoid.

So: we could keep the define but move it to maps.h. That runs the risk of some C code relying on that define with the assumption that it reflects the configured value. So I opted to remove the define instead.

lmb · 2023-04-19T13:35:48Z

The current state of the PR is not good to merge, since it doesn't enforce a dependency between the loader component and the map.

lmb · 2023-04-25T13:26:53Z

/test-runtime

pkg/maps/egressmap/policy.go

lmb · 2023-04-27T09:14:31Z

pkg/maps/egressmap/policy.go

-			return err
-		}
+func createPolicyMapFromDaemonConfig(daemonConfig *option.DaemonConfig, lc hive.Lifecycle, cfg PolicyConfig) bpf.MapOut[PolicyMap] {
+	if !daemonConfig.EnableIPv4EgressGateway {


Maybe we can get away without this? Ties into the whole "optional hive feature".

lmb · 2023-04-27T12:45:37Z

/test

Job 'Cilium-PR-K8s-1.26-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathServicesTest Checks N/S loadbalancing With host policy Tests NodePort

Failure Output

FAIL: Can not connect to service "http://192.168.56.12:30861" from outside cluster (1/10)

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.26-kernel-net-next/1953/

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.26-kernel-net-next so I can create one.

Then please upload the Jenkins artifacts to that issue.

jibi

Looks good, thanks! Just one nit and a question

jibi · 2023-04-27T14:56:23Z

pkg/datapath/cells.go

@@ -101,6 +101,7 @@ type datapathParams struct {
 	LC      hive.Lifecycle
 	WgAgent *wg.Agent

-	// Force map initialisation before loader
+	// Force map initialisation before loader. You should not use these otherwise.


nit: extra dot at the end

Hmm I don't understand, sorry.

pkg/egressgateway/manager_test.go

mhofstetter

LGTM - thanks for introducing the generic MapOut type

lmb · 2023-05-02T10:09:18Z

/test

lmb · 2023-05-02T11:20:01Z

Travis error is probably a stuck test #23509

joamaki

hivetest package looks good to me

lmb · 2023-05-02T14:00:35Z

runtime test is #25178

joestringer · 2023-05-02T23:31:03Z

Please make the tests green and get all codeowner groups' review.

hive requires splitting up object lifecycle into New, Start, Stop. It does this by injecting a hive.Lifecycle into constructors. This in turn means that we need a lifecycle to create objects during testing. Implement a minimal hive.Lifecycle for use in testing, which doesn't distinguish between New and Start phases and instead immediately executes any start hooks. Stop hooks are invoked at test cleanup time. Signed-off-by: Lorenz Bauer <lmb@isovalent.com>

Rework the egressmap from a global variable into a Cell. The map still has singleton behaviour due to map pinning, but at least for unit tests we can opt out of pinning and isolate the map from global changes. Stop writing out EGRESS_POLICY_MAP_SIZE into node_config.h since the map definition in C is never actually used. The map size is configured via a command line parameter instead. Hardcode the previously used default value to avoid errors when running BPF unit tests. This is simpler than wiring the new PolicyConfig into HeaderfileWriter. Reuse the MapOut strategy from configmap and authmap to ensure that the egressmap is initialized by the agent before the loader is invoked. Otherwise map creation might use the (incorrect) MaxElems from the compiled C code. Signed-off-by: Lorenz Bauer <lmb@isovalent.com>

lmb · 2023-05-03T13:46:35Z

@viktor-kurchenko could you take another look and approve?

lmb · 2023-05-04T08:42:19Z

/test

thorn3r

LGTM, this was a nice lil intro to hive for me 😎 nice work

lmb · 2023-05-04T16:06:42Z

I added ready-to-merge accidentally but am now getting 500 when trying to remove it.

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 Apr 13, 2023

lmb force-pushed the egressmap-cell branch from ba4d88f to e412d33 Compare April 13, 2023 13:29

lmb force-pushed the egressmap-cell branch from e412d33 to 046709b Compare April 13, 2023 15:52

lmb force-pushed the egressmap-cell branch from 046709b to ce64e65 Compare April 17, 2023 08:57

lmb added the release-note/misc This PR makes changes that have no direct user impact. label Apr 17, 2023

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 Apr 17, 2023

lmb marked this pull request as ready for review April 17, 2023 09:03

lmb requested review from a team as code owners April 17, 2023 09:03

lmb requested review from thorn3r, asauber, aspsk and ti-mo April 17, 2023 09:03

lmb commented Apr 17, 2023

View reviewed changes

pkg/egressgateway/policy.go Outdated Show resolved Hide resolved

asauber reviewed Apr 18, 2023

View reviewed changes

pkg/maps/egressmap/policy.go Show resolved Hide resolved

cilium/cmd/bpf_egress_list.go Show resolved Hide resolved

mhofstetter reviewed Apr 19, 2023

View reviewed changes

pkg/maps/egressmap/policy.go Outdated Show resolved Hide resolved

lmb commented Apr 19, 2023

View reviewed changes

lmb added the dont-merge/blocked Another PR must be merged before this one. label Apr 19, 2023

lmb force-pushed the egressmap-cell branch 2 times, most recently from c3ab40e to 180db68 Compare April 25, 2023 13:26

lmb force-pushed the egressmap-cell branch from 180db68 to b6e918b Compare April 26, 2023 09:18

lmb requested a review from a team as a code owner April 26, 2023 09:18

lmb commented Apr 27, 2023

View reviewed changes

lmb force-pushed the egressmap-cell branch 3 times, most recently from c74889d to 7174e0e Compare April 27, 2023 12:45

jibi approved these changes Apr 27, 2023

View reviewed changes

mhofstetter approved these changes May 1, 2023

View reviewed changes

lmb force-pushed the egressmap-cell branch from 7174e0e to 37086b9 Compare May 2, 2023 10:09

joamaki approved these changes May 2, 2023

View reviewed changes

lmb added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 2, 2023

joestringer removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 2, 2023

joestringer added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 2, 2023

lmb added 2 commits May 3, 2023 14:44

lmb force-pushed the egressmap-cell branch from 37086b9 to ae32e01 Compare May 3, 2023 13:44

thorn3r approved these changes May 4, 2023

View reviewed changes

maintainer-s-little-helper bot removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 4, 2023

lmb added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 4, 2023

viktor-kurchenko approved these changes May 4, 2023

View reviewed changes

maintainer-s-little-helper bot removed the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 4, 2023

lmb added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label May 4, 2023

joestringer merged commit 445ae6c into cilium:main May 4, 2023
56 checks passed

lmb deleted the egressmap-cell branch May 5, 2023 08:09

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

pkg/maps/egressmap: refactor into a cell #24865

pkg/maps/egressmap: refactor into a cell #24865

lmb commented Apr 13, 2023 •

edited

lmb commented Apr 13, 2023

lmb commented Apr 13, 2023

lmb commented Apr 13, 2023

lmb commented Apr 17, 2023

lmb Apr 19, 2023

viktor-kurchenko Apr 27, 2023

lmb Apr 27, 2023

viktor-kurchenko May 1, 2023

lmb commented Apr 19, 2023

lmb commented Apr 25, 2023

lmb Apr 27, 2023

lmb commented Apr 27, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

jibi left a comment

jibi Apr 27, 2023

lmb May 3, 2023

mhofstetter left a comment

lmb commented May 2, 2023

lmb commented May 2, 2023

joamaki left a comment

lmb commented May 2, 2023

joestringer commented May 2, 2023

lmb commented May 3, 2023

lmb commented May 4, 2023

thorn3r left a comment

lmb commented May 4, 2023

pkg/maps/egressmap: refactor into a cell #24865

pkg/maps/egressmap: refactor into a cell #24865

Conversation

lmb commented Apr 13, 2023 • edited

lmb commented Apr 13, 2023

lmb commented Apr 13, 2023

lmb commented Apr 13, 2023

lmb commented Apr 17, 2023

lmb Apr 19, 2023

Choose a reason for hiding this comment

viktor-kurchenko Apr 27, 2023

Choose a reason for hiding this comment

lmb Apr 27, 2023

Choose a reason for hiding this comment

viktor-kurchenko May 1, 2023

Choose a reason for hiding this comment

lmb commented Apr 19, 2023

lmb commented Apr 25, 2023

lmb Apr 27, 2023

Choose a reason for hiding this comment

lmb commented Apr 27, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

jibi left a comment

Choose a reason for hiding this comment

jibi Apr 27, 2023

Choose a reason for hiding this comment

lmb May 3, 2023

Choose a reason for hiding this comment

mhofstetter left a comment

Choose a reason for hiding this comment

lmb commented May 2, 2023

lmb commented May 2, 2023

joamaki left a comment

Choose a reason for hiding this comment

lmb commented May 2, 2023

joestringer commented May 2, 2023

lmb commented May 3, 2023

lmb commented May 4, 2023

thorn3r left a comment

Choose a reason for hiding this comment

lmb commented May 4, 2023

lmb commented Apr 13, 2023 •

edited

lmb commented Apr 27, 2023 •

edited by maintainer-s-little-helper bot