⚠️ Generic claim reconciler #203
Conversation
k8s-ci-robot
added
the
cncf-cla: no
|
Welcome @p-strusiewiczsurmacki-mobica! |
k8s-ci-robot
added
the
size/XL
|
/retest |
|
@p-strusiewiczsurmacki-mobica: Cannot trigger testing until a trusted user reviews the PR and leaves an In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
k8s-ci-robot
added
cncf-cla: yes
schrej
force-pushed
the
generic-claim-reconciler
branch
from
1e5d03c to
6775a51
Compare
k8s-ci-robot
added
the
needs-rebase
schrej
force-pushed
the
generic-claim-reconciler
branch
from
6775a51 to
3e7a3e7
Compare
k8s-ci-robot
removed
the
needs-rebase
| @@ -21,23 +21,16 @@ import ( | |||
| "fmt" | |||
|
|
|||
| "github.com/pkg/errors" | |||
| "golang.org/x/exp/slices" | |||
There was a problem hiding this comment.
If we bump to go v1.21 we can use the builtin slices: https://pkg.go.dev/slices#ContainsFunc
There was a problem hiding this comment.
Updated go to 1.21
| }), | ||
| ) | ||
|
|
||
| if err := r.Adapter.SetupWithManager(ctx, b); err != nil { |
There was a problem hiding this comment.
If SetupWithManager is not set, this will panic. Same for adapter.
Probably it is worth to check if both are null before calling them
There was a problem hiding this comment.
Also, if Adapter is required, this SetupWithManager should return an error if Adapter is null
There was a problem hiding this comment.
Added check for adapter == nil.
Adapter is an interface, so I think that anything passed has to define SetupWithManager, as it won't fulfill interface otherwise. Or is there something I am missing here?
pkg/ipamutil/reconciler.go
Outdated
| if res, err := handler.FetchPool(ctx); err != nil || res != nil { | ||
| if apierrors.IsNotFound(err) { | ||
| err := errors.New("pool not found") | ||
| log.Error(err, "the referenced pool could not be found") | ||
| if !claim.ObjectMeta.DeletionTimestamp.IsZero() { | ||
| return r.reconcileDelete(ctx, claim) | ||
| } | ||
| return ctrl.Result{}, nil | ||
| } | ||
| return ctrl.Result{}, errors.Wrap(err, "failed to fetch pool") | ||
| } | ||
|
|
||
| if pool := handler.GetPool(); pool != nil && annotations.HasPaused(pool) { | ||
| log.Info("IPAddressClaim references Pool which is paused, skipping reconciliation.", "IPAddressClaim", claim.GetName(), "Pool", pool.GetName()) | ||
| return ctrl.Result{}, nil | ||
| } |
There was a problem hiding this comment.
Is there a reason for FetchPool and IPPool being separate operations? IIUC you reconcile the Pool on one, then gets the reconciled Pool. Why not just get the pool?
There was a problem hiding this comment.
I mean, the FetchPool() could be called once, return an IPPool and then you use it. Unless the concern is like, people calling FetchPool on a crazy way and always forcing its reconciliation.
There was a problem hiding this comment.
iirc this just evolved this way. The idea was that the handler stores the pool on it's own and directly accesses it when it's needed. That way we don't need generics or type conversions. But since the reconciler needs to access the pools metadata in some cases I added that GetPool() method as well.
I think we can change FetchPool() to return the pool as well, and remove this.
There was a problem hiding this comment.
Changed FetchPool to return pool, res and err. Deleted GetPool.
|
sorry for the amount of comments and the delay!!! |
| "sigs.k8s.io/controller-runtime/pkg/reconcile" | ||
| ) | ||
|
|
||
| const ( |
There was a problem hiding this comment.
the finalizer usage here is a bit confusing to me.
When is ReleaseAddress used? At which time? And Protect?
Usually it helps me to describe on a more verbose way at some comment something like
"When a new claim arrives, it will do X, then Y, then add a finalizer to guarantee this or that"
But this can be a followup!!!! No need to sweat on it
There was a problem hiding this comment.
I think there is a lot of disagreement around Finalizer Naming. To me a Finalizer takes care of a specific action/function, and is removed after that task is done. When the same controller handles multiple Finalizers it's technically not necessary to have more than one. I think that's the reason while there is often just some generic finalizer to prevent deletion which gets removed after everything is done.
In this case the Finalizers serve different purposes though. The ProtectAddress one prevents deletion until the Claim is deleted. The ReleaseAddress one cleans up the allocated address after the claim was deleted. With the in-cluster provider this of course happens immediately, with e.g. Infoblox this can be asynchronous.
Whether this approach makes sense is debatable, especially since the ProtectAddress finalizer should also prevent the ReleaseAddress finalizer from executing, as we don't want to deallocate the address while it is still in use.
There was a problem hiding this comment.
ok, makes sense. I was looking into the perspective of incluster and keep forgetting that at some points the deletion may be an async operation
Please don't apologise for a thorough review, thanks for making time for it! |
Signed-off-by: Patryk Strusiewicz-Surmacki <[email protected]>
Signed-off-by: Patryk Strusiewicz-Surmacki <[email protected]>
|
@rikatz Thank you for your thorough review! It really helped to find issues I didn't notice before. :) |
| @@ -19,25 +19,18 @@ package controllers | |||
| import ( | |||
| "context" | |||
| "fmt" | |||
| "slices" | |||
There was a problem hiding this comment.
the failures here are happening because probably you also need to bump it into github actions like https://github.com/kubernetes-sigs/cluster-api-ipam-provider-in-cluster/blob/main/.github/workflows/pullrequests.yaml#L17 and https://github.com/kubernetes-sigs/cluster-api-ipam-provider-in-cluster/blob/main/.github/workflows/pullrequests.yaml#L30 and https://github.com/kubernetes-sigs/cluster-api-ipam-provider-in-cluster/blob/main/.github/workflows/pullrequests.yaml#L41
|
/lgtm I think as a next thing (I can help with it!) I would like to add an e2e test here, that compiles the container and keeps adding globalincluster, localincluster, ipaddress, etc so we can do a request/response and are sure that what we expect to happen really happens :D Thank you very very very much @p-strusiewiczsurmacki-mobica and @schrej this is great!!! |
k8s-ci-robot
added
the
lgtm
|
@schrej anything else I'm missing? |
k8s-ci-robot
removed
the
lgtm
|
/lgtm |
k8s-ci-robot
added
the
lgtm
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: p-strusiewiczsurmacki-mobica, schrej 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:
Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
approved
What this PR does / why we need it:
This PR introduces generic claim reconcile - an interface that makes underlying IP address pools transparent for the controller and therefore can be used for easy integration of other IPAM providers.
Changes in this PR were created with @schrej