-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
implement programmatic default providers in the engine and Go SDK #16105
base: master
Are you sure you want to change the base?
Conversation
Implement default providers in the engine and the Go SDK first (Go SDK for no other reason other than I'm most familiar with that language). The SDK sends a RegisterDefaultProvider request to the engine, which adds the provider to its default provider map, and explicitly disallows creating an implicit default provider going forward. The locking around this happens on the SDK side, since we need to disallow RegisterResource requests happening at the same time as DefaultProvider requests. Here we can simply take a read lock before creating the RegisterResource goroutine, and a write lock for the duration of the RegisterDefaultProvider call. This way we can have RegisterResource calls happen in parallel, but the RegisterDefaultProvider call will not go ahead before the read lock is released. Similarly, the next RegisterResource call will be locked until the RegisterDefaultProvider call has finished.
Changelog[uncommitted] (2024-05-06)Features
|
pkg/resource/deploy/source_eval.go
Outdated
pluginDownloadURL = "-" + pluginDownloadURL | ||
} | ||
key := pkg + version + pluginDownloadURL | ||
// TODO: this is a bit awkward with versioning. E.g. should we also set `d.providers[pkg] = ref` here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a critical aspect. I observe that the default provider may or may not have a version component, depending on which SDK is in use. I would argue that there should be one default provider per package, not per package+version.
See related: #16109
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it needs to be per package+version. Since users can pass in the version on resource registration, the following would not work otherwise:
_, err = random.NewRandomPet(ctx, "example", nil, pulumi.Version("4.2.0"))
if err != nil {
return err
}
_, err = random.NewRandomPet(ctx, "example2", nil, pulumi.Version("4.3.0"))
if err != nil {
return err
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would argue that there should be one default provider per package
Thomas is correct, this definitely needs to be per pkg+version. The fixes in #16109 remove the "unversioned" default provider, and that might be ok as well, need to check it doesn't break anything odd backcompat wise.
proto/pulumi/resource.proto
Outdated
@@ -238,3 +240,9 @@ message TransformResponse { | |||
google.protobuf.Struct properties = 1; // the transformed input properties. | |||
TransformResourceOptions options = 2; // the options for the resource. | |||
} | |||
|
|||
message RegisterDefaultProviderRequest { | |||
string provider = 1; // the provider to register as the default provider. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is provider
a urn, can the comment say that?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be a 'ref' of the form "::"
proto/pulumi/resource.proto
Outdated
@@ -238,3 +240,9 @@ message TransformResponse { | |||
google.protobuf.Struct properties = 1; // the transformed input properties. | |||
TransformResourceOptions options = 2; // the options for the resource. | |||
} | |||
|
|||
message RegisterDefaultProviderRequest { | |||
string provider = 1; // the provider to register as the default provider. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be a 'ref' of the form "::"
proto/pulumi/resource.proto
Outdated
|
||
message RegisterDefaultProviderRequest { | ||
string provider = 1; // the provider to register as the default provider. | ||
string version = 2; // the version of the provider to register as the default provider. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is needed? The engine can work everything out from the provider instance it has in memory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a couple of problems here that I was trying to solve with adding the version
and pluginDownloadURL
fields:
- I don't think the provider in the engine saves the
pluginDownloadURL
anywhere, so we'll have to pass that along, as it's part of the defaultprovider map key - Once the provider is constructed in the engine, it will always have a version. So there's no way to tell whether it was constructed with or without a version in the SDK. Now this is very closely related to the TODO Eron also commented on implement programmatic default providers in the engine and Go SDK #16105 (comment). If we decide that we should at least also always set the default provider for where the key is just
pkg
, I don't think we need to send the version along here, as we can just setd.providers[pkg]
andd.providers[pkg-<version>]
. Without sending the version field along here we would only setd.providers[pkg-<version>]
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think the provider in the engine saves the pluginDownloadURL anywhere, so we'll have to pass that along, as it's part of the defaultprovider map key
It should be on the provider input state.
If we decide that we should at least also always set the default provider for where the key is just pkg
We shouldn't! But if you get given a "ref" that points to a specific provider which will have a specific version (again on its input state). We're just going to be using that provider instance as the default for it's matching pkg-version.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, we'll need #16109 for that, but then I think it works.
Since |
They are. We do allow side-by-side versions here, and I think that's the right thing to do as well. We only disallow the creation of new internally created default providers with different versions than the programmatically set default providers. The idea is that otherwise it's easy for users to set a default providers, and then maybe pass a |
bfa1c22
to
194bc94
Compare
Implement default providers in the engine and the Go SDK first (Go SDK for no other reason other than I'm most familiar with that language).
The SDK sends a RegisterDefaultProvider request to the engine, which adds the provider to its default provider map, and explicitly disallows creating an implicit default provider going forward.
The locking around this happens on the SDK side, since we need to disallow RegisterResource requests happening at the same time as DefaultProvider requests. Here we can simply take a read lock before creating the RegisterResource goroutine, and a write lock for the duration of the RegisterDefaultProvider call.
This way we can have RegisterResource calls happen in parallel, but the RegisterDefaultProvider call will not go ahead before the read lock is released. Similarly, the next RegisterResource call will be locked until the RegisterDefaultProvider call has finished.
/xref #2059
This could benefit from a conformance test, however we do not have them for Go yet, and making them work also means implementing the changes in the Python and NodeJS SDKs. I was planning to do that in a separate PR to split the changes up a little bit, but can add more commits here if that's preferred.
Checklist
make tidy
to update any new dependenciesmake lint
to verify my code passes the lint checkgofumpt
make changelog
and committed thechangelog/pending/<file>
documenting my change