Merge pull request #16022 from zetaab/fix/servergroupname

openstack: fix instance group validation if using serverGroupName annotation

upup/pkg/fi/cloudup/openstack/cloud.go

@@ -604,22 +604,26 @@ func (c *openstackCloud) DeleteGroup(g *cloudinstances.CloudInstanceGroup) error
 }
 
 func deleteGroup(c OpenstackCloud, g *cloudinstances.CloudInstanceGroup) error {
-	grp := g.Raw.(*servergroups.ServerGroup)
-
-	for _, id := range grp.Members {
-		err := c.DeleteInstanceWithID(id)
+	instances, err := c.ListInstances(servers.ListOpts{
+		Name: fmt.Sprintf("^%s", g.InstanceGroup.Name),
+	})
+	if err != nil {
+		return err
+	}
+	for _, instance := range instances {
+		err := c.DeleteInstanceWithID(instance.ID)
 		if err != nil {
-			return fmt.Errorf("could not delete instance %q: %v", id, err)
+			return fmt.Errorf("could not delete instance %q: %v", instance.ID, err)
 		}
 	}
 
 	ports, err := c.ListPorts(ports.ListOpts{})
 	if err != nil {
-		return fmt.Errorf("Could not list ports %v", err)
+		return fmt.Errorf("could not list ports %v", err)
 	}
 
 	for _, port := range ports {
-		if strings.Contains(port.Name, grp.Name) {
+		if strings.HasPrefix(port.Name, fmt.Sprintf("port-%s", g.InstanceGroup.Name)) {
 			err := c.DeletePort(port.ID)
 			if err != nil {
 				return fmt.Errorf("could not delete port %q: %v", port.ID, err)
@@ -627,11 +631,29 @@ func deleteGroup(c OpenstackCloud, g *cloudinstances.CloudInstanceGroup) error {
 		}
 	}
 
-	err = c.DeleteServerGroup(grp.ID)
+	sgName := g.InstanceGroup.Name
+	if name, ok := g.InstanceGroup.Annotations[OS_ANNOTATION+SERVER_GROUP_NAME]; ok {
+		sgName = name
+	}
+	sgs, err := c.ListServerGroups(servergroups.ListOpts{})
 	if err != nil {
-		return fmt.Errorf("could not server group %q: %v", grp.ID, err)
+		return fmt.Errorf("could not list server groups %v", err)
 	}
 
+	cluster := g.Raw.(*kops.Cluster)
+	for _, sg := range sgs {
+		if fmt.Sprintf("%s-%s", cluster.Name, sgName) == sg.Name {
+			if len(sg.Members) == 0 {
+				err = c.DeleteServerGroup(sg.ID)
+				if err != nil {
+					return fmt.Errorf("could not delete server group %q: %v", sg.ID, err)
+				}
+			} else {
+				klog.Infof("Server group %q still has members (IDs: %s), delete not executed", sg.ID, strings.Join(sg.Members, ", "))
+			}
+			break
+		}
+	}
 	return nil
 }
 
@@ -642,27 +664,11 @@ func (c *openstackCloud) GetCloudGroups(cluster *kops.Cluster, instancegroups []
 func getCloudGroups(c OpenstackCloud, cluster *kops.Cluster, instancegroups []*kops.InstanceGroup, warnUnmatched bool, nodes []v1.Node) (map[string]*cloudinstances.CloudInstanceGroup, error) {
 	nodeMap := cloudinstances.GetNodeMap(nodes, cluster)
 	groups := make(map[string]*cloudinstances.CloudInstanceGroup)
-
-	serverGrps, err := c.ListServerGroups(servergroups.ListOpts{})
-	if err != nil {
-		return nil, fmt.Errorf("unable to list servergroups: %v", err)
-	}
-
-	for _, grp := range serverGrps {
-		name := grp.Name
-		instancegroup, err := matchInstanceGroup(name, cluster.ObjectMeta.Name, instancegroups)
+	for _, ig := range instancegroups {
+		var err error
+		groups[ig.ObjectMeta.Name], err = osBuildCloudInstanceGroup(c, cluster, ig, nodeMap)
 		if err != nil {
-			return nil, fmt.Errorf("error getting instance group for servergroup %q", name)
-		}
-		if instancegroup == nil {
-			if warnUnmatched {
-				klog.Warningf("Found servergrp with no corresponding instance group %q", name)
-			}
-			continue
-		}
-		groups[instancegroup.ObjectMeta.Name], err = osBuildCloudInstanceGroup(c, cluster, instancegroup, grp, nodeMap)
-		if err != nil {
-			return nil, fmt.Errorf("error getting cloud instance group %q: %v", instancegroup.ObjectMeta.Name, err)
+			return nil, fmt.Errorf("error getting cloud instance group %q: %v", ig.ObjectMeta.Name, err)
 		}
 	}
 	return groups, nil

upup/pkg/fi/cloudup/openstack/server_group.go

@@ -84,80 +84,53 @@ func listServerGroups(c OpenstackCloud, opts servergroups.ListOptsBuilder) ([]se
 	}
 }
 
-// matchInstanceGroup filters a list of instancegroups for recognized cloud groups
-func matchInstanceGroup(name string, clusterName string, instancegroups []*kops.InstanceGroup) (*kops.InstanceGroup, error) {
-	var instancegroup *kops.InstanceGroup
-	for _, g := range instancegroups {
-		var groupName string
-
-		switch g.Spec.Role {
-		case kops.InstanceGroupRoleControlPlane, kops.InstanceGroupRoleNode, kops.InstanceGroupRoleBastion:
-			groupName = clusterName + "-" + g.ObjectMeta.Name
-		default:
-			klog.Warningf("Ignoring InstanceGroup of unknown role %q", g.Spec.Role)
-			continue
-		}
-
-		if name == groupName {
-			if instancegroup != nil {
-				return nil, fmt.Errorf("found multiple instance groups matching servergrp %q", groupName)
-			}
-			instancegroup = g
-		}
-	}
-
-	return instancegroup, nil
-}
-
-func osBuildCloudInstanceGroup(c OpenstackCloud, cluster *kops.Cluster, ig *kops.InstanceGroup, g servergroups.ServerGroup, nodeMap map[string]*v1.Node) (*cloudinstances.CloudInstanceGroup, error) {
-	newLaunchConfigName := g.Name
+func osBuildCloudInstanceGroup(c OpenstackCloud, cluster *kops.Cluster, ig *kops.InstanceGroup, nodeMap map[string]*v1.Node) (*cloudinstances.CloudInstanceGroup, error) {
 	cg := &cloudinstances.CloudInstanceGroup{
-		HumanName:     newLaunchConfigName,
+		HumanName:     ig.Name,
 		InstanceGroup: ig,
 		MinSize:       int(fi.ValueOf(ig.Spec.MinSize)),
-		TargetSize:    int(fi.ValueOf(ig.Spec.MinSize)), // TODO: Retrieve the target size from OpenStack?
+		TargetSize:    int(fi.ValueOf(ig.Spec.MinSize)),
 		MaxSize:       int(fi.ValueOf(ig.Spec.MaxSize)),
-		Raw:           &g,
+		Raw:           cluster,
 	}
-	for _, i := range g.Members {
-		instanceId := i
-		if instanceId == "" {
-			klog.Warningf("ignoring instance with no instance id: %s", i)
+
+	instances, err := c.ListInstances(servers.ListOpts{
+		Name: fmt.Sprintf("^%s", ig.Name),
+	})
+	if err != nil {
+		return nil, err
+	}
+	for _, instance := range instances {
+		value, ok := instance.Metadata[TagKopsInstanceGroup]
+		if !ok || value != ig.Name {
 			continue
 		}
-		server, err := servers.Get(c.ComputeClient(), instanceId).Extract()
-		if err != nil {
-			return nil, fmt.Errorf("Failed to get instance group member: %v", err)
-		}
-		igObservedGeneration := server.Metadata[INSTANCE_GROUP_GENERATION]
-		clusterObservedGeneration := server.Metadata[CLUSTER_GENERATION]
+		igObservedGeneration := instance.Metadata[INSTANCE_GROUP_GENERATION]
+		clusterObservedGeneration := instance.Metadata[CLUSTER_GENERATION]
 		observedName := fmt.Sprintf("%s-%s", clusterObservedGeneration, igObservedGeneration)
 		generationName := fmt.Sprintf("%d-%d", cluster.GetGeneration(), ig.Generation)
 
 		status := cloudinstances.CloudInstanceStatusUpToDate
-		if generationName != observedName || server.Status == errorStatus {
+		if generationName != observedName || instance.Status == errorStatus {
 			status = cloudinstances.CloudInstanceStatusNeedsUpdate
 		}
-		cm, err := cg.NewCloudInstance(instanceId, status, nodeMap[instanceId])
+		cm, err := cg.NewCloudInstance(instance.ID, status, nodeMap[instance.ID])
 		if err != nil {
 			return nil, fmt.Errorf("error creating cloud instance group member: %v", err)
 		}
 
-		if server.Flavor["original_name"] != nil {
-			cm.MachineType = server.Flavor["original_name"].(string)
+		if instance.Flavor["original_name"] != nil {
+			cm.MachineType = instance.Flavor["original_name"].(string)
 		}
 
-		ip, err := GetServerFixedIP(server, server.Metadata[TagKopsNetwork])
+		ip, err := GetServerFixedIP(&instance, instance.Metadata[TagKopsNetwork])
 		if err != nil {
-			klog.Warningf("Unable to find fixed ip for %s: %v", server.Name, err)
+			klog.Warningf("Unable to find fixed ip for %s: %v", instance.Name, err)
 		}
 
 		cm.PrivateIP = ip
-
-		cm.Roles = []string{server.Metadata["KopsRole"]}
-
-		cm.State = cloudinstances.State(server.Status)
-
+		cm.Roles = []string{instance.Metadata["KopsRole"]}
+		cm.State = cloudinstances.State(instance.Status)
 	}
 	return cg, nil
 }