kubernetes-client icon indicating copy to clipboard operation
kubernetes-client copied to clipboard

The close method of defaultkubernetesclient returned by calling the defaultkubernetesclient innamespace method will close the original httpclient

Open walnut-tom opened this issue 3 years ago • 7 comments

Is your enhancement related to a problem? Please describe

NamespacedKubernetesClient kubernetesClient = new DefaultKubernetesClient(masterUrl);

NamespacedKubernetesClient client = kubernetesClient.inNamespace("default"); ..... client.close();

kubernetesClient.apps().deployments().withName(name).get();

will cause a KubernetesClientException Exception

Describe the solution you'd like

class DefaultKubernetesClient ... {

@Override public NamespacedKubernetesClient inNamespace(String name) { io.fabric8.kubernetes.client.Config updated = new io.fabric8.kubernetes.client.ConfigBuilder(getConfiguration()) .withNamespace(name) .build(); return new PooledNamespacedKubernetesClient(newState(updated)); }

private static class PooledNamespacedKubernetesClient extends DefaultKubernetesClient {

public PooledNamespacedKubernetesClient(ClientContext clientContext) {
  super(clientContext);
}

  @Override
  public void close() {
    // do nothing
  }

} }

Describe alternatives you've considered

No response

Additional context

No response

walnut-tom avatar May 30 '22 08:05 walnut-tom

This is expected behavior for the close call - it will close the shared resources of the client and any derived client. There is currently no notion of KubernetesClient pooling because the resources are already pooled under the client - to a single kubernetes instance / context you would not need more than one KubernetesClient. Can you elaborate on how / why you are pooling the clients?

shawkins avatar May 31 '22 13:05 shawkins

Do you still need to close the KubernetesClient after using it? If necessary, what is the significance of using okhttpclient (ConnectionPool) in the lower layer? And the KubernetesClient created through the inNamespace call closes resources that do not belong to itself. If you do not need to close it, what scenarios are used to implement the autoclosable interface?

walnut-tom avatar May 31 '22 15:05 walnut-tom

Do you still need to close the KubernetesClient after using it?

Yes

If necessary, what is the significance of using okhttpclient (ConnectionPool) in the lower layer?

Closing the KubernetesClient closes the okhttpclient resources.

And the KubernetesClient created through the inNamespace call closes resources that do not belong to itself.

It should not be understood as having separate resources. Only new DefaultKubernetesClient(masterUrl) is allocating the underlying resources.

If you do not need to close it, what scenarios are used to implement the autoclosable interface?

Mostly for what is shown in examples that is a standalone usage of the client:

try (KubernetesClient client = new DefaultKubernetesClient(masterUrl)) { ... }

In other scenarios, for example where the client is injected, close does not need to be directly called.

shawkins avatar May 31 '22 16:05 shawkins

Whether the resource should be destroyed by the creator, so DefaultKubernetesClient should track whether the OkHttpClient is created by the current object, and handle it accordingly when closing.The close method should be able to handle correctly.

walnut-tom avatar Jun 01 '22 02:06 walnut-tom

Your view is that the DefaultKubernetesClient is a KubernetesClient pool - that has never been the design / implementation. Regardless of the methods you call it is logically a single client instance surrounding a single http client.

If you feel strongly about it behaving like a pool, you could advocate for this change via a PR.

shawkins avatar Jun 01 '22 11:06 shawkins

Some notes, especially regarding the Kubernetes Client 6.0 release.

We start from a KubernetesClient instantiated by an HttpClient.Factory:

try (KubernetesClient client = new KubernetesClientBuilder().build()) { //..
// Or
try (KubernetesClient client = new KubernetesClientBuilder().withHttpClientFactory(factory).build()) { //..

When the client is built or instantiated, a new underlying, AutoCloseable, HttpClient (Fabric8) instance is created.

The contract for the implementations (OkHttp, Jdk, Jetty, and so on), establishes that a single pool of resources is created and shared amongst all of the HttpClient (Fabric8) instances derived from this one.

The following calls will generate a new KubernetesClient + HttpClient (Fabric8) instance but their underlying resources should be shared with those of the client they are deriving from

client.adapt(OpenShiftClient.class);
client.adapt(NamespacedKubernetesClient.class).inNamespace(namespace);
client.getHttpClient().newBuilder().build();

By closing any of the derived KubernetesClient instances, ---> (Fabric8) HttpClient instances, the pool of resources should be released.

So, yes a KubernetesClient should be closed whenever the application ends, but you don't need to keep track of each derived instance, since the resources should be shared among them.

manusa avatar Jun 06 '22 10:06 manusa

I will remove all try (NamespacedKubernetesClient client = ...) block

walnut-tom avatar Jun 07 '22 07:06 walnut-tom

This issue has been automatically marked as stale because it has not had any activity since 90 days. It will be closed if no further activity occurs within 7 days. Thank you for your contributions!

stale[bot] avatar Sep 05 '22 08:09 stale[bot]