🌱 server: split apart first tmc pieces #2795
Conversation
sttts
force-pushed
the
sttts-server-split
branch
from
73186ff
to
862893b
Compare
cmd/kcp-core/kcpcore.go
Outdated
|
|
||
| cmd := &cobra.Command{ | ||
| Use: "kcp", | ||
| Short: "Kube for Control Plane (KCP)", |
There was a problem hiding this comment.
Since kcp isn't technically an acronym, we should avoid using (KCP) here
cmd/kcp-core/options/options.go
Outdated
| Output io.Writer | ||
|
|
||
| Server serveroptions.Options | ||
| Extra ExtraOptions |
There was a problem hiding this comment.
Food for thought (not really applicable to this PR specifically) - why do we have an options struct and then we put options in an Extra field? i.e. here could we just have Options.RootDirectory instead of Options.Extra.RootDirectory?
There was a problem hiding this comment.
It's part of the pattern. Compare other options where there are 20 fields in Extra. It saves a lot of repetition and avoids mistakes.
There was a problem hiding this comment.
on completion, different type.
There was a problem hiding this comment.
Sometimes, but not always? e.g. I can see an Options/completedOptions pair that both have an ExtraOptions field. Is this just a way to make moving the options from Options to completedOptions a one-liner?
cmd/kcp/kcp.go
Outdated
| "github.com/kcp-dev/kcp/pkg/cmd/help" | ||
| "github.com/kcp-dev/kcp/pkg/embeddedetcd" | ||
| kcpfeatures "github.com/kcp-dev/kcp/pkg/features" | ||
| "github.com/kcp-dev/kcp/pkg/server" | ||
| "github.com/kcp-dev/kcp/pkg/server/options" | ||
| "github.com/kcp-dev/kcp/tmc/pkg/server" |
| serveroptions "github.com/kcp-dev/kcp/tmc/pkg/server/options" | ||
| ) | ||
|
|
||
| type Options struct { |
There was a problem hiding this comment.
What is the relationship between this and kcp-core? They look really similar?
There was a problem hiding this comment.
Factored out the generic bits around the root directory (and probably more in the future). Now both sides are simple and share the non-standard logic.
pkg/server/options/controllers.go
Outdated
| apiresource "github.com/kcp-dev/kcp/pkg/reconciler/apis/apiresource/options" | ||
| heartbeat "github.com/kcp-dev/kcp/pkg/reconciler/workload/heartbeat/options" |
| } | ||
|
|
||
| func (s *Server) Run(ctx context.Context) error { | ||
| logger := klog.FromContext(ctx).WithValues("component", "kcp") |
There was a problem hiding this comment.
| logger := klog.FromContext(ctx).WithValues("component", "kcp") | |
| logger := klog.FromContext(ctx).WithValues("component", "tmc") |
?
There was a problem hiding this comment.
for now I assume that we call the two layers "kcp-core" and "kcp". The latter includes tmc. If we want split out the workspace hierarchy even more, we need a third name:
- kcp-core, kcp, kcp-tmc
- or kcp-base, kcp-core, kcp
- or kcp-core, kcp, kcp-full.
What do we want? That's more of a communication question how we want to "sell" the layers.
There was a problem hiding this comment.
my vote goes for the first: kcp-core, kcp, kcp-tmc
There was a problem hiding this comment.
What's the different between kcp-core, kcp in kcp-core, kcp, kcp-tmc?
There was a problem hiding this comment.
kcp-core, kcp, kcp-tmc
That makes sense to me.
There was a problem hiding this comment.
So should we apply the change proposed (with kcp-tmc instead of tmc) ?
| dynamicClusterClient, | ||
| s.Core.DiscoveringDynamicSharedInformerFactory, | ||
| s.Core.KcpSharedInformerFactory.Workload().V1alpha1().SyncTargets(), | ||
| s.Core.KubeSharedInformerFactory.Core().V1().Namespaces(), |
There was a problem hiding this comment.
Nit: Move this one above to put all kcp ones together?
There was a problem hiding this comment.
It's pure move right now. Cleanups can come in follow-up.
| return nil // don't klog.Fatal. This only happens when context is cancelled. | ||
| } | ||
|
|
||
| go resourceScheduler.Start(ctx, 2) |
There was a problem hiding this comment.
I see 2 is passed in a number of places, should we make this a constant instead?
There was a problem hiding this comment.
@ncdc has a PR for that I believe
| @@ -24,7 +24,7 @@ import ( | |||
|
|
|||
| const ( | |||
| // WorkspaceTypes leads to creation of a number of default types beyond the universal type. | |||
| WorkspaceTypes = "cluster-workspace-types" | |||
| WorkspaceTypes = "workspace-types" | |||
There was a problem hiding this comment.
Don't we want to move the RootComputeWorkspace battery definition to the TMC side ?
There was a problem hiding this comment.
follow-up. This is just the first step. There must be many more.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sttts The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
bot
added
the
approved
|
/lgtm |
|
Linking to kcp-dev/contrib-tmc#76 |
create options for the
cmd/kcpbinary, and move root directory there (probably more flags will follow).Rule of thumb: flags that are not useful for embedding belong into the command.
create a
tmc/pkg/serverpackage and move controller initialization frompkg/server.create
cmd/kcp-core.This is just a start. Many more steps will follow.