Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Stop deleting underlying services when federation service is deleted #37353

Merged
merged 1 commit into from
Nov 30, 2016
Merged

Stop deleting underlying services when federation service is deleted #37353

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
60 changes: 0 additions & 60 deletions federation/pkg/federation-controller/service/servicecontroller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -393,41 +393,6 @@ func (s *ServiceController) updateFederationService(key string, cachedService *c
return nil, !retryable
}

func (s *ServiceController) deleteFederationService(cachedService *cachedService) (error, bool) {
// handle available clusters one by one
var hasErr bool
for clusterName, cluster := range s.clusterCache.clientMap {
err := s.deleteClusterService(clusterName, cachedService, cluster.clientset)
if err != nil {
hasErr = true
} else if err := s.ensureDnsRecords(clusterName, cachedService); err != nil {
hasErr = true
}
}
if hasErr {
// detail error has been dumpped inside the loop
return fmt.Errorf("Service %s/%s was not successfully updated to all clusters", cachedService.lastState.Namespace, cachedService.lastState.Name), retryable
}
return nil, !retryable
}

func (s *ServiceController) deleteClusterService(clusterName string, cachedService *cachedService, clientset *kubeclientset.Clientset) error {
service := cachedService.lastState
glog.V(4).Infof("Deleting service %s/%s from cluster %s", service.Namespace, service.Name, clusterName)
var err error
for i := 0; i < clientRetryCount; i++ {
err = clientset.Core().Services(service.Namespace).Delete(service.Name, &v1.DeleteOptions{})
if err == nil || errors.IsNotFound(err) {
glog.V(4).Infof("Service %s/%s deleted from cluster %s", service.Namespace, service.Name, clusterName)
delete(cachedService.endpointMap, clusterName)
return nil
}
time.Sleep(cachedService.nextRetryDelay())
}
glog.V(4).Infof("Failed to delete service %s/%s from cluster %s, %+v", service.Namespace, service.Name, clusterName, err)
return err
}

func (s *ServiceController) ensureClusterService(cachedService *cachedService, clusterName string, service *v1.Service, client *kubeclientset.Clientset) error {
var err error
var needUpdate bool
Expand Down Expand Up @@ -931,31 +896,6 @@ func (s *ServiceController) processServiceUpdate(cachedService *cachedService, s
// we should retry in that Duration.
func (s *ServiceController) processServiceDeletion(key string) (error, time.Duration) {
glog.V(2).Infof("Process service deletion for %v", key)
cachedService, ok := s.serviceCache.get(key)
if !ok {
return fmt.Errorf("Service %s not in cache even though the watcher thought it was. Ignoring the deletion.", key), doNotRetry
}
service := cachedService.lastState
cachedService.rwlock.Lock()
defer cachedService.rwlock.Unlock()
s.eventRecorder.Event(service, v1.EventTypeNormal, "DeletingDNSRecord", "Deleting DNS Records")
// TODO should we delete dns info here or wait for endpoint changes? prefer here
// or we do nothing for service deletion
//err := s.dns.balancer.EnsureLoadBalancerDeleted(service)
err, retry := s.deleteFederationService(cachedService)
if err != nil {
message := "Error occurs when deleting federation service"
if retry {
message += " (will retry): "
} else {
message += " (will not retry): "
}
s.eventRecorder.Event(service, v1.EventTypeWarning, "DeletingDNSRecordFailed", message)
return err, cachedService.nextRetryDelay()
}
s.eventRecorder.Event(service, v1.EventTypeNormal, "DeletedDNSRecord", "Deleted DNS Records")
s.serviceCache.delete(key)

cachedService.resetRetryDelay()
return nil, doNotRetry
}
30 changes: 30 additions & 0 deletions test/e2e/federated-service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,27 @@ var _ = framework.KubeDescribe("[Feature:Federation]", func() {
}()
waitForServiceShardsOrFail(nsName, service, clusters)
})

It("should not be deleted from underlying clusters when it is deleted", func() {
framework.SkipUnlessFederated(f.ClientSet)
nsName = f.FederationNamespace.Name
service = createServiceOrFail(f.FederationClientset_1_5, nsName, FederatedServiceName)
By(fmt.Sprintf("Successfully created federated service %q in namespace %q. Waiting for shards to appear in underlying clusters", service.Name, nsName))

waitForServiceShardsOrFail(nsName, service, clusters)

By(fmt.Sprintf("Deleting service %s", service.Name))
err := f.FederationClientset_1_5.Services(nsName).Delete(service.Name, &v1.DeleteOptions{})
framework.ExpectNoError(err, "Error deleting service %q in namespace %q", service.Name, service.Namespace)
By(fmt.Sprintf("Deletion of service %q in namespace %q succeeded.", service.Name, nsName))
By(fmt.Sprintf("Verifying that services in underlying clusters are not deleted"))
for clusterName, clusterClientset := range clusters {
_, err := clusterClientset.Core().Services(service.Namespace).Get(service.Name)
if err != nil {
framework.Failf("Unexpected error in fetching service %s in cluster %s, %s", service.Name, clusterName, err)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not entirely sure about failing here. It should log an error and continue?

Also, I would just put this in a defer block.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

aah the comment "Cleanup" was wrong. This is the test. We want to verify that services are not deleted from underlying clusters. Removed the comment.

AfterEach deletes these services.

}
}
})
})

var _ = Describe("DNS", func() {
Expand Down Expand Up @@ -162,6 +183,15 @@ var _ = framework.KubeDescribe("[Feature:Federation]", func() {
for i, DNSName := range svcDNSNames {
discoverService(f, DNSName, true, "federated-service-e2e-discovery-pod-"+strconv.Itoa(i))
}
By("Verified that DNS rules are working as expected")

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)
for i, DNSName := range svcDNSNames {
discoverService(f, DNSName, true, "federated-service-e2e-discovery-pod-"+strconv.Itoa(i))
}
By("Verified that deleting the service does not affect DNS records")
})

Context("non-local federated service", func() {
Expand Down