Creating an api module

[shawkins]

Description

start at #3645 - adding a kubernetes-client-api
removes operationcontext from sharedinformerfactory
removes rawoperation... completely
creates interfaces for metrics
does not yet address OpenShift, DefaultKubernetesClient, and uses reflection for loading okhttp client.

Type of change

• Bug fix (non-breaking change which fixes an issue)
• Feature (non-breaking change which adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change
• Chore (non-breaking change which doesn't affect codebase;
  test, version modification, documentation, etc.)

Checklist

• Code contributed by me aligns with current project license: Apache 2.0
• I Added CHANGELOG entry regarding this change
• I have implemented unit tests to cover my changes
• I have added/updated the javadocs and other documentation accordingly
• No new bugs, code smells, etc. in SonarCloud report
• I tested my code in Kubernetes
• I tested my code in OpenShift

[centos-ci]

Can one of the admins verify this patch?

[shawkins]

The next set of changes more fully refine the new interfaces - the node/pod metrics logic is a little more regular and includes a workaround for #3662

The only really big change here is the removal of RawCustomResource logic - but I have not yet converted the main test over yet, so there are build failures in kubernetes-tests.

This also removed the mifmif dependency - I don't see that it's directly needed by the client logic.

As you can see the api module is effectively an api + utils/commons. However as mentioned above we likely need to leave a lot of the common/util logic in place and/or it's too much effort to separate it.

Additional cleanups needed:

• both the api and client modules are generating a Version class
• a kube config is duplicated in both test resources
• the util tests were not separated, so they are currently all in the client
• this does not yet address packages, classes are still in their original location. As much as possible the classes in the client in packages that overlap with the api, should be moved. The big exception to this is DefaultKubernetesClient.
• openshift-client has not yet been done.
• Readiness has not been moved to a non-internal package.

[shawkins]

Disregard the previous version of this comment, I changed podmetrics to default to any namespace for consistency with the prior logic.

[shawkins]

The basic separation of both kubernetes-client and openshift-client is complete. This does not yet address repackaging things left in the client modules nor readiness, and does not yet propose an alternative way to create clients.

If there's general agreement on how this works I'll continue to flesh it out. If we want to opt instead for just a http client module, let me know.

[shawkins]

and does not yet propose an alternative way to create clients.

Added a Kubernetes/OpenShiftClientBuilder(s) using reflection - if that is not a good choice, we can switch to a service loader, or even do like slf4j and create/remove dummy classes in the api modules.

[metacosm]

I have a question regarding the informers' cache… If we want to change the cache directly (for optimisation purposes), how do we do it with this new API? Right now, we get the store and cast it to Cache<T> but this doesn't look feasible anymore (unless we cast to CacheImpl?) Is there no API to modify the cache anymore (granted, this might not be a good idea to start with)?
The use case here is that when our code deletes or creates a resource, we'd like to remove/put it in the cache directly so that we're sure it's already available without having to wait for the server to send the corresponding event.

[shawkins]

I have a question regarding the informers' cache… If we want to change the cache directly (for optimisation purposes), how do we do it with this new API?

If we move toward the model of an informer only saving what state is necessary for level event handling, then modifying the cache directly shouldn't be needed. You'd maintain a separate application level cache of the full resources.

Right now, we get the store and cast it to Cache but this doesn't look feasible anymore (unless we cast to CacheImpl?) Is there no API to modify the cache anymore (granted, this might not be a good idea to start with)?

It wasn't directly available before these changes - the Store/Indexer interfaces did not allow for modifications, you already have to cast to Cache.

The use case here is that when our code deletes or creates a resource, we'd like to remove/put it in the cache directly so that we're sure it's already available without having to wait for the server to send the corresponding event.

This seems like it would need to be done very carefully. If you don't lock modifications on the resourceVersion, locking on the cache is not sufficient:

Application Thread	Informer Websocket Thread
	add event for v1 processed
locks the cache	modification event of v2 can't proceed
modification of v1 - via unlocked patch/edit/replace
adds the result object v3 to the cache
unlocks the cache
	modification event of v2 now proceeds - this looks like a revert
	modification event of v3

The cache is eventually consistent, but there can be incorrect intermediate events. I'm honestly not sure if you can simply compare resourceVersions to discard events like these, but if you did then the event stream would have events missing - there would be no proper emitting of the update from v1 to v2.

With resourceVersion locking:

Application Thread	Informer Websocket Thread
	add event for v1 processed
modification of v1 - via unlocked patch/edit/replace fails
	modification event for v2 processed
retry modification now against v2
	there's still a window here for a modification event for v4 processed
adds the result object v3 to the cache

It seems like the logic adding v3 to the cache would need to track what the expected entry revision is. If not's not v2, then you can't add v3.

It seems like there are a lot of application level concerns and assumptions about consistency that the informer must take on or expose.

[manusa]

Review comments/notes

Commit #1 creating an api module with new interfaces ✔️

• Removes deprecated RawCustomResourceOperationsImpl customResource(CustomResourceDefinitionContext customResourceDefinition);
  We need to document this in the migration guide (reminder for next PR)
• Introduces interface for RunOperations. Maybe add some Javadoc for the interface declaration stating that this is used for a similar to kubectl run operation (reminder for next PR)
• No use of constants for Media/MIME/Content type
• Initial entry-point for HttpClient instantiation (HttpClientUtils#createHttpClient)
• Improvement of Metric related impls

Commit #2 adding an openshift-client-api ✔️

Commit #3 correcting errant reference to customResource entry point ✔️

• Update Cheat Sheet (replace reference to removed customResource with genericKubernetesResources)

Commit #4 adding a possible reflection based set of client builders ✔️

• Introduce builders to instantiate HttpClient via reflection 🤔

Commit #5 adding a subprotocol method for jdk client support ✔️

• WebSocket builder option option for Sec-WebSocket-Protocol header
  Needed due to a limitation in the JDK client (reserved header - Create an abstraction layer for okhttp usage #3547 (comment))

Commit #6 the jdk client will only share state if we explicitly share the instance ✔️

[shawkins]

Introduces interface for RunOperations. Maybe add some Javadoc for the interface declaration stating that this is used for a similar to kubectl run operation (reminder for next PR)

That interface simply takes the place of a class with the same name. There's nothing new from an api perspective - but yes new/additional docs never hurt.

[manusa]

That interface simply takes the place of a class with the same name. There's nothing new from an api perspective - but yes new/additional docs never hurt.

Yes, I know. Those are just reminders for possible improvements. Since this is now an Interface, it makes sense to document it properly (not that it didn't make sense before too).

[shawkins]

Introduce builders to instantiate HttpClient via reflection

Yes, I'm not convinced that reflection is appropriate either - #3654 (comment)

This was more to propose something that would logically take the place of direct access to the constructors. We can even back that out for now to be refined as a separate issue.

[manusa]

We can even back that out for now to be refined as a separate issue.

I think we can leave it for now and reevaluate, discuss for a subsequent PR.

Despite my personal preference for an SPI approach to achieve this, wouldn't this reflection procedure be problematic for Quarkus Native?

[shawkins]

Despite my personal preference for an SPI approach to achieve this, wouldn't this reflection procedure be problematic for Quarkus Native?

For injection Quakus wouldn't need to use the builder, it would just directly reference the DefaultKubernetesClient/DefaultOpenShiftClient as it is today. - sorry, that won't work currently. quarkus-kubernetes-client is responsible for this currently and has a compile dependency on kubernetes-client, and projects would have a compile dependency on quarkus-kubernetes-client. It would require quarkus to introduce a runtime dependency on whatever module is given responsibility for creating the client instances.

The complication with the service loader:

• we'll need to introduce another class into the mix, let's call it ClientFactory, that provides the constructor access.
• do you consider the clients the same service or different? If they are the same, then inclusion of the openshift client dependency should mean you can only create openshift clients (similar to what quarkus is doing now). If they are different, then there's little benefit to using the service loader mechanism, it might be better to do like slf4j and introduce dummy instances into the api that compiler will bind to, but then we remove from the jar.

[manusa]

I haven't give it a thought. I think the confusion here lies on what we're calling a "client" and what does the SPI provide. Probably we are referring to different things

In this PR you introduce the Builder pattern for creating instances of KubernetesClient (DefaultKubernetesClient/DefaultOpenShiftClient).

I still need to understand how does this play with https://github.com/shawkins/httpclient

What I mean is that the underlying implementation of the HttpClient should be retrievable via ServiceLoader. A valid implementation should then be loaded depending on what there is in the runtime classpath.

[shawkins]

What I mean is that the underlying implementation of the HttpClient should be retrievable via ServiceLoader. A valid implementation should then be loaded depending on what there is in the runtime classpath.

Yes they are somewhat separate concerns. Here's the thoughts on the HttpClient:

• two new modules will be created - okhttp-client, and jdkhttp-client - they will both depend on the kubernetes-client-api module. The okhttp-client module will in the initial release be a runtime dependency of kubernetes-client. The logic in HttpClientUtils will select what factory to use via a service loader - we'll keep the selection logic simple, either use the first implementation found, or default to okhttp (which will require the factory to provide an identifier).
• If you are a legacy user and want to keep using okhttp - nothing should change for you. You just have a dependency on kubernetes-client, you access DefaultKubernetesClient just as you were, and the logic in HttpClientUtils will select the okhttp-client implementation.
• If you want to use a different http client implementation you have two options. Either exclude the okhttp dependency and include whatever alternative you want and rely on the selection logic in HttpClientUtils, or do whatever you need to do with the dependencies and use something like the ClientBuilder(s) proposed to directly associate an HttpClient.Factory with the builder.

The plan was to address this as part of the work to bring in the jdk implementation.

[manusa]

Sounds good, more or less aligned with what I was thinking. Anyway, maybe we could sync on this (I'll reach out to you internally) because there are things that I'm not fully grasping.

[shawkins]

Repacking issues: #1285 #2838

[sonarqubecloud]

SonarCloud Quality Gate failed.   

7 Bugs
6 Vulnerabilities
5 Security Hotspots
238 Code Smells

25.6% Coverage
0.5% Duplication

[shawkins]

@manusa PodIT.evict seems to be a flaky test, I can get it to fail locally as well - but only sometimes.

[manusa]

@manusa PodIT.evict seems to be a flaky test, I can get it to fail locally as well - but only sometimes.

I'm aware 😅. We need to do something with this test and the Karat, I hate this situation.