Skip to content

Commit

Permalink
fix #4888: narrowing where the 0 resourceVersion is used
Browse files Browse the repository at this point in the history
  • Loading branch information
shawkins authored and manusa committed Mar 9, 2023
1 parent 73b2a04 commit 5490ec1
Show file tree
Hide file tree
Showing 13 changed files with 41 additions and 25 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@
* Fix #4823: (java-generator) handle special characters in field names
* Fix #4723: [java-generator] Fix a race in the use of JavaParser hitting large CRDs
* Fix #4885: addresses a potential hang in the jdk client with exec stream reading
* Fix #4888: narrowing where the 0 initial list resourceVersion is used for informers - in particular if a limit is set or initialState is used, then we should not use 0. Additionally for the informOnCondition / wait methods we'll also not use 0 - it's not expected that the user should test any state prior to the latest.
* Fix #4891: address vertx not completely reading exec streams
* Fix #4899: BuildConfigs.instantiateBinary().fromFile() does not time out
* Fix #4908: using the response headers in the vertx response
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -902,6 +902,8 @@ public CompletableFuture<List<T>> informOnCondition(Predicate<List<T>> condition
// create an informer that supplies the tester with events and empty list handling
SharedIndexInformer<T> informer = this.createInformer(0, Runnable::run);

informer.initialState(Stream.empty());

// prevent unnecessary watches and handle closure
future.whenComplete((r, t) -> informer.stop());

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,7 @@ public CompletableFuture<Void> start() {

if (initialState != null) {
initialState.forEach(indexer::put);
reflector.usingInitialState();
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -60,6 +60,8 @@ public class Reflector<T extends HasMetadata, L extends KubernetesResourceList<T

private CompletableFuture<Void> timeoutFuture;

private boolean cachedListing = true;

public Reflector(ListerWatcher<T, L> listerWatcher, SyncableStore<T> store) {
this.listerWatcher = listerWatcher;
this.store = store;
Expand Down Expand Up @@ -167,10 +169,8 @@ private CompletableFuture<L> processList(Set<String> nextKeys, String continueVa
CompletableFuture<L> futureResult = listerWatcher
.submitList(
new ListOptionsBuilder()
// start with 0 - meaning any cached version is fine for the initial listing
// but if we've already synced, then we have to get the latest as the version we're on
// is no longer valid
.withResourceVersion(lastSyncResourceVersion == null && continueVal == null ? "0" : null)
// if caching is allowed, start with 0 - meaning any cached version is fine for the initial listing
.withResourceVersion(isCachedListing(continueVal) ? "0" : null)
.withLimit(listerWatcher.getLimit()).withContinue(continueVal)
.build());

Expand All @@ -188,6 +188,11 @@ private CompletableFuture<L> processList(Set<String> nextKeys, String continueVa
});
}

private boolean isCachedListing(String continueVal) {
// allow an initial cached listing only if there's no initial state, no limit, we haven't already sync'd, and this isn't a continue request
return cachedListing && listerWatcher.getLimit() == null && lastSyncResourceVersion == null && continueVal == null;
}

private void stopWatch(Watch w) {
log.debug("Stopping watcher for {} at v{}", this, lastSyncResourceVersion);
w.close();
Expand Down Expand Up @@ -320,4 +325,8 @@ public void setExceptionHandler(ExceptionHandler handler) {
this.handler = handler;
}

public void usingInitialState() {
this.cachedListing = false;
}

}
Original file line number Diff line number Diff line change
Expand Up @@ -253,7 +253,7 @@ void testRunnableInformer() throws InterruptedException {
.build();

server.expect()
.withPath("/api/v1/namespaces/test/pods?labelSelector=my-label&resourceVersion=0")
.withPath("/api/v1/namespaces/test/pods?labelSelector=my-label")
.andReturn(HttpURLConnection.HTTP_OK,
new PodListBuilder().withNewMetadata().withResourceVersion("1").endMetadata().withItems(pod1).build())
.once();
Expand Down Expand Up @@ -320,7 +320,7 @@ void testListLimit() throws InterruptedException {
.build();

server.expect()
.withPath("/api/v1/namespaces/test/pods?limit=1&resourceVersion=0")
.withPath("/api/v1/namespaces/test/pods?limit=1")
.andReturn(HttpURLConnection.HTTP_OK,
new PodListBuilder().withNewMetadata()
.withResourceVersion("2")
Expand All @@ -337,7 +337,8 @@ void testListLimit() throws InterruptedException {
.once();

server.expect()
.withPath("/api/v1/namespaces/test/pods?resourceVersion=2&timeoutSeconds=600&allowWatchBookmarks=true&watch=true")
.withPath(
"/api/v1/namespaces/test/pods?resourceVersion=2&timeoutSeconds=600&allowWatchBookmarks=true&watch=true")
.andUpgradeToWebSocket()
.open()
.done()
Expand Down Expand Up @@ -387,7 +388,8 @@ void testInformWithAlternativeKeyFunction() throws InterruptedException {
.once();

server.expect()
.withPath("/api/v1/namespaces/test/pods?resourceVersion=1&timeoutSeconds=600&allowWatchBookmarks=true&watch=true")
.withPath(
"/api/v1/namespaces/test/pods?resourceVersion=1&timeoutSeconds=600&allowWatchBookmarks=true&watch=true")
.andUpgradeToWebSocket()
.open()
.done()
Expand Down Expand Up @@ -453,7 +455,8 @@ void testInformWithMinimalState() throws InterruptedException {
.once();

server.expect()
.withPath("/api/v1/namespaces/test/pods?resourceVersion=1&timeoutSeconds=600&allowWatchBookmarks=true&watch=true")
.withPath(
"/api/v1/namespaces/test/pods?resourceVersion=1&timeoutSeconds=600&allowWatchBookmarks=true&watch=true")
.andUpgradeToWebSocket()
.open()
.done()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -705,7 +705,7 @@ void testWait() throws InterruptedException {

server.expect()
.get()
.withPath("/api/v1/namespaces/test/pods?fieldSelector=metadata.name%3Dpod1&resourceVersion=0")
.withPath("/api/v1/namespaces/test/pods?fieldSelector=metadata.name%3Dpod1")
.andReturn(200, notReady)
.once();

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -208,7 +208,7 @@ void testScaleAndWait() {

// list for waiting
server.expect()
.withPath("/apis/apps/v1/namespaces/test/replicasets?fieldSelector=metadata.name%3Drepl1&resourceVersion=0")
.withPath("/apis/apps/v1/namespaces/test/replicasets?fieldSelector=metadata.name%3Drepl1")
.andReturn(200,
new ReplicaSetListBuilder().withItems(scaled).withMetadata(new ListMetaBuilder().build()).build())
.always();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -212,7 +212,7 @@ void testScaleAndWait() {

// list for waiting
server.expect()
.withPath("/api/v1/namespaces/test/replicationcontrollers?fieldSelector=metadata.name%3Drepl1&resourceVersion=0")
.withPath("/api/v1/namespaces/test/replicationcontrollers?fieldSelector=metadata.name%3Drepl1")
.andReturn(200,
new ReplicationControllerListBuilder().withItems(scaled).withMetadata(new ListMetaBuilder().build()).build())
.always();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,12 +159,12 @@ void testCreateOrReplaceWithoutDeleteExisting() throws Exception {
void testCreateOrReplaceWithDeleteExisting() throws Exception {
server.expect().delete().withPath("/api/v1/namespaces/ns1/services/my-service").andReturn(HTTP_OK, service).once();
server.expect().delete().withPath("/api/v1/namespaces/ns1/configmaps/my-configmap").andReturn(HTTP_OK, configMap).once();
server.expect().get().withPath("/api/v1/namespaces/ns1/services?fieldSelector=metadata.name%3Dmy-service&resourceVersion=0")
server.expect().get().withPath("/api/v1/namespaces/ns1/services?fieldSelector=metadata.name%3Dmy-service")
.andReturn(HTTP_OK,
new KubernetesListBuilder().withNewMetadata().endMetadata().build())
.once();
server.expect().get()
.withPath("/api/v1/namespaces/ns1/configmaps?fieldSelector=metadata.name%3Dmy-configmap&resourceVersion=0")
.withPath("/api/v1/namespaces/ns1/configmaps?fieldSelector=metadata.name%3Dmy-configmap")
.andReturn(HTTP_OK,
new KubernetesListBuilder().withNewMetadata().endMetadata().build())
.once();
Expand Down Expand Up @@ -202,8 +202,8 @@ void testSuccessfulWaitUntilCondition() throws InterruptedException {
.anyMatch(c -> "True".equals(c.getStatus()));

// The pods are never ready if you request them directly.
ResourceTest.list(server, noReady1, "0");
ResourceTest.list(server, noReady2, "0");
ResourceTest.list(server, noReady1, null);
ResourceTest.list(server, noReady2, null);

server.expect().get().withPath(
"/api/v1/namespaces/ns1/pods?fieldSelector=metadata.name%3Dpod1&resourceVersion=1&timeoutSeconds=600&allowWatchBookmarks=true&watch=true")
Expand Down Expand Up @@ -247,8 +247,8 @@ void testPartialSuccessfulWaitUntilCondition() {
.anyMatch(c -> "True".equals(c.getStatus()));

// The pods are never ready if you request them directly.
ResourceTest.list(server, noReady1, "0");
ResourceTest.list(server, noReady2, "0");
ResourceTest.list(server, noReady1, null);
ResourceTest.list(server, noReady2, null);

Status gone = new StatusBuilder()
.withCode(HTTP_GONE)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -327,7 +327,7 @@ void testWaitUntilReady() throws InterruptedException {
* @param pod
*/
private void list(Pod pod) {
list(server, pod, "0");
list(server, pod, null);
}

static void list(KubernetesMockServer server, Pod pod, String resourceVersion) {
Expand Down Expand Up @@ -355,7 +355,7 @@ void testWaitUntilExistsThenReady() throws InterruptedException {
// and again so that "periodicWatchUntilReady" successfully begins
server.expect()
.get()
.withPath("/api/v1/namespaces/test/pods?fieldSelector=metadata.name%3Dpod1&resourceVersion=0")
.withPath("/api/v1/namespaces/test/pods?fieldSelector=metadata.name%3Dpod1")
.andReturn(200, noReady)
.times(2);

Expand Down Expand Up @@ -715,7 +715,7 @@ void testFromServerWaitUntilConditionAlwaysGetsResourceFromServer() throws Excep
void testWaitNullDoesntExist() throws InterruptedException {
server.expect()
.get()
.withPath("/api/v1/namespaces/test/pods?fieldSelector=metadata.name%3Dpod1&resourceVersion=0")
.withPath("/api/v1/namespaces/test/pods?fieldSelector=metadata.name%3Dpod1")
.andReturn(200,
new PodListBuilder().withNewMetadata().withResourceVersion("1").endMetadata().build())
.once();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -210,11 +210,11 @@ void testWaitUntilReady() throws InterruptedException {
.addToPorts(new EndpointPortBuilder().withPort(8443).build())
.build())
.build();
server.expect().get().withPath("/api/v1/namespaces/ns1/endpoints?fieldSelector=metadata.name%3Dsvc1&resourceVersion=0")
server.expect().get().withPath("/api/v1/namespaces/ns1/endpoints?fieldSelector=metadata.name%3Dsvc1")
.andReturn(HttpURLConnection.HTTP_OK,
new EndpointsListBuilder().withItems(endpoint).withNewMetadata().withResourceVersion("1").endMetadata().build())
.once();
server.expect().get().withPath("/api/v1/namespaces/ns1/services?fieldSelector=metadata.name%3Dsvc1&resourceVersion=0")
server.expect().get().withPath("/api/v1/namespaces/ns1/services?fieldSelector=metadata.name%3Dsvc1")
.andReturn(HttpURLConnection.HTTP_OK,
new ServiceListBuilder().withItems(svc1).withNewMetadata().withResourceVersion("1").endMetadata().build())
.once();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -250,7 +250,7 @@ public void testScaleAndWait() {

// list for waiting
server.expect()
.withPath("/apis/apps/v1/namespaces/test/statefulsets?fieldSelector=metadata.name%3Drepl1&resourceVersion=0")
.withPath("/apis/apps/v1/namespaces/test/statefulsets?fieldSelector=metadata.name%3Drepl1")
.andReturn(200,
new StatefulSetListBuilder().withItems(scaled).withMetadata(new ListMetaBuilder().build()).build())
.always();
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -406,7 +406,7 @@ void testWaitUntilReady() throws InterruptedException {
.endStatus().build();
server.expect().get()
.withPath(
"/apis/apps.openshift.io/v1/namespaces/ns1/deploymentconfigs?fieldSelector=metadata.name%3Ddc1&resourceVersion=0")
"/apis/apps.openshift.io/v1/namespaces/ns1/deploymentconfigs?fieldSelector=metadata.name%3Ddc1")
.andReturn(HttpURLConnection.HTTP_OK,
new DeploymentConfigListBuilder().withItems(deploymentConfig).withMetadata(new ListMeta()).build())
.always();
Expand Down

0 comments on commit 5490ec1

Please sign in to comment.