Reflector list should handle expired resource version from watch handler as well

[aaruna]

Related Issues:

• Error Logs of ReflectorRunnable caused by too 'old resource version' #1580
• Error Logs of ReflectorRunnable caused by too 'old resource version' #1606

Related PRs

• Chore: Reducing rewatch logging verbosity upon normal re-connection #1611
• Fixes reflector list expiration #1543

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: aaruna
To complete the pull request process, please assign yue9944882 after the PR has been reviewed.
You can assign the PR to them by writing /assign @yue9944882 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

• OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[brendandburns]

This looks good to me, but I want @yue9944882 to take a look at this also, since he was the primary author of this code.

[aaruna]

@yue9944882 Any thoughts on this?

[yue9944882]

@aaruna upon 410 status code, the watch handler is supposed to return and we deal w/ the RV expiration at the list call. this is working as designed. https://github.com/kubernetes/kubernetes/blob/a96000311f5beca111debc8727018e42cbb5dc79/staging/src/k8s.io/client-go/tools/cache/reflector.go#L129-L131

[aaruna]

Are you referring to the next list call? Because the current watch handler won't throw an ApiException and only that is handled in the catch block above. (The unit test added in this PR shows an example of this scenario)

CC @jiangzhou in case I have missed anything in the scenario above.

[yue9944882]

Are you referring to the next list call?

yeah. IIRC watch handler doesnt directly throw an ApiException but it will deal w/ error status nested in the decoded watch event.

java/util/src/main/java/io/kubernetes/client/informer/cache/ReflectorRunnable.java

Lines 227 to 237 in c580990

	if (eventType.get() == EventType.ERROR) {
	if (item.status != null && item.status.getCode() == HttpURLConnection.HTTP_GONE) {
	log.info("Watch connection expired: {}", item.status.getMessage());
	return;
	} else {
	String errorMessage =
	String.format("got ERROR event and its status: %s", item.status.toString());
	log.error(errorMessage);
	throw new RuntimeException(errorMessage);
	}
	}

[aaruna]

But that's not resetting isLastSyncResourceVersionUnavailable=true. The unit test below has an example

[yue9944882]

/hold

for discussion

[brendandburns]

@yue9944882 what is the status of this PR/Discussion?

[tony-clarke-amdocs]

I have upgrade to client 13.0.0 and now we are floored with these messages. It looks like a very clear regression. I am getting deja vu. It looks like the same pattern, we are not resetting isLastSyncResourceVersionUnavailable back to true when HTTP_GONE occurs. @aaruna change looks good to me. 13.0.0 is unusable as it is. Waiting for update on this. @aaruna can you rebase?

[aaruna]

@tony-clarke-amdocs I have rebased the PR.

[tony-clarke-amdocs]

Thanks @aaruna. @brendandburns @yue9944882 Can you guys review when you get a chance. It's a blocker for us.

[tony-clarke-amdocs]

@brendandburns @yue9944882 Any ETA for this PR? If there is anything we can do to move it forward please let us know.

[yue9944882]

/hold cancel

[yue9944882]

/close

picked and merged #1927

[k8s-ci-robot]

@yue9944882: Closed this PR.

In response to this:

/close

picked and merged #1927

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.