✨ Add ClusterClass patch engine #5534

sbueringer · 2021-10-29T14:02:32Z

What this PR does / why we need it:
This PR implements the patch engine and variable calculation described in "Applying Patches" in: #5460 (Note: there are also code walkthroughs linked in the issue, the second one ~ still matches the code from this PR).

Goal of this PR is to add the patch engine and variable calculation.

The implementation for the inline JSON patches will be added after this PR has been merged via #5583.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

sbueringer · 2021-10-29T14:25:58Z

cc @vincepri @fabriziopandini @ykakarap @killianmuldoon @randomvariable @enxebre @schrej @vuil
(not sure who else might be interested at that stage, sorry for the ping if you're not :))

ykakarap · 2021-11-02T01:59:09Z

This is great @sbueringer!! 🚀

First pass at a high level review.
The overall implementation seems good. 👍

Some high level suggestions:

It would be nice if we can make a core a little more readable. Some of the variable and type names do not reflect their purpose/intent. Some examples:
- GenerateRequest - >Request? (similarly with GenerateResponse)
- requestToDesiredState -> updateDesiredState
- The variable patches/patch is used in several places with different meaning. In some places it is the GenerateResponse, in some places it is the Patch from the cluster class, etc. Can we rename some of the places to make the variable naming more consistent?
Breaking down engine.go into several files. Grouping some of the patch functions (like patchObject, patchTemplate, patchTemplateSpec), the helper functions (like copySpec, bytesToUnstructured), etc into their own files would help with code maintenance.

Leaving this at a high level review for now as this PR is still a WIP (there are also a few TODOs in the code).

Will do a full review once PR is ready. 🙂

sbueringer · 2021-11-02T10:02:55Z

It would be nice if we can make a core a little more readable. Some of the variable and type names do not reflect their purpose/intent. Some examples:

GenerateRequest - >Request? (similarly with GenerateResponse)

requestToDesiredState -> updateDesiredState

The variable patches/patch is used in several places with different meaning. In some places it is the GenerateResponse, in some places it is the Patch from the cluster class, etc. Can we rename some of the places to make the variable naming more consistent?

Breaking down engine.go into several files. Grouping some of the patch functions (like patchObject, patchTemplate, patchTemplateSpec), the helper functions (like copySpec, bytesToUnstructured), etc into their own files would help with code maintenance.

Thx, so far! :)

Some answers:

GenerateRequest and GenerateResponse were named like this as it's a common pattern in Kubernetes and gRPC to include the operation in the Request/Response name, e.g. AdmissionRequest, AdmissionResponse, TokenRequest, CertificateSigningRequest. I think it's about namespacing in general. Note: right now those structs are only part of an internal interface
requestToDesiredState -> updateDesiredState > Done
I'll rename Request/Response variables to req/resp where necessary
I'll move some helpers to patch.go. wouldn't split it up further as it doesn't necessarily make it more maintainable if the code is distributed over multiple files. (especially as the only consumer right now is the engine)

randomvariable · 2021-11-02T12:53:41Z

I probably won't be able to get round to a full review before Thursday due to other stuff going on, will queue up.

sbueringer · 2021-11-02T16:47:31Z

controllers/topology/internal/extensions/patches/engine_test.go

+	. "sigs.k8s.io/cluster-api/internal/matchers"
+)
+
+func TestApply(t *testing.T) {


I've currently only added one top-level unit test for engine.go. It seems way simpler to just have a few more cases on that level compared to more or less duplicating the whole test setup for request, applyPatchesToRequest and updateDesiredState. Please let's discuss if there are other opinions on that :)

sbueringer · 2021-11-02T16:49:49Z

controllers/topology/internal/extensions/patches/patch_test.go

+	"sigs.k8s.io/cluster-api/controllers/topology/internal/contract"
+)
+
+func TestCopySpec(t *testing.T) {


Similar to engine.go, I've only added one table test for copySpec. If preferred I can duplicate the test cases for the 3 patch funcs.

I thought testing copySpec covers the most cases and the 3 other funcs are transitively tested a lot by the tests in engine_test.go

sbueringer · 2021-11-02T16:53:56Z

PR should be ready for review now. (open TODOs are supposed to be solved in other PRs)

(cc @ykakarap)

sbueringer · 2021-11-04T14:32:32Z

Rebase on top of main, delta:

Aligned to changes in the MachineDeploymentClass builder
Fixed the TODO about requiring IgnoreAutogeneratedMetadata because the EqualObject helper is fixed now
Changed enum in the api package to use iota

fabriziopandini

@sbueringer great stuff!
Mostly nits; the only thing that requires some deep dive is the usage of mergepatch.
I still need to take a better look at test for the engine

docs/proposals/202105256-cluster-class-and-managed-topologies.md

controllers/topology/internal/extensions/patches/variables/variables.go

controllers/topology/internal/extensions/patches/engine.go

controllers/topology/internal/extensions/patches/engine_test.go

controllers/topology/internal/extensions/patches/api/interface.go

controllers/topology/internal/extensions/patches/util.go

controllers/topology/desired_state.go

controllers/topology/internal/extensions/patches/api/interface.go

fabriziopandini · 2021-11-09T17:39:40Z

lgtm pending squash. great stuff!

Signed-off-by: Stefan Büringer buringerst@vmware.com Co-authored-by: fabriziopandini <fpandini@vmware.com>

sbueringer · 2021-11-09T17:48:29Z

lgtm pending squash. great stuff!

Done :)

vincepri

/lgtm

fabriziopandini · 2021-11-09T18:12:43Z

/lgtm
/approve

k8s-ci-robot · 2021-11-09T18:13:00Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fabriziopandini

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~~ [fabriziopandini]

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 added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 29, 2021

k8s-ci-robot requested review from killianmuldoon and stmcginnis October 29, 2021 14:02

k8s-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Oct 29, 2021

sbueringer force-pushed the pr-clusterclass-patches-engine branch from b7ee1a5 to bab2a17 Compare October 29, 2021 14:05

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 29, 2021

sbueringer force-pushed the pr-clusterclass-patches-engine branch from bab2a17 to da56357 Compare October 29, 2021 14:06

sbueringer changed the title ~~[WIP] ✨ Add ClusterClass patches engine~~ [WIP] ✨ Add ClusterClass patch engine Oct 29, 2021

sbueringer mentioned this pull request Oct 29, 2021

ClusterClass patch implementation #5460

Closed

9 tasks

sbueringer force-pushed the pr-clusterclass-patches-engine branch from da56357 to 06c4bc0 Compare October 29, 2021 14:11

sbueringer force-pushed the pr-clusterclass-patches-engine branch from 06c4bc0 to 009c944 Compare November 2, 2021 16:16

sbueringer changed the title ~~[WIP] ✨ Add ClusterClass patch engine~~ ✨ Add ClusterClass patch engine Nov 2, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 2, 2021

sbueringer force-pushed the pr-clusterclass-patches-engine branch 2 times, most recently from 23fe99f to b247ec9 Compare November 2, 2021 16:22

sbueringer commented Nov 2, 2021

View reviewed changes

sbueringer force-pushed the pr-clusterclass-patches-engine branch from 6840cb9 to e3adfc9 Compare November 2, 2021 19:14

sbueringer mentioned this pull request Nov 3, 2021

ClusterClass builtin variables: labels & annotations semantic and use cases #5577

Closed

sbueringer force-pushed the pr-clusterclass-patches-engine branch 2 times, most recently from e81ad77 to bf6dccd Compare November 4, 2021 14:29

sbueringer mentioned this pull request Nov 4, 2021

✨ ClusterClass json patches #5583

Merged

fabriziopandini reviewed Nov 4, 2021

View reviewed changes

fabriziopandini reviewed Nov 5, 2021

View reviewed changes

randomvariable reviewed Nov 8, 2021

View reviewed changes

controllers/topology/internal/extensions/patches/api/interface.go Outdated Show resolved Hide resolved

sbueringer force-pushed the pr-clusterclass-patches-engine branch 4 times, most recently from 0dca2b5 to 17d5386 Compare November 9, 2021 09:35

fabriziopandini reviewed Nov 9, 2021

View reviewed changes

controllers/topology/internal/extensions/patches/util.go Outdated Show resolved Hide resolved

sbueringer force-pushed the pr-clusterclass-patches-engine branch from 17d5386 to a4dc339 Compare November 9, 2021 13:59

sbueringer mentioned this pull request Nov 9, 2021

[WIP][Experimental prototype] ClusterClass patches #5458

Closed

vincepri reviewed Nov 9, 2021

View reviewed changes

controllers/topology/desired_state.go Outdated Show resolved Hide resolved

controllers/topology/internal/extensions/patches/api/interface.go Show resolved Hide resolved

sbueringer mentioned this pull request Nov 9, 2021

📖 ClusterClass proposal: remove builtin labels & annotations variables, add new #5619

Merged

Add ClusterClass patch engine

1c52899

Signed-off-by: Stefan Büringer buringerst@vmware.com Co-authored-by: fabriziopandini <fpandini@vmware.com>

sbueringer force-pushed the pr-clusterclass-patches-engine branch from 3768cca to 1c52899 Compare November 9, 2021 17:48

vincepri approved these changes Nov 9, 2021

View reviewed changes

k8s-ci-robot assigned vincepri Nov 9, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 9, 2021

k8s-ci-robot assigned fabriziopandini Nov 9, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 9, 2021

k8s-ci-robot merged commit 7233fd7 into kubernetes-sigs:main Nov 9, 2021

k8s-ci-robot added this to the v1.1 milestone Nov 9, 2021

sbueringer deleted the pr-clusterclass-patches-engine branch November 9, 2021 18:30

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ Add ClusterClass patch engine #5534

✨ Add ClusterClass patch engine #5534

sbueringer commented Oct 29, 2021 •

edited

sbueringer commented Oct 29, 2021 •

edited

ykakarap commented Nov 2, 2021

sbueringer commented Nov 2, 2021

randomvariable commented Nov 2, 2021

sbueringer Nov 2, 2021

sbueringer Nov 2, 2021 •

edited

sbueringer commented Nov 2, 2021

sbueringer commented Nov 4, 2021

fabriziopandini left a comment

fabriziopandini commented Nov 9, 2021

sbueringer commented Nov 9, 2021

vincepri left a comment

fabriziopandini commented Nov 9, 2021

k8s-ci-robot commented Nov 9, 2021

✨ Add ClusterClass patch engine #5534

✨ Add ClusterClass patch engine #5534

Conversation

sbueringer commented Oct 29, 2021 • edited

sbueringer commented Oct 29, 2021 • edited

ykakarap commented Nov 2, 2021

sbueringer commented Nov 2, 2021

randomvariable commented Nov 2, 2021

sbueringer Nov 2, 2021

Choose a reason for hiding this comment

sbueringer Nov 2, 2021 • edited

Choose a reason for hiding this comment

sbueringer commented Nov 2, 2021

sbueringer commented Nov 4, 2021

fabriziopandini left a comment

Choose a reason for hiding this comment

fabriziopandini commented Nov 9, 2021

sbueringer commented Nov 9, 2021

vincepri left a comment

Choose a reason for hiding this comment

fabriziopandini commented Nov 9, 2021

k8s-ci-robot commented Nov 9, 2021

sbueringer commented Oct 29, 2021 •

edited

sbueringer commented Oct 29, 2021 •

edited

sbueringer Nov 2, 2021 •

edited