Handle ignoreChanges for Server-Side Apply mode #2074

lblackstone · 2022-07-13T21:46:50Z

Proposed changes

Related issues (optional)

Fix #2012

lblackstone · 2022-07-13T21:48:13Z

Note: I tagged @pulumi/platform in case anyone has suggestions for a better way to do this now. This implementation seems to work, but I wasn't sure if there was a function somewhere that handles this.

github-actions · 2022-07-13T21:51:17Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

viveklak · 2022-07-13T21:57:32Z

provider/pkg/provider/provider.go

+			if err != nil {
+				return nil, fmt.Errorf("failed to parse ignore path: %w", err)
+			}
+			for p := range detailedDiff {


Should we flip the loops so we go over detailedDiff at the top level and look through ignoreChanges in the nested loop? I assume ignore changes is a typically smaller slice?

Curious why we need to do this? I haven't looked through engine code here but I would expect the engine to ignore relevant things from the diff response on the engine side instead? cc @Frassle for comment.

@viveklak There's some additional context in #2012 (comment)

Comment posted from Unblocked.

I guess my question was if the engine should be doing the filtering of ignored changes given it has all the information it needs in order to perform the filtering on its side? Currently the provider has the opportunity to hand off diff to the engine with a DiffResponse_DIFF_UNKNOWN. Does this need to be as all-or-nothing as that?

I guess my question was if the engine should be doing the filtering of ignored changes given it has all the information it needs in order to perform the filtering on its side?

I'm not sure about this. With SSA, I'm not sure if the engine would always be operating on the same information as the provider.

Curious why we need to do this? I haven't looked through engine code here but I would expect the engine to ignore relevant things from the diff response on the engine side instead?

I remember being confused why there was an ignoreChanges passed to the provider at all when I first saw, thinking the same thing. @pgavlin added it in pulumi/pulumi#3005. The engine will have already reset the inputs map to any old values due to ignoreChanges, I'll admit I'm not sure why this is insufficient for tf/k8ssa 😕

https://pulumi.slack.com/archives/C011EMDQ8JE/p1649841058403479 slack thread where Pat tried to explain why it's needed.

So I'm not quite sure why this update is needed, but it's not working with the existing Diff code. The Update method isn't handling ignoreChanges at all, but it's also not doing any additional diff calculation, so perhaps it's deferring that work to the engine. Given the extra moving parts being introduced with Patch support, I'm hesitant to make major changes to the diff logic right now, so I think we should stick with this fix for the meantime.

github-actions · 2022-07-15T21:13:05Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

t0yv0 · 2022-07-18T13:45:28Z

tests/sdk/nodejs/server-side-apply/step3/index.ts

+
+import * as k8s from "@pulumi/kubernetes";
+
+// This test creates a Provider with `enableServerSideApply` enabled. The following scenarios are tested:


Thank you for the comments here! I wish we did this in all the tests that are otherwise not exactly readable as to their intent.

provider/pkg/provider/provider.go

t0yv0 · 2022-07-18T13:57:43Z

provider/pkg/provider/provider.go

+		}
+
+		if len(detailedDiff) > 0 {
+			for k := range patchObj {


I don't know this code very well but I wanted to ask if this change here is allright? This is now conditional. detailedDiff is now filtered by taking IgnoreChanges into account. The copying of patchObj to changes is now conditional. But it is not filtered. Is this OK? Consider:

IgnoreChanges is meaningful

detailedDiff got filtered, so delete(detailedDiff, diffPath.String()) fired a few times

remaining detailedDiff is non-empty; so len(detailedDiff) > 0

now changes becomes a copy of patchObj, including changes that are filtered out of detailedDiff?

Yes I think @t0yv0's observation is correct here? Seems like we should be scoping the keys to populate changes from detailedDiff instead (i.e. things that survived the pruning due to ignoreChanges).

Good points, thanks. I'll take another look at this logic today.

t0yv0

Looking good, have a few questions, not super familiar with this code though.

github-actions · 2022-07-18T14:45:09Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

github-actions · 2022-07-18T15:22:45Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

lblackstone · 2022-07-18T20:44:26Z

provider/pkg/provider/provider.go

@@ -1629,7 +1654,6 @@ func (k *kubeProvider) Diff(ctx context.Context, req *pulumirpc.DiffRequest) (*p
 		Replaces:            replaces,
 		Stables:             []string{},
 		DeleteBeforeReplace: deleteBeforeReplace,
-		Diffs:               changes,


Note: I got rid of the Diffs computation and am letting the engine handle this now. The DetailedDiff computation seems to be necessary to get an accurate diff with Server-side Apply. I tried setting DiffResponse_DIFF_UNKNOWN, but the result was incorrect.

Interesting... Could you add a comment here in code to document the same? (why diffs are omitted?)

FWIW, I think this might be a tiny compatibility break with some very old versions of the CLI (~v0.17.20, which was released in June 2019). I think that the only break would be in the display code, which may no longer render the list of changed properties. I think that this is acceptable.

github-actions · 2022-07-18T20:45:59Z

Does the PR have any schema changes?

Looking good! No breaking changes found.
No new resources/functions.

viveklak · 2022-07-18T21:33:26Z

provider/pkg/provider/provider.go

+			for _, ignorePath := range ignorePaths {
+				for _, diffPath := range diffPaths {
+					if ignorePath.Contains(diffPath) {
+						delete(detailedDiff, diffPath.String())


Note I have found some instances where resource.PropertyPath doesn't retain the format when calling .String(). I will have fixes for this as part of pulumi/pulumi#10095. Shouldn't affect this change as long as we pull in fixes after the above lands. FYI.

lblackstone requested review from viveklak and a team July 13, 2022 21:46

viveklak reviewed Jul 13, 2022

View reviewed changes

lblackstone added 3 commits July 17, 2022 18:21

Handle ignoreChanges for Server-Side Apply mode

e4bcbce

Avoid modifying map while iterating

482671e

Make a little more efficient

bd1e975

t0yv0 reviewed Jul 18, 2022

View reviewed changes

provider/pkg/provider/provider.go Outdated Show resolved Hide resolved

t0yv0 reviewed Jul 18, 2022

View reviewed changes

t0yv0 approved these changes Jul 18, 2022

View reviewed changes

lblackstone force-pushed the lblackstone/ignore-changes branch from a4aaa11 to bd1e975 Compare July 18, 2022 14:35

Fixes

112f3a6

lblackstone commented Jul 18, 2022

View reviewed changes

viveklak approved these changes Jul 18, 2022

View reviewed changes

viveklak reviewed Jul 18, 2022

View reviewed changes

lblackstone merged commit bd99d70 into master Jul 18, 2022

pulumi-bot deleted the lblackstone/ignore-changes branch July 18, 2022 21:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle ignoreChanges for Server-Side Apply mode #2074

Handle ignoreChanges for Server-Side Apply mode #2074

lblackstone commented Jul 13, 2022 •

edited

Loading

lblackstone commented Jul 13, 2022

github-actions bot commented Jul 13, 2022

viveklak Jul 13, 2022 •

edited

Loading

viveklak Jul 13, 2022 •

edited

Loading

lblackstone Jul 13, 2022

viveklak Jul 13, 2022 •

edited

Loading

lblackstone Jul 14, 2022

Frassle Jul 14, 2022

Frassle Jul 14, 2022

lblackstone Jul 15, 2022

github-actions bot commented Jul 15, 2022

t0yv0 Jul 18, 2022

t0yv0 Jul 18, 2022

viveklak Jul 18, 2022

lblackstone Jul 18, 2022

t0yv0 left a comment

github-actions bot commented Jul 18, 2022

github-actions bot commented Jul 18, 2022

lblackstone Jul 18, 2022

viveklak Jul 18, 2022

pgavlin Jul 18, 2022

github-actions bot commented Jul 18, 2022

viveklak Jul 18, 2022 •

edited

Loading


		import * as k8s from "@pulumi/kubernetes";

		// This test creates a Provider with `enableServerSideApply` enabled. The following scenarios are tested:

Handle ignoreChanges for Server-Side Apply mode #2074

Handle ignoreChanges for Server-Side Apply mode #2074

Conversation

lblackstone commented Jul 13, 2022 • edited Loading

Proposed changes

Related issues (optional)

lblackstone commented Jul 13, 2022

github-actions bot commented Jul 13, 2022

Does the PR have any schema changes?

viveklak Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

viveklak Jul 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Comment posted from Unblocked.

viveklak Jul 13, 2022 • edited Loading

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

github-actions bot commented Jul 15, 2022

Does the PR have any schema changes?

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

github-actions bot commented Jul 18, 2022

Does the PR have any schema changes?

github-actions bot commented Jul 18, 2022

Does the PR have any schema changes?

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jul 18, 2022

Does the PR have any schema changes?

viveklak Jul 18, 2022 • edited Loading

Choose a reason for hiding this comment

lblackstone commented Jul 13, 2022 •

edited

Loading

viveklak Jul 13, 2022 •

edited

Loading

viveklak Jul 13, 2022 •

edited

Loading

viveklak Jul 13, 2022 •

edited

Loading

viveklak Jul 18, 2022 •

edited

Loading