Skip to content
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

Refresh reorders set elements causing spurious diffs #1904

Open
t0yv0 opened this issue Apr 25, 2024 · 6 comments
Open

Refresh reorders set elements causing spurious diffs #1904

t0yv0 opened this issue Apr 25, 2024 · 6 comments
Labels
kind/bug Some behavior is incorrect or out of spec

Comments

@t0yv0
Copy link
Member

t0yv0 commented Apr 25, 2024

What happened?

When servicing pulumi refresh the provider only gets to influence the result in a Read call, but the diff will be computed outside of the provider by the Pulumi CLI and the engine within; the engine does not understand sets and therefore deals with a list projection. The inputs are ordered arbitrarily according to program order, but the state read from the cloud might be (should be?) ordered by TF-canonical order (using Set function that gives an identity to each element). This makes Pulumi display spurious reordering diffs the are not visible in TF.

What we could do here:

  • protocol change that lets bridged provider participate in computing diffs for refresh; make that set-aware

  • without a protocol change we can do a pass over Read results that attempts to reorder set element projection to match the order from inputs as much as possible, that is intentionally minimize the spurious diffs within the current model; this is doable quickly I suggest we stat here

Example

A good example for Plugin Framework based resources is provided in pulumi/pulumi-aws#3303

I'm searching for more examples for SDKv2 based resources.

Output of pulumi about

See above.

Additional context

N/A

Contributing

Vote on this issue by adding a 👍 reaction.
To contribute a fix for this issue, leave a comment (and link to your pull request, if you've opened one already).

@t0yv0 t0yv0 added needs-triage Needs attention from the triage team kind/bug Some behavior is incorrect or out of spec labels Apr 25, 2024
@iwahbe iwahbe removed the needs-triage Needs attention from the triage team label Apr 26, 2024
@iwahbe
Copy link
Member

iwahbe commented Apr 26, 2024

Thanks for raising the issue. We will prioritize next iteration.

@t0yv0
Copy link
Member Author

t0yv0 commented Apr 26, 2024

Slightly more involved proposal that could also fix for this: pulumi/pulumi#16072 - in this proposal the state out of Read would be threaded through the program again, so the usual Diff machinery would kick in that handles sets already.

@t0yv0
Copy link
Member Author

t0yv0 commented May 10, 2024

This might change significantly with pulumi/pulumi#16146 that plumbs refresh results through Diff, though I've not verifed.

@t0yv0
Copy link
Member Author

t0yv0 commented May 10, 2024

@mikhailshilkov reminds of an issue pulumi/pulumi-gcp#1935 affecting import (not just refresh). Possibly same fix if Read could canonicalize the ordering of sets.

@guineveresaenger
Copy link
Contributor

pulumi/pulumi-gcp#1935 was fixed via pulumi/pulumi#16024.

@t0yv0
Copy link
Member Author

t0yv0 commented Jun 12, 2024

I'm not sure I agree with pulumi/pulumi#16024 being a fix for this. It's a workaround for Providers returning inconsistent data. Consider an example where the set re-order is coupled with another independent actual change that makes the diff result DIFF_SOME. Then 16024 will not apply and 1935 will be right back.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Some behavior is incorrect or out of spec
Projects
None yet
Development

No branches or pull requests

3 participants