test: add ResourceGroup assertions to WatchForSync #1579
Conversation
sdowell
force-pushed
the
add-rg-assertions-watch-for-sync
branch
from
25f73b2 to
48b2528
Compare
e2e/nomostest/wait_for_sync.go
Outdated
| @@ -229,6 +229,17 @@ func (nt *NT) WatchForSync( | |||
| return fmt.Errorf("waiting for sync: %w", err) | |||
| } | |||
| nt.T.Logf("%s %s/%s is synced", gvk.Kind, namespace, name) | |||
| rgPredicates := []testpredicates.Predicate{ | |||
| // Wait until status.observedGeneration matches metadata.generation | |||
| testpredicates.HasObservedLatestGeneration(nt.Scheme), | |||
There was a problem hiding this comment.
I'm not really sure what the expected behavior was intended to be for the observedGeneration today. My guess is that nobody really considered how it would work with two controllers.
Anyway, I'm changing how it works in #1575
Instead of adding controller-specific observedGenerations, I was able to get it to work by having the applier inventory client handle setting it correctly when it updates the status, and then the RGC requires them to match before it will do its status updates, to avoid race conditions between spec & status updates.
This means that there's no status flag we can really use to confirm that the RGC has done its job. But that's sort of ok, for now.
It does mean that the kstatus fields will still be UNKNOWN until we fix cli-utils to pass it through to the inventory client so it can write them.
Another gotcha worth mentioning is that when the status is disabled, the observedGeneration is not updated. So this check likely needs to be embedded in resourceGroupHasReconciled.
sdowell
force-pushed
the
add-rg-assertions-watch-for-sync
branch
2 times, most recently
from
89ebbc1 to
72561c8
Compare
sdowell
force-pushed
the
add-rg-assertions-watch-for-sync
branch
3 times, most recently
from
58cbff9 to
46658f2
Compare
|
/retest |
| return nil | ||
| } | ||
|
|
||
| if len(rg.Spec.Resources) != len(rg.Status.ResourceStatuses) { |
There was a problem hiding this comment.
Do we want to validate that all the resources match in spec & status? Or is that too much here?
There was a problem hiding this comment.
That's exactly what the code below does.
There was a problem hiding this comment.
Ah, right. I was kinda confused by the resourceCount map.
It looks like you're validating that there's not more than one entry with the same ID, which seems good. But it's kinda hard to read.
I think there's a way to enforce that in the apiserver with schema validation, but it doesn't look like our CRD has those defined today. I know resources can define unique compound list keys. They added it for server-side apply list merging. I guess that's something we can add later.
What about building a fake []ResourceStatus from the status.resourceStatuses and then using semantic.DeepEquals to compare it to the real status.resources. Then you could error with a yaml diff that would make it easy to see how they compare?
There was a problem hiding this comment.
FWIW, this only works if they're sorted, which they might not be. So you might have to DeepCopy status.resources and then sort both the actual and expected lists before comparing them.
There was a problem hiding this comment.
Correct, the lists are not sorted - hence the decision to build a map to determine equality. Also it's not validating that the IDs are unique - this is purely verifying list equality. It's validating that the lists have the same set of IDs, including same number of duplicate IDs if there are duplicates.
There was a problem hiding this comment.
Alright. I think there's some readability/correctness changed we could make here, but I don't want to hold up our other PRs, and I don't really think it's all that important for general validation we use in every e2e test.
I wish we could just check the conditions here, and save all the other fields for validating in RG-specific tests, but that's not today's reality.
We can iterate on this later.
e2e/nomostest/sync.go
Outdated
| if rs.Actuation != v1alpha1.ActuationSucceeded { | ||
| return fmt.Errorf("resourceStatus.actuation is not %s. Got %s", v1alpha1.ActuationSucceeded, rs.Actuation) | ||
| } | ||
| if rs.SourceHash != status.TruncateSourceHash(sourceHash) { |
There was a problem hiding this comment.
Huh. I didn't even know these had a source hash field. Nifty.
So I guess you can tell if they're all on the same commit pretty easy, you just can't tell what commit it's supposed to be from the spec.
I guess if we have this in the status, maybe we should add a SourceHash to the spec, instead of adding a source-commit annotation. That would make it possible to check that the status matches the spec without external knowledge.
Guess we can do that in a different PR, but we're gonna need approval from @janetkuo
e2e/testdata/hydration/compiled-json/helm-components/wordpress/secret_my-wordpress.json
Outdated
Show resolved
Hide resolved
sdowell
force-pushed
the
add-rg-assertions-watch-for-sync
branch
5 times, most recently
from
d2b2b58 to
07d83e0
Compare
This adds validation for the ResourceGroup status in the WatchForSync function, which is used extensively in the test framework to validate RSyncs and wait for them to finish syncing. This will cause WatchForSync to now also wait for all synced objects to reconcile and for the ResourceGroup to be current. This is intended to provide test coverage for the resource-group-controller when integrated with Config Sync.
sdowell
force-pushed
the
add-rg-assertions-watch-for-sync
branch
from
07d83e0 to
e97ee76
Compare
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: karlkfi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
google-oss-prow
bot
merged commit bb19d8a
into
GoogleContainerTools:main
This adds validation for the ResourceGroup status in the WatchForSync function, which is used extensively in the test framework to validate RSyncs and wait for them to finish syncing.
This will cause WatchForSync to now also wait for all synced objects to reconcile and for the ResourceGroup to be current. This is intended to provide test coverage for the resource-group-controller when integrated with Config Sync.