operator: Add Keepalived Controller (v2) #3864
Conversation
Hello teddyandrieux,My role is to assist you with the merge of this Status report is not available. |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: |
TeddyAndrieux
changed the title
operator/config/crd/bases/metalk8s.scality.com_clusterconfigs.yaml
Outdated
Show resolved
Hide resolved
| if err != nil { | ||
| r.setVIPConfiguredCondition( | ||
| metav1.ConditionFalse, | ||
| "NamespaceDeletionError", |
There was a problem hiding this comment.
I would suggest using an enum for the various conditions in the types definition (hence it can be documented in the CRD as well) and will uses error handling for clients such as the UI
There was a problem hiding this comment.
For VirtualIPPool object, yes we can but for ClusterConfig, I was thinking about only one "global" condition Ready and then the SubReconciler can manage the Conditions they want, but ... 🤔 I can still do an enum, it just means that if a SubReconciler add a Condition they need to update the ENUM.
Ok fine, I will do it
There was a problem hiding this comment.
Not sure it is possible in Golang but can an Enum extend another one ?
There was a problem hiding this comment.
So I did not define a dedicated Type for it, since it was a mess then because we couldn't have used the metav1.Condition type directly (or we should have built a function to convert from one to the other). But I defined a const close to the CRD per conditions
| } | ||
| } | ||
| // There is no more Virtual Router ID free | ||
| return -1 |
There was a problem hiding this comment.
I wonder if using the return tuple (err, result) pattern wouldn't be more idiomatic in go
There was a problem hiding this comment.
Maybe, but not sure it's worth it here, it will make the usage of the function a bit more complex and it's really a case that should not happen (a user that wants to use more than 255 VIPs at the same time 😕 )
| return utils.Requeue(err) | ||
| } | ||
| for _, pool := range pools.Items { | ||
| r.retrieveVRID(ctx, pool) |
There was a problem hiding this comment.
Shall we handle the possible error here ?
There was a problem hiding this comment.
If I understand correctly the sequence here r.retrieveVRID is mutating r.usedVRID so when we use it we have to be aware of the exact sequence we need to follow. IMO we should try to avoid this imperative paradigm. Maybe we can use an intermediate facade 🤔
There was a problem hiding this comment.
Shall we handle the possible error here ?
Indeed, I forgot about it, I will add it
There was a problem hiding this comment.
I agree it's a bit weird, but I do not see any "clean" solution to do it
If I understand correctly the sequence here
r.retrieveVRIDis mutatingr.usedVRID
Yes
so when we use it we have to be aware of the exact sequence we need to follow
WDYM ?
To explain a bit the behavior here, we retrieve all the defined pools, we iter on those pools to retrieve the "potential" already defined VRID to "cache" them, so that if we need to pick a new VRID we pick one that is not already used
Maybe we can use an intermediate facade
I'm not sure to understand how it could work, except if we have "something" that basically manages all the VirtualIPPools at once, but it makes things complicated and not really good as well IMHO.
There was a problem hiding this comment.
Shall we handle the possible error here ?
Done
| // those nodes | ||
| number := math.Ceil(float64(len(r.instance.Spec.Addresses)) / float64(len(nodes))) | ||
| for node := range nodes { | ||
| nodes[node] = int(number) |
There was a problem hiding this comment.
Instead of casting to int shouldn't we round ?
There was a problem hiding this comment.
We already Ceil, not sure round is better than casting to int in this case, but we could cast to int only once when "computing" number, I will change it
There was a problem hiding this comment.
I moved the cast to int to do it only once
|
|
||
| // Reconcile is part of the main kubernetes reconciliation loop which aims to | ||
| // move the current state of the cluster closer to the desired state. | ||
| func (r *VirtualIPPoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { |
There was a problem hiding this comment.
nit: I'm not a big fan of naming vars with such short name, reviewing it is not sufficiently explicit in my mind
| func (r *VirtualIPPoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { | |
| func (virtualIPPoolReconciler *VirtualIPPoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { |
There was a problem hiding this comment.
I agree, but it's kind of "standard" in Go from what I saw
There was a problem hiding this comment.
Indeed, it seems that the short name indicates the object to which the current method applies. In storage-operator, we replaced it with self, similar to what we use in Python. Not sure which is best, but I think it's useful to differenciate this "method holder" from other variables.
TeddyAndrieux
force-pushed
the
improvement/add-keepalived-controller-v2
branch
10 times, most recently
from
3696efd
to
a55d019
Compare
So that we have a dedicated go `package` for this controller (and variable&co do not overlap between controllers)
This ObjectHandler allow to use a single function to Create, Update and Delete several objects, it also set some standard metadata like Labels and Owner. It use the same "mutate" function logic as the `controllerutil.CreateOrUpdate` function
Using operator-sdk ``` operator-sdk create api --version v1alpha1 --kind VirtualIPPool --resource --controller --namespaced=True make generate make manifests make metalk8s ```
``` make manifests make metalk8s ```
``` make manifests make metalk8s ```
TeddyAndrieux
force-pushed
the
improvement/add-keepalived-controller-v2
branch
3 times, most recently
from
782a592
to
c447811
Compare
| // TODO(user): your logic here | ||
|
|
||
| return utils.EndReconciliation() | ||
| } | ||
|
|
||
| func (r *VirtualIPPoolReconciler) retrieveVRID(ctx context.Context, pool metalk8sscalitycomv1alpha1.VirtualIPPool) error { |
There was a problem hiding this comment.
nit: the name is slightly misleading, we could expect to get a VRID as a result, but it's actually just storing them in "the cache"... Maybe cacheUsedVRIDs instead?
There was a problem hiding this comment.
Also, a quick comment explaining why we use this "cache" pattern could avoid questions in the future, or eager attempts to simplify it (IIUC, we need it as a consequence of the "magic create/update/delete with a mutate handler" pattern)
There was a problem hiding this comment.
IIUC, we need it as a consequence of the "magic create/update/delete with a
mutatehandler" pattern
Not really, we need it, because we do not want to use the same VRID as another VirtualIPPool VIP so we retrieve all the VRID from all the VirtualIPPools
I will rename it and add a comment
There was a problem hiding this comment.
Done (already squashed for this one)
There was a problem hiding this comment.
Yes that part I got, but the fact that we need it attached to the reconciler as a struct field is only because of the mutate, right? Because we refresh this cache on every Reconcile call, no? If we don't, then we may be missing invalidation (when we remove a pool / an address from a pool).
There was a problem hiding this comment.
Oh yes indeed, it could be just a variable in the reconcile function and then we add a "lambda" function so that we give this argument to the mutate, but it will be almost the same. And indeed it's re-computed at every reconcile loop
| // Spread the remaining nodes | ||
| for node, nb := range nodes { | ||
| for i := 0; i < nb; i++ { | ||
| for index := range desired.Addresses { | ||
| if desired.Addresses[index].Node == "" { | ||
| desired.Addresses[index].Node = node | ||
| break | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Isn't this loop slightly wrong? Imagine we have 3 nodes, and 4 VIPs to spread. Ceil(4/3) = 2, so we'll have nb = 2. Which means, IIUC, that we'll assign 2 VIPs to the first node, 2 more to the second node, and none to the third 😕
Is there any use case where we'd have a different nb per node selected in a pool? I don't remember any...
If not, I'd suggest we refactor the getNodesList method to only return a list of candidate node names, and compute the number (max per node) only once, in this mutate handler. Then the loops would be switched around:
for i := 0; i < maxAddressesPerNode; i++ {
for _, node := range nodes {
// assign one address
}
}
There was a problem hiding this comment.
Isn't this loop slightly wrong? Imagine we have 3 nodes, and 4 VIPs to spread.
Ceil(4/3) = 2, so we'll havenb = 2. Which means, IIUC, that we'll assign 2 VIPs to the first node, 2 more to the second node, and none to the third
🤔 hmm indeed, ok I will fix it
Is there any use case where we'd have a different
nbper node selected in a pool? I don't remember any...
If not, I'd suggest we refactor thegetNodesListmethod to only return a list of candidate node names, and compute thenumber(max per node) only once, in thismutatehandler.
The fact is that we do not want to change the address if it's already assigned to one node, see the loop above
if addr.Node != "" {
// The node is already defined
if nodes[addr.Node] == 0 {
// This node is not available, remove it
addr.Node = ""
} else {
nodes[addr.Node]--
}
}There was a problem hiding this comment.
Done 2c7edac
| pools := metalk8sscalitycomv1alpha1.VirtualIPPoolList{} | ||
| if err := r.client.List(ctx, &pools); err != nil { | ||
| return utils.Requeue(err) | ||
| } | ||
| for _, pool := range pools.Items { | ||
| if err := r.retrieveVRID(ctx, pool); err != nil { | ||
| return utils.Requeue(err) | ||
| } | ||
| } |
There was a problem hiding this comment.
An idea comes to mind, but let's keep it for another PR.
I'm actually concerned by this kind of logic within a Reconcile pass: we should be reconciling a single instance, without having to retrieve all instances again. I think we should be fine because the client already has a cache built in, so there wouldn't be an extra call sent to K8s API (neither for the "list pools" nor for the "get configmap").
Still, it would be cleaner (IMO) to have each Pool reconciliation update the cache (about the VRIDs it's using) without having to recreate it on each pass for each Pool.
There was a problem hiding this comment.
I agree that it's not ideal but I don't see any better approach.
Still, it would be cleaner (IMO) to have each Pool reconciliation update the cache (about the VRIDs it's using) without having to recreate it on each pass for each Pool.
It will not work well, it means the first VirtualIPPool to get reconciled may take one VRID already taken by another Pool, or we need to load all the pools at the startup of the controller but not ideal as well.
TBH, we will never have tonnes of VirtualIPPools at once so I don't think it's really a problem, even if I agree it's not ideal.
| configuredConditionName = "Configured" | ||
| availableConditionName = "Available" | ||
| readyConditionName = "Ready" |
There was a problem hiding this comment.
Not sure if that's the right place, or if we should add it to the description in OpenAPI spec, but it would be nice to explain each condition type and their nuances (e.g. Available = at least one up, Ready = all up).
There was a problem hiding this comment.
Ok I will add some explanation
There was a problem hiding this comment.
Done d79b19c
Re-generate go code and manifests using: ``` make generate make manifests make metalk8s ```
``` make manifests make metalk8s ```
Re-generate go code and manifests using: ``` make generate make manifests make metalk8s ```
Re-generate go code and manifests using: ``` make generate make manifests make metalk8s ```
``` make manifests make metalk8s ```
Since, today, the CNI config and Ingress server certificate are managed by Salt and we want to use the ClusterConfig Virtual IPs to access the Workload Plane Ingress, retrieve those configured Virtual IPs in Salt so that we use them to setup CNI and Ingress server certificate
``` make manifests make metalk8s ```
``` make manifests make metalk8s ```
TeddyAndrieux
force-pushed
the
improvement/add-keepalived-controller-v2
branch
from
2e23ee5
to
2ea50ae
Compare
|
/approve |
Waiting for approvalThe following approvals are needed before I can proceed with the merge:
Peer approvals must include at least 1 approval from the following list: The following reviewers are expecting changes from the author, or must review again: The following options are set: approve |
In the queueThe changeset has received all authorizations and has been added to the The changeset will be merged in:
The following branches will NOT be impacted:
There is no action required on your side. You will be notified here once IMPORTANT Please do not attempt to modify this pull request.
If you need this pull request to be removed from the queue, please contact a The following options are set: approve |
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
Please check the status of the associated issue None. Goodbye teddyandrieux. |
| // Set a condition on VirtualIPPool | ||
| func (v *VirtualIPPool) SetCondition(kind string, status metav1.ConditionStatus, reason string, message string) { | ||
| setCondition(v.Generation, &v.Status.Conditions, kind, status, reason, message) | ||
| } | ||
|
|
||
| // Get a condition from VirtualIPPool | ||
| func (v *VirtualIPPool) GetCondition(kind string) *Condition { | ||
| return getCondition(v.Status.Conditions, kind) | ||
| } | ||
|
|
||
| // Set Configured Condition | ||
| func (v *VirtualIPPool) SetConfiguredCondition(status metav1.ConditionStatus, reason string, message string) { | ||
| v.SetCondition(configuredConditionName, status, reason, message) | ||
| } | ||
|
|
||
| // Set Available Condition | ||
| func (v *VirtualIPPool) SetAvailableCondition(status metav1.ConditionStatus, reason string, message string) { | ||
| v.SetCondition(availableConditionName, status, reason, message) | ||
| } | ||
|
|
||
| // Set Ready Condition | ||
| func (v *VirtualIPPool) SetReadyCondition(status metav1.ConditionStatus, reason string, message string) { | ||
| v.SetCondition(readyConditionName, status, reason, message) | ||
| } | ||
|
|
||
| // Get Ready Condition | ||
| func (v *VirtualIPPool) GetReadyCondition() *Condition { | ||
| return v.GetCondition(readyConditionName) | ||
| } |
There was a problem hiding this comment.
just a minor note: randomly i saw this PR, not understand much but you can remove the comment, the function/method name are clean enough so self-documented
There was a problem hiding this comment.
Agree, but it's good practice in Golang to have a comment for every "exported" method (I'm not sure we do have a comment for all of them here... but ideally we should)
golint would complain if we do not have such comments (we should add golint validation in the CI btw)
(Second version of #3856, this time we have a dedicated CRD for VirtualIPPool)
pkgto ease "muli controller" approachVirtualIPPoolAPI and resourceVirtualIPPoolcreation/update/deletion from ClusterConfig controllerNOTE: For reviewers, you should review commits one by one
NOTE: Some features will be handled later (in other PRs)