✨ create ports #778

jsen- · 2021-03-10T11:20:29Z

What this PR does / why we need it:
We need to assign additional ports to the created instances with vnic_type=direct.
Attempting to assign this port to an existing VM will fail, so it needs to be done during the VM creation.

Which issue(s) this PR fixes:
Fixes #788

Special notes for your reviewer:
This is a draft to start a conversation, maybe the goal can be achieved in a better way.

Release note:

Added the ability to create and assign additional ports

/hold

k8s-ci-robot · 2021-03-10T11:20:37Z

Hi @jsen-. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

k8s-ci-robot · 2021-03-10T11:20:41Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: jsen-
To complete the pull request process, please assign jichenjc after the PR has been reviewed.
You can assign the PR to them by writing /assign @jichenjc in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

theopenlab-ci · 2021-03-10T13:43:00Z

Build succeeded.

cluster-api-provider-openstack-current-acceptance-test-v1.18 : SUCCESS in 57m 06s

jichenjc · 2021-03-11T01:22:29Z

/ok-to-test

jichenjc · 2021-03-11T01:23:47Z

@jsen- I think I understand most of the code and purpose of this PR
can we create an issue to track the reason/purpose of the issue so that we can know more background ? Thanks

jichenjc · 2021-03-11T01:52:30Z

pkg/cloud/services/compute/instance.go

+	for _, portCreateOpts := range i.Ports {
+		port, err := getOrCreatePort(is, i.Name+"-"+portCreateOpts.NameSuffix, portCreateOpts)
+		if err != nil {
+			return nil, err


should we delete those ports when delete cluster? I assume those ports are independent to VM lifecylce?

I implemented a cleanup orphaned port logic in one of my PRs before the sec group deletion. But it would be good if we try to delete all ports during machine deletion which we created for this machine before.

sbueringer · 2021-03-11T14:53:47Z

@jsen- I think I understand most of the code and purpose of this PR
can we create an issue to track the reason/purpose of the issue so that we can know more background ? Thanks

Yup, I would also like to understand why we need additional ports.

fyi. The v1alpha4 PR has been merged onto master. So if this feature is targeted towards master/v1alpha4 it has to
be implemented against the v1alpha4 CRDs now.

jsen- · 2021-03-16T12:44:53Z

Sorry for the delay, I'm actually out-of-office, but will try to be responsive.

can we create an issue to track the reason/purpose of the issue so that we can know more background ? Thanks

Done (#788)

should we delete those ports when delete cluster? I assume those ports are independent to VM lifecylce?

Definitely, I will try to implement cleanup of the ports and update the PR

So if this feature is targeted towards master/v1alpha4 it has to be implemented against the v1alpha4 CRDs now.

I'm assuming, rebase should be enough, am I correct?

sbueringer · 2021-03-16T15:41:29Z

@jsen- More or less. You have to rebase onto master and make your changes in the api/v1alpha4 folder instead of api/v1alpha3 but the structs are almost the same so there shouldn't be a lot to adjust

hidekazuna · 2021-03-19T02:44:06Z

api/v1alpha4/openstackmachine_types.go

@@ -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"`


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.

+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.

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 ? :)

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.

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.

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

iamemilio · 2021-03-24T22:48:51Z

Hi! We are actually working on something similar for RedHat OpenShift. Here is our work based off of a fork of the v1aplha1 API: openshift#170. The only difference in functionality between these two is that your patch does not account for allowing portSecurity to be set. I am not sure if this applies to other platforms, but in our platform, this would be a blocker. Other than that, where our implementation diverges the most is that we chose to automate the creation of the additional ports in the network and subnet param, but I do like your changes to allow for additional ports to be added explicitly.

I am hoping that we could align our work on this feature, since we are hoping to convert to v1alpha4, or potentially v1beta at some point in the future.

jichenjc · 2021-03-25T02:48:04Z

I am hoping that we could align our work on this feature, since we are hoping to convert to v1alpha4, or potentially v1beta at some point in the future.

+1 to see this happen , thanks @iamemilio

sbueringer · 2021-03-25T04:23:11Z

I am hoping that we could align our work on this feature, since we are hoping to convert to v1alpha4, or potentially v1beta at some point in the future.

+1 to see this happen , thanks @iamemilio

+1 for me too. I think this should be possible

adduarte · 2021-03-25T16:55:16Z

api/v1alpha4/types.go

@@ -116,6 +120,28 @@ type SubnetFilter struct {
 	NotTagsAny      string `json:"notTagsAny,omitempty"`
 }

+type PortOpts struct {
+	NetworkID           string              `json:"networkId" required:"true"`


is there any value to adding a SubnetID field?
I would seem overkill, but there always seems to be a case where it would be nice to easily lookup the subnetID based on port. or the lookup ports based on subnetIDs

Also, if your goal is to support SR-IOV, then you will need to be able to set (disable) portSecurity on the ports.

iamemilio · 2021-03-25T17:39:14Z

👾 Our team discussed things, and due to our timeline, we need to merge the patch in our fork for now so that we can unblock our quality assurance team. However, we are still going to collaborate with you on this feature and will also be filing a bug against our platform to ensure we meet feature and api parity with the results of this pull request. 😃

iamemilio · 2021-03-29T14:22:55Z

api/v1alpha4/types.go

+	AllowedAddressPairs []ports.AddressPair `json:"allowedAddressPairs,omitempty"`
+
+	// The ID of the host where the port is allocated
+	HostID string `json:"binding:host_id,omitempty"`


Suggested change

HostID string `json:"binding:host_id,omitempty"`

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

I would prefer to stick to camel case

iamemilio · 2021-03-29T14:23:07Z

api/v1alpha4/types.go

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


k8s-ci-robot · 2021-04-15T17:57:08Z

@jsen-: The following test failed, say /retest to rerun all failed tests:

Test name	Commit	Details	Rerun command
pull-cluster-api-provider-openstack-test	`d453ee3`	link	`/test pull-cluster-api-provider-openstack-test`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

iamemilio · 2021-04-21T15:09:30Z

api/v1alpha4/types.go

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


^ 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.

macaptain · 2021-05-12T15:56:26Z

api/v1alpha4/types.go

@@ -116,6 +120,26 @@ type SubnetFilter struct {
 	NotTagsAny      string `json:"notTagsAny,omitempty"`
 }

+type PortOpts struct {
+	NetworkID           string              `json:"networkID" required:"true"`


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

sbueringer · 2021-05-12T17:36:59Z

@jsen- Please let me know if you are able to address this comments. I think we have a few other folks which would be happy to pick this up and geht it merged :)

jsen- · 2021-05-14T15:48:37Z

@sbueringer apologies for the delays, we're still interested to get this into master, but I will be busy with other stuff for couple more weeks :( so don't mind if someone is willing to pick this up.

sbueringer · 2021-05-14T16:31:40Z

@sbueringer apologies for the delays, we're still interested to get this into master, but I will be busy with other stuff for couple more weeks :( so don't mind if someone is willing to pick this up.

Okay, no problem. Thx for the information. @macaptain as you expressed interest (#788 (comment)), do you want to pick this up?

macaptain · 2021-05-14T16:51:20Z

@sbueringer apologies for the delays, we're still interested to get this into master, but I will be busy with other stuff for couple more weeks :( so don't mind if someone is willing to pick this up.

Okay, no problem. Thx for the information. @macaptain as you expressed interest (#788 (comment)), do you want to pick this up?

Hey @sbueringer and @jsen- . Thanks for letting me know. Yes, I'd be happy to pick this up and try and get this feature into master.

sbueringer · 2021-05-14T17:00:28Z

@macaptain Great! It probably makes sense to wait a bit until: #862 is merged as it refactors related code quite a bit. But this shouldn't take too long

k8s-ci-robot · 2021-05-15T11:04:41Z

@jsen-: PR needs rebase.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@jsen-

This commit adds a new field to the v1alpha4 API: OpenStackMachineTemplateSpec.ports. The list of ports are added per instance. Each port may be customized with options. These ports are created in addition to any ports created in the `networks:` field. If `networks:` is not specified, only the ports specified in `ports:` will be created and attached to the instance. If neither `networks:` nor `ports:` are specified, the instance will be connected to the default cluster network and subnet. This commit is very much based on the work on @jsen-: kubernetes-sigs#778 and also aims to bring Cluster API OpenStack provider into greater alignment with the OpenShift fork.

@jsen-

This commit adds a new field to the v1alpha4 API: OpenStackMachineTemplateSpec.ports. The list of ports are added per instance. Each port may be customized with options. These ports are created in addition to any ports created in the `networks:` field. If `networks:` is not specified, only the ports specified in `ports:` will be created and attached to the instance. If neither `networks:` nor `ports:` are specified, the instance will be connected to the default cluster network and subnet. This commit is very much based on the work on @jsen-: kubernetes-sigs#778 and also aims to bring Cluster API OpenStack provider into greater alignment with the OpenShift fork.

@jsen-

This commit adds a new field to the v1alpha4 API: OpenStackMachineTemplateSpec.ports. The list of ports are added per instance. Each port may be customized with options. These ports are created in addition to any ports created in the `networks:` field. If `networks:` is not specified, only the ports specified in `ports:` will be created and attached to the instance. If neither `networks:` nor `ports:` are specified, the instance will be connected to the default cluster network and subnet. This feature is very much based on the work on @jsen-, and a lot of credit has to go there for the implementation: kubernetes-sigs#778

@jsen-

This commit adds a new field to the v1alpha4 API: OpenStackMachineTemplateSpec.ports. The list of ports are added per instance. Each port may be customized with options. These ports are created in addition to any ports created in the `networks:` field. If `networks:` is not specified, only the ports specified in `ports:` will be created and attached to the instance. If neither `networks:` nor `ports:` are specified, the instance will be connected to the default cluster network and subnet. This feature is very much based on the work on @jsen-, and a lot of credit goes there for the implementation: kubernetes-sigs#778

@jsen-

This commit adds a new field to the v1alpha4 API: OpenStackMachineTemplateSpec.ports. The list of ports are added per instance. Each port may be customized with options. These ports are created in addition to any ports created in the `networks:` field. If `networks:` is not specified, only the ports specified in `ports:` will be created and attached to the instance. If neither `networks:` nor `ports:` are specified, the instance will be connected to the default cluster network and subnet. This feature is very much based on the work on @jsen-, and a lot of credit goes there for the implementation: kubernetes-sigs#778

@jsen-

This commit adds a new field to the v1alpha4 API: OpenStackMachineTemplateSpec.ports. The list of ports are added per instance. Each port may be customized with options. These ports are created in addition to any ports created in the `networks:` field. If `networks:` is not specified, only the ports specified in `ports:` will be created and attached to the instance. If neither `networks:` nor `ports:` are specified, the instance will be connected to the default cluster network and subnet. This feature is very much based on the work on @jsen-, and a lot of credit goes there for the implementation: kubernetes-sigs#778

This commit adds a new field to the v1alpha4 API: OpenStackMachineTemplateSpec.ports. The list of ports are added per instance. Each port may be customized with options. These ports are created in addition to any ports created in the `networks:` field. If `networks:` is not specified, only the ports specified in `ports:` will be created and attached to the instance. If neither `networks:` nor `ports:` are specified, the instance will be connected to the default cluster network and subnet. This feature is very much based on the work on jsen-, and a lot of credit goes there for the implementation: kubernetes-sigs#778

sbueringer · 2021-06-09T04:12:54Z

Can be closed as it has been implemented in #876
/close

(correct me if I'm wrong :))

k8s-ci-robot · 2021-06-09T04:12:58Z

@sbueringer: Closed this PR.

In response to this:

Can be closed as it has been implemented in #876
/close

(correct me if I'm wrong :))

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

This commit adds a new field to the v1alpha4 API: OpenStackMachineTemplateSpec.ports. The list of ports are added per instance. Each port may be customized with options. These ports are created in addition to any ports created in the `networks:` field. If `networks:` is not specified, only the ports specified in `ports:` will be created and attached to the instance. If neither `networks:` nor `ports:` are specified, the instance will be connected to the default cluster network and subnet. This feature is very much based on the work on jsen-, and a lot of credit goes there for the implementation: kubernetes-sigs#778

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 10, 2021

k8s-ci-robot added the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Mar 10, 2021

k8s-ci-robot requested review from hidekazuna and prankul88 March 10, 2021 11:20

k8s-ci-robot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Mar 10, 2021

k8s-ci-robot added ok-to-test Indicates a non-member PR verified by an org member that is safe to test. and removed needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Mar 11, 2021

jichenjc reviewed Mar 11, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 11, 2021

jsen- force-pushed the master branch from cfcf4e6 to e1a1328 Compare March 18, 2021 10:11

k8s-ci-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 18, 2021

hidekazuna reviewed Mar 19, 2021

View reviewed changes

adduarte reviewed Mar 25, 2021

View reviewed changes

iamemilio mentioned this pull request Mar 25, 2021

Port Count openshift/cluster-api-provider-openstack#170

Closed

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Mar 26, 2021

iamemilio reviewed Mar 29, 2021

View reviewed changes

iamemilio reviewed Apr 21, 2021

View reviewed changes

macaptain reviewed May 12, 2021

View reviewed changes

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label May 15, 2021

macaptain mentioned this pull request May 21, 2021

✨ Add feature to create ports with custom options #876

Merged

7 tasks

k8s-ci-robot closed this Jun 9, 2021

macaptain mentioned this pull request Oct 6, 2021

REQUEST: New membership for macaptain kubernetes/org#3030

Closed

7 tasks

	// 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"`

	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"`
	}

	HostID string `json:"binding:host_id,omitempty"`
	HostID string `json:"binding:hostID,omitempty"`

✨ create ports #778

✨ create ports #778

Uh oh!

Conversation

jsen- commented Mar 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

k8s-ci-robot commented Mar 10, 2021

Uh oh!

k8s-ci-robot commented Mar 10, 2021

Uh oh!

theopenlab-ci bot commented Mar 10, 2021

Uh oh!

jichenjc commented Mar 11, 2021

Uh oh!

jichenjc commented Mar 11, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbueringer commented Mar 11, 2021

Uh oh!

jsen- commented Mar 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbueringer commented Mar 16, 2021

Uh oh!

hidekazuna Mar 19, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamemilio Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamemilio Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamemilio commented Mar 24, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jichenjc commented Mar 25, 2021

Uh oh!

sbueringer commented Mar 25, 2021

Uh oh!

adduarte Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamemilio Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

iamemilio commented Mar 25, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Apr 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbueringer commented May 12, 2021

Uh oh!

jsen- commented May 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

sbueringer commented May 14, 2021

jsen- commented Mar 10, 2021 •

edited

Loading

jsen- commented Mar 16, 2021 •

edited

Loading

hidekazuna Mar 19, 2021 •

edited

Loading

iamemilio Mar 24, 2021 •

edited

Loading

iamemilio Mar 25, 2021 •

edited

Loading

iamemilio commented Mar 24, 2021 •

edited

Loading

adduarte Mar 25, 2021 •

edited

Loading

iamemilio Mar 25, 2021 •

edited

Loading

iamemilio commented Mar 25, 2021 •

edited

Loading

jsen- commented May 14, 2021 •

edited

Loading