Preview of update plans #8448

Frassle · 2021-11-18T12:42:28Z

Description

This PR adds the concept of "update plans" to pulumi. Plans can be generated via a preview command with the extra argument "--save-plan=<file_path>.json". This will run the preview and save the set of expected operations to the given JSON file. The user can then run update with "--plan=<file_path>.json" to run a constrained update. This will only allow operations that are either recorded in the plan file or are deemed a safe subset of those operations.

Some examples of safe subsets:

If a plan file says nothing about resource R but the update doesn't want to change anything about resource R this is not a constraint error.
If a plan says resource R should be updated to value X, but at update time it's already at value X this is not a constraint error.

Using this feature requires the environment variable PULUMI_EXPERIMENTAL to be true.

Fixes #2318

Checklist

I have added tests that prove my fix is effective or that my feature works

I have updated the CHANGELOG-PENDING file with my change

Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

Todo

Auto refresh in update if --constraint-file is given
Work out what to do about auto preview in update if --constraint-file is given

sdk/go/common/apitype/plan.go

sdk/go/common/resource/properties_diff.go

t0yv0 · 2022-01-21T16:03:18Z

pkg/resource/deploy/plan.go

+	for _, e := range os.Environ() {
+		// golang has so many questionable design decisions, lets just hope no one puts '=' in there env var names.
+		pair := strings.SplitN(e, "=", 2)
+		environmentVariables[pair[0]] = pair[1]


There was a comment elsewhere these were encrypted (at the struct property), but there's no encryption I can see. So passwords from env vars would go into EnvironmentVariables prop. Should we change the comments here?

config values are encrypted/decrypted as used. env vars are encrypted when we write them out. We could change this so we pass a crypter in to NewPlan and just encrypt the envvars straight away.

Just curious whether or not we need the envvars? More information is probably good, but I might leave them out of the preview if we're not yet sure how we'll use them.

They were in the prototype branch and I just kept them, I'm not sure if we would ever do anything with them. There's so many other ways users could cause their programs to be non-deterministic.

pkg/resource/deploy/plan.go

t0yv0 · 2022-01-21T16:04:36Z

pkg/resource/deploy/plan.go

+	// the resource's checked input properties we expect to change.
+	InputDiff PlanDiff
+	// the resource's output properties we expect to change (only set for RegisterResourceOutputs)
+	OutputDiff *PlanDiff


*PlanDiff intentional vs PlanDiff? So for non RegisterResourceOutputs it's nil?

t0yv0 · 2022-01-21T16:06:25Z

pkg/resource/deploy/plan.go

+	Outputs resource.PropertyMap
+}
+
+func (rp *ResourcePlan) diffURNs(a, b []resource.URN) (message string, changed bool) {


Are these sorted/deduped, that is are these [] slices actually representing sets? Should we be defensive and sort/dedup here?

Yes they are sets that's why we make the two string maps and then diff that (see inside diffStringSets)

t0yv0 · 2022-01-21T16:07:17Z

pkg/resource/deploy/plan.go

+	return rp.diffStrings(stringsA, stringsB)
+}
+
+func (rp *ResourcePlan) diffStrings(a, b []string) (message string, changed bool) {


Ah ok set logic here. "diffStringSets"? perhaps a more pointed name.

t0yv0 · 2022-01-21T16:09:16Z

pkg/resource/deploy/plan.go

+	return nil
+}
+
+func (rp *ResourcePlan) checkGoal(


This code is written with early termination semantic (return first error), would it be sometimes helpful to collect and present all errors or it does not matter?

Probably, need to think about how best to display large error sets to the user. I think this will be ok for preview release.

t0yv0

Finished reading through - LGTM, added a few comments/ questions. My familiarity with some of the code is a bit light, esp around stateful struct updates + flow through the system. I believe @pgavlin was intending to do a review pass here also.

pkg/cmd/pulumi/preview.go

pkg/cmd/pulumi/util.go

pgavlin · 2022-01-26T18:29:42Z

pkg/resource/deploy/deployment_executor.go

+					// We haven't done a delete for this resource check if it was in the snapshot,
+					// if it's already gone this wasn't done because it wasn't needed


How does this scenario arise?

If you planned to delete a resource, then someone else deletes it, then you run your plan. End result is the same, the resource is deleted so it didn't feel like it should be an error case.

pgavlin · 2022-01-26T18:31:53Z

pkg/resource/deploy/plan.go

+	for _, e := range os.Environ() {
+		// golang has so many questionable design decisions, lets just hope no one puts '=' in there env var names.
+		pair := strings.SplitN(e, "=", 2)
+		environmentVariables[pair[0]] = pair[1]


Just curious whether or not we need the envvars? More information is probably good, but I might leave them out of the preview if we're not yet sure how we'll use them.

pgavlin · 2022-01-26T18:52:49Z

pkg/resource/deploy/plan.go

+// PlanDiff holds the results of diffing two object property maps.
+type PlanDiff struct {
+	Adds    resource.PropertyMap   // the resource's properties we expect to add.
+	Deletes []resource.PropertyKey // the resource's properties we expect to delete.
+	Updates resource.PropertyMap   // the resource's properties we expect to update.
+}


I'm a little surprised by the way we're storing diffs.

I think that using the inputs like this imposes a very strong requirement that all changes are visible via the inputs--i.e. there are no input vs. state differences. This is different from the model used by providers, where diffs can be observed either from input <-> input or from input <-> state. An alternative approach would be to store the detailed diff as returned by the provider (polyfilling a detailed diff where necessary), then checking that the detailed diff from the update is constrained to the detailed diff from the preview, with old values sourced from the old inputs or old state as directed by the detailed diff.

I think that what is implemented might be okay assuming that in the vast majority of cases all input <-> state differences are essentially input <-> input differences b/c the differing property has the same value in the state and the inputs (though that assumption requires that the provider's Read method can accurately reflect values from the state back to the inputs).

I was also a little surprised that we're storing a flat list of top-level diffs rather than the tree diff (ala resource.ValueDiff). Is there a benefit to the flat list? Storing the tree diff might make error presentation a little more precise.

I think that using the inputs like this imposes a very strong requirement that all changes are visible via the inputs

What would an input state difference be? I'm not sure I follow how we could have a state difference and not also have an input difference?

Is there a benefit to the flat list?

Depends on what semantics we're going for. Like if you have a dict property and you run a plan to go from{a: 1, b: 2} to {a: 1, c: 3} is that recording "update to {a: 1, c: 3}" or "delete key b, add key c with 3"? Because the former only allows a plan to execute if it ends up with the value {a: 1, c: 3} but the later would allow someone to come and change the dict to something like {b: 2, d: 4} and the plan would allow the value {c: 3, d: 4} but not allow the value {a: 1, c: 3}.

Might be that second semantic is useful but it seemed more complex to code and explain and I don't have a concrete scenario that would use it so I err'ed on simplicity.

What would an input state difference be? I'm not sure I follow how we could have a state difference and not also have an input difference?

Consider a resource R with (inputs, state) => ({foo: bar}, {foo: bar}). If we refresh R and the provider returns ({foo: bar}, {foo: baz}), the provider is still allowed to indicate that R needs to be updated on a subsequent run even if the program presents the inputs {foo: bar} b/c foo has different properties in the inputs and the state. This is arguably a bug, and I'm not sure it happens in practice, but it is a "feature" of the model that we might need to consider.

More common is the case where an input has no corresponding entry in the state, so diffs are only detectable from input to input (e.g. k8s secrets).

the later would allow someone to come and change the dict to something like {b: 2, d: 4} and the plan would allow the value {c: 3, d: 4} but not allow the value {a: 1, c: 3}.

I think the latter would still allow {a: 1, c: 3} b/c the same on c is constrained to the add in the diff. I think the argument here is the same as for the analogous case at top level in that we want to allow the base state to differ as long as the diffs are compatible.

This is good feedback and it sounds like we need to give it a thought. Would it be okay to go ahead with this PR to kick off examples testing and do this work as a follow-up?

Yep, definitely. We can probably even ship the preview as-is and revisit this while we gather more data.

pgavlin and others added 30 commits October 29, 2021 08:42

Implement resource plans in the engine

33e8980

Plumb plans through the CLI.

e101f80

Update wording

13f8d34

plan renderer

bbfd825

constraints

2818c98

Renames

6de090c

Update message

908c3ae

fixes for rebase breaks and diffs

24a0eb0

WIP: outputs in plans

954bdc0

fix diff

365bfd2

fixup

90b7514

Merge remote-tracking branch 'origin/master' into ctpp

0d9043f

Liniting and test fixing

155c035

Test and fix PropertyPath.String()

1b59952

Fix colors

8fae881

Fix cmdutil.PrintTable to handle non-simple strings

83e2cd7

More tests

b1e23a5

Merge remote-tracking branch 'origin/master' into ctpp

b5eb955

Readd test_plan.go

fdea5bb

lint

5352638

Test expected deletes

505e4c5

Test expected delete

d995901

Test missing create

f4c2805

Fix test for missing creates

3e73c03

Merge remote-tracking branch 'origin/master' into ctpp

12c0188

rm Paths()

775ace8

property set shrink test

82222cc

notes

1db1ab5

More tests

c41981a

Pop op before constraint check

9865fa5

t0yv0 reviewed Jan 21, 2022

View reviewed changes

sdk/go/common/apitype/plan.go Show resolved Hide resolved

t0yv0 reviewed Jan 21, 2022

View reviewed changes

sdk/go/common/resource/properties_diff.go Outdated Show resolved Hide resolved

t0yv0 reviewed Jan 21, 2022

View reviewed changes

pkg/resource/deploy/plan.go Show resolved Hide resolved

t0yv0 reviewed Jan 21, 2022

View reviewed changes

t0yv0 self-requested a review January 21, 2022 16:09

t0yv0 approved these changes Jan 21, 2022

View reviewed changes

Frassle added 8 commits January 24, 2022 10:15

Add comment about check ops with failures

f1d7768

Fix CheckedInputs comment

3fe02f2

OutputDiff doesn't need to be a pointer

c6d2c77

Fix checks against computed

5177703

Merge remote-tracking branch 'origin/master' into ctpp

0a492a0

diffStringSets

007ca81

lint

43f5a20

lint pkg

3bc7d17

pgavlin reviewed Jan 26, 2022

View reviewed changes

Frassle added 9 commits January 27, 2022 12:00

Use 4 space indent

cf6218f

Don't wrap Buffer in Writer

a85e293

Mark flags hidden rather than disabled

b599bf1

Merge remote-tracking branch 'origin/master' into ctpp

95fb433

Remove envvars from plans

ad74025

Assert MarkHidden error

a028502

Merge remote-tracking branch 'origin/master' into ctpp

95cb5a8

Add to changelog

bc337f8

Note plan/save-plan is experimental

fd346bc

Frassle merged commit 40eee58 into master Jan 31, 2022

pulumi-bot deleted the fraser/planPrototype branch January 31, 2022 10:31

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Preview of update plans #8448

Preview of update plans #8448

Frassle commented Nov 18, 2021 •

edited

t0yv0 Jan 21, 2022

Frassle Jan 24, 2022

pgavlin Jan 26, 2022

Frassle Jan 26, 2022

t0yv0 Jan 21, 2022

t0yv0 Jan 21, 2022

Frassle Jan 24, 2022 •

edited

t0yv0 Jan 21, 2022

Frassle Jan 24, 2022

t0yv0 Jan 21, 2022

Frassle Jan 24, 2022

t0yv0 left a comment

pgavlin Jan 26, 2022

Frassle Jan 26, 2022

pgavlin Jan 26, 2022

pgavlin Jan 26, 2022

Frassle Jan 26, 2022

pgavlin Jan 26, 2022

mikhailshilkov Jan 27, 2022

pgavlin Jan 27, 2022

		// We haven't done a delete for this resource check if it was in the snapshot,
		// if it's already gone this wasn't done because it wasn't needed

Preview of update plans #8448

Preview of update plans #8448

Conversation

Frassle commented Nov 18, 2021 • edited

Description

Checklist

Todo

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frassle Jan 24, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

t0yv0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Frassle commented Nov 18, 2021 •

edited

Frassle Jan 24, 2022 •

edited