Consume Topology CR #670
Consume Topology CR #670
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: fmount 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 |
|
@fmount: The following tests failed, say
Full PR test history. Your PR dashboard. 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-sigs/prow repository. I understand the commands that are listed here. |
fmount
force-pushed
the
topology-1
branch
4 times, most recently
from
2c2d3f8 to
01614fb
Compare
| @@ -866,6 +866,13 @@ func (r *GlanceReconciler) apiDeploymentCreateOrUpdate( | |||
| if instance.Spec.KeystoneEndpoint == apiName { | |||
| apiAnnotations[glance.KeystoneEndpoint] = "true" | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
can the topology of a deployment be updated after it get created, or is it immutable? We should probably watch the topology, like we do with the secrets. if it can be updated, all good, if not we have to think about what we should do in that case.
There was a problem hiding this comment.
It's not immutable and I'm able to see reconciliation loops being triggered by a change in the topology.
Adding the same watching logic here would simply be redundant, because glanceapi_controller (where I pass the topology reference) has already such watcher, and it triggers a reconciliation if something changes even when the topologyRef is a glance top level parameter (tested locally).
There was a problem hiding this comment.
ah right, have not looked closely at the api controller
| @@ -866,6 +866,13 @@ func (r *GlanceReconciler) apiDeploymentCreateOrUpdate( | |||
| if instance.Spec.KeystoneEndpoint == apiName { | |||
| apiAnnotations[glance.KeystoneEndpoint] = "true" | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
ah right, have not looked closely at the api controller
| topologyFn := func(_ context.Context, o client.Object) []reconcile.Request { | ||
| result := []reconcile.Request{} | ||
|
|
||
| // get all GlanceAPIs CRs | ||
| glanceAPIs := &glancev1.GlanceAPIList{} | ||
| listOpts := []client.ListOption{ | ||
| client.InNamespace(o.GetNamespace()), | ||
| } | ||
| if err := r.Client.List(context.Background(), glanceAPIs, listOpts...); err != nil { | ||
| r.Log.Error(err, "Unable to retrieve GlanceAPI CRs %w") | ||
| return nil | ||
| } | ||
|
|
||
| for _, cr := range glanceAPIs.Items { | ||
| if cr.Spec.Topology != nil { | ||
| if o.GetName() == cr.Spec.Topology.Name { | ||
| name := client.ObjectKey{ | ||
| Namespace: o.GetNamespace(), | ||
| Name: cr.Name, | ||
| } | ||
| r.Log.Info(fmt.Sprintf("Topology %s is used by GlanceAPI CR %s", o.GetName(), cr.Name)) | ||
| result = append(result, reconcile.Request{NamespacedName: name}) | ||
| } | ||
| } | ||
| } | ||
| if len(result) > 0 { | ||
| return result | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
wondering if we should align all those watch funcs and instead use IndexField where we have named objects to watch and use the findObjectsForSrc. I could see indexing the field is more efficient, but can not confirm.
There was a problem hiding this comment.
Let me try this one and I get back with an update
There was a problem hiding this comment.
I did a few local tests and I here my considerations:
we use a glanceAPIWatchFields []string, which is not a const but it points to some static fields we expect to be defined.
findObjectsForSrc might just work fine as long as I add some logic to inject the dynamic field I want to check with something like:
glanceWatchFields = addFieldToWatchList(glanceAPIWatchFields, fmt.Sprintf(".spec.%s.topology", instance.APIName()))where addFieldToWatchList would be something like:
// addToWatchList - utility function to pull the element out of the watchlist
func addFieldToWatchList(glanceAPIWatchFields []string, field string) []string {
for _, v := range glanceAPIWatchFields {
if v == field {
return glanceAPIWatchFields
}
}
return append(glanceAPIWatchFields, field)
}However, considering that TopologyRef might not exist at any point in time (because I simply edited my ctlplane CR and I removed the reference), I need to pop that value from the glanceAPIWatchList. I can also avoid doing that by simply reassigning the glanceWatchField the the original slice, but I do see that concurrent reconciliation loops might not see the latest version of this glanceWatchFields []string variable, leading to some errors because we're trying to get a field that does not exist.
I also have concerns in the way we IndexField for a dynamic one (based on the CR name).
For this reason, and to keep things simple, I would leave Topology to his dedicated function, but I agree (looking at the current code), that we can potentially map a few other parameters to this existing function, simplifying the code further. That would be part of a dedicated change.
If you think there's a better way to watch a dynamic field that can be created/updated/removed during the lifecycle of a GlanceAPI object, I can try to experiment more to find the right solution.
Signed-off-by: Francesco Pantano <[email protected]>
No description provided.