-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Better handle property dependencies and deletedWith
#16088
Conversation
Changelog[uncommitted] (2024-05-03)Bug Fixes
|
e152b62
to
876b227
Compare
deletedWith
correctlydeletedWith
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope this is fine. Better even.
@Frassle @tgummerer Test failures caused by (hopefully better and more accurate) snapshot integrity checks led me to find missing cases in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changes look to me, but I'd like one more to sanity check this. I'm not at 100% and might of missed something.
parentResp, err := monitor.RegisterResource("pkgA:m:typA", "resA", true, deploytest.ResourceOptions{ | ||
DeletedWith: expectedUrn, | ||
DeletedWith: deletionDep.URN, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nope this is fine. Better even.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
A
depends on a resourceB
if:B
isA
'sProvider
.B
isA
'sParent
.B
appears inA
'sDependencies
.B
appears in one or more ofA
'sPropertyDependencies
.B
is referenced byA
'sDeletedWith
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) 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) anddeletedWith
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.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).