/test

Perhaps I'm missing some context about this change, but it is intentional that the clustermesh/kvstoremesh hives are separate, as each is run by its own individual sub-command (in turn executed in two separate containers, if both are enabled at the same time). I don't think there's the risk that any option is not responsive, because they are registered to the corresponding sub-command, as well as only one instance of gops/pprof and alike is guaranteed to be executed at any time depending on which command is executed.

It could be theoretically possible to consolidate everything into a single command, but I'm not sure that would have a lot of benefits, at the cost of worse separation of concerns (e.g., only one needs to access the k8s api-server, etcd clients are separate so that rate limiting of one doesn't impact the other and so on). It would also require a quite significant refactoring to allow enabling/disabling the different components independently, while that currently achieved via the separate sub-commands.

OK thanks a lot for that context. I haven't fully formed my thoughts on what this should look like, but here's the background I'm coming from:

• We previously had a bunch of bugs in flag handling because the same flag was registered more than once in a single binary. I've been combing through the codebase to find patterns that could cause this to happen and fix them.
• To help with the above, I tried to add a hive subcommand similar to what we have for other binaries in the tree so I could inspect the hive for this binary.
• I noticed that there are multiple hives declared in this single binary. We generally expect there to be one Hive and for it to be static, so it is easy to determine right from initial bootstrap the full set of Cells. Given this wasn't the case, I tried combining the Hive so I could inspect it, and followed the rabbit hole from there.
• As part of this I realized that the following Cells are common across both commands:

    	cell.Config(option.DefaultLegacyClusterMeshConfig),
    	cell.Config(cmtypes.DefaultClusterInfo),
    	cell.Config(pprofConfig),
  
    	pprof.Cell,
    	gops.Cell(defaults.EnableGops, defaults.GopsPortApiserver),
  
    	health.HealthAPIServerCell,
  
    	cmmetrics.Cell,
    	controller.Cell,
    	kvstore.Cell,
    	store.Cell,
  
    	cell.Provide(func(ss syncstate.SyncState) *kvstore.ExtraOptions {
    		return &kvstore.ExtraOptions{
    			BootstrapComplete: ss.WaitChannel(),
    		}
    	}),
  

I think that largely leads us to this PR.

Looking forward:

1. I don't think there is currently a problem with overlapping flags in this binary.
2. I'm not sure if it's worth tweaking the top-level usage of hives. I could maybe look into creating two Hive subcommands to inspect the individual "commands" rather than one top-level Hive.
3. It might simplify the code a bit if the common Cells listed above are combined into a "clustermesh-common" Cell which could then be imported once from both commands, so the lists of common Cells are in one place.

More than anything, this gave me an excuse to poke around in the top level of this command to try to understand the functionality better, so that goal of mine is also achieved. Another alternative option going forward is just for me to abandon this PR.

Thanks a lot for the context, makes sense! And obviously feel free to ask if you have questions about this binary or clustermesh in general.

• To help with the above, I tried to add a hive subcommand similar to what we have for other binaries in the tree so I could inspect the hive for this binary.

The commands to inspect the hive are already available, although as part of the individual sub-commands (not at the root level):

• $ gor clustermesh-apiserver/main.go clustermesh hive
• $ gor clustermesh-apiserver/main.go kvstoremesh hive

Essentially, the root of the clustermesh-apiserver is just a container for the other commands, and it does nothing by itself (besides printing the help message).

I noticed that there are multiple hives declared in this single binary. We generally expect there to be one Hive and for it to be static, so it is easy to determine right from initial bootstrap the full set of Cells. Given this wasn't the case, I tried combining the Hive so I could inspect it, and followed the rabbit hole from there.

Yes, the clustermesh-apiserver is a bit special in this respect as it bundles multiple independent functionalities as separate sub-commands, essentially to avoid requiring separate containers/binaries that have already proved problematic in the past. The hive configuration is still static as for e.g., the Cilium agent, but there are actually two, one for each core command (the other commands are instead more trivial and implemented the old way).

As part of this I realized that the following Cells are common across both commands: [...]

Yep, these could be grouped together in a dedicated module and imported by both. It wouldn't change anything from the functional point of view, but I agree it would remove a bit of code duplication.

OK, I'll refocus this PR on just creating the common Cell in that case - I think we'll get most of the cleanup benefits from that alone. I'm still not 100% confident about my statement "[there's no] problem with overlapping flags in this binary." given the way Hive is used in these binaries, but I have an idea about how to prove that out.

/ci-clustermesh

/test

/test

/ci-clustermesh

/test