Merge pull request #9554 from olemarkus/openstack-fixes

Openstack fixes

pkg/apis/kops/v1alpha2/defaults.go

@@ -46,6 +46,7 @@ func SetDefaults_ClusterSpec(obj *ClusterSpec) {
 		obj.Topology.DNS.Type = DNSTypePublic
 	}
 
+	if obj.CloudProvider != "openstack" {
 		if obj.API == nil {
 			obj.API = &AccessSpec{}
 		}
@@ -67,6 +68,8 @@ func SetDefaults_ClusterSpec(obj *ClusterSpec) {
 			obj.API.LoadBalancer.Type = LoadBalancerTypePublic
 		}
 
+	}
+
 	if obj.Authorization == nil {
 		obj.Authorization = &AuthorizationSpec{}
 	}

pkg/model/openstackmodel/firewall.go

@@ -479,7 +479,6 @@ func (b *FirewallModelBuilder) Build(c *fi.ModelBuilderContext) error {
 	if b.UseLoadBalancerForAPI() && b.UseVIPACL() {
 		useVIPACL = true
 	}
-	if b.UseLoadBalancerForAPI() {
 	sg := &openstacktasks.SecurityGroup{
 		Name:             s(b.Cluster.Spec.MasterPublicName),
 		Lifecycle:        b.Lifecycle,
@@ -490,7 +489,6 @@ func (b *FirewallModelBuilder) Build(c *fi.ModelBuilderContext) error {
 	}
 	c.AddTask(sg)
 	sgMap[b.Cluster.Spec.MasterPublicName] = sg
-	}
 	for _, role := range roles {
 
 		// Create Security Group for Role

pkg/model/openstackmodel/servergroup.go

@@ -159,28 +159,28 @@ func (b *ServerGroupModelBuilder) buildInstances(c *fi.ModelBuilderContext, sg *
 			case kops.InstanceGroupRoleBastion:
 				t := &openstacktasks.FloatingIP{
 					Name:      fi.String(fmt.Sprintf("%s-%s", "fip", *instanceTask.Name)),
-					Server:    instanceTask,
 					Lifecycle: b.Lifecycle,
 				}
 				c.AddTask(t)
+				instanceTask.FloatingIP = t
 			case kops.InstanceGroupRoleMaster:
 				if b.Cluster.Spec.CloudConfig.Openstack.Loadbalancer == nil {
 					t := &openstacktasks.FloatingIP{
 						Name:      fi.String(fmt.Sprintf("%s-%s", "fip", *instanceTask.Name)),
-						Server:    instanceTask,
 						Lifecycle: b.Lifecycle,
 					}
 					c.AddTask(t)
 					b.associateFIPToKeypair(t)
+					instanceTask.FloatingIP = t
 				}
 			default:
-				if !b.UsesSSHBastion() {
+				if b.Cluster.Spec.Topology == nil || b.Cluster.Spec.Topology.Nodes == "public" {
 					t := &openstacktasks.FloatingIP{
 						Name:      fi.String(fmt.Sprintf("%s-%s", "fip", *instanceTask.Name)),
-						Server:    instanceTask,
 						Lifecycle: b.Lifecycle,
 					}
 					c.AddTask(t)
+					instanceTask.FloatingIP = t
 				}
 			}
 		} else if b.Cluster.Spec.CloudConfig.Openstack != nil && b.Cluster.Spec.CloudConfig.Openstack.Router == nil {

pkg/model/openstackmodel/servergroup_test.go

@@ -140,7 +140,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				masterFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-master-1-cluster"),
-					Server:    masterInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				nodeServerGroup := &openstacktasks.ServerGroup{
@@ -186,7 +185,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				nodeFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-node-1-cluster"),
-					Server:    nodeInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				return map[string]fi.Task{
@@ -226,6 +224,9 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 							Region: "region",
 						},
 					},
+					Topology: &kops.TopologySpec{
+						Nodes: "private",
+					},
 				},
 			},
 			instanceGroups: []*kops.InstanceGroup{
@@ -325,7 +326,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				masterFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-master-1-cluster"),
-					Server:    masterInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				nodeServerGroup := &openstacktasks.ServerGroup{
@@ -411,7 +411,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				bastionFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-bastion-1-cluster"),
-					Server:    bastionInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				return map[string]fi.Task{
@@ -592,7 +591,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				masterAFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-master-a-1-cluster"),
-					Server:    masterAInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				masterBServerGroup := &openstacktasks.ServerGroup{
@@ -639,7 +637,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				masterBFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-master-b-1-cluster"),
-					Server:    masterBInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				masterCServerGroup := &openstacktasks.ServerGroup{
@@ -686,7 +683,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				masterCFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-master-c-1-cluster"),
-					Server:    masterCInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				nodeAServerGroup := &openstacktasks.ServerGroup{
@@ -732,7 +728,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				nodeAFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-node-a-1-cluster"),
-					Server:    nodeAInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				nodeBServerGroup := &openstacktasks.ServerGroup{
@@ -778,7 +773,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				nodeBFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-node-b-1-cluster"),
-					Server:    nodeBInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				nodeCServerGroup := &openstacktasks.ServerGroup{
@@ -824,7 +818,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				nodeCFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-node-c-1-cluster"),
-					Server:    nodeCInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				return map[string]fi.Task{
@@ -1144,7 +1137,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				nodeAFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-node-a-1-cluster"),
-					Server:    nodeAInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				nodeBServerGroup := &openstacktasks.ServerGroup{
@@ -1190,7 +1182,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				nodeBFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-node-b-1-cluster"),
-					Server:    nodeBInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				nodeCServerGroup := &openstacktasks.ServerGroup{
@@ -1236,7 +1227,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				nodeCFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-node-c-1-cluster"),
-					Server:    nodeCInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				loadbalancer := &openstacktasks.LB{
@@ -1827,7 +1817,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				masterAFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-master-1-cluster"),
-					Server:    masterAInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				masterBPort := &openstacktasks.Port{
@@ -1866,7 +1855,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				masterBFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-master-2-cluster"),
-					Server:    masterBInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				masterCPort := &openstacktasks.Port{
@@ -1905,7 +1893,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				masterCFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-master-3-cluster"),
-					Server:    masterCInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				nodeServerGroup := &openstacktasks.ServerGroup{
@@ -1951,7 +1938,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				nodeAFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-node-1-cluster"),
-					Server:    nodeAInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				nodeBPort := &openstacktasks.Port{
@@ -1989,7 +1975,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				nodeBFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-node-2-cluster"),
-					Server:    nodeBInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				nodeCPort := &openstacktasks.Port{
@@ -2027,7 +2012,6 @@ func Test_ServerGroupModelBuilder(t *testing.T) {
 				}
 				nodeCFloatingIP := &openstacktasks.FloatingIP{
 					Name:      s("fip-node-3-cluster"),
-					Server:    nodeCInstance,
 					Lifecycle: &clusterLifecycle,
 				}
 				return map[string]fi.Task{
@@ -3040,11 +3024,7 @@ func compareFloatingIPs(t *testing.T, actualTask fi.Task, expected *openstacktas
 
 	compareStrings(t, "Name", actual.Name, expected.Name)
 	compareLifecycles(t, actual.Lifecycle, expected.Lifecycle)
-	if pointersAreBothNil(t, "Server", actual.Server, expected.Server) {
 	compareLoadbalancers(t, actual.LB, expected.LB)
-	} else {
-		compareInstances(t, actual.Server, expected.Server)
-	}
 }
 
 func compareLifecycles(t *testing.T, actual, expected *fi.Lifecycle) {

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

@@ -83,7 +83,7 @@ var readBackoff = wait.Backoff{
 // writeBackoff is the backoff strategy for openstack write retries.
 var writeBackoff = wait.Backoff{
 	Duration: time.Second,
-	Factor:   1.5,
+	Factor:   2,
 	Jitter:   0.1,
 	Steps:    5,
 }
@@ -296,7 +296,6 @@ type OpenstackCloud interface {
 
 	ListFloatingIPs() (fips []floatingips.FloatingIP, err error)
 	ListL3FloatingIPs(opts l3floatingip.ListOpts) (fips []l3floatingip.FloatingIP, err error)
-	CreateFloatingIP(opts floatingips.CreateOpts) (*floatingips.FloatingIP, error)
 	CreateL3FloatingIP(opts l3floatingip.CreateOpts) (fip *l3floatingip.FloatingIP, err error)
 	DeleteFloatingIP(id string) error
 	DeleteL3FloatingIP(id string) error

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

@@ -43,24 +43,6 @@ func (c *openstackCloud) GetFloatingIP(id string) (fip *floatingips.FloatingIP,
 	return fip, nil
 }
 
-func (c *openstackCloud) CreateFloatingIP(opts floatingips.CreateOpts) (fip *floatingips.FloatingIP, err error) {
-	done, err := vfs.RetryWithBackoff(writeBackoff, func() (bool, error) {
-
-		fip, err = floatingips.Create(c.ComputeClient(), opts).Extract()
-		if err != nil {
-			return false, fmt.Errorf("CreateFloatingIP: create floating IP failed: %v", err)
-		}
-		return true, nil
-	})
-	if !done {
-		if err == nil {
-			err = wait.ErrWaitTimeout
-		}
-		return fip, err
-	}
-	return fip, nil
-}
-
 func (c *openstackCloud) AssociateFloatingIPToInstance(serverID string, opts floatingips.AssociateOpts) (err error) {
 	done, err := vfs.RetryWithBackoff(writeBackoff, func() (bool, error) {
 		err = floatingips.AssociateInstance(c.ComputeClient(), serverID, opts).ExtractErr()

upup/pkg/fi/cloudup/openstacktasks/floatingip.go

@@ -20,7 +20,6 @@ import (
 	"fmt"
 	"time"
 
-	"github.com/gophercloud/gophercloud/openstack/compute/v2/extensions/floatingips"
 	l3floatingip "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/layer3/floatingips"
 	"k8s.io/apimachinery/pkg/util/wait"
 	"k8s.io/klog"
@@ -33,8 +32,8 @@ import (
 type FloatingIP struct {
 	Name         *string
 	ID           *string
-	Server       *Instance
 	LB           *LB
+	IP           *string
 	Lifecycle    *fi.Lifecycle
 	ForAPIServer bool
 }
@@ -71,27 +70,12 @@ func findL3Floating(cloud openstack.OpenstackCloud, opts l3floatingip.ListOpts)
 	return result, nil
 }
 
-func (e *FloatingIP) findServerFloatingIP(context *fi.Context, cloud openstack.OpenstackCloud) (*string, error) {
-	ips, err := cloud.ListServerFloatingIPs(fi.StringValue(e.Server.ID))
-	if err != nil {
-		return nil, err
-	}
-	// assumes that we do have only one interface and 0-1 floatingip in server, the setup that kops does
-	if len(ips) > 0 {
-		return ips[0], nil
-	}
-	return nil, nil
-}
-
 func (e *FloatingIP) IsForAPIServer() bool {
 	return e.ForAPIServer
 }
 
 func (e *FloatingIP) FindIPAddress(context *fi.Context) (*string, error) {
 	if e.ID == nil {
-		if e.Server != nil && e.Server.ID == nil {
-			return nil, nil
-		}
 		if e.LB != nil && e.LB.ID == nil {
 			return nil, nil
 		}
@@ -110,12 +94,6 @@ func (e *FloatingIP) FindIPAddress(context *fi.Context) (*string, error) {
 			return &fips[0].FloatingIP, nil
 		}
 		return nil, fmt.Errorf("Could not find port floatingips port=%s", fi.StringValue(e.LB.PortID))
-	} else if e.ID == nil && e.Server != nil {
-		floating, err := e.findServerFloatingIP(context, cloud)
-		if err != nil {
-			return nil, fmt.Errorf("Could not find server (\"%s\") floating ip: %v", fi.StringValue(e.Server.ID), err)
-		}
-		return floating, nil
 	}
 
 	fip, err := cloud.GetFloatingIP(fi.StringValue(e.ID))
@@ -129,9 +107,6 @@ func (e *FloatingIP) FindIPAddress(context *fi.Context) (*string, error) {
 func (e *FloatingIP) GetDependencies(tasks map[string]fi.Task) []fi.Task {
 	var deps []fi.Task
 	for _, task := range tasks {
-		if _, ok := task.(*Instance); ok {
-			deps = append(deps, task)
-		}
 		if _, ok := task.(*LB); ok {
 			deps = append(deps, task)
 		}
@@ -177,34 +152,25 @@ func (e *FloatingIP) Find(c *fi.Context) (*FloatingIP, error) {
 		}
 		e.ID = actual.ID
 		return actual, nil
-	} else if e.Server != nil {
+	}
-		fips, err := cloud.ListFloatingIPs()
+	fips, err := cloud.ListL3FloatingIPs(l3floatingip.ListOpts{})
 	if err != nil {
-			return nil, fmt.Errorf("Failed to list floating ip's: %v", err)
+		return nil, fmt.Errorf("failed to list layer 3 floating ip's: %v", err)
 	}
-		if len(fips) == 0 {
+	for _, fip := range fips {
-			return nil, nil
+		if fip.Description == fi.StringValue(e.Name) {
-		}
+			actual := &FloatingIP{
-		var actual *FloatingIP
+				ID:        fi.String(fips[0].ID),
-		for i, fip := range fips {
-			if fip.InstanceID == fi.StringValue(e.Server.ID) {
-				if actual != nil {
-					return nil, fmt.Errorf("Multiple floating ip's associated to server: %s", fi.StringValue(e.Server.ID))
-				}
-				actual = &FloatingIP{
 				Name:      e.Name,
-					ID:        fi.String(fips[i].ID),
+				IP:        fi.String(fip.FloatingIP),
-					Server:    e.Server,
 				Lifecycle: e.Lifecycle,
 			}
-				break
-			}
-		}
-		if actual != nil {
 			e.ID = actual.ID
-		}
+			e.IP = actual.IP
 			return actual, nil
 		}
+	}
+
 	return nil, nil
 }
 
@@ -262,43 +228,18 @@ func (f *FloatingIP) RenderOpenstack(t *openstack.OpenstackAPITarget, a, e, chan
 
 			e.ID = fi.String(fip.ID)
 
-		} else if e.Server != nil {
-
-			if err := e.Server.WaitForStatusActive(t); err != nil {
-				return fmt.Errorf("Failed to associate floating IP to instance %s", *e.Name)
-			}
-
-			// recheck is there floatingip already in port
-			// this can happen for instance when recreating bastion host
-			fips, err := cloud.ListFloatingIPs()
-			if err != nil {
-				return fmt.Errorf("Failed to list floating ip's: %v", err)
-			}
-			for _, fip := range fips {
-				if fip.InstanceID == fi.StringValue(e.Server.ID) {
-					e.ID = fi.String(fip.ID)
-					klog.V(2).Infof("Openstack::RenderOpenstack floatingip found after server is active")
-					return nil
-				}
-			}
-
-			fip, err := cloud.CreateFloatingIP(floatingips.CreateOpts{
-				Pool: external.Name,
-			})
-			if err != nil {
-				return fmt.Errorf("Failed to create floating IP: %v", err)
-			}
-			err = cloud.AssociateFloatingIPToInstance(fi.StringValue(e.Server.ID), floatingips.AssociateOpts{
-				FloatingIP: fip.IP,
-			})
-			if err != nil {
-				return fmt.Errorf("Failed to associated floating IP to instance %s: %v", *e.Name, err)
-			}
-
-			e.ID = fi.String(fip.ID)
-
 		} else {
-			return fmt.Errorf("Must specify either Port or Server!")
+
+			fip, err := cloud.CreateL3FloatingIP(l3floatingip.CreateOpts{
+				FloatingNetworkID: external.ID,
+				Description:       fi.StringValue(e.Name),
+			})
+			if err != nil {
+				return fmt.Errorf("failed to create floating IP: %v", err)
+			}
+			e.ID = fi.String(fip.ID)
+			e.IP = fi.String(fip.FloatingIP)
+
 		}
 		return nil
 	}

upup/pkg/fi/cloudup/openstacktasks/instance.go

@@ -21,6 +21,7 @@ import (
 	"strconv"
 
 	"github.com/gophercloud/gophercloud/openstack/compute/v2/extensions/bootfromvolume"
+	"github.com/gophercloud/gophercloud/openstack/compute/v2/extensions/floatingips"
 	"github.com/gophercloud/gophercloud/openstack/compute/v2/extensions/keypairs"
 	"github.com/gophercloud/gophercloud/openstack/compute/v2/extensions/schedulerhints"
 	"github.com/gophercloud/gophercloud/openstack/compute/v2/servers"
@@ -45,6 +46,7 @@ type Instance struct {
 	Metadata         map[string]string
 	AvailabilityZone *string
 	SecurityGroups   []string
+	FloatingIP       *FloatingIP
 
 	Lifecycle    *fi.Lifecycle
 	ForAPIServer bool
@@ -64,6 +66,9 @@ func (e *Instance) GetDependencies(tasks map[string]fi.Task) []fi.Task {
 		if _, ok := task.(*Port); ok {
 			deps = append(deps, task)
 		}
+		if _, ok := task.(*FloatingIP); ok {
+			deps = append(deps, task)
+		}
 	}
 
 	if e.UserData != nil {
@@ -109,7 +114,8 @@ func (e *Instance) Find(c *fi.Context) (*Instance, error) {
 	if e == nil || e.Name == nil {
 		return nil, nil
 	}
-	serverPage, err := servers.List(c.Cloud.(openstack.OpenstackCloud).ComputeClient(), servers.ListOpts{
+	client := c.Cloud.(openstack.OpenstackCloud).ComputeClient()
+	serverPage, err := servers.List(client, servers.ListOpts{
 		Name: fmt.Sprintf("^%s$", fi.StringValue(e.Name)),
 	}).AllPages()
 	if err != nil {
@@ -160,7 +166,11 @@ func (_ *Instance) CheckChanges(a, e, changes *Instance) error {
 }
 
 func (_ *Instance) ShouldCreate(a, e, changes *Instance) (bool, error) {
-	return a == nil, nil
+	if a == nil {
+		return true, nil
+	}
+
+	return false, nil
 }
 
 func (_ *Instance) RenderOpenstack(t *openstack.OpenstackAPITarget, a, e, changes *Instance) error {
@@ -214,12 +224,29 @@ func (_ *Instance) RenderOpenstack(t *openstack.OpenstackAPITarget, a, e, change
 		e.ID = fi.String(v.ID)
 		e.ServerGroup.Members = append(e.ServerGroup.Members, fi.StringValue(e.ID))
 
-		klog.V(2).Infof("Creating a new Openstack instance, id=%s", v.ID)
+		if e.FloatingIP != nil {
-
+			err = associateFloatingIP(t, e)
-		return nil
+		}
+		if err != nil {
+			return err
 		}
 
-	klog.V(2).Infof("Openstack task Instance::RenderOpenstack did nothing")
+		klog.V(2).Infof("Creating a new Openstack instance, id=%s", v.ID)
+
+	}
+
+	return nil
+}
+
+func associateFloatingIP(t *openstack.OpenstackAPITarget, e *Instance) error {
+	cloud := t.Cloud.(openstack.OpenstackCloud)
+
+	err := cloud.AssociateFloatingIPToInstance(fi.StringValue(e.ID), floatingips.AssociateOpts{
+		FloatingIP: fi.StringValue(e.FloatingIP.IP),
+	})
+	if err != nil {
+		return fmt.Errorf("failed to associated floating IP to instance %s: %v", *e.Name, err)
+	}
 	return nil
 }