fix: discovery fail when at least one api is unavailable #2732

rossignolloic · 2023-07-18T15:31:47Z

Exceptions raised when calling an API to obtain all the resources it manages are now ignored. So when an API is unavailable, discovery doesn't stop, and all others APIs are processed.

This PR means that when ModelMapper calls Discovery#findAll and at least one api is unavailable, all metadata related to resources managed by unavailable APIs will not be updated in ModelMapper until the next refresh (30 minutes by default).

In addition, this PR allows the KubectlApiResources class (the extended client equivalent of kubectl api-resources) to list all resources managed by available API .

linux-foundation-easycla · 2023-07-18T15:31:51Z

The committers listed above are authorized under a signed CLA.

✅ login: rossignolloic / name: Rossignol Loïc (c5dcaa0, d4b1f65, f95f5a7, 9979f68, 3569b6f)

k8s-ci-robot · 2023-07-18T15:31:56Z

Welcome @rossignolloic!

It looks like this is your first PR to kubernetes-client/java 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-client/java has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. 😃

linux-foundation-easycla · 2023-07-18T15:36:26Z

The committers listed above are authorized under a signed CLA.

✅ login: rossignolloic / name: Rossignol Loïc (c5dcaa0, d4b1f65)

brendandburns · 2023-07-19T14:31:33Z

util/src/main/java/io/kubernetes/client/Discovery.java

+      V1APIResourceList resourceList = resourceDiscovery(path);
+      return groupResourcesByName(group, versions, preferredVersion, resourceList);
+    } catch (ApiException e) {
+      LOGGER.warn("Api {} returns a {} code, all api resources managed by this api cannot be discovered", path, e.getCode(), e);


Rather than doing this here, which I think is going to confuse a developer who uses this to actually call the discovery API, can you add the catch in the Kubectl code that is executing this?

If I understand correctly, the suggestion is to ignore the ApiException in this code:

java/extended/src/main/java/io/kubernetes/client/extended/kubectl/Kubectl.java

Lines 235 to 238 in c8923e6

try {

ModelMapper.refresh(new Discovery(apiClient));

} catch (ApiException e) {

throw new KubectlException(e);

If I ignore the exception at this location, when an api raises an error, this code will never be executed:

java/util/src/main/java/io/kubernetes/client/util/ModelMapper.java

Lines 275 to 282 in c8923e6

addModelMap(

apiResource.getGroup(),

version,

apiResource.getKind(),

apiResource.getResourcePlural(),

apiResource.getNamespaced(),

objClass,

objListClass);

Then, this method will always return null:

java/util/src/main/java/io/kubernetes/client/util/ModelMapper.java

Line 226 in c8923e6

public static GroupVersionResource getGroupVersionResourceByClass(Class<?> clazz) {

And we will get the folowing exception when executing the client request:

java/extended/src/main/java/io/kubernetes/client/extended/kubectl/Kubectl.java

Line 271 in c8923e6

throw new KubectlException("Unexpected unknown resource type: " + apiTypeClass);

If I'm not mistaken, this suggestion does not fix the initial issue.
What do you think about this?

initModelMap() is always called statically:

java/util/src/main/java/io/kubernetes/client/util/ModelMapper.java

Line 74 in c8923e6

static {

So that still should work correctly.

Also, concretely, the suggestion is not to ignore all ApiExceptions, just to ignore this specific exception using the HTTP code/message to detect it.

initModelMap() is always called statically:

java/util/src/main/java/io/kubernetes/client/util/ModelMapper.java

Line 74 in c8923e6

static {

So that still should work correctly.

Unfortunatly, the static initModelMap is not enougth to make extended client work without the discovery. This method does'nt init vars classesByGVR, and as demonstrated earlier, when this vars is not set, the extended client will lead to throwing another exception.

Also, concretely, the suggestion is not to ignore all ApiExceptions, just to ignore this specific exception using the HTTP code/message to detect it.

I ignore all exception because my goal is to simulate the kubectl command behavior. This command just print an error message to alert users that all api-resources cannot be listed when an api is in error (whatever the error is). All other apis are present.

Here an example of kubectl command returns:

$ kubectl api-resources NAME SHORTNAMES APIVERSION NAMESPACED KIND bindings v1 true Binding ... volumeattachments storage.k8s.io/v1 false VolumeAttachment error: unable to retrieve the complete list of server APIs: bug.reproduce.java-client/v1alpha1: the server is currently unable to handle the request

I updated the warning message to be more like the one returned by the kubectl command.

I took the initiative of checking our respective implementations by running the program locally, and I confirm my previous analysis. Maybe if you're not convinced, you can check it locally too.

Thanks for doing the analysis.

The problem here is that the Discovery class isn't just for Kubectl, it needs to be general purpose for any client, and if we catch an exception in the Discovery class then we are obfuscating an error for a person who wants to use the Discovery class outside of Kubectl

So emulating kubectl behavior inside of the Kubectl package is 100% fine, but other utility classes can't eat error messages.

I think that if you catch the exception here: https://github.com/kubernetes-client/java/blob/master/util/src/main/java/io/kubernetes/client/util/ModelMapper.java#L269 and return an empty Set I believe that it will have the same effect as adding the catch where you added it.

I think that if you catch the exception here: https://github.com/kubernetes-client/java/blob/master/util/src/main/java/io/kubernetes/client/util/ModelMapper.java#L269 and return an empty Set I believe that it will have the same effect as adding the catch where you added it.

An empty Set at this location will not call addModelMap and will lead to an exception thrown by the Kubectl client as already explained. Do you agree with that?

The problem here is that the Discovery class isn't just for Kubectl, it needs to be general purpose for any client, and if we catch an exception in the Discovery class then we are obfuscating an error for a person who wants to use the Discovery class outside of Kubectl

So emulating kubectl behavior inside of the Kubectl package is 100% fine, but other utility classes can't eat error messages.

Ok, I didn't understood that. So, in my opinion we have two solutions to fix this issue:

Create a new checked exception, which contains a reference to all dicovered api resources, and will be thrown by discovery class. To ensure that the api contract is not broken by adding this new exception, it will extend ApiException. In library's code, this exception will be catched explicitly with api resources got from it. For user point of view, they will get an ApiException like before change.

Update ModelMapper's methods to make them failover to prebuild metadata. All informations needed for this are not available in prebuild metadata

java/util/src/main/java/io/kubernetes/client/util/ModelMapper.java

Lines 329 to 331 in 7f95758

public static Boolean isNamespaced(Class<?> clazz) {

return isNamespacedByClasses.get(clazz);

}

We need to update KubernetesType to add a method isNamespaced, and update all implementation as a result. Personally, this solution is not my favorite one, I don't checked the impact of this modification, and it's pretty huge change.

Thanks for the additional thought here. I'm ok with adding an adding a specialized checked exception here to handle this case.

I think it is also valuable to implement the fallback in the ModelMapper that you describe (I believe that this is what the Golang client does in kubectl) but I agree that it is extra work for this specific problem.

I commited changes, would you please check it?

brendandburns · 2023-08-07T19:16:02Z

Code looks good, looks like you need to fix formatting. Please run mvn spotless::apply

brendandburns · 2023-08-08T17:02:25Z

/lgtm
/approve

k8s-ci-robot · 2023-08-08T17:02:40Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: brendandburns, rossignolloic

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:

~~OWNERS~~ [brendandburns]

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

k8s-ci-robot requested review from brendandburns and yue9944882 July 18, 2023 15:31

k8s-ci-robot added the cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. label Jul 18, 2023

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 18, 2023

fix: discovery fail when at least one api is unavailable

c5dcaa0

rossignolloic force-pushed the fix/discovery_fail_when_api_unavailable branch from b9f9c8f to c5dcaa0 Compare July 18, 2023 15:36

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Jul 18, 2023

fix java 1.8 compatibility

d4b1f65

brendandburns reviewed Jul 19, 2023

View reviewed changes

rossignolloic added 2 commits July 27, 2023 11:26

update warning message

f95f5a7

use checked exception

9979f68

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Aug 4, 2023

mvn spotless::apply

3569b6f

k8s-ci-robot assigned brendandburns Aug 8, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 8, 2023

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 8, 2023

k8s-ci-robot merged commit 5b0e4ea into kubernetes-client:master Aug 8, 2023
16 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: discovery fail when at least one api is unavailable #2732

fix: discovery fail when at least one api is unavailable #2732

rossignolloic commented Jul 18, 2023

linux-foundation-easycla bot commented Jul 18, 2023 •

edited

Loading

k8s-ci-robot commented Jul 18, 2023

linux-foundation-easycla bot commented Jul 18, 2023 •

edited

Loading

brendandburns Jul 19, 2023

rossignolloic Jul 19, 2023

brendandburns Jul 25, 2023 •

edited

Loading

brendandburns Jul 25, 2023

rossignolloic Jul 27, 2023

brendandburns Jul 28, 2023 •

edited

Loading

rossignolloic Jul 31, 2023

brendandburns Aug 2, 2023

rossignolloic Aug 4, 2023

brendandburns commented Aug 7, 2023

brendandburns commented Aug 8, 2023

k8s-ci-robot commented Aug 8, 2023

	try {
	ModelMapper.refresh(new Discovery(apiClient));
	} catch (ApiException e) {
	throw new KubectlException(e);

	addModelMap(
	apiResource.getGroup(),
	version,
	apiResource.getKind(),
	apiResource.getResourcePlural(),
	apiResource.getNamespaced(),
	objClass,
	objListClass);

	public static Boolean isNamespaced(Class<?> clazz) {
	return isNamespacedByClasses.get(clazz);
	}

fix: discovery fail when at least one api is unavailable #2732

fix: discovery fail when at least one api is unavailable #2732

Conversation

rossignolloic commented Jul 18, 2023

linux-foundation-easycla bot commented Jul 18, 2023 • edited Loading

k8s-ci-robot commented Jul 18, 2023

linux-foundation-easycla bot commented Jul 18, 2023 • edited Loading

brendandburns Jul 19, 2023

Choose a reason for hiding this comment

rossignolloic Jul 19, 2023

Choose a reason for hiding this comment

brendandburns Jul 25, 2023 • edited Loading

Choose a reason for hiding this comment

brendandburns Jul 25, 2023

Choose a reason for hiding this comment

rossignolloic Jul 27, 2023

Choose a reason for hiding this comment

brendandburns Jul 28, 2023 • edited Loading

Choose a reason for hiding this comment

rossignolloic Jul 31, 2023

Choose a reason for hiding this comment

brendandburns Aug 2, 2023

Choose a reason for hiding this comment

rossignolloic Aug 4, 2023

Choose a reason for hiding this comment

brendandburns commented Aug 7, 2023

brendandburns commented Aug 8, 2023

k8s-ci-robot commented Aug 8, 2023

linux-foundation-easycla bot commented Jul 18, 2023 •

edited

Loading

linux-foundation-easycla bot commented Jul 18, 2023 •

edited

Loading

brendandburns Jul 25, 2023 •

edited

Loading

brendandburns Jul 28, 2023 •

edited

Loading