[occm] Get IP addresses of neutron subports v2 #2270

jingczhang · 2023-06-06T20:36:11Z

Vlan-aware VMs are commonly used in telecom.
Those VMs are plugged into flat networks and use neutron trunk:
https://docs.openstack.org/neutron/latest/admin/config-trunking.html

Currenlty, getAttachedPorts() only returns neutron ports directly attached to VM. For vlan-aware VMs, IP addresses are assigned on neutron subports. Subports are attached to the trunk; they are not attached to the VM directly. This pull request changes getAttachedPorts() to return IP addresses of neutron subports when they exist. Without this change, Kubernetes is unable to IP addresses of vlan-aware VMs. This change is transparent to VMs not using neutron trunk.

Special notes for reviewers:
This is a rebase of the following PR:
#2228

Release note:

NONE

k8s-ci-robot · 2023-06-06T20:36:20Z

Hi @jingczhang. Thanks for your PR.

I'm waiting for a kubernetes 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.

mdbooth · 2023-06-07T16:49:13Z

/ok-to-test
/assign

jingczhang · 2023-06-11T01:11:53Z

Hi @mdbooth, what is the minimum occm release version for getAttachedPorts() to work? 1.27?

pkg/openstack/instances.go

zetaab · 2023-06-18T12:12:36Z

/retest

zetaab

/approve

ping @mdbooth as you assigned this to yourself

mdbooth

I don't understand why we need to fetch ports again when they should already contain trunk details, which also means we should not require an additional configuration option.

Can you explain why trunk details are not included?

pkg/openstack/instances.go

docs/openstack-cloud-controller-manager/using-openstack-cloud-controller-manager.md

zetaab · 2023-06-20T05:33:15Z

/retest

rjsadow · 2023-06-20T11:01:56Z

/retest

rjsadow · 2023-06-20T13:24:26Z

/retest

mdbooth

This looks on the right lines to me. I still have major reservations about the maintainability of this version, though, but unlike the previous patches I think they're easily fixable. Nice work!

pkg/openstack/instances.go

pkg/openstack/openstack_test.go

pkg/openstack/instances.go

mdbooth · 2023-06-20T15:03:33Z

pkg/openstack/instances.go

+	}
+
+	klog.V(5).Infof("Node %s has %d interfaces '%v'", serverID, len(interfaces), interfaces)
+	return interfaces, nil


Looking at how they're used above, I wonder if we should return the return signature of this method to:

func getAttachedPorts(client *gophercloud.ServiceClient, serverID string) (attachedPorts []ports.Port, subPorts []ports.Ports, err error)

And we return direct attached and subports separately. This would mean we wouldn't need the extremely confusing and error prone length conditional above. We could rename addExtraNodeAddresses to addSubPortNodeAddresses and call it unconditionally with the returned subports. If they're empty or nil it just wouldn't do anything.

Hmm, since the caller of getAttachedPorts does not care the additional level of details I am not sure changing its signature is needed (right thing to do), I can for sure change addExtraNodeAddresses to addSubPortNodeAddresses, for code clarity.

The critical difference is that we subsequently treat subports differently to directly attached ports because the former are not included in server addresses but the latter are. By returning them as a single un-differentiated slice we're making our lives more complicated later when we have to determine which is which.

An alternative might be to not fetch subports here at all and just return directly attached ports. We could scan for subports later when we unconditionally call addSubPortNodeAddresses. That would require changing the return signature of this function to return []portWithTrunkDetails.

The alternative sounds reasonable, will check and update.

The alternative seems a better idea, and it results in the same code structure as in the older releases, PR is updated.

mdbooth · 2023-06-20T15:07:16Z

/hold

dulek · 2023-06-28T14:46:16Z

/retest

This looks great to me, are we waiting for anything else here?

k8s-ci-robot · 2023-06-28T15:08:32Z

@jingczhang: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
openstack-cloud-controller-manager-e2e-test	`6b23f13`	link	true	`/test openstack-cloud-controller-manager-e2e-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.

mdbooth

This still isn't there. We need to avoid the second ports query.

mdbooth · 2023-07-03T10:49:54Z

pkg/openstack/instances.go

@@ -633,6 +635,34 @@ func nodeAddresses(srv *servers.Server, ports []ports.Port, networkingOpts Netwo
 		return nil, err
 	}

+	// Add the addresses assigned on subports via trunk
+	// This exposes the vlan networks to which subports are attached
+	if client != nil {


It would be a programming error for this to be nil. The only place I can see that passes nil here are the tests. We should fix that.

yes, this checking is added so unit test would not fail

mdbooth · 2023-07-03T11:09:40Z

pkg/openstack/instances.go

+	// Add the addresses assigned on subports via trunk
+	// This exposes the vlan networks to which subports are attached
+	if client != nil {
+		subports, err := getSubports(client, srv.ID)


Note that we're calling getSubports unconditionally for everybody here, but we're not passing in the list of ports that we already fetched, which already contains the subports.

yes, you are right, will take care of this.

mdbooth · 2023-07-03T11:10:29Z

pkg/openstack/instances.go

+	allPages, err := ports.List(client, listOpts).AllPages()
+	if err != nil {
+		return subports, err
+	}


We can't do this: it's adding the second port list for everybody. We've already done this query. We need to re-use the existing ports query.

ok, same as above, rebase results in a new PR, will take care of the comments in the new PR:
#2306

k8s-ci-robot · 2023-07-17T11:35:53Z

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.

jingczhang · 2023-07-18T00:55:20Z

Sorry for the late response, I was on vacation, I will take care of the release and new comment soon.

k8s-ci-robot · 2023-07-24T17:07:24Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: zetaab

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

Needs approval from an approver in each of these files:

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 added release-note-none Denotes a PR that doesn't merit a release note. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jun 6, 2023

k8s-ci-robot requested review from anguslees and FengyunPan2 June 6, 2023 20:36

k8s-ci-robot added needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Jun 6, 2023

jingczhang mentioned this pull request Jun 6, 2023

[occm] Get IP addresses of neutron subports #2228

Closed

k8s-ci-robot added the ok-to-test Indicates a non-member PR verified by an org member that is safe to test. label Jun 7, 2023

k8s-ci-robot assigned mdbooth Jun 7, 2023

k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jun 7, 2023

zetaab reviewed Jun 14, 2023

View reviewed changes

pkg/openstack/instances.go Outdated Show resolved Hide resolved

zetaab reviewed Jun 14, 2023

View reviewed changes

pkg/openstack/instances.go Outdated Show resolved Hide resolved

zetaab approved these changes Jun 18, 2023

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jun 18, 2023

mdbooth suggested changes Jun 19, 2023

View reviewed changes

pkg/openstack/instances.go Outdated Show resolved Hide resolved

mdbooth reviewed Jun 19, 2023

View reviewed changes

docs/openstack-cloud-controller-manager/using-openstack-cloud-controller-manager.md Outdated Show resolved Hide resolved

mdbooth suggested changes Jun 20, 2023

View reviewed changes

k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. 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 Jun 20, 2023

mdbooth suggested changes Jul 3, 2023

View reviewed changes

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

jingczhang closed this Jul 24, 2023

jingczhang force-pushed the dev/trunk-support branch 2 times, most recently from 6b23f13 to 8a0e73f Compare July 24, 2023 17:07

k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. and removed approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 24, 2023

jingczhang mentioned this pull request Jul 24, 2023

[occm] Get IP addresses of neutron subports v3 #2306

Merged

[occm] Get IP addresses of neutron subports v2 #2270

[occm] Get IP addresses of neutron subports v2 #2270

Uh oh!

Conversation

jingczhang commented Jun 6, 2023

Uh oh!

k8s-ci-robot commented Jun 6, 2023

Uh oh!

mdbooth commented Jun 7, 2023

Uh oh!

jingczhang commented Jun 11, 2023

Uh oh!

Uh oh!

Uh oh!

zetaab commented Jun 18, 2023

Uh oh!

zetaab left a comment

Choose a reason for hiding this comment

Uh oh!

mdbooth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

zetaab commented Jun 20, 2023

Uh oh!

rjsadow commented Jun 20, 2023

Uh oh!

rjsadow commented Jun 20, 2023

Uh oh!

mdbooth left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!

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!

mdbooth commented Jun 20, 2023

Uh oh!

dulek commented Jun 28, 2023

Uh oh!

k8s-ci-robot commented Jun 28, 2023

Uh oh!

mdbooth left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

k8s-ci-robot commented Jul 17, 2023

Uh oh!

jingczhang commented Jul 18, 2023

Uh oh!

k8s-ci-robot commented Jul 24, 2023