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

Refine resource replacement logic for providers #2767

Merged
merged 9 commits into from
Jun 3, 2019

Conversation

swgillespie
Copy link
Contributor

This commit touches an intersection of a few different provider-oriented
features that combined to cause a particularly severe bug that made it
impossible for users to upgrade provider versions without seeing
replacements with their resources.

For some context, Pulumi models all providers as resources and places
them in the snapshot like any other resource. Every resource has a
reference to the provider that created it. If a Pulumi program does not
specify a particular provider to use when performing a resource
operation, the Pulumi engine injects one automatically; these are called
"default providers" and are the most common ways that users end up with
providers in their snapshot. Default providers can be identified by
their name, which is always prefixed with "default".

Recently, in an effort to make the Pulumi engine more flexible with
provider versions, it was made possible for the engine to have multiple
default providers active for a provider of a particular type, which was
previously not possible. Because a provider is identified as a tuple of
package name and version, it was difficult to find a name for these
duplicate default providers that did not cause additional problems. The
provider versioning PR gave these default providers a name that was
derived from the version of the package. This proved to be a problem,
because when users upgraded from one version of a package to another,
this changed the name of their default provider which in turn caused all
of their resources created using that provider (read: everything) to be
replaced.

To combat this, this PR introduces a rule that the engine will apply
when diffing a resource to determine whether or not it needs to be
replaced: "If a resource's provider changes, and both old and new
providers are default providers whose properties do not require
replacement, proceed as if there were no diff." This allows the engine
to gracefully recognize and recover when a resource's default provider changes
names, as long as the provider's config has not changed.

@swgillespie
Copy link
Contributor Author

Fixes #2753. I'm currently adding more tests and will update the changelog.

@@ -41,6 +41,11 @@ func IsProviderType(typ tokens.Type) bool {
return typ.Module() == "pulumi:providers" && typ.Name() != ""
}

// IsDefaultProvider returns true if this URN refers to a default Pulumi provider.
func IsDefaultProvider(urn resource.URN) bool {
return IsProviderType(urn.Type()) && strings.HasPrefix(urn.Name().String(), "default")
Copy link
Member

Choose a reason for hiding this comment

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

Do we have code somewhere to ban default as a prefix for user-instantiated providers?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't believe we do, for default or otherwise.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also do not believe we have such things. I think it would be reasonable for us to ban creating of first class providers which have names of this shape, but I would also be fine doing that later.

@lukehoban
Copy link
Member

It was a little surprising to me that we’re solving by changing the interpretation of the diff in the resources, instead of avoiding the replacement of the provider in the first place.

I had expected we would do something similar to the aliases changes but hard coded to these default providers, to allow a default-* provider without an old to implicitly have an old that is the default provider of the same type.

That seems like it limits this change to default providers, instead of impacting diff logic on all resources?

@swgillespie
Copy link
Contributor Author

I had expected we would do something similar to the aliases changes but hard coded to these default providers, to allow a default-* provider without an old to implicitly have an old that is the default provider of the same type.

As I worked through this PR in my head, I struggled to come up with an answer to "when does the engine conclusively determine that a provider resource aliases another, older provider"? We'd have to be doing a lot of the same logic that we're doing here: when a default provider comes in, we'd need to look for its corresponding alias (if one exists) - but it's hard to do that, since the best way to determine whether or not a provider is "the same" is by observing that it is replacing the Provider field on a resource.

aliases accomplishes the "diff" part of this problem, but getting to a point where we can construct an accurate alias map is not straightforward and I don't believe it's simpler than this approach.

@lukehoban
Copy link
Member

aliases accomplishes the "diff" part of this problem, but getting to a point where we can construct an accurate alias map is not straightforward and I don't believe it's simpler than this approach.

FWIW - I'm happy with this answer - and okay with the changes as-is if @ellismg and/or @pgavlin are happy with it.

@pgavlin pgavlin self-assigned this May 30, 2019
Copy link
Contributor

@ellismg ellismg left a comment

Choose a reason for hiding this comment

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

LGTM.

swgillespie and others added 6 commits May 31, 2019 15:30
This commit touches an intersection of a few different provider-oriented
features that combined to cause a particularly severe bug that made it
impossible for users to upgrade provider versions without seeing
replacements with their resources.

For some context, Pulumi models all providers as resources and places
them in the snapshot like any other resource. Every resource has a
reference to the provider that created it. If a Pulumi program does not
specify a particular provider to use when performing a resource
operation, the Pulumi engine injects one automatically; these are called
"default providers" and are the most common ways that users end up with
providers in their snapshot. Default providers can be identified by
their name, which is always prefixed with "default".

Recently, in an effort to make the Pulumi engine more flexible with
provider versions, it was made possible for the engine to have multiple
default providers active for a provider of a particular type, which was
previously not possible. Because a provider is identified as a tuple of
package name and version, it was difficult to find a name for these
duplicate default providers that did not cause additional problems. The
provider versioning PR gave these default providers a name that was
derived from the version of the package. This proved to be a problem,
because when users upgraded from one version of a package to another,
this changed the name of their default provider which in turn caused all
of their resources created using that provider (read: everything) to be
replaced.

To combat this, this PR introduces a rule that the engine will apply
when diffing a resource to determine whether or not it needs to be
replaced: "If a resource's provider changes, and both old and new
providers are default providers whose properties do not require
replacement, proceed as if there were no diff." This allows the engine
to gracefully recognize and recover when a resource's default provider changes
names, as long as the provider's config has not changed.
@pgavlin pgavlin force-pushed the swgillespie/provider-diff-v2 branch from 2df764c to 909625c Compare June 1, 2019 00:42
Copy link
Contributor

@ellismg ellismg left a comment

Choose a reason for hiding this comment

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

Pushed a commit to appease the linter on your behalf, @pgavlin. It might be nice to take a pass over some of the comments in the lifecycle test now that we create multiple resource across possible different versions of the default providers, but otherwise LGTM.


// This test simulates the upgrade scenario of old-style default providers to new-style versioned default providers.
//
// The first update creates a stack using a language host that does not report a version to the engine. As a result,
Copy link
Contributor

Choose a reason for hiding this comment

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

These comments are all slightly misleading after your refactoring to create two resources (with possible different versions) FWIW.

@ellismg
Copy link
Contributor

ellismg commented Jun 3, 2019

Merged master into this branch with 16ef1e0, @pgavlin and addressed the conflicts, but maybe you will want to take a look at the above commit before merging.

@pgavlin pgavlin merged commit 2870518 into master Jun 3, 2019
@pulumi-bot pulumi-bot deleted the swgillespie/provider-diff-v2 branch June 3, 2019 19:16
@lukehoban lukehoban mentioned this pull request Jun 5, 2019
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.

None yet

4 participants