Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: Introducing GenericKubernetesResource #3185

Merged
merged 6 commits into from
Jun 8, 2021

Conversation

manusa
Copy link
Member

@manusa manusa commented May 27, 2021

Description

Introduction of the GenericKubernetesResource

This class holds the minimum structure for any valid resource served by the Kubernetes API. i.e. A HasMetadata implementation with an ObjectMeta metadata field and a Map for any additional property/field.

The main purpose of this class is to be able to Serialize/Deserialize any resource served by Kubernetes regardless of a specific type previously registered for that resource.

The Serializer now looks for any registered type for a given resource and in case none is found, the resource is deserialized as a GenericKubernetesResource. The immediate benefit is that deserialization will never fail.

This resource can be later on used in a specific OperationSupport implementation that will provide consistent operation support for any resource.

RawCustomResourceOperationsImpl uses GenericKubernetesResourceOperationsImpl

This brings consistency to the client, but no specific behavioral changes should be noticed. Some changes might be noticed in case of the MockServer and white box testing, since some of the requests have changed, but the result should be no different.

The only major difference is that a .get() for a non-existent resource previously threw an Exception and now returns null (just like the rest of OperationSupport implementations for any other resource).

Redundant methods have been marked as deprecated. We should consider removing them definitely after the next few releases.

Next steps

Improve KubernetesClient#load to support operations for GenericKubernetesResource. This means requesting the applicable CustomResourceDefinition from the cluster in order to infer the plural and scope of the managed resource.

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

@manusa manusa added the wip label May 27, 2021
@centos-ci
Copy link

Can one of the admins verify this patch?

@manusa manusa force-pushed the feat/generic-resource branch 7 times, most recently from 12f40e2 to 4fb28ea Compare May 31, 2021 05:34
@manusa manusa self-assigned this May 31, 2021
@manusa manusa force-pushed the feat/generic-resource branch 4 times, most recently from e4892c9 to 9775769 Compare June 1, 2021 13:11
@shawkins
Copy link
Contributor

shawkins commented Jun 1, 2021

This change will be needed to rework/remove the raw websocket watch logic - it seems broken in the current state because it doesn't fully understand the events (http gone, updating the resource version, lists, etc.).

@manusa manusa force-pushed the feat/generic-resource branch 2 times, most recently from 76ef4a6 to fc899a0 Compare June 1, 2021 15:48
@shawkins
Copy link
Contributor

shawkins commented Jun 1, 2021

Should we hold off on #3156 until after this work? If so, I will split off the additional logic that addresses #3194 as a separate pr.

@manusa manusa force-pushed the feat/generic-resource branch from fc899a0 to b75fe84 Compare June 1, 2021 16:27
@manusa
Copy link
Member Author

manusa commented Jun 2, 2021

Should we hold off on #3156 until after this work? If so, I will split off the additional logic that addresses #3194 as a separate pr.

My intention is to at least finish the initial work on this by today (I already spent 2 days working on it and can't spend more time).

This PR introduces the Generic Resource that can be further used in many areas of the client but those should be tackled separately.

@manusa manusa force-pushed the feat/generic-resource branch 3 times, most recently from 2a3c63b to 83f6b63 Compare June 2, 2021 10:11
@shawkins
Copy link
Contributor

shawkins commented Jun 2, 2021

This PR introduces the Generic Resource that can be further used in many areas of the client but those should be tackled separately.

Okay, I'll hold off on adopting this for now in the mock. In the meantime I've rebased #3156 to master, when you get a chance please review.

@manusa manusa force-pushed the feat/generic-resource branch from 83f6b63 to 319aa79 Compare June 2, 2021 11:39
@manusa manusa removed the wip label Jun 2, 2021
@manusa manusa force-pushed the feat/generic-resource branch from 34560e3 to bab9256 Compare June 2, 2021 12:56
@manusa manusa changed the title WIP feat: Introducing GenericKubernetesResource feat: Introducing GenericKubernetesResource Jun 2, 2021
@manusa manusa marked this pull request as ready for review June 2, 2021 14:00
@manusa manusa requested a review from shawkins June 2, 2021 14:00
@manusa manusa requested a review from oscerd June 2, 2021 14:00
@rohanKanojia rohanKanojia requested a review from iocanel June 2, 2021 14:20
@manusa manusa force-pushed the feat/generic-resource branch 2 times, most recently from fb8bead to e167402 Compare June 3, 2021 13:37
Copy link
Contributor

@shawkins shawkins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@manusa
Copy link
Member Author

manusa commented Jun 4, 2021

Hi @lburgazzoli, since (I think) you are an active user of the RawCustomResources, it would be very interesting to count with your review and check in case something is broken for you with the changes suggested in this PR. 🙏

@lburgazzoli
Copy link
Contributor

@manusa I'll try to build the PR and use it in my project beginning of next week, my knowledge of the k8s client internals is too limited to give any valuable review. If there is an example that show how to use the new resource, I may take a look at it today to see if I spot something missing.

@manusa
Copy link
Member Author

manusa commented Jun 4, 2021

If there is an example that show how to use the new resource, I may take a look at it today to see if I spot something missing.

Basically there is no way to explicitly use the new Resource (as of now). The partial purpose of this PR is for RawCustomResourceOperationsImpl instead of re-implementing everything from scratch (and leading to issues like some of the ones you reported (#3023, and indirectly #2929)) to use the regular base implementations by delegating its logic to the new GenericKubernetesResourceOperationsImpl.

So if you're still using the "unstructured" RawCustomResourceOperationsImpl (kubernetesClient.customResource(CustomResourceDefinitionContext)), the new functionality will kick in. Any troubles you may have had previously with this DSL should remain fixed, and functionality should be complete.

This is also the base to any enhancement or feature we want to perform regarding #2929.

@lburgazzoli
Copy link
Contributor

ok, so I'll try to compile it and see if it breaks something

@manusa manusa force-pushed the feat/generic-resource branch from e167402 to fc292cf Compare June 7, 2021 11:01
@sonarqubecloud
Copy link

sonarqubecloud bot commented Jun 7, 2021

@lburgazzoli
Copy link
Contributor

@manusa I've updated my operator to use the code from this PR and nothing seems to be broken, so LGTM

@manusa
Copy link
Member Author

manusa commented Jun 8, 2021

@manusa I've updated my operator to use the code from this PR and nothing seems to be broken, so LGTM

Great, thx for trying it out. Merging 🚀

@manusa manusa merged commit f050493 into fabric8io:master Jun 8, 2021
@manusa manusa deleted the feat/generic-resource branch June 8, 2021 12:47
@manusa manusa added this to the 5.5.0 milestone Jun 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants