Fix leaking ingress resources in federated ingress e2e test. #34652
Conversation
ghost
added
priority/important-soon
ghost
mentioned this pull request
k8s-github-robot
added
the
size/S
| @@ -268,6 +276,15 @@ func deleteIngressOrFail(clientset *fedclientset.Clientset, namespace string, in | |||
| framework.ExpectNoError(err, "Error deleting ingress %q from namespace %q", ingressName, namespace) | |||
| } | |||
|
|
|||
| // TODO: quinton: This is largely a cut 'n paste of the above. Yuck! Refactor as soon as we have a common interface implmented by both fedclientset.Clientset and kubeclientset.Clientset | |||
| func deleteClusterIngressOrFail(clusterName string, clientset *kubeclientset.Clientset, namespace string, ingressName string) { | |||
There was a problem hiding this comment.
Instead of passing the clientset, you can pass IngressesGetter or IngressInterface or just a func(namespace, ingressName) that deletes the ingress. You wont need to duplicate the func then.
|
lgtm with one suggestion |
| @@ -64,7 +64,7 @@ var _ = framework.KubeDescribe("Federated ingresses [Feature:Federation]", func( | |||
|
|
|||
| It("should be created and deleted successfully", func() { | |||
| framework.SkipUnlessFederated(f.Client) | |||
|
|
|||
| framework.SkipUnlessProviderIs("gce", "gke") // TODO: Federated ingress is not yet supported on non-GCP platforms. | |||
| framework.ExpectNoError(err, "Error deleting ingress %q in namespace %q", ingress.Name, ns) | ||
| framework.ExpectNoError(err, "Error deleting ingress %q/%q in federation", ns, ingress.Name) | ||
| for clusterName, cluster := range clusters { | ||
| err := cluster.Ingresses(ns).Delete(ingress.Name, &v1.DeleteOptions{}) |
There was a problem hiding this comment.
Is there a reason you cannot use deleteClusterIngressOrFail() here?
There was a problem hiding this comment.
Also, I think you need to do something akin to cleanupServiceShardsAndProviderResources() (https://github.com/kubernetes/kubernetes/blob/master/test/e2e/federated-ingress.go#L139) to ensure the underlying cloud resources are cleaned up.
For an Ingress specific example, see the Ingress e2e test - https://github.com/kubernetes/kubernetes/blob/master/test/e2e/ingress_utils.go#L311
But let's get this PR in first.
There was a problem hiding this comment.
Regarding your first comment, I followed the existing style used for the deletion of the federated ingresses.
Agreed about the GCE resources. I'll add that in a separate PR.
madhusudancs
added
the
lgtm
|
@k8s-bot test this issue #IGNORE |
|
@k8s-bot test this [submit-queue is verifying that this PR is safe to merge] |
|
Automatic merge from submit-queue |
jessfraz
added
the
cherry-pick-approved
|
Commit found in the "release-1.4" branch appears to be this PR. Removing the "cherrypick-candidate" label. If this is an error find help to get your PR picked. |
…ck-of-#34652-origin-release-1.4 Automatic merge from submit-queue Automated cherry pick of kubernetes#34652 Cherry pick of kubernetes#34652 on release-1.4. kubernetes#34652: Fix leaking ingress resources in federated ingress e2e test.
Originally the federated ingresses were being deleted, but due to the lack of cascading deletion, the cluster ingresses were never being deleted, leading to leaked GCE loadbalancer resources. This fixes that.
This change is