Skip to content

k8swatch: convert PodWatcher to reconciler#4537

Merged
milas merged 6 commits into
masterfrom
milas/k8swatch-reconciler
May 19, 2021
Merged

k8swatch: convert PodWatcher to reconciler#4537
milas merged 6 commits into
masterfrom
milas/k8swatch-reconciler

Conversation

@milas

@milas milas commented May 7, 2021

Copy link
Copy Markdown
Contributor

PodWatcher is now kubernetesdiscovery.Reconciler. Almost all
the logic is the same, though some stuff moved around. Most notable
change is that the check to decide whether to skip a status update
now happens against the API object instead of the store object.

milas added 2 commits May 7, 2021 17:23
Currently, the `PodWatcher` tests cover both:
 * Manifest -> KubernetesDiscovery spec creation
 * KubernetesDiscovery spec -> Pod discovery + status update

The latter will be covered by the reconciler tests - it's not
desirable to have the reconciler tests needing to create manifests
and transit via the `ManifestSubscriber`, so these test specifically
test the former behavior so that the reconciler tests can focus on
the latter.
`PodWatcher` is now `kubernetesdiscovery.Reconciler`. Almost all
the logic is the same, though some stuff moved around. Most notable
change is that the check to decide whether to skip a status update
now happens against the API object instead of the store object.
@milas
milas requested review from maiamcc and nicks May 7, 2021 21:29
@milas

milas commented May 7, 2021

Copy link
Copy Markdown
Contributor Author

There's a few more test cases from pod_watcher_test.go that vanished here that I want to re-introduce. Some of them (e.g. making sure we don't do bad ResourceVersion string comparisons) should be reconciler tests; some of them are more about manifest behavior and need to get covered by the k8swatch.ManifestSubscriber tests (I think they all are, but made a list and need to compare to ensure match-y match-y behavior.) There might be a couple that become "upper" tests because they are exercising some weird inter-play between manifest-driven specs + actual watch behavior.

FYI There are already (many) "integration" tests in upper_test.go which are currently passing (🎉) using the k8swatch.ManifestSubscriber store subscriber + kubernetesdiscovery.Reconciler apiserver controller.

Base automatically changed from milas/k8swatch-ms-tests to master May 17, 2021 14:49
@milas
milas merged commit 6c06d63 into master May 19, 2021
@milas
milas deleted the milas/k8swatch-reconciler branch May 19, 2021 16:31
@milas

milas commented May 19, 2021

Copy link
Copy Markdown
Contributor Author

This also fixes #4557. When there is no controller registered for a type, the controller-runtime client caching does not behave as expected, and will consistently return NotFound for some entities. When this happens, even adjusting the resync period to be very short doesn't cause it to become consistent.

We need to dive into the client caching behavior more, but this at least ensures things don't behave erratically.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants