Change pulumi refresh to report diff relative to desired state instead of relative to only output changes
#16146
Conversation
lukehoban
force-pushed
the
lukehoban/ignoreChangesRefresh
branch
from
f3b983d
to
9cc0f2c
Compare
Changelog[uncommitted] (2024-06-12)Features
|
pkg/resource/deploy/step.go
Outdated
| // * 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
2 times, most recently
from
d266a15
to
dc8439b
Compare
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.
| 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.
pkg/engine/lifecycletest/testdata/output/TestCanceledRefresh/diff.stdout.txt
Show resolved
Hide resolved
pkg/resource/deploy/step.go
Outdated
|
|
||
| // 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.
|
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? |
Yes! @lunaris is picking up taking this over the finish line. 🙌 |
lunaris
force-pushed
the
lukehoban/ignoreChangesRefresh
branch
from
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. |
# Description There are a number of parts of the deployment process that require context about and configuration for the operation being executed. For instance: * Source evaluation -- evaluating programs in order to emit resource registrations * Step generation -- processing resource registrations in order to generate steps (create this, update that, delete the other, etc.) * Step execution -- executing steps in order to action a deployment. Presently, these pieces all take some form of `Options` struct or pass explicit arguments. This is problematic for a couple of reasons: * It could be possible for different parts of the codebase to end up operating in different contexts/with different configurations, whether due to different values being passed explicitly or due to missed copying/instantiation. * Some parts need less context/configuration than others, but still accept full `Options`, making it hard to discern what information is actually necessary in any given part of the process. This commit attempts to clean things up by moving deployment options directly into the `Deployment` itself. Since step generation and execution already refer to a `Deployment`, they get a consistent view of the options for free. For source evaluation, we introduce an `EvalSourceOptions` struct for configuring just the options necessary there. At the top level, the engine configures a single set of options to flow through the deployment steps later on. As part of this work, a few other things have been changed: * Preview/dry-run parameters have been incorporated into options. This lets up lop off another argument and mitigate a bit of "boolean blindness". We don't appear to flip this flag within a deployment process (indeed, all options seem to be immutable) and so having it as a separate flag doesn't seem to buy us anything. * Several methods representing parts of the deployment process have lost arguments in favour of state that is already being carried on (or can be carried on) their receiver. For instance, `deployment.run` no longer takes actions or preview configuration. While doing so means that a `deployment` could be run multiple times with different actions/preview arguments, we don't currently exploit this fact anywhere, so moving this state to the point of construction both simplifies things considerably and reduces the possibility for error (e.g. passing different values of `preview` when instantiating a `deployment` and subsequently calling `run`). * Event handlers have been split out of the options object and attached to `Deployment` separately. This means we can talk about options at a higher level without having to `nil` out/worry about this field and mutate it correctly later on. * Options are no longer mutated during deployment. Presently there appears to be only one case of this -- when handling `ContinueOnError` in the presence of `IgnoreChanges` (e.g. when performing a refresh). This case has been refactored so that the mutation is no longer necessary. # Notes * This change is in preparation for #16146, where we'd like to add an environment variable to control behaviour and having a single unified `Options` struct would make it easier to pass this configuration down with introducing (more) global state into deployments. Indeed, this change should make it easier to factor global state into `Options` so that it can be controlled and tested more easily/is less susceptible to bugs, race conditions, etc. * I've tweaked/extended some comments while I'm here and have learned things the hard way (e.g. `Refresh` vs `isRefresh`). Feedback welcome on this if we'd rather not conflate. * This change does mean that if in future we wanted e.g. to be able to run a `Deployment` in multiple different ways with multiple sets of actions, we'd have to refactor. Pushing state to the point of object construction reduces the flexibility of the code. However, since we are not presently using that flexibility (nor is there an obvious [to my mind] use case in the near future), this seems like a good trade-off to guard against bugs/make it simpler to move that state around. * I've left some other review comments in the code around questions/changes that might be a bad idea; happy to receive feedback on it all though!
lunaris
force-pushed
the
lukehoban/ignoreChangesRefresh
branch
4 times, most recently
from
5163342
to
6f6a1e3
Compare
|
@bernhardloos I've added an environment variable |
lunaris
force-pushed
the
lukehoban/ignoreChangesRefresh
branch
from
6f6a1e3
to
7e05c70
Compare
lunaris
force-pushed
the
lukehoban/ignoreChangesRefresh
branch
2 times, most recently
from
4dfa899
to
1ef9018
Compare
Presently, the behaviour of diffing during refresh steps is incomplete,
returning only an "output diff" that presents the changes in outputs.
This commit changes refresh steps so that:
* they compute a diff similar to the one that would be computed if a
`preview` were run immediately after the refresh, which is more
typically what users expect and want; and
* `IgnoreChanges` resource options are respected when performing the new
desired-state diffs, so that property additions or changes reported by
a refresh can be ignored.
In particular, `IgnoreChanges` can now be used to acknowledge that part
or all of a resource may change in the provider, but the user is OK with
this and doesn't want to be notified about it during a refresh.
Importantly, this means that the diff won't be reported, but also that
the changes won't be applied to state.
The implementation covers the following:
* A diff is computed using the inputs from the program and then
inverting the result, since in the case of a refresh the diff is being
driven by the provider side and not the program. This doesn't change
what is stored back into the state, but it does produce a diff that is
more aligned with the "true changes to the desired state".
* `IgnoreChanges` resource options are now stored in state, so that this
information can be used in refresh operations that do not have access
to/run the program.
* In the context of a refresh operation, `IgnoreChanges` applies to
*both* input and output properties. This differs from the behaviour of
a normal update operation, where `IgnoreChanges` only considers input
properties.
* The special `"*"` value for `IgnoreChanges` can be used to ignore all
properties. It _also_ ignores the case where the resource cannot be
found in the provider, and instead keeps the resource intact in state
with its existing input and output properties.
Because the program is not run for refresh operations, `IgnoreChanges`
options must be applied separately before a refresh takes place. This
can be accomplished using e.g. a `pulumi up` that applies the options
prior to a refresh. In future we could introduce a `pulumi state set
...` command or the like to modify `IgnoreChanges` resource options in
state directly, but this commit does not address this.
For use cases relying on the legacy refresh diff provider, the
`PULUMI_USE_LEGACY_REFRESH_DIFF` environment variable can be set, which
will disable desired-state diff computation. We only need to perform
checks in `RefreshStep.{ResultOp,Apply}`, since downstream code will
work correctly based on the presence or absence of a `DetailedDiff` in
the step.
Co-authored-by: Luke Hoban <lukehoban@gmail.com>
lunaris
force-pushed
the
lukehoban/ignoreChangesRefresh
branch
from
1ef9018
to
8f96c80
Compare
This is a great point. As well as adding some to test the flag going forward, I've since gone back and ran the lifecycle tests from |
Presently, the behaviour of diffing during refresh steps is incomplete, returning only an "output diff" that presents the changes in outputs. This commit changes refresh steps so that:
previewwere run immediately after the refresh, which is more typically what users expect and want; andIgnoreChangesresource options are respected when performing the new desired-state diffs, so that property additions or changes reported by a refresh can be ignored.In particular,
IgnoreChangescan now be used to acknowledge that part or all of a resource may change in the provider, but the user is OK with this and doesn't want to be notified about it during a refresh. Importantly, this means that the diff won't be reported, but also that the changes won't be applied to state.The implementation covers the following:
IgnoreChangesresource options are now stored in state, so that this information can be used in refresh operations that do not have access to/run the program.IgnoreChangesapplies to both input and output properties. This differs from the behaviour of a normal update operation, whereIgnoreChangesonly considers input properties."*"value forIgnoreChangescan be used to ignore all properties. It also ignores the case where the resource cannot be found in the provider, and instead keeps the resource intact in state with its existing input and output properties.Because the program is not run for refresh operations,
IgnoreChangesoptions must be applied separately before a refresh takes place. This can be accomplished using e.g. apulumi upthat applies the options prior to a refresh. We should investigate perhaps providing apulumi state set ...-like CLI to make these sorts of changes directly to a state.For use cases relying on the legacy refresh diff provider, the
PULUMI_USE_LEGACY_REFRESH_DIFFenvironment variable can be set, which will disable desired-state diff computation. We only need to perform checks inRefreshStep.{ResultOp,Apply}, since downstream code will work correctly based on the presence or absence of aDetailedDiffin the step.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
pulumi refreshwith json output is not showing any diff #16278