Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,6 @@ COPY . .

RUN go build -o ./machine-controller-manager ./cmd/manager

FROM registry.ci.openshift.org/openshift/origin-v4.7:base
FROM registry.ci.openshift.org/origin/4.8:base

COPY --from=builder /go/src/sigs.k8s.io/cluster-api-provider-openstack/machine-controller-manager /
23 changes: 22 additions & 1 deletion pkg/apis/openstackproviderconfig/v1alpha1/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -134,7 +134,18 @@ type NetworkParam struct {
NoAllowedAddressPairs bool `json:"noAllowedAddressPairs,omitempty"`
// PortTags allows users to specify a list of tags to add to ports created in a given network
PortTags []string `json:"portTags,omitempty"`
VNICType string `json:"vnicType,omitempty"`
// VNICType specifies the nic type of the ports created on this network
// Default: nova default
// +optional
VNICType string `json:"vnicType,omitempty"`
// PortCount is a positive, non-zero, integer representing the number of ports in this network to attach to each node
// Default: 1
// +optional
PortCount uint `json:"portCount,omitempty"`
// PortSecurity is a boolean value that indicates whether security is enabled on ports created for this subnet
// Default: Nova default
// +optional
PortSecurity *bool `json:"portSecurity,omitempty"`
}

type Filter struct {
Expand Down Expand Up @@ -165,6 +176,16 @@ type SubnetParam struct {

// PortTags are tags that are added to ports created on this subnet
PortTags []string `json:"portTags,omitempty"`

// PortCount is a positive, non-zero, intiger representing the number of ports in this subnet to attach to each node
// Default: 1
// +optional
PortCount uint `json:"portCount,omitempty"`

// PortSecurity is a boolean value that indicates whether security is enabled on ports created for this subnet
// Default: Nova default
// +optional
PortSecurity *bool `json:"portSecurity,omitempty"`
}

type SubnetFilter struct {
Expand Down
143 changes: 84 additions & 59 deletions pkg/cloud/openstack/clients/machineservice.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ import (
netext "github.com/gophercloud/gophercloud/openstack/networking/v2/extensions"
"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/attributestags"
"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/portsbinding"
"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/portsecurity"
"github.com/gophercloud/gophercloud/openstack/networking/v2/extensions/trunks"
"github.com/gophercloud/gophercloud/openstack/networking/v2/networks"
"github.com/gophercloud/gophercloud/openstack/networking/v2/ports"
Expand Down Expand Up @@ -95,11 +96,26 @@ type Instance struct {
}

type ServerNetwork struct {
networkID string
subnetID string
portTags []string
vnicType string
networkID string
subnetID string
portTags []string
vnicType string
portSecurity *bool
portName string
}

// for geting vnic type when listing ports
type portWithPortsbinding struct {
ports.Port
portsbinding.PortsBindingExt
}

// for updating port security
var portWithPortSecurityExtensions struct {
ports.Port
portsecurity.PortSecurityExt
}

type InstanceListOpts struct {
// Name of the image in URL format.
Image string `q:"image"`
Expand Down Expand Up @@ -421,35 +437,61 @@ func (is *InstanceService) InstanceCreate(clusterName string, name string, clust
if err != nil {
return nil, err
}

// Set default number of ports created per network to 1
portCount := 1
if net.PortCount > 1 {
portCount = int(net.PortCount)
}

for _, netID := range ids {
if net.NoAllowedAddressPairs {
netsWithoutAllowedAddressPairs[netID] = struct{}{}
}
if net.Subnets == nil {
nets = append(nets, ServerNetwork{
networkID: netID,
portTags: net.PortTags,
vnicType: net.VNICType,
})
// Create one NIC per count
for i := 0; i < portCount; i++ {
nets = append(nets, ServerNetwork{
networkID: netID,
portTags: net.PortTags,
vnicType: net.VNICType,
portSecurity: net.PortSecurity,
portName: fmt.Sprintf("%s-%d", name, i),
Copy link

@MaysaMacedo MaysaMacedo Mar 24, 2021

Choose a reason for hiding this comment

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

I'm not sure where the instance name is enforced, but could it have the following format <infra-id>-master-<n>? and consequently port name <infra-id>-master-<n>-<n>?

Copy link
Author

@iamemilio iamemilio Mar 24, 2021

Choose a reason for hiding this comment

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

hmm, I am not sure actually. Can CAPO manage control plane resources? If so, then this is a possibility. For workers the name scheme for machines is <cluster-id>-<machineset name>-<unique hash>

Copy link
Author

Choose a reason for hiding this comment

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

Ok, it does, so this could be a problem. The way I see it, there are 2 ways to handle this:

  1. Change the naming scheme of all instances to follow the <cluster-id>-<machineset name>-<unique hash> model.
  2. Only append a number at the end of a port's name if more than one port is requested, that way the only valid usage is for worker nodes (for now)

@MaysaMacedo I am not sure if either of these would have consequences for kuryr, but if either is acceptable, I would lean towards 1.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, I thought about this, and while its ugly for now, I would rather just leave it and make it a consideration in a bz we are going to file to eventually have upstream parity with this feature based on this work happening upstream: kubernetes-sigs#778

We will fix this post FF by allowing the port names to be explicitly setable.

Choose a reason for hiding this comment

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

We're working on moving away from relying on ports names on 4.8, so it's fine. I was just not sure is the name of that Port would be something like ostest-qxzjd-master-port-0-3 and if it that would be expected.

Copy link
Author

Choose a reason for hiding this comment

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

yeah, that would be a possibility for the master nodes. Its ugly, but we are going to fix it by letting you specify port names when we adopt kubernetes-sigs#778

})
}
}

for _, snetParam := range net.Subnets {
sopts := subnets.ListOpts(snetParam.Filter)
sopts.ID = snetParam.UUID
sopts.NetworkID = netID

// inherit port security settings from network if not set on subnet
portSecurity := net.PortSecurity
if snetParam.PortSecurity != nil {
portSecurity = snetParam.PortSecurity
}

if snetParam.PortCount > 0 {
portCount = int(snetParam.PortCount)
}

// Query for all subnets that match filters
snetResults, err := getSubnetsByFilter(is, &sopts)
if err != nil {
return nil, err
}
for _, snet := range snetResults {
nets = append(nets, ServerNetwork{
networkID: snet.NetworkID,
subnetID: snet.ID,
portTags: append(net.PortTags, snetParam.PortTags...),
vnicType: net.VNICType,
})
for i := 0; i < portCount; i++ {
nets = append(nets, ServerNetwork{
networkID: snet.NetworkID,
subnetID: snet.ID,
portTags: append(net.PortTags, snetParam.PortTags...),
vnicType: net.VNICType,
portSecurity: portSecurity,
portName: fmt.Sprintf("%s-%d", name, i),
})
}
}
}
}
Expand Down Expand Up @@ -478,40 +520,41 @@ func (is *InstanceService) InstanceCreate(clusterName string, name string, clust
}
}

// Convert nets list into a list of servers.Network to be passed as NICs for instance create
userData := base64.StdEncoding.EncodeToString([]byte(cmd))
var ports_list []servers.Network
for _, net := range nets {
if net.networkID == "" {
return nil, fmt.Errorf("No network was found or provided. Please check your machine configuration and try again")
}
allPages, err := ports.List(is.networkClient, ports.ListOpts{
Name: name,
NetworkID: net.networkID,
}).AllPages()
if err != nil {
return nil, fmt.Errorf("Searching for existing port for server err: %v", err)
var port ports.Port
secGroups := &securityGroups
addrPairs := &allowedAddressPairs
if net.portSecurity != nil && *net.portSecurity == false {
secGroups = &[]string{}
addrPairs = &[]ports.AddressPair{}
}
portList, err := ports.ExtractPorts(allPages)
if _, ok := netsWithoutAllowedAddressPairs[net.networkID]; ok {
addrPairs = &[]ports.AddressPair{}
}

port, err = CreatePort(is, net.portName, net, secGroups, addrPairs)
if err != nil {
return nil, fmt.Errorf("Searching for existing port for server err: %v", err)
return nil, fmt.Errorf("Failed to create port err: %v", err)
}
var port ports.Port
if len(portList) == 0 {
// create server port
if _, ok := netsWithoutAllowedAddressPairs[net.networkID]; ok {
// create ports without address pairs
port, err = CreatePort(is, name, net, &securityGroups, &[]ports.AddressPair{})
} else {
port, err = CreatePort(is, name, net, &securityGroups, &allowedAddressPairs)
}
if err != nil {
return nil, fmt.Errorf("Failed to create port err: %v", err)
}
} else {
port = portList[0]

// Update the port with the correct port security settings
// TODO(egarcia): figure out if possible to make this part of the prior create and update api calls
updateOpts := portsecurity.PortUpdateOptsExt{
UpdateOptsBuilder: ports.UpdateOpts{},
PortSecurityEnabled: net.portSecurity,
}
err = ports.Update(is.networkClient, port.ID, updateOpts).ExtractInto(&portWithPortSecurityExtensions)
if err != nil {
return nil, fmt.Errorf("Failed to update port security on port %s: %v", port.ID, err)
}

portTags := deduplicateList(append(machineTags, port.Tags...))
portTags := deduplicateList(append(machineTags, net.portTags...))
_, err = attributestags.ReplaceAll(is.networkClient, "ports", port.ID, attributestags.ReplaceAllOpts{
Tags: portTags}).Extract()
if err != nil {
Expand All @@ -522,31 +565,13 @@ func (is *InstanceService) InstanceCreate(clusterName string, name string, clust
})

if config.Trunk == true {
allPages, err := trunks.List(is.networkClient, trunks.ListOpts{
trunkCreateOpts := trunks.CreateOpts{
Name: name,
PortID: port.ID,
}).AllPages()
if err != nil {
return nil, fmt.Errorf("Searching for existing trunk for server err: %v", err)
}
trunkList, err := trunks.ExtractTrunks(allPages)
trunk, err := trunks.Create(is.networkClient, trunkCreateOpts).Extract()
if err != nil {
return nil, fmt.Errorf("Searching for existing trunk for server err: %v", err)
}
var trunk trunks.Trunk
if len(trunkList) == 0 {
// create trunk with the previous port as parent
trunkCreateOpts := trunks.CreateOpts{
Name: name,
PortID: port.ID,
}
newTrunk, err := trunks.Create(is.networkClient, trunkCreateOpts).Extract()
if err != nil {
return nil, fmt.Errorf("Create trunk for server err: %v", err)
}
trunk = *newTrunk
} else {
trunk = trunkList[0]
return nil, fmt.Errorf("Create trunk for server err: %v", err)
}

_, err = attributestags.ReplaceAll(is.networkClient, "trunks", trunk.ID, attributestags.ReplaceAllOpts{
Expand Down
Loading