Better handle property dependencies and deletedWith
#16088
Conversation
Changelog[uncommitted] (2024-05-03)Bug Fixes
|
lunaris
force-pushed
the
wjones/16052-bugs
branch
2 times, most recently
from
e152b62
to
876b227
Compare
lunaris
changed the title
deletedWith correctlydeletedWith
lunaris
force-pushed
the
wjones/16052-bugs
branch
2 times, most recently
from
67be186
to
5710869
Compare
| parentResp, err := monitor.RegisterResource("pkgA:m:typA", "resA", true, deploytest.ResourceOptions{ | ||
| DeletedWith: expectedUrn, | ||
| DeletedWith: deletionDep.URN, |
There was a problem hiding this comment.
This test and the one following it previously omitted a resource being referred to. Since all they do is test the inheritance, I've added the relevant resource to the tests in question. I don't think this compromises what they are testing but calling this out in case I've made a mistake.
|
@Frassle @tgummerer Test failures caused by (hopefully better and more accurate) snapshot integrity checks led me to find missing cases in |
| parentResp, err := monitor.RegisterResource("pkgA:m:typA", "resA", true, deploytest.ResourceOptions{ | ||
| DeletedWith: expectedUrn, | ||
| DeletedWith: deletionDep.URN, |
There was a problem hiding this comment.
This looks good to me, though as discussed in slack, we should probably also update the OnlyDependsOn method.
I'm also slightly worried about the snapshot validation now being more strict, and that breaking with some existing snapshot. I'm not sure how risky that is though, we might want to consider just warning in a first iteration of this.
| for _, other := range snap.Resources[i+1:] { | ||
| if other.URN == dep { | ||
| return fmt.Errorf( | ||
| "resource %s's property dependency %s (from property %s) comes after it", |
There was a problem hiding this comment.
Making the snapshot validation stricter is what makes me worried most about this change. It should be fine, but we also thought that in previous changes where we made some validation stricter. I'm wondering if we should make this a warning, including asking people to open an issue if they encounter it before actually returning an error here? Or alternatively have an escape hatch, e.g. an environment variable that allows people to opt out of the new validation bits for now, without having to revert to an earlier pulumi version?
A resource `A` depends on a resource `B` if: 1. `B` is `A`'s `Provider`. 2. `B` is `A`'s `Parent`. 3. `B` appears in `A`'s `Dependencies`. 4. `B` appears in one or more of `A`'s `PropertyDependencies`. 5. `B` is referenced by `A`'s `DeletedWith` field. While cases 1, 2, and 3 (providers, parents, and dependencies) are handled fairly consistently, there have been a number of cases where the newer features of `PropertyDependencies` (case 4) and `DeletedWith` (case 5) have been neglected. This commit addresses some of these omissions. Specifically: * When refreshing state, it's important that we remove URNs that point to resources that we've identified as deleted. Presently we check pointers to parents and dependencies, but not property dependencies or `deletedWith`. This commit fixes these gaps where dangling URNs could lurk. * Linked to the above, this commit extends snapshot integrity checks to include property dependencies and `deletedWith`. Some tests that previously used now invalid states have to be repaired or removed as a result of this. * Fixing snapshot integrity checking reveals that dependency graph checks also fail to consider property dependencies and `deletedWith`. This probably hasn't bitten us since property dependencies are currently rolled up into dependencies (for temporary backwards compatibility) and `deletedWith` is typically an optimisation (moreover, one that only matters during deletes), so operations being parallelised due a perceived lack of dependency still succeed. However, tests that previously passed now fail as we can spot these races with our better integrity checks. This commit thus fixes up dependency graph construction and bulks out its test suite to cover the new cases.
To be merged after #16119 merges. Tentative changelog: - [backend] Fix concurrent reads from and writes to display resource timer maps [#16101](#16101) - [engine] Better handle property dependencies and deleted-with relationships when pruning URNs, verifying snapshot integrity and computing dependency graphs. [#16088](#16088) - [programgen/python] Sort generated requirements.txt files when generating Python programs [#16115](#16115)
A resource
Adepends on a resourceBif:BisA'sProvider.BisA'sParent.Bappears inA'sDependencies.Bappears in one or more ofA'sPropertyDependencies.Bis referenced byA'sDeletedWithfield.While cases 1, 2, and 3 (providers, parents, and dependencies) are handled fairly consistently, there have been a number of cases where the newer features of
PropertyDependencies(case 4) andDeletedWith(case 5) have been neglected. This commit addresses some of these omissions. Specifically:When refreshing state, it's important that we remove URNs that point to resources that we've identified as deleted. Presently we check pointers to parents and dependencies, but not property dependencies or
deletedWith. This commit fixes these gaps where dangling URNs could lurk.Linked to the above, this commit extends snapshot integrity checks to include property dependencies and
deletedWith. Some tests that previously used now invalid states have to be repaired or removed as aresult of this.
Fixing snapshot integrity checking reveals that dependency graph checks also fail to consider property dependencies and
deletedWith. This probably hasn't bitten us since property dependencies are currently rolled up into dependencies (for temporary backwards compatibility) anddeletedWithis typically an optimisation (moreover, one that only matters during deletes), so operations being parallelised due a perceived lack of dependency still succeed. However, tests that previously passed now fail as we can spot these races with our better integrity checks. This commit thus fixes up dependency graph construction and bulks out its test suite to cover the new cases.These bugs were discovered as part of the investigation into #16052, though they may not be directly responsible for it (though the issues with dependency graphs are certainly a candidate).