Merge pull request #54 from hello2mao/bug-fix-for-extract-annotation

bug fix for extract annotation

docs/controllers/service/examples/nginx-EIP-Postpaid-ByBandwidth-50M.yaml

@@ -32,4 +32,4 @@ spec:
       - name: nginx
         image: nginx
         ports:
-        - containerPort: 80
+        - containerPort: 80

pkg/cloud-provider/load_balancer.go

@@ -18,13 +18,13 @@ package cloud_provider
 
 import (
 	"context"
+	"fmt"
 
 	"github.com/golang/glog"
 	"k8s.io/api/core/v1"
 	"k8s.io/kubernetes/pkg/cloudprovider"
 
 	"k8s.io/cloud-provider-baiducloud/pkg/cloud-sdk/blb"
-	"fmt"
 )
 
 // LoadBalancer returns a balancer interface. Also returns true if the interface is supported, false otherwise.
@@ -39,7 +39,10 @@ func (bc *Baiducloud) LoadBalancer() (cloudprovider.LoadBalancer, bool) {
 func (bc *Baiducloud) GetLoadBalancer(ctx context.Context, clusterName string, service *v1.Service) (status *v1.LoadBalancerStatus, exists bool, err error) {
 	// workaround to support old version, can be removed if not support old version
 	bc.workAround(service)
-	result := ExtractServiceAnnotation(service)
+	result, err := ExtractServiceAnnotation(service)
+	if err != nil {
+		return nil, false, err
+	}
 
 	if len(result.LoadBalancerId) == 0 {
 		return nil, false, nil
@@ -72,8 +75,11 @@ func (bc *Baiducloud) EnsureLoadBalancer(ctx context.Context, clusterName string
 		clusterName, service.Namespace, service.Name, bc.Region, service.Spec.LoadBalancerIP, service.Spec.Ports, service.Annotations)
 	// workaround to support old version, can be removed if not support old version
 	bc.workAround(service)
-	result := ExtractServiceAnnotation(service)
-	err := bc.validateService(service)
+	result, err := ExtractServiceAnnotation(service)
+	if err != nil {
+		return nil, err
+	}
+	err = bc.validateService(service)
 	if err != nil {
 		return nil, err
 	}
@@ -91,17 +97,6 @@ func (bc *Baiducloud) EnsureLoadBalancer(ctx context.Context, clusterName string
 	// ensure EIP
 	pubIP, err := bc.ensureEIP(ctx, clusterName, service, nodes, result, lb)
 	if err != nil {
-		glog.V(3).Infof("[%v %v] EnsureLoadBalancer: ensureEIP failed, so delete BLB. ensureEIP error: %s", service.Namespace, service.Name, err)
-		args := blb.DeleteLoadBalancerArgs{
-			LoadBalancerId: lb.BlbId,
-		}
-		deleteLoadBalancerErr := bc.clientSet.Blb().DeleteLoadBalancer(&args)
-		if deleteLoadBalancerErr != nil {
-			glog.V(3).Infof("[%v %v] EnsureLoadBalancer: delete BLB error: %s", service.Namespace, service.Name, deleteLoadBalancerErr)
-		}
-		if service.Annotations != nil {
-			delete(service.Annotations, ServiceAnnotationLoadBalancerId)
-		}
 		return nil, err
 	}
 
@@ -129,8 +124,17 @@ func (bc *Baiducloud) UpdateLoadBalancer(ctx context.Context, clusterName string
 func (bc *Baiducloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName string, service *v1.Service) error {
 	// workaround to support old version, can be removed if not support old version
 	bc.workAround(service)
-	result := ExtractServiceAnnotation(service)
+	result, err := ExtractServiceAnnotation(service)
+	if err != nil {
+		// if annotation has error, then creation must be failed. So return nil to tell k8s lb has been deleted.
+		return nil
+	}
 	serviceName := getServiceName(service)
+	if len(result.LoadBalancerId) == 0 {
+		glog.V(1).Infof("[%v %v] EnsureLoadBalancerDeleted: target load balancer not create successful. So, no need to delete BLB and EIP", serviceName, clusterName)
+		return nil
+	}
+
 	glog.V(2).Infof("[%v %v] EnsureLoadBalancerDeleted: START lbId=%q", serviceName, clusterName, result.LoadBalancerId)
 
 	// reconcile logic is capable of fully reconcile, so we can use this to delete
@@ -186,4 +190,4 @@ func (bc *Baiducloud) EnsureLoadBalancerDeleted(ctx context.Context, clusterName
 
 	glog.V(2).Infof("[%v %v] EnsureLoadBalancerDeleted: delete %v FINISH", serviceName, clusterName, serviceName)
 	return nil
-}
+}

pkg/cloud-provider/load_balancer_blb.go

@@ -14,6 +14,30 @@ import (
 func (bc *Baiducloud) ensureBLB(ctx context.Context, clusterName string, service *v1.Service, nodes []*v1.Node, serviceAnnotation *ServiceAnnotation) (*blb.LoadBalancer, error) {
 	var lb *blb.LoadBalancer
 	var err error
+
+	// if serviceAnnotation.LoadBalancerId is none, we need to double check from cloud since user can update yaml in a short time causing annotation not attach.
+	// LOG:
+	// Event(v1.ObjectReference{Kind:"Service", Namespace:"default", Name:"eip-lb-service5",
+	// UID:"887fc095-3b25-11e9-8eab-fa163e80f2ac", APIVersion:"v1", ResourceVersion:"172548", FieldPath:""}): type: 'Warning'
+	// reason: 'CreatingLoadBalancerFailed' Error creating load balancer (will retry): not persisting update to service
+	// 'default/eip-lb-service5' that has been changed since we received it: Operation cannot be fulfilled on services
+	// "eip-lb-service5": the object has been modified; please apply your changes to the latest version and try again
+	if len(serviceAnnotation.LoadBalancerId) == 0 {
+		var exists bool
+		lb, exists, err = bc.getBCELoadBalancer(bc.ClusterID + "/" + getServiceName(service))
+		if err != nil {
+			return nil, err
+		}
+		if exists {
+			glog.V(3).Infof("[%v %v] EnsureLoadBalancer serviceAnnotation.LoadBalancerId is none, but we got blb from cloud", service.Namespace, service.Name)
+			serviceAnnotation.LoadBalancerId = lb.BlbId
+			if service.Annotations == nil {
+				service.Annotations = make(map[string]string)
+			}
+			service.Annotations[ServiceAnnotationLoadBalancerId] = lb.BlbId
+		}
+	}
+
 	if len(serviceAnnotation.LoadBalancerId) == 0 { // blb not exist, create one and update annotation
 		glog.V(3).Infof("[%v %v] EnsureLoadBalancer create blb!", service.Namespace, service.Name)
 		vpcId, subnetId, err := bc.getVpcInfoForBLB()
@@ -47,7 +71,7 @@ func (bc *Baiducloud) ensureBLB(ctx context.Context, clusterName string, service
 		if len(lbs) != 1 {
 			tryCount := 0
 			for {
-				tryCount ++
+				tryCount++
 				if tryCount > 10 {
 					return nil, fmt.Errorf("EnsureLoadBalancer create blb success but query get none")
 				}
@@ -68,7 +92,7 @@ func (bc *Baiducloud) ensureBLB(ctx context.Context, clusterName string, service
 			service.Annotations = make(map[string]string)
 		}
 		service.Annotations[ServiceAnnotationLoadBalancerId] = lb.BlbId
-	} else { // blb already exist, get info from cloud
+	} else if lb == nil { // blb already exist, get info from cloud
 		var exists bool
 		lb, exists, err = bc.getBCELoadBalancerById(serviceAnnotation.LoadBalancerId)
 		if err != nil {
@@ -79,6 +103,7 @@ func (bc *Baiducloud) ensureBLB(ctx context.Context, clusterName string, service
 		}
 		glog.V(3).Infof("[%v %v] EnsureLoadBalancer: blb already exists: %v", service.Namespace, service.Name, lb)
 	}
+
 	lb, err = bc.waitForLoadBalancer(lb)
 	if err != nil {
 		return nil, err
@@ -131,4 +156,4 @@ func (bc *Baiducloud) waitForLoadBalancer(lb *blb.LoadBalancer) (*blb.LoadBalanc
 	}
 
 	return lb, nil
-}
+}

pkg/cloud-provider/load_balancer_blb_listener.go

@@ -21,8 +21,8 @@ import (
 
 	"k8s.io/api/core/v1"
 
-	"k8s.io/cloud-provider-baiducloud/pkg/cloud-sdk/blb"
 	"github.com/golang/glog"
+	"k8s.io/cloud-provider-baiducloud/pkg/cloud-sdk/blb"
 )
 
 // PortListener describe listener port
@@ -210,4 +210,4 @@ func (bc *Baiducloud) deleteListener(lb *blb.LoadBalancer, pl []PortListener) er
 		return err
 	}
 	return nil
-}
+}

pkg/cloud-provider/load_balancer_util.go

@@ -69,6 +69,9 @@ func (bc *Baiducloud) validateService(service *v1.Service) error {
 }
 
 func (bc *Baiducloud) getBCELoadBalancer(name string) (lb *blb.LoadBalancer, exists bool, err error) {
+	if len(name) == 0 {
+		return nil, false, fmt.Errorf("LoadBalancerName is empty")
+	}
 	args := blb.DescribeLoadBalancersArgs{
 		LoadBalancerName: name,
 	}
@@ -85,6 +88,9 @@ func (bc *Baiducloud) getBCELoadBalancer(name string) (lb *blb.LoadBalancer, exi
 }
 
 func (bc *Baiducloud) getBCELoadBalancerById(id string) (lb *blb.LoadBalancer, exists bool, err error) {
+	if len(id) == 0 {
+		return nil, false, fmt.Errorf("LoadBalancerId is empty")
+	}
 	args := blb.DescribeLoadBalancersArgs{
 		LoadBalancerId: id,
 	}

pkg/cloud-provider/options.go

@@ -19,6 +19,7 @@ package cloud_provider
 import (
 	"strconv"
 
+	"fmt"
 	"github.com/golang/glog"
 	"k8s.io/api/core/v1"
 )
@@ -109,7 +110,7 @@ type NodeAnnotation struct {
 }
 
 // ExtractServiceAnnotation extract annotations from service
-func ExtractServiceAnnotation(service *v1.Service) *ServiceAnnotation {
+func ExtractServiceAnnotation(service *v1.Service) (*ServiceAnnotation, error) {
 	glog.V(4).Infof("start to ExtractServiceAnnotation: %v", service.Annotations)
 	result := &ServiceAnnotation{}
 	annotation := make(map[string]string)
@@ -141,7 +142,7 @@ func ExtractServiceAnnotation(service *v1.Service) *ServiceAnnotation {
 	if exist {
 		i, err := strconv.Atoi(loadBalancerHealthCheckTimeoutInSecond)
 		if err != nil {
-			glog.V(4).Infof("ServiceAnnotationLoadBalancerHealthCheckTimeoutInSecond must be int")
+			return nil, fmt.Errorf("ServiceAnnotationLoadBalancerHealthCheckTimeoutInSecond must be int")
 		} else {
 			result.LoadBalancerHealthCheckTimeoutInSecond = i
 		}
@@ -151,7 +152,7 @@ func ExtractServiceAnnotation(service *v1.Service) *ServiceAnnotation {
 	if exist {
 		i, err := strconv.Atoi(loadBalancerHealthCheckInterval)
 		if err != nil {
-			glog.V(4).Infof("ServiceAnnotationLoadBalancerHealthCheckInterval must be int")
+			return nil, fmt.Errorf("ServiceAnnotationLoadBalancerHealthCheckInterval must be int")
 		} else {
 			result.LoadBalancerHealthCheckInterval = i
 		}
@@ -161,7 +162,7 @@ func ExtractServiceAnnotation(service *v1.Service) *ServiceAnnotation {
 	if exist {
 		i, err := strconv.Atoi(loadBalancerUnhealthyThreshold)
 		if err != nil {
-			glog.V(4).Infof("ServiceAnnotationLoadBalancerUnhealthyThreshold must be int")
+			return nil, fmt.Errorf("ServiceAnnotationLoadBalancerUnhealthyThreshold must be int")
 		} else {
 			result.LoadBalancerUnhealthyThreshold = i
 		}
@@ -171,7 +172,7 @@ func ExtractServiceAnnotation(service *v1.Service) *ServiceAnnotation {
 	if exist {
 		i, err := strconv.Atoi(loadBalancerHealthyThreshold)
 		if err != nil {
-			glog.V(4).Infof("ServiceAnnotationLoadBalancerHealthyThreshold must be int")
+			return nil, fmt.Errorf("ServiceAnnotationLoadBalancerHealthyThreshold must be int")
 		} else {
 			result.LoadBalancerHealthyThreshold = i
 		}
@@ -201,7 +202,7 @@ func ExtractServiceAnnotation(service *v1.Service) *ServiceAnnotation {
 	if exist {
 		i, err := strconv.Atoi(elasticIPBandwidthInMbps)
 		if err != nil {
-			glog.V(4).Infof("ServiceAnnotationElasticIPBandwidthInMbps must be int")
+			return nil, fmt.Errorf("ServiceAnnotationElasticIPBandwidthInMbps must be int")
 		} else {
 			result.ElasticIPBandwidthInMbps = i
 		}
@@ -211,17 +212,17 @@ func ExtractServiceAnnotation(service *v1.Service) *ServiceAnnotation {
 	if exist {
 		i, err := strconv.Atoi(elasticIPReservationLength)
 		if err != nil {
-			glog.V(4).Infof("ServiceAnnotationElasticIPReservationLength must be int")
+			return nil, fmt.Errorf("ServiceAnnotationElasticIPReservationLength must be int")
 		} else {
 			result.ElasticIPReservationLength = i
 		}
 	}
 
-	return result
+	return result, nil
 }
 
 // ExtractNodeAnnotation extract annotations from node
-func ExtractNodeAnnotation(node *v1.Node) *NodeAnnotation {
+func ExtractNodeAnnotation(node *v1.Node) (*NodeAnnotation, error) {
 	glog.V(4).Infof("start to ExtractNodeAnnotation: %v", node.Annotations)
 	result := &NodeAnnotation{}
 	annotation := make(map[string]string)
@@ -249,5 +250,5 @@ func ExtractNodeAnnotation(node *v1.Node) *NodeAnnotation {
 		result.CCMVersion = ccmVersion
 	}
 
-	return result
+	return result, nil
 }

pkg/cloud-provider/routes.go

@@ -217,7 +217,10 @@ func (bc *Baiducloud) ensureRouteInfoToNode(nodeName, vpcId, vpcRouteTableId, vp
 	if curNode.Annotations == nil {
 		curNode.Annotations = make(map[string]string)
 	}
-	nodeAnnotation := ExtractNodeAnnotation(curNode)
+	nodeAnnotation, err := ExtractNodeAnnotation(curNode)
+	if err != nil {
+		return err
+	}
 	if nodeAnnotation.VpcId != vpcId {
 		curNode.Annotations[NodeAnnotationVpcId] = vpcId
 	}