sandboxclaim fix TemplateNotFound (#444)#445
sandboxclaim fix TemplateNotFound (#444)#445phantooom wants to merge 2 commits intokubernetes-sigs:mainfrom
Conversation
✅ Deploy Preview for agent-sandbox canceled.
|
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: phantooom The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
k8s-ci-robot
added
the
cncf-cla: yes
|
Welcome @phantooom! |
k8s-ci-robot
added
the
needs-ok-to-test
|
Hi @phantooom. Thanks for your PR. I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with Regular contributors should join the org to skip this step. Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. DetailsInstructions 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. |
k8s-ci-robot
added
the
size/M
|
/ok-to-test |
k8s-ci-robot
added
ok-to-test
|
Do you have any suggestions about this pr. thanks @igooch |
|
LGTM, thanks! Would be nice to add some tests. |
|
@igooch @vicentefb - How do we test the impact of this PR on scale latencies and P numbers ? |
| if err := mgr.GetFieldIndexer().IndexField(context.Background(), &extensionsv1alpha1.SandboxClaim{}, templateRefIndexField, func(obj client.Object) []string { | ||
| claim, ok := obj.(*extensionsv1alpha1.SandboxClaim) | ||
| if !ok { | ||
| return nil |
There was a problem hiding this comment.
I'm always wary of silently ignoring errors. I would probably at least log, and in this case panic. Given that I'm suggesting a panic, I think you could just claim := obj.(*extensionsv1alpha1.SandboxClaim)
| // that reference the given SandboxTemplate. This ensures that when a | ||
| // SandboxTemplate is created or updated, any claims waiting for it get reconciled. | ||
| func (r *SandboxClaimReconciler) findClaimsForTemplate(ctx context.Context, obj client.Object) []reconcile.Request { | ||
| template, ok := obj.(*extensionsv1alpha1.SandboxTemplate) |
There was a problem hiding this comment.
Same here TBH, I would probably just do a template := obj.(*extensionsv1alpha1.SandboxClaim)
| client.InNamespace(template.Namespace), | ||
| client.MatchingFields{templateRefIndexField: template.Name}, | ||
| ); err != nil { | ||
| log.FromContext(ctx).Error(err, "failed to list SandboxClaims for template", "template", template.Name) |
There was a problem hiding this comment.
Contrary to the above case, I agree with this not panicking. We don't expect it, but it's maybe "less unexpected" than getting the wrong object type. 👍 for logging so we can see if it is actually happening
|
Thank you @phantooom I actually see this as fixing the issue that we should re-reconcile a claim whenever the template changes / is created. It will help performance, but I don't see this as a performance fix. Do we have any tests that rely on this working correctly? (For example, a test where we create the SandboxClaim and then the SandboxTemplate - and if not, can we tweak one of the existing tests to make sure we are going through this code path). This PR is not marked as fixing #444 and I think that's correct, I actually think the fix for #444 is to return ErrTemplateNotFound i.e. delete this code: But I think to do that we want to merge this PR first. I have a habit of being verbose; blockers are 1) can we just cast to SandboxTemplate and 2) do we have a test or can we reorder an existing test to rely on this index? |
6 participants
#444
https://asciinema.org/a/67w6KQGZBjs09cGl