✨ Allow Trunk configuration at a Port level #934

Xenwar · 2021-07-12T10:06:10Z

Allow Trunk configuration at a Port level

This PR enables and disables port level trunk setting as follows.

If user provides trunk field (true or false) at Port level, then that value is used for the port.
If user does not provide trunk field at Port level, then the port inherits the trunk field of the instance.

Example spec:

apiVersion: infrastructure.cluster.x-k8s.io/v1alpha4
kind: OpenStackMachineTemplate
metadata:
  name: basic-1-control-plane
  namespace: default
spec:
  template:
    spec:
  template:
    spec:
      trunk: true
      ports:
      - description: "Port 1, no trunk specified"
      - description: "Port 2, trunk disabled"
        trunk: false
      - description: "Port 3, trunk enabled"
        trunk: true
      cloudName: openstack-1
      flavor: 2C-4GB-100GB
      identityRef:
        kind: Secret
        name: basic-1-cloud-config
      image: Ubuntu_20.04_node

Output:

(openstack) network trunk list  --sort-column Name
+--------------------------------------+-------------------------------+--------------------------------------+-----------------------------------------------------------+
| ID                                   | Name                          | Parent Port                          | Description                                               |
+--------------------------------------+-------------------------------+--------------------------------------+-----------------------------------------------------------+
| 03bbab0b-96d9-4226-bded-08ed8b3449ce | basic-1-control-plane-th8l2-0 | 6b6445f6-13af-4669-a301-cc79a79bf8ef | Created by cluster-api-provider-openstack cluster basic-1 |
| c09f979e-333f-4651-a4ff-93d7a72afc2d | basic-1-control-plane-th8l2-2 | fcd0a7ff-138c-4125-9f2a-42a2c32b13e5 | Created by cluster-api-provider-openstack cluster basic-1 |
+--------------------------------------+-------------------------------+--------------------------------------+-----------------------------------------------------------+

(openstack) port list -c Name -c Description -c ID --sort-column Name
+-------------------------------+----------------------------+--------------------------------------+
| Name                          | Description                | ID                                   |
+-------------------------------+----------------------------+--------------------------------------+
|                               |                            | 1dc33e63-3e4e-492a-86c4-70b304b58986 |
|                               |                            | 35830ee6-d984-4a1f-8e1a-703e024caca8 |
|                               |                            | e650d8ec-4786-4790-a6c9-befe7db57fae |
| basic-1-control-plane-th8l2-0 | Port 1, no trunk specified | 6b6445f6-13af-4669-a301-cc79a79bf8ef |
| basic-1-control-plane-th8l2-1 | Port 2, trunk disabled     | a944ec80-e147-41e8-aa6b-bbef1a47fe37 |
| basic-1-control-plane-th8l2-2 | Port 3, trunk enabled      | fcd0a7ff-138c-4125-9f2a-42a2c32b13e5 |
+-------------------------------+----------------------------+--------------------------------------+
(openstack)

Signed-off-by: Anwar Hassen [email protected]

k8s-ci-robot · 2021-07-12T10:06:18Z

Hi @Xenwar. 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.

Xenwar · 2021-07-12T10:21:34Z

/hidekazuna

jichenjc · 2021-07-12T12:21:10Z

/ok-to-test

dulek · 2021-07-13T11:36:23Z

This is awesome, I'm just interested if the logic here is fine. I'd expect that trunk setting at instance level would just change the default behavior for all ports but you could still be able to overwrite it by specifying it explicitly on the port level. If I understand the proposal correctly, currently the logic only allows that when instance level trunk is true.

Xenwar · 2021-07-13T11:39:53Z

This is awesome, I'm just interested if the logic here is fine. I'd expect that trunk setting at instance level would just change the default behavior for all ports but you could still be able to overwrite it by specifying it explicitly on the port level. If I understand the proposal correctly, currently the logic only allows that when instance level trunk is true.

True. At the moment, instance level overrides all ports if it is set to false.
If you have a use to make the port-level as a source of truth, we can make changes as well.

Xenwar · 2021-07-13T11:45:13Z

We can remove the conditional here https://github.com/kubernetes-sigs/cluster-api-provider-openstack/pull/934/files#diff-5e7dbfd52305a21a11fb94fab201062ae2c7b851db93c2a8443a1d82d7aa7936R506
Then, ports would dictate trunk setting, regardless instance level value.

dulek · 2021-07-13T12:56:25Z

Thank you, this seems more logical to me. The main reason here is that I'm looking at this from Kuryr perspective which requires you to have trunk set only on the main port of the instance. This means that I can now set instance's trunk to false and only set trunk to true for that single port that requires it. The previous version of this code would force me to set trunk to false for each port that does not require it.

Xenwar · 2021-07-13T19:17:24Z

/retest

Xenwar · 2021-07-14T05:26:08Z

/test pull-cluster-api-provider-openstack-e2e-test

mdbooth · 2021-07-15T10:58:54Z

pkg/cloud/services/compute/instance.go

+		iTags := []string{}
+		if len(instance.Tags) > 0 {
+			iTags = instance.Tags
+		}


Why is iTags necessary here? Why can't we just pass instance.Tags in all cases? We were previously passing it to replaceAllAttributeTags as-is.

Tried to pass the instance.Tags as is. However, it is not always passed to the createInstance. line 195 is a guard against that.

I don't understand. What breaks?

Will try to get more specific data on this.

Got some logs.
Cause: When no tag is provided OpenStackMachineTemplate.spec.template.spec.template.spec.tags, the following error occurs.

logs:

E0716 20:27:52.384885 846 openstackmachine_controller.go:389] controller-runtime/manager/controller/openstackmachine "msg"="OpenStack instance cannot be created: error creating Openstack instance: Missing input for argument [Tags]" "error"="UpdateError" "cluster"="basic-1" "machine"="basic-1-control-plane-wsrg7" "name"="basic-1-control-plane-4p2jc" "namespace"="default" "openStackCluster"="basic-1" "openStackMachine"="basic-1-control-plane-4p2jc" "reconciler group"="infrastructure.cluster.x-k8s.io" "reconciler kind"="OpenStackMachine"

The problem does not occur for all ports when user specified multiple ports. It only occurs to some of them. Which is the reason why I added that check.

Any particular reason why it's part of the loop? I would expect the block to be moved up, before you loop over instance networks.

The ports are created in that loop and trying to make use of that.

mdbooth · 2021-07-15T11:24:15Z

pkg/cloud/services/compute/instance.go

+
+	if portOpts.Trunk {
+		trunkDescription := names.GetDescription(clusterName)
+		trunk, err := s.getOrCreateTrunk(eventObject, trunkDescription, port.Name, port.ID)


This doesn't look right to me. This looks like it's creating a new trunk for every trunked port. My understanding was that all trunked ports would use the same trunk on a single instance. I understood that the change here was that some ports can now optionally not use that trunk, not that we would have multiple trunks.

Yes, this PR is creating a new trunk for every port marked as trunked.
However, it is configurable as is shown in the description of this PR.

Please ignore this because it's out of scope for your change and consistent with the previous behaviour. However, now that I've seen it I don't understand why trunks are used this way. That could just be because I don't properly understand trunks, though.

@mdbooth: It's a Neutron trunk, which might be a bit confusing. In Neutron a port can have a trunk created and then you add other ports as subports in that trunk. In Kuryr we use that to dynamically add subports which serve as pods interfaces. What you're proposing is doable, but I'm not sure if I see value in subports added statically. Anyway if we'd want to implement that, I'd rather add a subports field in PortOpts and get subports defined there.

@dulek is the name of the trunk singinificant to kuryr? Does kuryr look up trunks per port (parent port.name for example), or by instance

mdbooth · 2021-07-15T12:14:40Z

pkg/cloud/services/compute/instance.go

@@ -508,10 +504,22 @@ func (s *Service) getOrCreatePort(eventObject runtime.Object, clusterName string
 	}

 	record.Eventf(eventObject, "SuccessfulCreatePort", "Created port %s with id %s", port.Name, port.ID)
+
+	if portOpts.Trunk {


This is a problem.

Previously if we specified trunk on the instance and a number of ports without any trunk setting, those ports would inherit the trunk setting of the instance. Here we're always using the trunk setting of the port, which now defaults to False rather than inheriting the trunk setting of the instance. This will cause a change of behaviour.

I think we want the following behaviour:

instance=true, port=undefined, trunk=true

instance=true, port=true, trunk=true

instance=true, port=false, trunk=false

instance=false, port=undefined, trunk=false

instance=false, port=true, trunk=true

instance=false, port=false, trunk=false

If we define port=undefined to be false (the behaviour here), that breaks case 1.
If we define port=undefined to be true, that breaks case 4.

IOW think we need to make portOpts.Trunk a *bool instead of a bool.

We are not inheriting instance level setting. As discussed with @dulek above, each port makes its own decision, regardless of what the instance says.
for (1) and (4), the check was on if a port is specified via Portopts, if not it is set to the instance level

cluster-api-provider-openstack/pkg/cloud/services/compute/instance.go

Line 178 in 1673261

Trunk: openStackMachine.Spec.Trunk,

Is it so that there are other ways of specifying multiple ports ?

However, I think we previously did inherit the instance setting. Let's take a specific example:

trunk: true ports: - description: "primary 1" foo: bar - description: "primary 2" foo: baz

Without this change, both of these ports will be trunked. With this change these ports are no longer trunked because trunk is not specified on the port (because it did not previously exist), and being a bool it will default to false. IOW this change causes a change in previously defined behaviour. I think that would be a regression.

I think the fix is simple: if we make trunk on the port a *bool instead of a bool we can define a third behaviour for the undefined case which is compatible with the previous behaviour.

Thanks. Now, I understood the consequence of not making it a pointer.
Making changes to address it. Will also update the PR description accordingly.

Thanks @mdbooth, I missed this detail.

mdbooth · 2021-07-15T12:28:04Z

Also, where are we on testing this kind of functionality?

Xenwar · 2021-07-15T17:55:14Z

Also, where are we on testing this kind of functionality?

I was waiting for the merge of #935 before adding tests, but add a test for it.

mdbooth

I still think this change would introduce a regression without changing portopts.trunk from bool to *bool.

mdbooth · 2021-07-16T09:32:21Z

pkg/cloud/services/compute/instance.go

+		iTags := []string{}
+		if len(instance.Tags) > 0 {
+			iTags = instance.Tags
+		}


I don't understand. What breaks?

mdbooth · 2021-07-16T09:45:34Z

pkg/cloud/services/compute/instance.go

@@ -508,10 +504,22 @@ func (s *Service) getOrCreatePort(eventObject runtime.Object, clusterName string
 	}

 	record.Eventf(eventObject, "SuccessfulCreatePort", "Created port %s with id %s", port.Name, port.ID)
+
+	if portOpts.Trunk {


However, I think we previously did inherit the instance setting. Let's take a specific example:

trunk: true ports: - description: "primary 1" foo: bar - description: "primary 2" foo: baz

Without this change, both of these ports will be trunked. With this change these ports are no longer trunked because trunk is not specified on the port (because it did not previously exist), and being a bool it will default to false. IOW this change causes a change in previously defined behaviour. I think that would be a regression.

I think the fix is simple: if we make trunk on the port a *bool instead of a bool we can define a third behaviour for the undefined case which is compatible with the previous behaviour.

mdbooth · 2021-07-16T09:48:52Z

pkg/cloud/services/compute/instance.go

+
+	if portOpts.Trunk {
+		trunkDescription := names.GetDescription(clusterName)
+		trunk, err := s.getOrCreateTrunk(eventObject, trunkDescription, port.Name, port.ID)


Please ignore this because it's out of scope for your change and consistent with the previous behaviour. However, now that I've seen it I don't understand why trunks are used this way. That could just be because I don't properly understand trunks, though.

Xenwar · 2021-08-05T11:02:32Z

Once trunk is supported, a test for trunk with the following content can be added https://gist.github.com/Xenwar/d2a9a120531aef09f7670b57d43e0857

Xenwar · 2021-08-05T12:10:56Z

The trunk feature is tested on separate openstack environment with trunk support.
I was unable to add the E2E test due to the CAPO test infra does not support trunk yet, report #961

Once it supports trunk, the following test can be added, PortSecurity is also tested
https://gist.github.com/Xenwar/d2a9a120531aef09f7670b57d43e0857

tobiasgiese

two nits

tobiasgiese · 2021-08-05T14:48:51Z

pkg/cloud/services/compute/instance.go

+		pOpts := &openStackMachine.Spec.Ports[i]
+		// No Trunk field specified for the port, inherits openStackMachine.Spec.Trunk.
+		if pOpts.Trunk == nil {
+			pOpts.Trunk = &openStackMachine.Spec.Trunk
+		}


You have to create the variable pOpts to not override port.Trunk, correct?
Or can we do sth like this? And pass PortOpts: &port in l.164 and l.172

Suggested change

pOpts := &openStackMachine.Spec.Ports[i]

// No Trunk field specified for the port, inherits openStackMachine.Spec.Trunk.

if pOpts.Trunk == nil {

pOpts.Trunk = &openStackMachine.Spec.Trunk

}

// No Trunk field specified for the port, inherits openStackMachine.Spec.Trunk.

if port.Trunk == nil {

port.Trunk = &openStackMachine.Spec.Trunk

}

pOpts is created to avoid code duplication.
The check needs to be done for each Port over the loop.

tobiasgiese · 2021-08-05T14:50:42Z

pkg/cloud/services/compute/instance.go

 	return port, nil
 }

-func (s *Service) getOrCreateTrunk(eventObject runtime.Object, clusterName, trunkName, portID string) (*trunks.Trunk, error) {
+func (s *Service) getOrCreateTrunk(eventObject runtime.Object, description, trunkName, portID string) (*trunks.Trunk, error) {


I would keep consistent to the other methods and stay with clusterName as an argument. We're doing this several times like this

Thanks. done.

jichenjc · 2021-08-11T03:21:37Z

The trunk feature is tested on separate openstack environment with trunk support.
I was unable to add the E2E test due to the CAPO test infra does not support trunk yet, report #961

Once it supports trunk, the following test can be added, PortSecurity is also tested
https://gist.github.com/Xenwar/d2a9a120531aef09f7670b57d43e0857

thanks for the info, let's merge this after review comments done, I agree we can track the issue and fix later on

Signed-off-by: Anwar Hassen <[email protected]>

Xenwar · 2021-08-30T09:19:51Z

I have addressed on the of the comments. @tobiasgiese would you please check again to see if I have addresses the issues.
Thanks.

tobiasgiese · 2021-08-30T10:54:11Z

/lgtm

Xenwar · 2021-08-31T05:44:50Z

@jichenjc
The last comments are addressed. Do we merge it now ?
Thanks.

jichenjc · 2021-08-31T05:49:33Z

/approve

thanks a lot for your help~

k8s-ci-robot · 2021-08-31T05:49:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc, Xenwar

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

The pull request process is described here

Needs approval from an approver in each of these files:

~~OWNERS~~ [jichenjc]

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

k8s-ci-robot requested review from hidekazuna and prankul88 July 12, 2021 10:06

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Jul 12, 2021

k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 12, 2021

Xenwar force-pushed the toggle_trunk branch from 6654a57 to 731f1da Compare July 12, 2021 10:21

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 Jul 12, 2021

Xenwar mentioned this pull request Jul 13, 2021

Allow mix of trunk/untrunked ports. #929

Closed

Xenwar force-pushed the toggle_trunk branch from 731f1da to 257683c Compare July 13, 2021 12:35

adduarte mentioned this pull request Jul 14, 2021

Bug 1964540: Extend trunk configuration to port level in machineset openshift/cluster-api-provider-openstack#185

Merged

Xenwar force-pushed the toggle_trunk branch from 257683c to 1673261 Compare July 14, 2021 11:49

hidekazuna mentioned this pull request Jul 15, 2021

v0.4.0 (v1alpha4) #745

Closed

3 tasks

mdbooth requested changes Jul 15, 2021

View reviewed changes

mdbooth requested changes Jul 16, 2021

View reviewed changes

Xenwar force-pushed the toggle_trunk branch 3 times, most recently from c563f2a to 2908576 Compare July 17, 2021 08:44

k8s-ci-robot removed the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jul 17, 2021

Xenwar force-pushed the toggle_trunk branch 2 times, most recently from a314d3a to d8fe11c Compare August 4, 2021 21:37

Xenwar mentioned this pull request Aug 5, 2021

Enable trunk on CAPO CI devstack #961

Closed

Xenwar force-pushed the toggle_trunk branch 2 times, most recently from 0b64323 to dd8dc0e Compare August 5, 2021 10:58

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Aug 5, 2021

Xenwar changed the title ~~[WIP] ✨ Allow Trunk configuration at a Port level~~ ✨ Allow Trunk configuration at a Port level Aug 5, 2021

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 5, 2021

tobiasgiese reviewed Aug 5, 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 Aug 12, 2021

Xenwar force-pushed the toggle_trunk branch from dd8dc0e to 3fd9e66 Compare August 30, 2021 07:26

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

Xenwar force-pushed the toggle_trunk branch 3 times, most recently from bfa84f4 to 44240bd Compare August 30, 2021 08:09

Allow Trunk configuration at a Port level.

8f3bf57

Signed-off-by: Anwar Hassen <[email protected]>

Xenwar force-pushed the toggle_trunk branch from 44240bd to 8f3bf57 Compare August 30, 2021 08:22

k8s-ci-robot assigned tobiasgiese Aug 30, 2021

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Aug 30, 2021

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 31, 2021

k8s-ci-robot merged commit 1fd76a5 into kubernetes-sigs:master Aug 31, 2021

✨ Allow Trunk configuration at a Port level #934

✨ Allow Trunk configuration at a Port level #934

Conversation

Xenwar commented Jul 12, 2021 • edited Loading

k8s-ci-robot commented Jul 12, 2021

Xenwar commented Jul 12, 2021

jichenjc commented Jul 12, 2021

dulek commented Jul 13, 2021

Xenwar commented Jul 13, 2021

Xenwar commented Jul 13, 2021

dulek commented Jul 13, 2021

Xenwar commented Jul 13, 2021

Xenwar commented Jul 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xenwar Jul 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdbooth Jul 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdbooth commented Jul 15, 2021

Xenwar commented Jul 15, 2021

mdbooth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mdbooth Jul 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Xenwar commented Aug 5, 2021

Xenwar commented Aug 5, 2021

tobiasgiese left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tobiasgiese Aug 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jichenjc commented Aug 11, 2021

Xenwar commented Aug 30, 2021

tobiasgiese commented Aug 30, 2021

Xenwar commented Aug 31, 2021

jichenjc commented Aug 31, 2021

k8s-ci-robot commented Aug 31, 2021

Xenwar commented Jul 12, 2021 •

edited

Loading

Xenwar Jul 16, 2021 •

edited

Loading

mdbooth Jul 16, 2021 •

edited

Loading

mdbooth Jul 16, 2021 •

edited

Loading

tobiasgiese Aug 5, 2021 •

edited

Loading