Check trunk support before enabling trunk at port level #1014

Xenwar · 2021-09-28T12:07:58Z

This PR adds a check on trunk support before enabling trunk at port level.

netlify · 2021-09-28T12:08:04Z

✔️ Deploy Preview for kubernetes-sigs-cluster-api-openstack ready!

🔨 Explore the source changes: c125a4a

🔍 Inspect the deploy log: https://app.netlify.com/sites/kubernetes-sigs-cluster-api-openstack/deploys/617a644bc136ff00071bd2f7

😎 Browse the preview: https://deploy-preview-1014--kubernetes-sigs-cluster-api-openstack.netlify.app

k8s-ci-robot · 2021-09-28T12:08:05Z

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.

namnx228

LGTM

macaptain

Extracting the function will help keep the error messages for trunk support checking consistent in the code, but I think as it stands, instance deletion will be broken by this change. Additionally, I think the function naming could be made clearer.

macaptain · 2021-09-28T18:47:50Z

pkg/cloud/services/compute/instance.go

@@ -582,3 +584,14 @@ func getTimeout(name string, timeout int) time.Duration {
 	}
 	return time.Duration(timeout)
 }
+
+func (s *Service) trunkNotSupported() (notSupported bool, err error) {


This function is named a bit strangely:

The logic would read more clearly if the name was a positive instead of a negative.

I can't guess from the function name that it returns a boolean without side effect.

I think I would call this function isTrunkExtSupported or maybe better: isTrunkExtEnabled.

+1 to make it positive case

macaptain · 2021-09-28T18:49:02Z

pkg/cloud/services/compute/instance.go

+func (s *Service) trunkNotSupported() (notSupported bool, err error) {
+	trunkSupport, err := s.networkingService.GetTrunkSupport()
+	if err != nil {
+		return true, fmt.Errorf("there was an issue verifying whether trunk support is available, please disable it: %v", err)


I don't think the instruction to 'please disable it' is helpful. We don't know what the error is.

That was the original text. But, changing it based on the next comment.

macaptain · 2021-09-28T18:50:17Z

pkg/cloud/services/compute/instance.go

+		return true, fmt.Errorf("there was an issue verifying whether trunk support is available, please disable it: %v", err)
+	}
+	if !trunkSupport {
+		return true, fmt.Errorf("there is no trunk support. Please disable it")


'Please disable it' isn't a clear instruction. The user has two options to proceed with their configuration: either they enable the trunk extension in their OpenStack deployment, or they give up on using trunking. I think a more helpful message would be "Please ensure that the trunk extension is enabled in your OpenStack deployment."

macaptain · 2021-09-28T18:55:48Z

pkg/cloud/services/compute/instance.go

+	trunkNotSupported, err := s.trunkNotSupported()
+	if trunkNotSupported {
+		return err


Does this mean we return an error when trying to delete an instance whenever trunking is not enabled in the environment? I think you only want to return err here if s.trunkNotSupported() returned an error. If I have understood this code, this is broken and will break instance deletion in all environments without the trunk extension enabled.

yes, I think the delete we don't need check trunk support at all
it's weird that we can create trunk on openstack but not able to delete it ..
so delete the trunk if we enabled it , that's good enough:)

I haven't added this part. It was already there. see from the master branch

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

Line 447 in 180f320

trunkSupport, err := s.networkingService.GetTrunkSupport()

It would be great to remove the check!

I think it is being used here

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

Lines 457 to 461 in 180f320

if trunkSupport {

if err = s.networkingService.DeleteTrunk(eventObject, port.PortID); err != nil {

return err

}

}

.
Should I remove both ?

macaptain · 2021-09-28T18:59:43Z

pkg/cloud/services/compute/instance.go

@@ -135,6 +132,13 @@ func (s *Service) constructNetworks(openStackCluster *infrav1.OpenStackCluster,
 		if pOpts.Trunk == nil {
 			pOpts.Trunk = &openStackMachine.Spec.Trunk
 		}
+		// verify trunk support
+		if *pOpts.Trunk {


Since *pOpts.Trunk defaults to openstackMachine.Spec.Trunk, do we need to check trunk support for openstackMachine.Spec.Trunk explicitly in lines 75-79 above? This here looks like the correct place to check.

That assignment is conditional.

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

Line 132 in ed8a592

if pOpts.Trunk == nil {

jichenjc · 2021-09-29T00:08:18Z

/ok-to-test

mdbooth

Some unit test coverage would also be really helpful here.

mdbooth · 2021-09-29T12:59:02Z

pkg/cloud/services/compute/instance.go

+			if port.Trunk == nil {
+				*port.Trunk = openStackMachine.Spec.Trunk
+			}
+			if openStackMachine.Spec.Trunk || *port.Trunk {


Given that you just set this above, you could simplify this to:

if *port.Trunk {

There is nowhere else where openStackMachine.Spec.Trunk is checked. So, we need both.

What we're saying here is that if Trunk is set on the Machine it will be set on all ports, regardless of the explicit setting for that port. Is that correct?

The opposite:

If set@port => take port level and ignore Machine level

if not set@port => take Machine level

on verifying trunk support:
(a) No matter how trunk was setup via (1) or (2): check at port level (*port.Trunk)
(b) In case no port was supplied => check at instance level (openStackMachine.Spec.Trunk)

pkg/cloud/services/compute/instance.go

mdbooth · 2021-09-29T13:06:32Z

pkg/cloud/services/compute/instance.go

+				trunkSupported, err := s.isTrunkExtSupported()
+				if err != nil {
+					return nil, err
+				}


Please can you move this outside the loop? This is adding an extra API call per port.

When it was outside, it was checking only instance level before instanceSpec.Trunk = openStackMachine.Spec.Trunk. Now, in the for loop, it is checking both instance level and port level.

I agree, in the current format, it is making API calls per port. I am trying to make it efficient by stopping at the first occurrence trunk-set port. I have not succeeded so far as combing return and break is not possible.

Note that making API calls per port is not avoidable. The other location where the check could be added is here. Both do iterate over the ports.

The only thing you need to move outside the loop is the code above which sets trunkSupported. The value of trunkSupported will never change during the loop. You can leave the code below, which generates the error, inside the loop.

Thanks. Moving it out.

Xenwar · 2021-09-29T16:26:06Z

Some unit test coverage would also be really helpful here.

Thanks, there is an ongoing work by @macaptain on the tests. We needed to make the change on this PR in order to test the check.

mdbooth · 2021-09-30T21:08:43Z

pkg/cloud/services/compute/instance.go

+			if port.Trunk == nil {
+				*port.Trunk = openStackMachine.Spec.Trunk
+			}
+			if openStackMachine.Spec.Trunk || *port.Trunk {


What we're saying here is that if Trunk is set on the Machine it will be set on all ports, regardless of the explicit setting for that port. Is that correct?

mdbooth · 2021-09-30T21:11:28Z

pkg/cloud/services/compute/instance.go

+				trunkSupported, err := s.isTrunkExtSupported()
+				if err != nil {
+					return nil, err
+				}


The only thing you need to move outside the loop is the code above which sets trunkSupported. The value of trunkSupported will never change during the loop. You can leave the code below, which generates the error, inside the loop.

mdbooth · 2021-09-30T21:19:11Z

pkg/cloud/services/compute/instance.go

@@ -113,6 +104,25 @@ func (s *Service) CreateInstance(openStackCluster *infrav1.OpenStackCluster, mac
 	if err != nil {
 		return nil, err
 	}
+	// verify trunk support if trunk is set on at least one Port.
+	for _, net := range nets {


Here we're iterating over nets, which is the return value of constructNetworks(). constructNetworks returns nets generated from both Spec.Networks and Spec.Ports.

I believe this means we're now potentially trunking ports generated for both Spec.Networks and Spec.Ports, where previously we were only trunking Spec.Ports.

I'm not clear on the use case difference between these 2 fields, but I feel like this is a significant change which we should make deliberately. Perhaps @iamemilio or @dulek know?

I agree. To avoid that, one option is to move the inheriting logic to

cluster-api-provider-openstack/pkg/cloud/services/networking/port.go

Lines 166 to 179 in 1be73ec

if portOpts.Trunk != nil && *portOpts.Trunk {

trunk, err := s.getOrCreateTrunk(eventObject, clusterName, port.Name, port.ID)

if err != nil {

record.Warnf(eventObject, "FailedCreateTrunk", "Failed to create trunk for port %s: %v", portName, err)

return nil, err

}

if err = s.replaceAllAttributesTags(eventObject, trunk.ID, tags); err != nil {

record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace trunk tags %s: %v", portName, err)

return nil, err

}

}

return port, nil

}

And, pass the openstackmachine resource to it.

I do think the inheriting logic belongs in constructNetworks() and we should make a separate PR if we need to address the inconsistency in inheriting the machine's trunk value between ports and networks. This location in CreateInstance is a good place to check if we need to make an API call to check for trunk support in OpenStack.

Yep, lets leave the inheriting logic in constructNetworks(). I also think defaulting it there means we don't have to check it here and just look directly at the ports.

Due to the lack of map/reduce in Go, how about something like:

trunkingRequired := func() bool { for _, port := range ports { if *port.Trunk != nil && *port.Trunk { return true } } return false }() if trunkingRequired { if !s.isTrunkExtSupported() { ... err } }

Or something more idiomatic if you know what that is 🙂

This means we only check trunk support once, and only if we actually need it.

Thanks, done

mdbooth · 2021-10-01T08:38:28Z

pkg/cloud/services/compute/instance.go

@@ -113,6 +104,25 @@ func (s *Service) CreateInstance(openStackCluster *infrav1.OpenStackCluster, mac
 	if err != nil {
 		return nil, err
 	}
+	// verify trunk support if trunk is set on at least one Port.
+	trunkSupported, err := s.isTrunkExtSupported()


👍, but you need to move the error checking (currently lines 117-119) up here, too.

At that point, the second par to the following (i.e. *port.Trunk) is not available outside of the loop. We can only check the instance level outside of the loop.
if openStackMachine.Spec.Trunk || *port.Trunk

As we have it now, the error checking detached from the actual api call, perhaps, we need to move 108 back inside ?

Thanks, done

mdbooth · 2021-10-01T09:30:23Z

pkg/cloud/services/compute/instance.go

@@ -113,6 +104,25 @@ func (s *Service) CreateInstance(openStackCluster *infrav1.OpenStackCluster, mac
 	if err != nil {
 		return nil, err
 	}
+	// verify trunk support if trunk is set on at least one Port.
+	for _, net := range nets {


Yep, lets leave the inheriting logic in constructNetworks(). I also think defaulting it there means we don't have to check it here and just look directly at the ports.

Due to the lack of map/reduce in Go, how about something like:

trunkingRequired := func() bool { for _, port := range ports { if *port.Trunk != nil && *port.Trunk { return true } } return false }() if trunkingRequired { if !s.isTrunkExtSupported() { ... err } }

Or something more idiomatic if you know what that is 🙂

This means we only check trunk support once, and only if we actually need it.

Xenwar · 2021-10-10T19:15:52Z

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

Xenwar · 2021-10-25T07:33:44Z

If there are no further comments on this issue, could this PR be approved ?

hidekazuna · 2021-10-26T00:09:53Z

/approve

Xenwar · 2021-10-27T12:47:03Z

Made some changes based on a feedback received from slack.

macaptain · 2021-10-28T08:55:33Z

Now that the constructNetworks function isn't changed in this PR, this looks good to me. I think there is no change except that we run a trunk support check when trunk is enabled on at least one of the ports, or at the instance level. Now there are no changes to the existing inheritance behavior of trunking (which could be debated in a separate issue).

/lgtm

Xenwar · 2021-10-28T09:08:10Z

@mdbooth, the PR is now approved and lgtm-ed
I am putting a hold to get your view on the current version.
/hold

mdbooth

/lgtm

mdbooth · 2021-11-01T09:48:00Z

pkg/cloud/services/compute/instance.go

+func (s *Service) isTrunkExtSupported() (trunknSupported bool, err error) {
+	trunkSupport, err := s.networkingService.GetTrunkSupport()
+	if err != nil {
+		return false, fmt.Errorf("there was an issue verifying whether trunk support is available, please disable trunk on your manifests: %v", err)


nit: please disable trunk on your manifests is probably the wrong advice here, as any error returned by this api call is unlikely to be related to trunk support. Please try again later would be better advice and we'll do that anyway, so probably just log it.

Thanks, fixed.

k8s-ci-robot · 2021-11-01T09:50:34Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hidekazuna, macaptain, mdbooth, 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~~ [hidekazuna]

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

jichenjc · 2021-11-01T10:00:35Z

/hold cancel

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

macaptain · 2021-11-01T11:09:59Z

/lgtm

k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 28, 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 Sep 28, 2021

k8s-ci-robot requested review from chrischdi and hidekazuna September 28, 2021 12:08

k8s-ci-robot added the size/S Denotes a PR that changes 10-29 lines, ignoring generated files. label Sep 28, 2021

Xenwar force-pushed the port_level_trunk_check branch from 140bc24 to ed8a592 Compare September 28, 2021 14:15

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 28, 2021

namnx228 reviewed Sep 28, 2021

View reviewed changes

macaptain suggested changes Sep 28, 2021

View reviewed changes

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 Sep 29, 2021

Xenwar force-pushed the port_level_trunk_check branch 6 times, most recently from d59ad70 to aa2648d Compare September 29, 2021 12:54

mdbooth requested changes Sep 29, 2021

View reviewed changes

Xenwar force-pushed the port_level_trunk_check branch from aa2648d to 06465a5 Compare September 29, 2021 16:42

mdbooth reviewed Sep 30, 2021

View reviewed changes

Xenwar force-pushed the port_level_trunk_check branch from 06465a5 to 7525e1e Compare October 1, 2021 06:23

mdbooth requested changes Oct 5, 2021

View reviewed changes

macaptain mentioned this pull request Oct 6, 2021

REQUEST: New membership for macaptain kubernetes/org#3030

Closed

7 tasks

Xenwar force-pushed the port_level_trunk_check branch from 7525e1e to 11048f1 Compare October 8, 2021 07:09

Xenwar force-pushed the port_level_trunk_check branch 3 times, most recently from dc22b42 to ad1a7d3 Compare October 19, 2021 08:54

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

hidekazuna mentioned this pull request Oct 26, 2021

cluster-api v1beta1 adoption ? #1024

Closed

Xenwar force-pushed the port_level_trunk_check branch from ad1a7d3 to b658554 Compare October 27, 2021 12:45

Xenwar force-pushed the port_level_trunk_check branch 3 times, most recently from 4b079c6 to c125a4a Compare October 28, 2021 08:50

k8s-ci-robot assigned macaptain Oct 28, 2021

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

macaptain approved these changes Oct 28, 2021

View reviewed changes

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 28, 2021

mdbooth approved these changes Nov 1, 2021

View reviewed changes

k8s-ci-robot assigned mdbooth Nov 1, 2021

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 1, 2021

Check trunk support before enabling trunk at port level

74dfbf3

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

Xenwar force-pushed the port_level_trunk_check branch from c125a4a to 74dfbf3 Compare November 1, 2021 10:06

k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 1, 2021

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

k8s-ci-robot merged commit 82bc42e into kubernetes-sigs:main Nov 1, 2021

mdbooth mentioned this pull request Dec 13, 2021

REQUEST: New membership for mdbooth kubernetes/org#3142

Closed

7 tasks

	if trunkSupport {
	if err = s.networkingService.DeleteTrunk(eventObject, port.PortID); err != nil {
	return err
	}
	}

	if portOpts.Trunk != nil && *portOpts.Trunk {
	trunk, err := s.getOrCreateTrunk(eventObject, clusterName, port.Name, port.ID)
	if err != nil {
	record.Warnf(eventObject, "FailedCreateTrunk", "Failed to create trunk for port %s: %v", portName, err)
	return nil, err
	}
	if err = s.replaceAllAttributesTags(eventObject, trunk.ID, tags); err != nil {
	record.Warnf(eventObject, "FailedReplaceTags", "Failed to replace trunk tags %s: %v", portName, err)
	return nil, err
	}
	}

	return port, nil
	}

Check trunk support before enabling trunk at port level #1014

Check trunk support before enabling trunk at port level #1014

Conversation

Xenwar commented Sep 28, 2021

netlify bot commented Sep 28, 2021 • edited Loading

k8s-ci-robot commented Sep 28, 2021

namnx228 left a comment

Choose a reason for hiding this comment

macaptain left a 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

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

jichenjc commented Sep 29, 2021

mdbooth left a 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

Xenwar commented Sep 29, 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

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

Xenwar commented Oct 10, 2021

Xenwar commented Oct 25, 2021

hidekazuna commented Oct 26, 2021

Xenwar commented Oct 27, 2021

macaptain commented Oct 28, 2021 • edited Loading

Xenwar commented Oct 28, 2021

mdbooth left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k8s-ci-robot commented Nov 1, 2021

jichenjc commented Nov 1, 2021

macaptain commented Nov 1, 2021

netlify bot commented Sep 28, 2021 •

edited

Loading

macaptain commented Oct 28, 2021 •

edited

Loading