Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Connect to vCluster Platform earlier #2029

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

rmweir
Copy link
Contributor

@rmweir rmweir commented Aug 6, 2024

vCluster is now able to request a data source from vCluster Platform. vCluster is only able to use the data source if it attempts to connect to the vCluster Platform prior to deploying its own control plane. vCluster Platform is now connected before control plane is deployed but the platform controllers and tailscale server are started after because they require the control plane to be running.

What issue type does this pull request address? (keep at least one, remove the others)
/kind feature

What does this pull request do? Which issues does it resolve? (use resolves #<issue_number> if possible)
Part of ENG-4279
Part of enabling using the same database server credential for multiple vcluster datastores by creating new database and user, then returning credentials for the created database and user.

Please provide a short message that should be published in the vcluster release notes
Fixed an issue where vcluster ...

What else do we need to know?

Copy link

netlify bot commented Aug 6, 2024

Deploy Preview for vcluster-docs canceled.

Built without sensitive environment variables

Name Link
🔨 Latest commit bdf6379
🔍 Latest deploy log https://app.netlify.com/sites/vcluster-docs/deploys/66d79501fda2620008733f62

@@ -89,7 +89,12 @@ func ExecuteStart(ctx context.Context, options *StartOptions) error {
// set features for plugins to recognize
plugin.DefaultManager.SetProFeatures(pro.LicenseFeatures())

// check if we should create certs
// connect to vCluster platform if configured
startPlatformServersAndControllers, err := pro.ConnectToPlatform(ctx, vConfig)
Copy link
Member

@FabianKramm FabianKramm Aug 8, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not a super fan of returning a function, but its okay I guess

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FabianKramm I was not so sure about this myself. The reason why I chose to do it is because the start controller logic is highly dependent on dependencies created above it. Decoupling them would mean returning a handful of variables that I do not think makes sense from a reader's perspective, nor are they useful to other functions. It would also mean introducing a new pro function that I do not think is necessary. There are other solutions but I'm not convinced they are nicer than this. The server/controllers are related to ConnectToPlatform at least.

Another solution that I did consider was starting it in a go routine within ConnectToPlatform but blocking it with an empty channel, waitgroup, or some other sync mechanism, and then returning that. Do you think that would be better?

pkg/pro/platform.go Outdated Show resolved Hide resolved
@rmweir rmweir force-pushed the external-db-shared branch 2 times, most recently from 72168bd to 27388b6 Compare August 9, 2024 03:27
pkg/pro/platform.go Outdated Show resolved Hide resolved
vCluster is now able to request a data source from vCluster Platform.
vCluster is only able to use the data source if it attempts to connect
to the vCluster Platform prior to deploying its own control plane.
vCluster Platform is now connected before control plane is deployed
but the platform controllers and tailscale server are started after
because they require the control plane to be running.
@rmweir rmweir force-pushed the external-db-shared branch 2 times, most recently from f6f7a79 to bdf6379 Compare September 3, 2024 23:00
@FabianKramm FabianKramm merged commit 00bd813 into loft-sh:main Sep 4, 2024
61 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants