daemon: Top-level composition into a hierarchy of cells #21736

joamaki · 2022-10-14T10:22:16Z

First commit cleans up the cilium-agent objects output.
Second commit adds daemon/cmd/cells.go that expresses the agent as a composition of 3 top-level modules and refactors daemon_test.go and test/controlplane to use the cmd.ControlPlane. This removes the last blocker from being able to start implementing more of the agent as cells.

Here is how the output of cilium-agent objects looks now. The agent is divided into 3 top-level modules,
infrastructure, control plane and datapath. The infrastructure and datapath modules are meant to contain implementations that talk to real systems, either external or the local kernel. Control plane is meant to contain pure business logic that can be integration tested in non-privileged platform independent tests (e.g. on both macOS and Linux) by mocking the infrastructure and datapath modules. Preferably over time we would have the source code organization reflect this division (if we can stomach the backport pain).

Cells:

  Ⓜ️ Cilium Agent:
      Ⓜ️ Infrastructure:
          Ⓜ️ gops:
              ⚙️ gops.GopsConfig:
                  • gops-port[uint16] = 9890
              🛠️ gops.registerGopsHooks (cell.go:38): func(hive.Lifecycle, logrus.FieldLogger, gops.GopsConfig)

          Ⓜ️ k8s-client:
              ⚙️ client.Config:
                  • enable-k8s-api-discovery[bool]  = false
                  • k8s-api-server[string]          = 
                  • k8s-client-burst[int]           = 0
                  • k8s-client-qps[float32]         = 0
                  • k8s-heartbeat-timeout[duration] = 30s
                  • k8s-kubeconfig-path[string]     = 
              🚧 client.newClientset (cell.go:106): func(hive.Lifecycle, logrus.FieldLogger, client.Config) (client.Clientset, error)


      Ⓜ️ Control Plane:
          🚧 node.newLocalNodeStore (local_node_store.go:71): func(node.LocalNodeStoreParams) (node.LocalNodeStore, error)
          Ⓜ️ Daemon:
              🚧 cmd.newDaemonPromise (daemon_main.go:1614): func(cmd.daemonParams) promise.Promise[*cmd.Daemon]
              🛠️ cmd.glob..func1 (daemon_main.go:1601): func(promise.Promise[*cmd.Daemon])


      Ⓜ️ Datapath:
          🚧 cmd.newDatapath (daemon_main.go:1545): func(hive.Lifecycle) datapath.Datapath

joamaki · 2022-10-16T13:28:56Z

Next steps after this in #21746 which converts few of the easy "outer" components in *Daemon into modules. Worth taking a look as that perhaps gives a clearer picture of where this is heading.

joamaki · 2022-10-16T13:35:19Z

/test

squeed · 2022-10-18T12:35:50Z

daemon/cmd/daemon_main.go

@@ -1679,7 +1702,8 @@ func runDaemon(ctx context.Context, cleaner *daemonCleanup, shutdowner hive.Shut
 	}
 	bootstrapStats.k8sInit.End(true)
 	restoreComplete := d.initRestore(restoredEndpoints)
-	if wgAgent != nil {
+
+	if wgAgent, ok := d.datapath.WireguardAgent().(*wireguard.Agent); ok && wgAgent != nil {


Rather than casting, can you add whatever methods are missing to the interface?

That will create an import cycle (ipcache depends on pkg/datapath, Init() takes *IPCache). I'll instead pass *wg.Agent down as a separate param. It'll be a bit ugly for a short while with this "init after datapath restored". Moving datapath related initialization into pkg/datapath is not far off now though. This was done in the "top-level composition" commit.

squeed · 2022-10-18T13:10:45Z

daemon/cmd/daemon_main.go

@@ -1090,17 +1106,6 @@ func initEnv() {
 		neighborsmap.SizeofNeighKey6+neighborsmap.SizeOfNeighValue,
 		lbmap.SizeofSockRevNat6Key+lbmap.SizeofSockRevNat6Value)

-	// Prepopulate option.Config with options from CLI.
-	option.Config.Populate(Vp)


A bunch of other stuff in initEnv relies on option.Config -- is this going to be a problem?

As far as I followed, the init() function earlier will ensure that cobra.OnInitialize() is set up to call option.Config.Populate(Vp), so then when main() -> cmd.Execute() -> RootCmd.Execute() is called, it should call the OnInitialize() functions to populate the config and then subsequently call RootCmd.Run -> runApp() -> initEnv() and retain the same ordering property as today.

Handy link for cross-reference: https://pkg.go.dev/github.com/spf13/cobra#OnInitialize

Yep, the order remains the same, I basically just split it so that all commands get option.Config.Populate but only root command calls initEnv.

joestringer

I like the three-layering. I didn't take a closer look at the subsequent PR yet but at a high level it seems like a reasonable step in the right direction.

While reviewing the second commit, I wondered if some of the refactoring / code moves could be split into dedicated commits in order to make the rest of the functional changes a bit more obvious. I know it's not all that much code, for example the factoring out of the datapath initialization into newDatapath(), but every line of code that is only moving from one place to another into a dedicated function makes it more difficult to pay attention to the functional changes. I realize the wrapping in the function is also then essential to some of the refactoring/reordering, but if there's a way to e.g. move the code to a function without changing anything else in one commit, then do the functional change separately, that would be easier to review. I noted one other example below with only reordering of some node init logic that might be another candidate for a dedicated commit.

At a glance of the testsuite, it seems like the monitor initialization ordering is being changed. I was scanning through to try to locate why this might be, but it wasn't obvious to me just yet. The reason I suspect this is in this failure:

⚠️  Number of "level=warning" in logs: 53
...
Top 3 errors/warnings:
Notifying monitor about endpoint regeneration failed
Failed to send agent start monitor message

Notably also, other failures in the ginkgo suite appear to be giving a signal that the monitor event processing / output might not be keeping up.

After one pass, I'm not confident I understand the totality of the changes well enough to give an explicit approval, but generally they seem like reasonable changes that are a necessary step on the path to modularizing the agent. 👍 Maybe I'll sleep on it and take another pass.

joestringer · 2022-10-21T00:46:48Z

daemon/cmd/daemon_main.go

+	lc.Append(hive.Hook{
+		OnStart: func(context.Context) error {
+			if err := enableIPForwarding(); err != nil {
+				log.Fatalf("enabling IP forwarding via sysctl failed: %s", err)
+			}
+
+			iptablesManager.Init()
+			return nil
+		}})


Now that this is in the OnStart(), this logic is actually deferred until later instead of happening in between the node initialization and the wireguard initialization right? I don't know if that's consequential but it wasn't called out in the commit message.

Double checked that it's OK and modified commit message.

joestringer · 2022-10-21T00:54:14Z

daemon/cmd/daemon_main.go

-	// If there is one device specified, use it to derive better default
-	// allocation prefixes
-	node.InitDefaultPrefix(option.Config.DirectRoutingDevice)
-
-	if option.Config.IPv6NodeAddr != "auto" {
-		if ip := net.ParseIP(option.Config.IPv6NodeAddr); ip == nil {
-			log.WithField(logfields.IPAddr, option.Config.IPv6NodeAddr).Fatal("Invalid IPv6 node address")
-		} else {
-			if !ip.IsGlobalUnicast() {
-				log.WithField(logfields.IPAddr, ip).Fatal("Invalid IPv6 node address: not a global unicast address")
-			}
-
-			node.SetIPv6(ip)
-		}
-	}
-
-	if option.Config.IPv4NodeAddr != "auto" {
-		if ip := net.ParseIP(option.Config.IPv4NodeAddr); ip == nil {
-			log.WithField(logfields.IPAddr, option.Config.IPv4NodeAddr).Fatal("Invalid IPv4 node address")
-		} else {
-			node.SetIPv4(ip)
-		}
-	}
-


Any chance we could make this ordering change before as a separate commit, in case this has an unknown consequence? It would be helpful to be able to git bisect down to this specific change if it triggers an issue. I know it's called out in the commit message, but it's hard to say whether that would be enough to pinpoint the change.

For review purposes I ended up doing this myself, which made it a bit easier for me to reason about anyway (no changes, just git commit splitting): https://github.com/joestringer/cilium/pull/new/review/agent-composition

Thanks for splitting it up. Reset to your branch.

Reverted this change in the end to minimize the number of functional changes in this PR as I hit too many surprises.

joestringer · 2022-10-21T01:03:47Z

daemon/cmd/daemon_main.go

-	if params.Config.SkipDaemon {
-		return nil
-	}
+func newDaemonPromise(params daemonParams) promise.Promise[*Daemon] {


Could you paint a bit more of a picture of the rationale behind the promise usage here? I'm sure there's good reasons, but I was not able to parse them from the commit message :-)

Here's the current explanation:

To facilitate gradual move of components into cells from Daemon struct, this expose it as promise.Promise[*Daemon]. This can be then further projected down to individual objects as needed. E.g. by adding provider: func projectIPCache(promise.Promise[*Daemon]) promise.Promise[*IPCache].

Looking through the commits, I only see daemonPromise.Await(ctx) being used in unit tests, so maybe it's making some aspect of dependency injection or synchronization easier? My naive read of the description above is "use promises in the Daemon so that we can use promises in subsequent components".

Thinking a little bit more, the overall structure right now is effectively one giant function that triggers all of the initializers across the entire codebase. I think the rationale here is about trying to make each individual dependency more explicit, so that over time we can split up the giant function into lots of smaller modules that are defined independently with only the logic relevant to those modules. Then it'll be hopefully easier to reason about initialization ordering for each hunk of logic, since there will only be a list of promises that are required and provided and anything beyond that is not relevant to this module.

Second day thoughts on the above is that most dependencies should already be handled by the Hive / Cell design & parameters being automatically resolved. But probably there is more use for promises in cases where a module runs its own dedicated goroutines for module-internal logic, and that logic needs to rely on the availability of some other module. I may have a good use for this in ipcache, since there's currently some ugly logic around doing some initial setup and then retrying the main loop if it gets triggered before identity allocation or k8s is fully synced. Not sure how much of that can be resolved via Cells vs. promises yet, but we can figure that out over time.

The main reason for the Promise[*Daemon] right now is that we're using it in tests (daemon_test.go, controlplane) to grab *Daemon and use it to validate results.

We can also use it to "lift" things out from *Daemon and depend on them in other components, but of course preferably we would not depend on Promise[*Foo] and would rather provide *Foo normally via injection. I'll reword the commits to not suggest this and just document the current use.

joestringer

Overall the changes look good to me. There's several ordering modifications in the last commit, so it'd be nice if we could split each ordering change into a dedicated commit in a similar style to how I did here just to make things a bit more bisectable in case of problems. Other than that, I have a few discussion points above but I don't think that any of these are particularly consequential to the structural changes being proposed in this PR.

joestringer · 2022-10-22T02:15:15Z

daemon/cmd/daemon_main.go


 			// Start running the daemon in the background (blocks on API server's Serve()) to allow rest
 			// of the start hooks to run.
-			go runDaemon(ctx, cleaner, params.Shutdowner, params.Clientset)
+			if !option.Config.DryMode {


Interesting! That means that the other stuff in runDaemon is no longer necessary for unit testing 👍

I noticed that the runDaemon() function still has some !option.Config.DryMode checks inside, so we can probably drop those since the check now occurs here?

Removed the DryMode checks from runDaemon and initEnv.

joamaki · 2022-10-24T11:19:35Z

/test

joamaki · 2022-11-01T13:17:32Z

/test-1.16-4.9

joamaki · 2022-11-02T13:24:41Z

/test

Job 'Cilium-PR-K8s-1.24-kernel-4.19' has 3 failures but they might be new flakes since it also hit 1 known flakes: #17628 (92.24)

joamaki · 2022-11-02T16:25:38Z

/test

Job 'Cilium-PR-K8s-1.24-kernel-4.19' has 2 failures but they might be new flakes since it also hit 1 known flakes: #17628 (92.24)

joestringer · 2022-11-03T23:31:20Z

ConformanceIngress (Shared LB) is recently enabled, I'm not sure how stable those tests are yet (cc @sayboras @youngnick ).
1.23 failures look like #21735 (awaiting @cilium/sig-datapath investigation).
1.24 failures look like #21979 (now mitigated in master, rebase should pick that up).

sayboras · 2022-11-04T00:07:02Z

The original flaky tests were fixed as part of Gateway API work, however it seems like there is one more case, for the time being, we can mark it back to optional.

I will take a closer look once I am back to work.

joestringer · 2022-11-04T00:43:46Z

Confirmed one of the failures on ConformanceIngress (Shared LB) has the same symptoms as #21993 . This failure also had a couple of other slightly different symptoms, not sure if those are also flakes or not.

I've marked that job as no longer required for now.

Package names within github.com/cilium/cilium are unique enough that we can trim it down: github.com/cilium/cilium/pkg/k8s/resource.Resource[*github.com/cilium/cilium/.../v1.Service] -> resource.Resource[*v1.Service] In the rare cases there is ambiquity (like above, is it corev1 or slim_corev1?), it's easily resolvable looking at the constructor implementation. Keeping the output human readable is more important. Signed-off-by: Jussi Maki <jussi@isovalent.com>

Move the datapath initialization code to a new dedicated function. The IptablesManager.Init call is deferred to a start hook to allow "cilium-agent objects" to work. This does not change the initialization order as nothing in newDatapath uses iptablesManager and the returned Datapath interface is only used when initializing *Daemon which happens from a start hook. Signed-off-by: Jussi Maki <jussi@isovalent.com> Signed-off-by: Joe Stringer <joe@cilium.io>

Move the CRD identity allocation mode check from initEnv to NewDaemon as it checks if k8s-client is enabled, which is not yet configured when initEnv() is invoked. Signed-off-by: Jussi Maki <jussi@isovalent.com> Signed-off-by: Joe Stringer <joe@cilium.io>

initEnv() is only relevant when running the agent as root via the cilium-agent binary and should be skipped in integration tests. This moves the initEnv call into runApp() from registerDaemonHooks(). Since we do want option.Config.Populate and logging to be set up for all commands (dot-graph, objects), these are moved from initEnv() into cobra.OnInitialize hook (executed prior to the command run hook). Signed-off-by: Jussi Maki <jussi@isovalent.com> Signed-off-by: Joe Stringer <joe@cilium.io>

This is the first step towards composing the agent as a hierarchy of cells divided into 3 layers: infrastructure, control plane and datapath. By dividing into these layers we can start having a clearer separation of responsibilities and allow writing integration tests without having to use "DryMode" as a crutch. The "DryMode" becomes mocked "Infrastructure" and "Datapath". daemon tests and test/controlplane are refactored to run the agent via cmd.ControlPlane rather than constructing it directly via NewDaemon. Since the tests use *Daemon to validate, provide it asynchronously via Promise[*Daemon]. Providing it directly would require splitting NewDaemon into pure object construction and start hook which is currently non-trivial. Signed-off-by: Jussi Maki <jussi@isovalent.com> Signed-off-by: Joe Stringer <joe@cilium.io>

initEnv and runDaemon are now only invoked when not running in DryMode, so DryMode checks in these can now be removed. Signed-off-by: Jussi Maki <jussi@isovalent.com>

Since we're still using the global getters and setters in pkg/node it's fairly dangerous to swap it out with SetLocalNodeStore as any code that runs prior to that call and uses pkg/node may end up modifying state that gets thrown away. To make the transition safer, revert back to the old semantics and keep a singleton LocalNodeStore that needs to be reset in tests via Uninitialize. We'll need to revisit this when the global getters&setters are removed. This fixes an issue in a prior commit that moved wireguard agent initialization earlier than SetLocalNodeStore. Signed-off-by: Jussi Maki <jussi@isovalent.com>

joamaki · 2022-11-15T08:09:22Z

/test

joamaki · 2022-11-15T10:54:04Z

/ci-external-workloads

joamaki · 2022-11-15T10:54:27Z

/test-runtime

joamaki · 2022-11-15T10:54:35Z

/ci-multicluster

joamaki · 2022-11-15T10:55:25Z

test-1.16-4.9 failing due to known flake (#21735). Other tests had GKE cluster creation issues, so re-triggering them.

ci-multicluster and ci-external-workloads both hit #21652:

ERROR: (gcloud.container.clusters.create) Operation [<Operation
 clusterConditions: [<StatusCondition
 canonicalCode: CanonicalCodeValueValuesEnum(INVALID_ARGUMENT, 3)
 message: 'The network "default" does not have available private IP space in 10.0.0.0/9 to reserve a /14 block for pods for cluster {Zone=us-west2-a, ProjectNum=185287498374, ProjectName=***, ClusterName=cilium-cilium-3469774359-vm, ClusterHash=8d8803e7a95c4f42b763a03e1c14e2ceb5dc53d30a7b4f2e9bd01ee653ced1dc}.'>]

These passed earlier and they are failing in master as well currently (https://github.com/cilium/cilium/actions/runs/3468923193), so marking ready-to-merge.

joamaki requested a review from dylandreimerink October 14, 2022 10:22

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 Oct 14, 2022

joamaki force-pushed the pr/joamaki/agent-composition branch 3 times, most recently from 97226cd to ee44b4c Compare October 15, 2022 09:56

joamaki mentioned this pull request Oct 15, 2022

Resource API refactoring and shared resources #21744

Merged

joamaki added the release-note/misc This PR makes changes that have no direct user impact. label Oct 16, 2022

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 Oct 16, 2022

joamaki requested review from joestringer and aanm October 16, 2022 12:22

joamaki force-pushed the pr/joamaki/agent-composition branch from ee44b4c to 41f0707 Compare October 16, 2022 12:28

joamaki mentioned this pull request Oct 16, 2022

EndpointManager and NodeManager Cells #21746

Merged

joamaki requested a review from squeed October 16, 2022 13:52

joamaki marked this pull request as ready for review October 16, 2022 14:02

joamaki requested review from a team as code owners October 16, 2022 14:02

joamaki requested a review from nebril October 16, 2022 14:02

squeed reviewed Oct 18, 2022

View reviewed changes

joestringer reviewed Oct 21, 2022

View reviewed changes

joestringer approved these changes Oct 22, 2022

View reviewed changes

joestringer reviewed Oct 22, 2022

View reviewed changes

joamaki force-pushed the pr/joamaki/agent-composition branch from 41f0707 to 0610a04 Compare October 24, 2022 09:39

squeed approved these changes Oct 24, 2022

View reviewed changes

joamaki force-pushed the pr/joamaki/agent-composition branch from 0610a04 to 4218d92 Compare October 25, 2022 08:50

joamaki force-pushed the pr/joamaki/agent-composition branch from 0668d3f to b3e1d5f Compare November 1, 2022 12:32

joamaki force-pushed the pr/joamaki/agent-composition branch 2 times, most recently from fa4b096 to 7ea5b04 Compare November 2, 2022 11:19

joamaki force-pushed the pr/joamaki/agent-composition branch from 7ea5b04 to ef6b3b3 Compare November 2, 2022 16:21

aanm approved these changes Nov 3, 2022

View reviewed changes

aanm removed the request for review from nebril November 3, 2022 14:11

joamaki force-pushed the pr/joamaki/agent-composition branch from ef6b3b3 to f08d8c8 Compare November 14, 2022 13:14

joamaki and others added 7 commits November 14, 2022 16:01

daemon: Drop now unnecessary DryMode checks

1c6f1a1

initEnv and runDaemon are now only invoked when not running in DryMode, so DryMode checks in these can now be removed. Signed-off-by: Jussi Maki <jussi@isovalent.com>

joamaki force-pushed the pr/joamaki/agent-composition branch from f08d8c8 to 8233887 Compare November 14, 2022 16:01

joamaki added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Nov 15, 2022

tklauser approved these changes Nov 16, 2022

View reviewed changes

tklauser merged commit be04818 into cilium:master Nov 16, 2022

joestringer mentioned this pull request Jan 27, 2023

Improve the modularity and extensibility of the codebase #23425

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

daemon: Top-level composition into a hierarchy of cells #21736

daemon: Top-level composition into a hierarchy of cells #21736

joamaki commented Oct 14, 2022 •

edited

joamaki commented Oct 16, 2022 •

edited

joamaki commented Oct 16, 2022

squeed Oct 18, 2022

joamaki Oct 25, 2022 •

edited

squeed Oct 18, 2022

joestringer Oct 21, 2022 •

edited

joamaki Oct 28, 2022

joestringer left a comment •

edited

joestringer Oct 21, 2022

joamaki Oct 24, 2022

joestringer Oct 21, 2022

joestringer Oct 22, 2022

joamaki Oct 24, 2022

joamaki Oct 28, 2022

joestringer Oct 21, 2022

joestringer Oct 21, 2022

joestringer Oct 22, 2022

joamaki Oct 24, 2022

joestringer left a comment

joestringer Oct 22, 2022

joamaki Oct 24, 2022

joamaki commented Oct 24, 2022

joamaki commented Nov 1, 2022

joamaki commented Nov 2, 2022 •

edited by maintainer-s-little-helper bot

joamaki commented Nov 2, 2022 •

edited by maintainer-s-little-helper bot

joestringer commented Nov 3, 2022

sayboras commented Nov 4, 2022

joestringer commented Nov 4, 2022 •

edited

joamaki commented Nov 15, 2022

joamaki commented Nov 15, 2022

joamaki commented Nov 15, 2022

joamaki commented Nov 15, 2022

joamaki commented Nov 15, 2022 •

edited

daemon: Top-level composition into a hierarchy of cells #21736

daemon: Top-level composition into a hierarchy of cells #21736

Conversation

joamaki commented Oct 14, 2022 • edited

joamaki commented Oct 16, 2022 • edited

joamaki commented Oct 16, 2022

Choose a reason for hiding this comment

joamaki Oct 25, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joestringer Oct 21, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joestringer left a comment • 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

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

joestringer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

joamaki commented Oct 24, 2022

joamaki commented Nov 1, 2022

joamaki commented Nov 2, 2022 • edited by maintainer-s-little-helper bot

joamaki commented Nov 2, 2022 • edited by maintainer-s-little-helper bot

joestringer commented Nov 3, 2022

sayboras commented Nov 4, 2022

joestringer commented Nov 4, 2022 • edited

joamaki commented Nov 15, 2022

joamaki commented Nov 15, 2022

joamaki commented Nov 15, 2022

joamaki commented Nov 15, 2022

joamaki commented Nov 15, 2022 • edited

joamaki commented Oct 14, 2022 •

edited

joamaki commented Oct 16, 2022 •

edited

joamaki Oct 25, 2022 •

edited

joestringer Oct 21, 2022 •

edited

joestringer left a comment •

edited

joamaki commented Nov 2, 2022 •

edited by maintainer-s-little-helper bot

joamaki commented Nov 2, 2022 •

edited by maintainer-s-little-helper bot

joestringer commented Nov 4, 2022 •

edited

joamaki commented Nov 15, 2022 •

edited