🌱 tmc: split apart virtual workspaces #2836
Conversation
sttts
force-pushed
the
sttts-server-split-virtual
branch
from
cc29eb9 to
61f7d79
Compare
There was a problem hiding this comment.
I lack a background in APIServer machinery to do a meaningful review of the underlying code. Still had a few questions.
How do Virtual workspaces deal with different paths?
I can see rootAPIServer now claims const DefaultRootPathPrefix string = "/services" and looks like TMC is not doing any at the moment, but in theory, it could? Or this is not something desired here?
| delegateAPIServer := delegationTarget | ||
| for _, vw := range c.Extra.VirtualWorkspaces { | ||
| var err error | ||
| delegateAPIServer, err = vw.Register(vw.Name, c.Generic, delegateAPIServer) |
There was a problem hiding this comment.
Do I get this right? Root API server now can take list of virtual workspaces servers and becomes routing layer to route to delegated child API servers?
There was a problem hiding this comment.
It was like that already before. What changes is that the plumbing allows adding to that list after the options have been completed, i.e. in the config or server layer. Before authorization was wired in a way that this list could not be changed anymore.
| @@ -30,7 +30,7 @@ import ( | |||
| virtualworkspacesoptions "github.com/kcp-dev/kcp/cmd/virtual-workspaces/options" | |||
| corev1alpha1 "github.com/kcp-dev/kcp/pkg/apis/core/v1alpha1" | |||
| workloadv1alpha1 "github.com/kcp-dev/kcp/pkg/apis/workload/v1alpha1" | |||
| syncerbuilder "github.com/kcp-dev/kcp/pkg/virtual/syncer/builder" | |||
| syncerbuilder "github.com/kcp-dev/kcp/tmc/pkg/virtual/syncer/builder" | |||
There was a problem hiding this comment.
Does this mean that syncer is now part of the TMC stack? Moves out of the core with this change?
There was a problem hiding this comment.
the virtual workspace for syncer, yes
| "github.com/kcp-dev/kcp/pkg/virtual/syncer/upsyncer" | ||
| apireconciler2 "github.com/kcp-dev/kcp/tmc/pkg/virtual/syncer/controllers/apireconciler" | ||
| transformations2 "github.com/kcp-dev/kcp/tmc/pkg/virtual/syncer/transformations" | ||
| upsyncer2 "github.com/kcp-dev/kcp/tmc/pkg/virtual/syncer/upsyncer" |
There was a problem hiding this comment.
why upsyncer2 and not just upsyncer ? Versioning?
sttts
force-pushed
the
sttts-server-split-virtual
branch
from
bc905b7 to
3cadf3f
Compare
|
/lgtm |
|
/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 |
1 similar comment
|
/lgtm |
Allows us to run a tmc-enabled virtual workspace command.
This PR converts the plumbing of VWs to the common options/config/server pattern, and with that every layer can be tweaked in a derived command. Especially, this means we can add more virtual workspaces to the base command (which does not include tmc).
Note: this PR does not yet create a tmc-less command. But it could be now with these changes.