Removing generated ResourceHandlers #3327

shawkins · 2021-07-17T03:18:57Z

Description

Before I go too far down this path, I wanted to solicit some feedback. The initial changes to just refine the usage of CustomResource (the first two commits - which had not yet rationalized the SharedInformerFactory) made me want to see what things would look like without any generated ResourceHandlers. This pr has that work mostly completed for the kubernetes/openshift client modules - there are more validations and tests that should be added, but the kubernetes-tests run cleanly.

The idea is to replace the generated classes with ResourceHandlerImpl and ResourceOperationsImpl (which is also used by the resources/customResources dsl entry points). This means that any HasMetadata that is properly annotated can be used as a Resource - whether it's custom, built-in, or from a different client version provided HasMetadata and the annotations are compatible. Some of the SharedInformerFactory logic could be re-targeted to this as well.

The downsides to this approach:

it means that instances of the generated resourcehandlers / operations don't exist, so you have to use static Handlers methods to obtain them.
has reflective association/usage of Builders (based upon similar to the logic for automatically obtaining custom list type)
requires explicit registration of specialty HasMetadataOperations - which for now was put as a static block in BaseKubernetesClient and DefaultOpenShiftClient. This allows those operations to be used from the NamespaceVisitFromServer* classes rather than the default ResourceOperationsImpl.

Not really related to these changes it appears that OpenShiftNamespaceVisitFromServerGetWatchDeleteRecreateWaitApplicableImpl is not being used.

I'm also thinking it would be good to have more guardrails on KubernetesDeserializer.registerCustomKind given its static nature.

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

iocanel

I like where this is going.
This will reduce the size of the client.
If we manage to completely eliminate the use of velocity transformations we will further reduce the number of transitives too.

The tricky part seemed to be the handling of custom impls, which you seem to handle already with the static block.

+1

metacosm · 2021-07-17T10:01:42Z

kubernetes-client/src/main/java/io/fabric8/kubernetes/client/BaseKubernetesClient.java

@@ -132,6 +127,21 @@
 * It is thread safe.
 */
 public abstract class BaseKubernetesClient<C extends Client> extends BaseClient implements GenericKubernetesClient<C> {
+
+  static {
+    Handlers.register(Pod.class, PodOperationsImpl::new);


Perhaps the Handlers class could be generated during the build process so that we don't need to manually register new ones?

There are a couple of approaches to be weighed:

something generated - but that will need the some templating mechanism, which we should otherwise be able to remove

just a reflective association by convention similar to List and Builder classes - bit it seemed more involved to determine the package name, so I didn't take that approach yet. There is some validation logic in my next commit thought that could easily determine if an OperationsImpl is not being picked up by the Handler system that would be applicable here.

more logic around the initialization - this is what I've started to added in the next commit. If the *ExtensionAdapter class uses references the *Client then that can trigger the static block to register the handlers. Alternatively that registration logic could be moved onto the *ExtensionAdapter (which is probably a better option than the former).

shawkins · 2021-07-17T20:09:45Z

This next commit rationalizes the SharedInformerFactory logic and updates the ServiceCatalog client to the new approach. The DefaultSharedIndexInformerTest.testCustomResourceInformerWithDifferentVersions is not working yet because the direct usage of the OperationContext means that I'll need to add logic to override the ResourceDefinitionContext.

centos-ci · 2021-07-17T20:09:46Z

Can one of the admins verify this patch?

shawkins · 2021-07-18T03:50:13Z

I've combined the last two commits and updated things mentioned in the last couple of comments to where this now compiles and runs through the kubernetes-tests

shawkins · 2021-07-19T16:44:46Z

@rohanKanojia @manusa this now should be pretty representative of these changes - it should build without error not withstanding the latest unsuccessful run. With this only the SharedInformerFactory.sharedIndexInformerFor(Class apiTypeClass, long resyncPeriodInMillis) is not deprecated.

manusa · 2021-07-20T08:42:15Z

I haven't deeply reviewed it, but I really like what's going on.

metacosm · 2021-07-20T09:04:31Z

I haven't deeply reviewed it, but I really like what's going on.

Same here, though I see several Todos in different spots… Are these expected to be addressed in subsequent PRs or is this PR still a work in progress?

shawkins · 2021-07-20T11:23:49Z

@metacosm @manusa I've resolved the TODOs that seem important for now. The ones that remain are a reminder about KubernetesDeserializer type registration and a reminder to remove some of the remaining overlap between the SharedInformerFactory and BaseOperation wrt creating informers. I can change those comments if needed.

- deprecated methods dealing with CustomResources, and added resources methods where appropriate. This means that nearly every method on sharedinformerfactory is now deprecated - replaced CustomResourceDefinitionContext with ResourceDefinitionContext in the api where appropriate - what were generated handlers are now obtained through Handlers methods - passing the type/listType to the base HasMetadataOperation to validate that the operation is registered - moved OperationImpl registration onto the ExtensionAdapters

shawkins · 2021-07-23T13:39:28Z

Took a pass through to rebase and cleanup the import changes.

The things to look at for review - deprecated / new methods on KubernetesClient and SharedInformerFactory

There's nothing special about the CustomResource base class after these changes, so methods dealing with CustomResources have been deprecated, and similar methods involving "resources" were added. This means that nearly every method on sharedinformerfactory is now deprecated

replaced CustomResourceDefinitionContext with ResourceDefinitionContext in the api where appropriate

Does that seem like a good classname?

what were generated handlers are now obtained through Handlers methods,
There's new reflective access was added for Builders. Where we look for List/Builder classes was expanded to include the classloader of the type itself.
We're now passing the type/listType to the base HasMetadataOperation to validate
that the operation is registered
moved OperationImpl registration onto the ExtensionAdapters, with a static block in BaseKubernetesClient to load the default operations and trigger the load of all the others.

shawkins · 2021-07-23T15:20:15Z

@manusa I'm afraid that keeping this up-to-date will become a full-time job. If possible let's come up with a plan for inclusion to minimize conflicts.

cr_refinement # Conflicts: # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/BaseKubernetesClient.java # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/V1AdmissionRegistrationAPIGroupClient.java # kubernetes-client/src/main/java/io/fabric8/kubernetes/client/V1beta1AdmissionRegistrationAPIGroupClient.java

...talog/client/src/main/java/io/fabric8/servicecatalog/client/DefaultServiceCatalogClient.java

manusa · 2021-07-27T10:24:57Z

extensions/tekton/client/src/main/java/io/fabric8/tekton/client/V1beta1APIGroupClient.java

  }

  @Override
  public NonNamespaceOperation<ClusterTask, ClusterTaskList, Resource<ClusterTask>> clusterTasks() {
-    return new ClusterTaskOperationsImpl(this.getHttpClient(), this.getConfiguration());
+    return Handlers.getOperation(ClusterTask.class, ClusterTaskList.class, this.getHttpClient(), this.getConfiguration());


#3327 (review)

I'm confused now. I assumed this was possible until I saw the code in the linked comment. Now I don't get why in those lines we can follow the same procedure.

You could. ClusterServiceBrokerOperationsImpl will be registered so you can get it through Handlers. However I was attempting to create the minimal changes at first, which would just be to remove the usage of the generated ones.

Captured as #3357

...nt/src/main/java/io/fabric8/kubernetes/client/dsl/internal/KubernetesListOperationsImpl.java

...t/src/main/java/io/fabric8/kubernetes/client/dsl/internal/core/v1/ServiceOperationsImpl.java

metacosm · 2021-07-27T19:54:41Z

Initial tests look positive though, surprisingly, for the same app, the native binary size is larger now than it previously was… For comparison, the same code went from 122 MB with 5.4.1 to 138MB with 5.6 to 140MB with 5.7-SNAPSHOT and this PR, which is quite surprising to me. :(

shawkins · 2021-07-27T20:32:26Z

which is quite surprising to me. :(

It is surprising but indicates that the native compilation is very efficient. So the takeaway is that the jar sizes should go down, but the native doesn't really change.

sonarcloud · 2021-07-27T20:42:28Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities
0 Security Hotspots
15 Code Smells

35.8% Coverage
4.1% Duplication

manusa

LGTM, thx!

😍

shawkins requested review from metacosm, manusa and rohanKanojia July 17, 2021 03:18

iocanel reviewed Jul 17, 2021

View reviewed changes

metacosm reviewed Jul 17, 2021

View reviewed changes

shawkins force-pushed the cr_refinement branch from 5ef517b to 196d086 Compare July 18, 2021 03:47

shawkins force-pushed the cr_refinement branch 3 times, most recently from 2fd9243 to 201487d Compare July 19, 2021 12:16

shawkins force-pushed the cr_refinement branch from 201487d to cd304ba Compare July 19, 2021 17:13

shawkins marked this pull request as ready for review July 19, 2021 22:30

shawkins force-pushed the cr_refinement branch from cd304ba to 7037b9f Compare July 23, 2021 13:02

shawkins force-pushed the cr_refinement branch from 7037b9f to 6730ecb Compare July 23, 2021 13:33

shawkins mentioned this pull request Jul 23, 2021

Allow delete with optimistic lock #3323

Merged

11 tasks

shawkins mentioned this pull request Jul 23, 2021

Remove or refine CreateOnlyResourceOperationsImpl #3350

Closed

manusa requested review from metacosm and iocanel July 27, 2021 09:33

manusa reviewed Jul 27, 2021

View reviewed changes

...talog/client/src/main/java/io/fabric8/servicecatalog/client/DefaultServiceCatalogClient.java Show resolved Hide resolved

manusa reviewed Jul 27, 2021

View reviewed changes

...nt/src/main/java/io/fabric8/kubernetes/client/dsl/internal/KubernetesListOperationsImpl.java Outdated Show resolved Hide resolved

manusa reviewed Jul 27, 2021

View reviewed changes

...t/src/main/java/io/fabric8/kubernetes/client/dsl/internal/core/v1/ServiceOperationsImpl.java Show resolved Hide resolved

shawkins mentioned this pull request Jul 27, 2021

Access all OperationsImpl through Handlers #3357

Closed

adding a changelog and several small refinements

418886d

shawkins force-pushed the cr_refinement branch from 234702e to 418886d Compare July 27, 2021 20:13

rohanKanojia approved these changes Jul 28, 2021

View reviewed changes

metacosm approved these changes Jul 28, 2021

View reviewed changes

manusa approved these changes Jul 28, 2021

View reviewed changes

manusa merged commit 8d380a3 into fabric8io:master Jul 28, 2021

manusa added this to the 5.7.0 milestone Jul 28, 2021

This was referenced Aug 1, 2021

resource and resourceList methods #3364

Closed

resources should return the registered operation when possible #3378

Closed

shawkins mentioned this pull request Aug 14, 2021

fix: use proper computation logic of Kind and API version #2679

Closed

11 tasks

This was referenced Aug 20, 2021

Upgrade to 2.1.3.Final and fix 999-SNAPSHOT quarkus-qe/quarkus-openshift-test-suite#326

Merged

Unable to get resources from Handlers.get(kind, apiVersion) #3413

Closed

manusa mentioned this pull request Sep 7, 2021

Issue with Editable* Resources since 5.7.0 #3433

Closed

shawkins mentioned this pull request Sep 22, 2021

Add Group and Version annotations to generated classes #2972

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Removing generated ResourceHandlers #3327

Removing generated ResourceHandlers #3327

shawkins commented Jul 17, 2021 •

edited

iocanel left a comment

metacosm Jul 17, 2021

shawkins Jul 17, 2021

shawkins commented Jul 17, 2021

centos-ci commented Jul 17, 2021

shawkins commented Jul 18, 2021

shawkins commented Jul 19, 2021

manusa commented Jul 20, 2021

metacosm commented Jul 20, 2021

shawkins commented Jul 20, 2021

shawkins commented Jul 23, 2021

shawkins commented Jul 23, 2021

manusa Jul 27, 2021

shawkins Jul 27, 2021

shawkins Jul 27, 2021

metacosm commented Jul 27, 2021

shawkins commented Jul 27, 2021

sonarcloud bot commented Jul 27, 2021

manusa left a comment

Removing generated ResourceHandlers #3327

Removing generated ResourceHandlers #3327

Conversation

shawkins commented Jul 17, 2021 • edited

Description

Type of change

Checklist

iocanel left a comment

Choose a reason for hiding this comment

metacosm Jul 17, 2021

Choose a reason for hiding this comment

shawkins Jul 17, 2021

Choose a reason for hiding this comment

shawkins commented Jul 17, 2021

centos-ci commented Jul 17, 2021

shawkins commented Jul 18, 2021

shawkins commented Jul 19, 2021

manusa commented Jul 20, 2021

metacosm commented Jul 20, 2021

shawkins commented Jul 20, 2021

shawkins commented Jul 23, 2021

shawkins commented Jul 23, 2021

manusa Jul 27, 2021

Choose a reason for hiding this comment

shawkins Jul 27, 2021

Choose a reason for hiding this comment

shawkins Jul 27, 2021

Choose a reason for hiding this comment

metacosm commented Jul 27, 2021

shawkins commented Jul 27, 2021

sonarcloud bot commented Jul 27, 2021

manusa left a comment

Choose a reason for hiding this comment

shawkins commented Jul 17, 2021 •

edited