Change pulumi refresh to report diff relative to desired state instead of relative to only output changes
#16146
Conversation
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
lukehoban
force-pushed
the
lukehoban/ignoreChangesRefresh
branch
from
f3b983d
to
9cc0f2c
Compare
Changelog[uncommitted] (2024-05-20)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.
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.
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.
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 |
lukehoban
force-pushed
the
lukehoban/ignoreChangesRefresh
branch
from
26059f7
to
d266a15
Compare
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.
lukehoban
force-pushed
the
lukehoban/ignoreChangesRefresh
branch
from
d266a15
to
dc8439b
Compare
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.
There was a problem hiding this comment.
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.
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.
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.
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.
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.
I'm not 100% certain how DiffUnknown should be treated here.
This changes
refreshsteps to compute a diff similar to what would happen if apreviewwere 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:
etags - are no longer shown as refresh diffs (but are updated in the state outputs).We also now store the
ignoreChangesresource option into the state file, so that it is visible during a refresh. Unfortunately, applying newignoreChangesrequires anupdate, which is awkward but unavoidable whilerefreshdoesn'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 orthogonalignoreChangespassed to Diff, so the ability to ignore changes on refresh doesn't currently work for Azure Native.Fixes #16072.