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

jingczhang · 2023-07-24T17:15:14Z

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, nodeAddresses() 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 nodeAddresses() 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 neturon trunk.

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

Release note:

NONE

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

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.

jingczhang · 2023-07-24T17:30:36Z

Rebase caused the creation of this new PR.
I will update the PR for last comment from #2270.

jichenjc · 2023-07-26T03:46:55Z

/ok-to-test

dulek · 2023-07-26T16:13:17Z

@mdbooth, you probably want to take another look here.

jingczhang · 2023-07-27T02:13:07Z

Hi @mdbooth, I updated the PR per your comment, pls review. I am testing with k8s 1.23, I am unable to retrieve the trunk_details as you expect, I used continue to skip the code changes for now.

mdbooth · 2023-07-27T10:03:19Z

Hi @mdbooth, I updated the PR per your comment, pls review. I am testing with k8s 1.23, I am unable to retrieve the trunk_details as you expect, I used continue to skip the code changes for now.

Thanks! This is high on my todo list but unfortunately it doesn't look like I'm going to get to it this week. Hopefully Monday.

jingczhang · 2023-07-28T00:50:33Z

Hi @mdbooth, I managed to test with k8s 1.23, all good, works as expected. It is amazing the master branch works with k8s 1.23.

pkg/openstack/instances.go

mdbooth

/lgtm

mdbooth · 2023-07-31T08:53:51Z

pkg/openstack/instances.go

+	for nw, ips := range subportAddresses {
+		srvAddresses, ok := addresses[nw]
+		if !ok {
+			addresses[nw] = ips
+		} else {
+			// this is to take care the corner case
+			// where the same network is attached to the node both directly and via trunk
+			addresses[nw] = append(srvAddresses, ips...)
+		}
+	}
+


Suggestion: we're already looping over this above (in the FixedIPs loop). You could move this to there and eliminate subportAddresses.

mdbooth · 2023-07-31T09:23:52Z

/test pull-cloud-provider-openstack-check
/test pull-cloud-provider-openstack-test

mdbooth · 2023-07-31T09:26:02Z

@jingczhang Those test failures were weird. I'm currently assuming they don't relate to the current version of your PR.

Please could you squash the commits before merging?

mdbooth · 2023-07-31T09:52:41Z

Oh, nm. I had some stale state locally. Yeah, you need to fix the unit tests. If you do that and also squash the commits this still lgtm.

dulek · 2023-07-31T15:36:47Z

Yup, looks good, just needs the unit tests to be fixed. Thanks!

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, nodeAddresses() 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 nodeAddresses() 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 neturon trunk. Signed-off-by: Jing Zhang <[email protected]>

jingczhang · 2023-07-31T19:16:42Z

Hi @mdbooth, your last comment has been taken care of, please review.

jichenjc · 2023-08-01T02:55:42Z

/approve

I am ok with current code but looks the UT only modify existing cases
can we follow up create more UT To cover, e.g multiple VLAN address returned ?

k8s-ci-robot · 2023-08-01T02:55:50Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jichenjc

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:

~~pkg/openstack/OWNERS~~ [jichenjc]

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

dulek · 2023-08-01T09:39:24Z

/lgtm

mdbooth · 2023-08-02T12:53:21Z

/approve

I am ok with current code but looks the UT only modify existing cases can we follow up create more UT To cover, e.g multiple VLAN address returned ?

The last time we looked at this the problem was that it's extremely hard to do without being able to mock OpenStack API calls. We can do this in CAPO now, but the solution was quite intrusive and it's not easy to share. It's on my longer-term todo list to find a way to do this, ideally directly in gophercloud.

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, nodeAddresses() 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 nodeAddresses() 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 neturon trunk. Signed-off-by: Jing Zhang <[email protected]>

k8s-ci-robot added the release-note-none Denotes a PR that doesn't merit a release note. label Jul 24, 2023

k8s-ci-robot requested review from anguslees and FengyunPan2 July 24, 2023 17:15

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 Jul 24, 2023

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 24, 2023

jingczhang changed the title ~~[occm] Get IP addresses of neutron subports~~ [occm] Get IP addresses of neutron subports v3 Jul 24, 2023

jingczhang mentioned this pull request Jul 24, 2023

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

Closed

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 26, 2023

mdbooth reviewed Jul 31, 2023

View reviewed changes

pkg/openstack/instances.go Show resolved Hide resolved

mdbooth reviewed Jul 31, 2023

View reviewed changes

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

mdbooth reviewed Jul 31, 2023

View reviewed changes

k8s-ci-robot assigned mdbooth Jul 31, 2023

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 31, 2023

jingczhang force-pushed the dev/trunk-support branch from c2d1c1e to 9878cab Compare July 31, 2023 17:47

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 31, 2023

jingczhang force-pushed the dev/trunk-support branch from 9878cab to 3e01293 Compare July 31, 2023 18:35

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

k8s-ci-robot assigned dulek Aug 1, 2023

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

k8s-ci-robot merged commit b0429dc into kubernetes:master Aug 1, 2023

jingczhang deleted the dev/trunk-support branch August 1, 2023 11:59

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

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

Uh oh!

Conversation

jingczhang commented Jul 24, 2023

Uh oh!

k8s-ci-robot commented Jul 24, 2023

Uh oh!

jingczhang commented Jul 24, 2023

Uh oh!

jichenjc commented Jul 26, 2023

Uh oh!

dulek commented Jul 26, 2023

Uh oh!

jingczhang commented Jul 27, 2023

Uh oh!

mdbooth commented Jul 27, 2023

Uh oh!

jingczhang commented Jul 28, 2023

Uh oh!

Uh oh!

Uh oh!

mdbooth left a comment

Choose a reason for hiding this comment

Uh oh!

mdbooth Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

jingczhang Jul 31, 2023

Choose a reason for hiding this comment

Uh oh!

mdbooth commented Jul 31, 2023

Uh oh!

mdbooth commented Jul 31, 2023

Uh oh!

mdbooth commented Jul 31, 2023

Uh oh!

dulek commented Jul 31, 2023

Uh oh!

jingczhang commented Jul 31, 2023

Uh oh!

jichenjc commented Aug 1, 2023

Uh oh!

k8s-ci-robot commented Aug 1, 2023

Uh oh!

dulek commented Aug 1, 2023

Uh oh!

mdbooth commented Aug 2, 2023

Uh oh!

Uh oh!