Fixed target group leaking problems #60
Conversation
liwenwu-amazon changed the title 
| ResourceArn: tg.Arn, | ||
| } | ||
| | ||
| tagsOutput, err := vpcLatticeSess.ListTagsForResourceWithContext(ctx, &tagsInput) |
There was a problem hiding this comment.
Check out https://github.com/uber-go/multierr if you're not familiar. I think it probably makes sense to just short circuit, rather than aggregating though.
There was a problem hiding this comment.
add logic to set tagsOutput to nil so the caller will know there is no tags associated to this target group
| "k8s.io/apimachinery/pkg/types" | ||
| "sigs.k8s.io/controller-runtime/pkg/client" | ||
| "sigs.k8s.io/gateway-api/apis/v1alpha2" | ||
| mcs_api "sigs.k8s.io/mcs-api/pkg/apis/v1alpha1" |
There was a problem hiding this comment.
kube style for this is typically to use v1alpha1 or mcsv1alpha1` if there's a conflict.
There was a problem hiding this comment.
will create an issue to track this and cleanup the whole code base #61
| | ||
| httpRoute := &v1alpha2.HTTPRoute{} | ||
| | ||
| if err := t.client.Get(ctx, httprouteName, httpRoute); err == nil { |
There was a problem hiding this comment.
Need to handle this err. I'd recommend inverting this if statement
if err := ...; err != nil { // handle error } // business logic There was a problem hiding this comment.
well this is not an error, just means that the parent HTTPRoute does not exist. I have reverted if and else for it. so it might helped the readability
There was a problem hiding this comment.
What if you get a transient network error? Or a permissions error? 404 is only one type of error that. can occur.
| glog.V(6).Infof("tgname %v is not used by httproute %v\n", tgName, httpRoute) | ||
| } | ||
| } else { | ||
| glog.V(6).Infof("parent httpRoute %v not found for TG name %v namespace %v", httprouteName, srvName, srvNamespace) |
There was a problem hiding this comment.
The current logic will trigger this for more than not found errors. Any transient network error will trigger this.
| } | ||
| } | ||
| | ||
| if tg, err := t.latticeDataStore.GetTargetGroup(*sdkTG.getTargetGroupOutput.Name, true); err == nil { |
There was a problem hiding this comment.
Do we need to handle NotFound and TransientError separately?
There was a problem hiding this comment.
+1, seems if other TransientError is not handled, these targetGroup will be deleted.
| } | ||
| | ||
| return ctrl.Result{}, err | ||
| return ctrl.Result{RequeueAfter: time.Minute * 10}, err |
There was a problem hiding this comment.
I'd recommend against this. Golang's error return pattern is a "result or error" semantic. In this case, you're returning a "result and error". This violates the standard go pattern, and IIUC, controller runtime will ignore this value in 100% of cases since error will always be set.
There was a problem hiding this comment.
Do you have any other mechanism to force controller reconcile every 10 minutes
There was a problem hiding this comment.
Why do you need to force reconcile every 10 minutes?
3 participants
Issue #, if available:
#15
Description of changes:
Use tags to track whether target group should be deleted
Testings:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.