configmap & utime sync: provide via hive cell #24830

mhofstetter · 2023-04-12T06:33:20Z

Currently, the interaction with the BPF map configmap and the corresponding utime sync is mainly via global accessors.

This commit refactors this, by introducing hive cells for the configmap and the utime sync.

mhofstetter · 2023-04-12T07:27:04Z

pkg/datapath/linux/utime/cell.go

+)
+
+func initUtimeSync(lifecycle hive.Lifecycle, configMap configmap.Map) {
+	controllerManager := controller.NewManager()


@joamaki would it be worth to have a provider for the controller manager?

What do you think of #24558?

interesting - will take a look.

i think something like this could help in this case - and could be refactored in the future (once its merged)

joamaki · 2023-04-13T12:28:03Z

pkg/datapath/linux/utime/cell.go

+)
+
+func initUtimeSync(lifecycle hive.Lifecycle, configMap configmap.Map) {
+	controllerManager := controller.NewManager()


What do you think of #24558?

pkg/maps/configmap/cell.go

joamaki · 2023-04-13T12:29:02Z

pkg/maps/configmap/cell.go

+				return fmt.Errorf("failed to init config map: %w", err)
+			}
+
+			return nil


Stop hook missing that closes it

thanks - added!

jrajahalme

LGTM

mhofstetter · 2023-04-20T07:28:03Z

rebased and resolved conflicts

pkg/maps/mapregistry/cell.go

pkg/maps/configmap/cell.go

pkg/maps/mapregistry/cell.go

joamaki · 2023-04-20T11:19:08Z

pkg/maps/configmap/cell.go

+	}
+}
+
+type MapOut struct {


I usually try to avoid writing cell.Outs with group values for the fear of typoing the group name. You can get around this with a helper, e.g. func NewMapOut[Map any](m Map) MapOut[Map], but perhaps not worth adding it just yet.

mhofstetter · 2023-04-24T05:47:41Z

/test

mhofstetter · 2023-04-24T08:42:14Z

/test-1.27-net-next

lmb · 2023-04-24T09:26:00Z

pkg/maps/cells.go

+)
+
+// Cell contains all cells which are providing BPF Maps.
+var Cell = cell.Module(


When should I add something here vs. to the datapath cell as done for the configmap?

Ah nvm utime.Cell is not a map.

exactly - utime just syncs the utime and uses the configmap to write from user- to kernelspace.

the cell of the configmap itself is part of the maps cell.

Currently, the interaction with the BPF map configmap and the corresponding utime sync is mainly via global accessors. This commit refactors this, by providing hive cells for the configmap and the utime sync. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

This commit lets the datapath depend on all BPF maps which are managed by a hive cell. BPF Map cells can register their map to the registry via hive value group `bpf-maps` and the new BpfMap marker interface. This way, we ensure that BPF maps are initialized via their cell before the loader. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

Currently, the initialization of the auth map is mainly driven by the usage in the auth manager. To always guarantee the initialization before the bpf loader, with this commit, the auth map gets registered in the map registry. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

To always guarantee the initialization of the config bpf map before the bpf loader, with this commit, the auth map gets registered in the map registry. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

mhofstetter · 2023-04-24T11:35:11Z

rebased to main

mhofstetter · 2023-04-24T11:35:18Z

/test

Job 'Cilium-PR-K8s-1.25-kernel-5.4' failed:

Click to show.

Test Name

K8sDatapathConfig Host firewall With VXLAN and endpoint routes

Failure Output

FAIL: Failed to reach 10.0.0.229:80 from testclient-host-2jfhj

Jenkins URL: https://jenkins.cilium.io/job/Cilium-PR-K8s-1.25-kernel-5.4/107/

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

Job 'Cilium-PR-K8s-1.16-kernel-4.19' hit: #25068 (85.93% similarity)

mhofstetter · 2023-04-24T12:24:38Z

vagrant issues
/test-1.27-net-next

mhofstetter · 2023-04-24T12:37:44Z

test-1.16-4.19 (https://jenkins.cilium.io/job/Cilium-PR-K8s-1.16-kernel-4.19/1081/) flaked with #25068
test-1.25-5.4 (https://jenkins.cilium.io/job/Cilium-PR-K8s-1.25-kernel-5.4/107/) flaked with #24966

mhofstetter · 2023-04-24T14:54:51Z

referenced ci-flake issues & code owners covered -> marking as ready-to-merge

michi-covalent · 2023-04-25T01:48:21Z

why is this ready to merge? it's not approved by cilium/sig-datapath yet right?

mhofstetter · 2023-04-25T05:53:39Z

why is this ready to merge? it's not approved by cilium/sig-datapath yet right?

@michi-covalent jussi is part of sig-datapath and looks like the perfect reviewer for this :)

michi-covalent · 2023-04-25T06:02:02Z

@michi-covalent jussi is part of sig-datapath and looks like the perfect reviewer for this :)

oh yeah he is. sorry i got confused because yutaro is still listed as the code owner from sig-dapath.

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

mhofstetter marked this pull request as ready for review April 12, 2023 07:23

mhofstetter requested a review from a team as a code owner April 12, 2023 07:23

mhofstetter requested review from YutaroHayakawa and joamaki April 12, 2023 07:23

mhofstetter commented Apr 12, 2023

View reviewed changes

mhofstetter requested a review from jrajahalme April 12, 2023 07:27

mhofstetter mentioned this pull request Apr 12, 2023

auth: Use authmap for auth_required policies #24410

Merged

joamaki reviewed Apr 13, 2023

View reviewed changes

mhofstetter force-pushed the pr/mhofstetter/hive-config-map branch from 48bc4fe to c40e3da Compare April 13, 2023 13:09

mhofstetter requested a review from joamaki April 13, 2023 13:10

mhofstetter force-pushed the pr/mhofstetter/hive-config-map branch from c40e3da to 2a69f22 Compare April 14, 2023 16:03

jrajahalme approved these changes Apr 19, 2023

View reviewed changes

mhofstetter force-pushed the pr/mhofstetter/hive-config-map branch from 2a69f22 to 02a8a33 Compare April 20, 2023 07:27

joamaki reviewed Apr 20, 2023

View reviewed changes

mhofstetter force-pushed the pr/mhofstetter/hive-config-map branch from 691bdff to 4de20d8 Compare April 20, 2023 12:24

mhofstetter requested a review from joamaki April 20, 2023 12:28

joamaki approved these changes Apr 24, 2023

View reviewed changes

lmb reviewed Apr 24, 2023

View reviewed changes

mhofstetter added 4 commits April 24, 2023 13:34

configmap: register map

b796c01

To always guarantee the initialization of the config bpf map before the bpf loader, with this commit, the auth map gets registered in the map registry. Signed-off-by: Marco Hofstetter <marco.hofstetter@isovalent.com>

mhofstetter force-pushed the pr/mhofstetter/hive-config-map branch from 4de20d8 to b796c01 Compare April 24, 2023 11:34

maintainer-s-little-helper bot mentioned this pull request Apr 24, 2023

CI: K8sAgentHubbleTest Hubble Observe Test L3/L4 Flow #25068

Closed

mhofstetter added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Apr 24, 2023

michi-covalent added the dont-merge/waiting-for-user-feedback Waiting for feedback from user before the PR should be merged. label Apr 25, 2023

michi-covalent removed the dont-merge/waiting-for-user-feedback Waiting for feedback from user before the PR should be merged. label Apr 25, 2023

michi-covalent merged commit a194628 into cilium:main Apr 25, 2023
54 of 56 checks passed

mhofstetter deleted the pr/mhofstetter/hive-config-map branch April 25, 2023 06:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

configmap & utime sync: provide via hive cell #24830

configmap & utime sync: provide via hive cell #24830

mhofstetter commented Apr 12, 2023 •

edited

mhofstetter Apr 12, 2023

joamaki Apr 13, 2023

mhofstetter Apr 13, 2023

joamaki Apr 13, 2023

joamaki Apr 13, 2023

mhofstetter Apr 13, 2023

jrajahalme left a comment

mhofstetter commented Apr 20, 2023

joamaki Apr 20, 2023

mhofstetter commented Apr 24, 2023

mhofstetter commented Apr 24, 2023

lmb Apr 24, 2023

lmb Apr 24, 2023

mhofstetter Apr 24, 2023

mhofstetter commented Apr 24, 2023

mhofstetter commented Apr 24, 2023 •

edited by maintainer-s-little-helper bot

Test Name

Failure Output

mhofstetter commented Apr 24, 2023 •

edited

mhofstetter commented Apr 24, 2023 •

edited

mhofstetter commented Apr 24, 2023 •

edited

michi-covalent commented Apr 25, 2023

mhofstetter commented Apr 25, 2023

michi-covalent commented Apr 25, 2023

configmap & utime sync: provide via hive cell #24830

configmap & utime sync: provide via hive cell #24830

Conversation

mhofstetter commented Apr 12, 2023 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrajahalme left a comment

Choose a reason for hiding this comment

mhofstetter commented Apr 20, 2023

Choose a reason for hiding this comment

mhofstetter commented Apr 24, 2023

mhofstetter commented Apr 24, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mhofstetter commented Apr 24, 2023

mhofstetter commented Apr 24, 2023 • edited by maintainer-s-little-helper bot

Test Name

Failure Output

mhofstetter commented Apr 24, 2023 • edited

mhofstetter commented Apr 24, 2023 • edited

mhofstetter commented Apr 24, 2023 • edited

michi-covalent commented Apr 25, 2023

mhofstetter commented Apr 25, 2023

michi-covalent commented Apr 25, 2023

mhofstetter commented Apr 12, 2023 •

edited

mhofstetter commented Apr 24, 2023 •

edited by maintainer-s-little-helper bot

mhofstetter commented Apr 24, 2023 •

edited

mhofstetter commented Apr 24, 2023 •

edited

mhofstetter commented Apr 24, 2023 •

edited