-
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
Change pulumi refresh
to report diff relative to desired state instead of relative to only output changes
#16146
base: master
Are you sure you want to change the base?
Conversation
f3b983d
to
9cc0f2c
Compare
Changelog[uncommitted] (2024-06-07)Features
|
// * newInputs where oldInputs are expected | ||
// * newOutputs where oldOutputs are expected | ||
// * oldInputs where newInputs are expected | ||
diff, err := diffResource(s.new.URN, s.new.ID, s.new.Inputs, s.new.Outputs, s.old.Inputs, prov, preview, |
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.
Assuming this is going to call Diff(), I love that because that automatically engages bridge machinery for planning changes suppressing changes and computing detailed diffs, that is going to be very helpful to run that. The downside is that new inputs returned from Read() are not check-clean and Diff expects these to be checked clean inputs so this may uncover some latent issues in bridged providers when rolled out.
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.
new inputs returned from Read() are not check-clean
I think the implied requriement even before these changes is that they are, as they have always been stored directly into the state file, and only "check-clean" inputs are allowed to be stored in state. And that's a reasonable requirement, because these are inputs provided directly by a provider, which is capable of ensuring anything it returns is check-clean. Check is only needed to take an arbitrary bag of inputs from a users and turn it into something the provider knows is valid.
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.
Agreed that's reasonable. I was worried this will promote pulumi/pulumi-aws#2318 to a refresh issue but it's empirically not the case, works the same as before. So that's very good.
Doing another pass here, really 👍 on this, "Changes to output-only properties are no longer shown as refresh diffs" is very valuable as well. This accomplishes most of the benefit of #16072 without running the program on |
d266a15
to
dc8439b
Compare
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.
A few notes:
- Ideally we would offer a way to opt-out of this via env var. The changes are coupled across (a) RefreshStep in the engine (b) diff renderer (c) progress renderer. We would need to incorporate the env var into all three places and maintain old and new behaviour in both. But I think we likely do need to do this.
- I am nervous about the wide variety of allowed Diff behaviours, and how few of them are tested generally here, especially relative to the true Diff behaviours in providers. Ideally we would have tf-bridge-like, kubernetes-like, and az-native-like diff implementations we could test against here to validate and baseline refresh (and many other features) against.
@@ -435,19 +435,14 @@ func (data *resourceRowData) getInfoColumn() string { | |||
} | |||
|
|||
func getDiffInfo(step engine.StepEventMetadata, action apitype.UpdateKind) string { | |||
diffOutputs := action == apitype.RefreshUpdate |
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.
We are no longer treating a refresh as an "output diff". It is now a diff of inferred inputs, preferably from the detailedDiff
computed by the step.
} | ||
|
||
// If this is the output step for an import or refresh, we actually want to display the diff at this point. | ||
if payload.Metadata.Op == deploy.OpImport || (refresh && payload.Metadata.Op == deploy.OpUpdate) { |
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.
For refresh, we now render a normal input diff but at ResourceOutputsEvent time (because we don't know the diff until we run the step). Very similar to Import. However, the step gets rewritten into a "OpUpdate" if there are changes, so look at that.
I believe it is right to ignore the OpSame and OpDelete cases that an OpRefresh could also be rewritten to - but it's possible there are cases where those should also go through this path?
var yes bool | ||
|
||
cmd := &cobra.Command{ | ||
Use: "set-ignore-changes [resource URN] --path [property-path-1] ...", |
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 honestly wish we had a more general command or set of commands for modifying components of state directly like this, and didn't need to invent a new mechanism here. Open to feedback on the CLI UX here - it's not particularly "nice", but seems as well aligned as possible with surrounding CLI UX and with being able to technically express all use cases.
<{%fg 2%}> + foo: <{%reset%}><{%fg 2%}>"bar"<{%reset%}><{%fg 2%}> | ||
<{%reset%}><{%fg 13%}><{%bold%}>Resources:<{%reset%}> | ||
<{%fg 3%}>~ 1 updated<{%reset%}> | ||
1 unchanged |
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.
This and other similar changes are intentional - the only change is in outputs so it doesn't report as a diff.
Note that these tests do the equivalent of --show-sames
, so they actually do show the diff for the sames, which wouldn't be "normal" in practice.
|
||
// ResultOp returns the operation that corresponds to the change to this resource after reading its current state, if | ||
// any. | ||
func (s *RefreshStep) ResultOp() display.StepOp { | ||
if s.new == nil { | ||
return OpDelete | ||
} | ||
if s.new == s.old || s.old.Outputs.Diff(s.new.Outputs) == nil { | ||
if s.new == s.old || s.diff.Changes == plugin.DiffNone { |
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'm not 100% certain how DiffUnknown
should be treated here.
Curious if we expect to land this anytime soon? @VenelinMartinov and I found a root cause of a very specific TF bug that uses old 2019 normalizeNullValues code to replace empty maps with null that currently causes a lot of dirty refreshes in both TF and stock Pulumi, that is alleviated by this change. We're evaluating tailoring a workaround in the bridge, but if we are able to land this change it might be not worth it? |
Makes two changes to the handling of ignoreChanges: 1. Stores the ignoreChanges configuration for each resource in the state file 2. Applies the ignoreChanges configuration from the state file as part of processing a refresh This allows users to use `ignoreChanges` to prevent refresh diffs being reported when changes happen in the cloud provider that they do not want to bring back into their Pulumi state. This can be used to acknowledge that part or all of a resource may change in the cloud provider, but the user is okay with this and doesn't want to be notified about it during refresh. Importantly, this means that the diff won't be reported, but also that the changes won't be applied to state. The ignoreChanges option when applied to a refresh operation applies to *both* input and output properties of the resource. This is different from the behavior during a normal update operation, where ignoreChanges only applies to input properties. The special "*" property path ignores all properties, but also ignores the case where the resource is no longer found in the cloud, and instead keeps the resource with all of it's existing input and output properties. Because the program is not run for refresh operations, users must apply the `ignoreChanges` via a `pulumi up` first, and then future `pulumi refresh` or `pulumi refresh --preview-only` oeprations will ignore the specificd changes. This is unfortunate, but the only option we have unless/until Pulumi fundamentally changes it's approach to refresh and decides to run the program as part of computing the resource state (and options) to use during the refresh.
Change refresh steps to compute a diff similar to what would happen if a `preview` were run immediately after the refresh using the inputs from the program, but inverted. This doesn't change what is stored back into state, but does produce a diff that is more aligned with true "change to desired state". This fixes several corner cases that are unfortuante in the prior implementation: 1. Changes to output-only proeprties - like `etag`s - are no longer shown as refresh diffs (but are updated in the state outputs). 2. IgnoreChanges now works to ignore property additions or changes (the changes are still updated in state so that future updates don't overwrite them again). TODO: - [ ] The diff display isn't updated yet - only the progres row display. - [ ] Tests
The provider previously triggers replaces on every update, because it replaces on `prefix` but doesn't store it into outputs. This *also* triggers the new refresh logic to report an update as part of the refresh, but it seems it is indeed just "broken" even outside of refresh, we just weren't testing multiple consecutive updates to these resources previously.
We now need to render the diff relative to the inputs instead of presenting an "output diff".
Enables setting the `ignoreChanges` resource option on a resource directly in the state. This is important for use cases where `ignoreChanges` must be applied ahead of a `refresh` operation, without being able to do a `pulumi up` in between to add the resource option via code.
Yes! @lunaris is picking up taking this over the finish line. 🙌 |
7a08bb8
to
db946da
Compare
I always found the current refresh behavior quite useful, especially for keeping an eye on complex resources that get changed by other systems outside of pulumi. So an option to keep the current behavior would be much appreciated. |
This changes
refresh
steps to compute a diff similar to what would happen if apreview
were run immediately after the refresh using the inputs from the program, but inverted.This doesn't change what is stored back into state, but does produce a diff that is more aligned with true "change to desired state".
This fixes several corner cases that are unfortunate in the prior implementation:
etag
s - are no longer shown as refresh diffs (but are updated in the state outputs).We also now store the
ignoreChanges
resource option into the state file, so that it is visible during a refresh. Unfortunately, applying newignoreChanges
requires anupdate
, which is awkward but unavoidable whilerefresh
doesn't run the program. We should add a state edit command to directly modify this in the CLI.Notes:
diff: -tags,tagsAll~tags,tagsAll
#16144 affects some of these cases - though its technically orthogonalignoreChanges
passed to Diff, so the ability to ignore changes on refresh doesn't currently work for Azure Native.Fixes #16072.
Note quite #12346, but likely replaces the need for that.