From bcddb157e4a737f29ce9d9dbd22880f2001474b0 Mon Sep 17 00:00:00 2001 From: "Madhusudan.C.S" Date: Thu, 1 Dec 2016 15:27:42 -0800 Subject: [PATCH 1/3] [Federation] Separate the cleanup phases of service and service shards so that service shards can be cleaned up even after the service is deleted elsewhere. This separation is necessary because "Federated Service DNS should be able to discover a federated service" e2e test recently added a case where it deletes the service from federation but not the shards from the underlying clusters. Because of the way cleanup was implemented in the AfterEach block currently, we did not cleanup any of the underlying shards. However, separating the two phases of the cleanup needs this separation. --- test/e2e/federated-service.go | 39 +++++++++++++++++++++++++++++------ 1 file changed, 33 insertions(+), 6 deletions(-) diff --git a/test/e2e/federated-service.go b/test/e2e/federated-service.go index ae465a1c372..8320565f322 100644 --- a/test/e2e/federated-service.go +++ b/test/e2e/federated-service.go @@ -24,6 +24,7 @@ import ( "time" "k8s.io/kubernetes/pkg/api/v1" + "k8s.io/kubernetes/pkg/conversion" "k8s.io/kubernetes/test/e2e/framework" . "github.com/onsi/ginkgo" @@ -139,7 +140,8 @@ var _ = framework.KubeDescribe("[Feature:Federation]", func() { var _ = Describe("DNS", func() { var ( - service *v1.Service + service *v1.Service + serviceShard *v1.Service ) BeforeEach(func() { @@ -168,8 +170,25 @@ var _ = framework.KubeDescribe("[Feature:Federation]", func() { } createBackendPodsOrFail(clusters, nsName, FederatedServicePodName) + service = createServiceOrFail(f.FederationClientset_1_5, nsName, FederatedServiceName) - waitForServiceShardsOrFail(nsName, service, clusters) + obj, err := conversion.NewCloner().DeepCopy(service) + // Cloning shouldn't fail. On the off-chance it does, we should + // shallow copy service to serviceShard before failing. + if err != nil { + serviceCopy := *service + serviceShard = &serviceCopy + framework.ExpectNoError(err, fmt.Sprintf("Error in deep copying service %q", service.Name)) + } + var ok bool + serviceShard, ok = obj.(*v1.Service) + if !ok { + serviceCopy := *service + serviceShard = &serviceCopy + framework.ExpectNoError(err, fmt.Sprintf("Unexpected service object copied %T", obj)) + } + + waitForServiceShardsOrFail(nsName, serviceShard, clusters) }) AfterEach(func() { @@ -180,14 +199,19 @@ var _ = framework.KubeDescribe("[Feature:Federation]", func() { if service != nil { deleteServiceOrFail(f.FederationClientset_1_5, nsName, service.Name) - - By(fmt.Sprintf("Deleting service shards and their provider resources in underlying clusters for service %q in namespace %q", service.Name, nsName)) - cleanupServiceShardsAndProviderResources(nsName, service, clusters) - service = nil } else { By("No service to delete. Service is nil") } + + if serviceShard != nil { + By(fmt.Sprintf("Deleting service shards and their provider resources in underlying clusters for service %q in namespace %q", service.Name, nsName)) + cleanupServiceShardsAndProviderResources(nsName, service, clusters) + serviceShard = nil + } else { + By("No service shards to delete. `serviceShard` is nil") + } + // Delete the kube-dns config map from all clusters. for clusterName, cluster := range clusters { By(fmt.Sprintf("Deleting kube dns config map from cluster: %s", clusterName)) @@ -218,6 +242,9 @@ var _ = framework.KubeDescribe("[Feature:Federation]", func() { By("Deleting the service to verify that DNS rules still work") err := f.FederationClientset_1_5.Services(nsName).Delete(FederatedServiceName, &v1.DeleteOptions{}) framework.ExpectNoError(err, "Error deleting service %q in namespace %q", service.Name, service.Namespace) + // Service is deleted, unset the test block-global service variable. + service = nil + for i, DNSName := range svcDNSNames { discoverService(f, DNSName, true, "federated-service-e2e-discovery-pod-"+strconv.Itoa(i)) } From ae03433a70ddb01b9c2be052a9ea0810395ff368 Mon Sep 17 00:00:00 2001 From: "Madhusudan.C.S" Date: Fri, 2 Dec 2016 10:40:43 -0800 Subject: [PATCH 2/3] Added a comment about the reasoning. --- test/e2e/federated-service.go | 16 ++++++++++++++-- 1 file changed, 14 insertions(+), 2 deletions(-) diff --git a/test/e2e/federated-service.go b/test/e2e/federated-service.go index 8320565f322..da78b605c8e 100644 --- a/test/e2e/federated-service.go +++ b/test/e2e/federated-service.go @@ -173,8 +173,18 @@ var _ = framework.KubeDescribe("[Feature:Federation]", func() { service = createServiceOrFail(f.FederationClientset_1_5, nsName, FederatedServiceName) obj, err := conversion.NewCloner().DeepCopy(service) - // Cloning shouldn't fail. On the off-chance it does, we should - // shallow copy service to serviceShard before failing. + // Cloning shouldn't fail. On the off-chance it does, we + // should shallow copy service to serviceShard before + // failing. If we don't do this we will never really + // get a chance to clean up the underlying services + // when the cloner fails for reasons not in our + // control. For example, cloner bug. That will cause + // the resources to leak, which in turn causes the + // test project to run out of quota and the entire + // suite starts failing. So we must try as hard as + // possible to cleanup the underlying services. So + // if DeepCopy fails, we are going to try with shallow + // copy as a last resort. if err != nil { serviceCopy := *service serviceShard = &serviceCopy @@ -182,6 +192,8 @@ var _ = framework.KubeDescribe("[Feature:Federation]", func() { } var ok bool serviceShard, ok = obj.(*v1.Service) + // Same argument as above about using shallow copy + // as a last resort. if !ok { serviceCopy := *service serviceShard = &serviceCopy From 28a7ff3cd089f1445b0d1bc8aee0df76083b72e8 Mon Sep 17 00:00:00 2001 From: "Madhusudan.C.S" Date: Fri, 2 Dec 2016 10:42:02 -0800 Subject: [PATCH 3/3] Test owners update. --- test/e2e/BUILD | 1 + test/test_owners.csv | 6 ++++++ 2 files changed, 7 insertions(+) diff --git a/test/e2e/BUILD b/test/e2e/BUILD index 6b35a0ba74d..bab200f59b1 100644 --- a/test/e2e/BUILD +++ b/test/e2e/BUILD @@ -151,6 +151,7 @@ go_library( "//pkg/controller/petset:go_default_library", "//pkg/controller/replicaset:go_default_library", "//pkg/controller/replication:go_default_library", + "//pkg/conversion:go_default_library", "//pkg/dns/federation:go_default_library", "//pkg/fields:go_default_library", "//pkg/kubectl:go_default_library", diff --git a/test/test_owners.csv b/test/test_owners.csv index 751b4754b1e..bff46124370 100644 --- a/test/test_owners.csv +++ b/test/test_owners.csv @@ -141,6 +141,7 @@ Federated Services DNS non-local federated service should be able to discover a Federated Services DNS should be able to discover a federated service,derekwaynecarr,1 Federated Services Service creation should create matching services in underlying clusters,jbeda,1 Federated Services Service creation should not be deleted from underlying clusters when it is deleted,sttts,0 +Federated Services Service creation should not be deleted from underlying clusters when it is deleted,madhusudancs,0 Federated Services Service creation should succeed,rmmh,1 Federated ingresses Federated Ingresses Ingress connectivity and DNS should be able to connect to a federated ingress via its load balancer,rmmh,1 Federated ingresses Federated Ingresses should be created and deleted successfully,dchen1107,1 @@ -446,6 +447,7 @@ Services should be able to up and down services,bprashanth,0 Services should check NodePort out-of-range,bprashanth,0 Services should create endpoints for unready pods,maisem,0 Services should only allow access from service loadbalancer source ranges,sttts,0 +Services should only allow access from service loadbalancer source ranges,madhusudancs,0 Services should preserve source pod IP for traffic thru service cluster IP,Random-Liu,1 Services should prevent NodePort collisions,bprashanth,0 Services should provide secure master service,bprashanth,0 @@ -557,6 +559,9 @@ k8s.io/kubernetes/pkg/api/v1,vulpecula,1 k8s.io/kubernetes/pkg/api/v1/endpoints,sttts,0 k8s.io/kubernetes/pkg/api/v1/pod,sttts,0 k8s.io/kubernetes/pkg/api/v1/service,sttts,0 +k8s.io/kubernetes/pkg/api/v1/endpoints,madhusudancs,0 +k8s.io/kubernetes/pkg/api/v1/pod,madhusudancs,0 +k8s.io/kubernetes/pkg/api/v1/service,madhusudancs,0 k8s.io/kubernetes/pkg/api/validation,smarterclayton,1 k8s.io/kubernetes/pkg/api/validation/path,luxas,1 k8s.io/kubernetes/pkg/apimachinery,gmarek,1 @@ -791,6 +796,7 @@ k8s.io/kubernetes/pkg/registry/storage/storageclass,brendandburns,1 k8s.io/kubernetes/pkg/registry/storage/storageclass/etcd,eparis,1 k8s.io/kubernetes/pkg/runtime,wojtek-t,0 k8s.io/kubernetes/pkg/runtime/schema,sttts,0 +k8s.io/kubernetes/pkg/runtime/schema,madhusudancs,0 k8s.io/kubernetes/pkg/runtime/serializer,wojtek-t,0 k8s.io/kubernetes/pkg/runtime/serializer/json,wojtek-t,0 k8s.io/kubernetes/pkg/runtime/serializer/protobuf,wojtek-t,0