Skip to content

✨ create ports #778

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

Closed
wants to merge 5 commits into from
Closed

✨ create ports #778

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
c59d29e
create ports
jsen- Mar 18, 2021
7f1ef1e
remove DeviceID and DeviceOwner options
jsen- Apr 1, 2021
7495c55
property case (networkID, hostID, vnicType)
jsen- Apr 1, 2021
ccdd19c
add function to calculate instance port name
jsen- Apr 1, 2021
d453ee3
wip: delete ports with instance (untested)
jsen- Apr 1, 2021
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
100 changes: 82 additions & 18 deletions api/v1alpha3/zz_generated.conversion.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions api/v1alpha4/openstackmachine_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,9 @@ type OpenStackMachineSpec struct {
// UUID, IP address of a port from this subnet will be marked as AccessIPv4 on the created compute instance
Subnet string `json:"subnet,omitempty"`

// create and assign additional ports to instances
Ports []PortOpts `json:"ports,omitempty"`
Copy link
Contributor

@hidekazuna hidekazuna Mar 19, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

We only need VNICType actually. I try to find a light solution.
In my understanding, SR-IOV setting is done per compute node, but real use case is that create network for SR-IOV and create port for it. How about adding VNICType to NetworkParam? At least we can use existing network with SR-IOV.

Copy link
Contributor

@iamemilio iamemilio Mar 24, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

+1 Since Network param and Subnet Param are no longer nested in v1aplha4 api, it would also make sense to be able to set the vnicType in the subnet, in case that is a user's preferred way to add an interface to a compute instance.

Copy link
Member

Choose a reason for hiding this comment

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

I'm basically fine with whatever option you get consensus on :)
But just asking. We didn't change NetworkParam and SubnetParam from v1alpha4 and as far as I can see they are still nested. Did I miss something that they are not nested anymore ? :)

Copy link
Contributor

@iamemilio iamemilio Mar 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I could be misunderstanding, but:

cluster-api-provider-openstack/api/v1alpha4/openstackmachine_types.go

Lines 57 to 62 in e1a1328

// A networks object. Required parameter when there are multiple networks defined for the tenant.
// When you do not specify the networks parameter, the server attaches to the only network created for the current tenant.
Networks []NetworkParam `json:"networks,omitempty"`
// UUID, IP address of a port from this subnet will be marked as AccessIPv4 on the created compute instance
Subnet string `json:"subnet,omitempty"`

It looks like they are their own isolated api entry. Its very possible that I am incorrect though. My frame of reference is from 4 api versions ago.

Copy link
Member

Choose a reason for hiding this comment

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

Ah now I get what lead to this assumption. This Subnet is only for the AccessIPv4 "selection". (it was introduced in #756)

SubnetParam is still nested.

cluster-api-provider-openstack/api/v1alpha4/types.go

Lines 61 to 70 in e1a1328

type NetworkParam struct {
// The UUID of the network. Required if you omit the port attribute.
UUID string `json:"uuid,omitempty"`
// A fixed IPv4 address for the NIC.
FixedIP string `json:"fixedIp,omitempty"`
// Filters for optional network query
Filter Filter `json:"filter,omitempty"`
// Subnet within a network to use
Subnets []SubnetParam `json:"subnets,omitempty"`
}

As mentioned elsewhere we have to rethink our design / CRD sooner or later as it's not that thought out / consistent.

Copy link
Contributor

Choose a reason for hiding this comment

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

ah ok, that makes sense now. thanks for explaining!


// The floatingIP which will be associated to the machine, only used for master.
// The floatingIP should have been created and haven't been associated.
FloatingIP string `json:"floatingIP,omitempty"`
Expand Down
25 changes: 25 additions & 0 deletions api/v1alpha4/types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,10 @@ limitations under the License.

package v1alpha4

import (
"github.com/gophercloud/gophercloud/openstack/networking/v2/ports"
)

// OpenStackMachineTemplateResource describes the data needed to create a OpenStackMachine from a template.
type OpenStackMachineTemplateResource struct {
// Spec is the specification of the desired behavior of the machine.
Expand Down Expand Up @@ -116,6 +120,26 @@ type SubnetFilter struct {
NotTagsAny string `json:"notTagsAny,omitempty"`
}

type PortOpts struct {
NetworkID string `json:"networkID" required:"true"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Since NetworkID is a mandatory field, would it make sense if ports: were children of the list items of networks:?

NameSuffix string `json:"nameSuffix" required:"true"`
Description string `json:"description,omitempty"`
AdminStateUp *bool `json:"adminStateUp,omitempty"`
MACAddress string `json:"macAddress,omitempty"`
FixedIPs []ports.IP `json:"fixedIPs,omitempty"`
Copy link
Contributor

@iamemilio iamemilio Apr 13, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

also the values for ports.IP are all underscored, we should probably create our own version and pass it in so we can keep the api camel case

Copy link
Member

Choose a reason for hiding this comment

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

Agree

TenantID string `json:"tenantID,omitempty"`
ProjectID string `json:"projectID,omitempty"`
SecurityGroups *[]string `json:"securityGroups,omitempty"`
AllowedAddressPairs []ports.AddressPair `json:"allowedAddressPairs,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

^ same as Fixed IPs :)

Sorry I keep hitting you with these so late. I missed these when reviewing and am bumping into them now that I am testing.


// The ID of the host where the port is allocated
HostID string `json:"binding:hostID,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
HostID string `json:"binding:hostID,omitempty"`
HostID string `json:"hostID,omitempty"`


// The virtual network interface card (vNIC) type that is bound to the
// neutron port.
VNICType string `json:"binding:vnicType,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
VNICType string `json:"binding:vnicType,omitempty"`
VNICType string `json:"vnicType,omitempty"`

}

type Instance struct {
ID string `json:"id,omitempty"`
Name string `json:"name,omitempty"`
Expand All @@ -124,6 +148,7 @@ type Instance struct {
SecurityGroups *[]string `json:"securigyGroups,omitempty"`
Networks *[]Network `json:"networks,omitempty"`
Subnet string `json:"subnet,omitempty"`
Ports []PortOpts `json:"ports,omitempty"`
Tags []string `json:"tags,omitempty"`
Image string `json:"image,omitempty"`
Flavor string `json:"flavor,omitempty"`
Expand Down
54 changes: 54 additions & 0 deletions api/v1alpha4/zz_generated.deepcopy.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading