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

adduarte · 2021-07-07T09:57:37Z

Extend trunk configuration to port level in machineset
With the introduction of new technologies, like SR-IOV,
it is required that the user have the ability to
configured trunked and not-trunked ports for the same machines
using spec.ports.trunk.

Extended trunk support needed check to cover spec.port and spec.network
Makes ports inherit trunk setting from spec.Trunk if not specified.
Refactored trunking to its own function. Trunk name matches port name
Added trunking to spec.ports

OSASINFRA-2504

adduarte · 2021-07-08T20:02:53Z

As it currently stands, this patch brings us up to par with kubernetes-sigs/cluster-api-provider-openstack functionality:
you can define interfaces under "network" and "port" and you can trunk All of them or None of them.

This make a deployment using kuryr but with SR-IOV ports not trunked impossible.

adduarte · 2021-07-14T08:21:00Z

/retest

adduarte · 2021-07-14T08:28:36Z

This functionality is being addressed upstream in pr kubernetes-sigs#934.
Is best if we rework this pr to mirror upstream code and logic.

dulek · 2021-07-21T08:39:25Z

Looks good to me, but I'll leave the lgtm label for folks that are more familiar with this code.

adduarte · 2021-07-21T15:21:28Z

/uncc Fedosin
/cc mandre

adduarte · 2021-07-21T15:21:43Z

/approve (just to be explicit)

openshift-ci · 2021-07-21T15:21:47Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adduarte

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

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [adduarte]

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

mandre

I think it should work - CI at least shows there is no regression. Were you able to test this patch?

adduarte · 2021-07-22T09:31:55Z

I should have enough testing done locally to verify its functionality soon.

adduarte · 2021-07-23T05:02:43Z

@mandre just spend the day testing and it seems to work as expected. Made some trivial changes to name trunks consistently.
But I tested
network.trunk = true & port.trunk=(nil|false,true) and network.trunk=false & port.trunk=(nil,false,true) and all seems to work as expected.

openshift-ci · 2021-07-23T09:19:43Z

@adduarte: This pull request references Bugzilla bug 1964540, which is invalid:

expected the bug to target the "4.9.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 1964540: Extend trunk configuration to port level in machineset

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.

mandre · 2021-07-23T09:21:43Z

/bugzilla refresh

openshift-ci · 2021-07-23T09:21:53Z

@mandre: This pull request references Bugzilla bug 1964540, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.9.0) matches configured target release for branch (4.9.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @eurijon

Details

In response to this:

/bugzilla refresh

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.

openshift-ci · 2021-07-23T09:21:54Z

@openshift-ci[bot]: GitHub didn't allow me to request PR reviews from the following users: eurijon.

Note that only openshift members and repo collaborators can review this PR, and authors cannot review their own PRs.

Details

In response to this:

@mandre: This pull request references Bugzilla bug 1964540, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)

bug target release (4.9.0) matches configured target release for branch (4.9.0)

bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

Requesting review from QA contact:
/cc @eurijon

In response to this:

/bugzilla refresh

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.

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.

adduarte · 2021-07-25T06:30:14Z

Addressed comment about trunkName. See inline reply

adduarte · 2021-07-25T06:36:04Z

@dulek could you take a ,ook at the code that sets the name of the trunk.
Perhaps we shoudl not change the name of the trunk at all...

iamemilio · 2021-07-26T14:10:19Z

we should probably wait for the upstream patch to settle and merge before approving this, it seems like they are still addressing some comments.

mandre · 2021-07-26T16:14:14Z

we should probably wait for the upstream patch to settle and merge before approving this, it seems like they are still addressing some comments.

Keep in mind we're talking about downstream CAPO and we want to retire this code soon anyway. We don't have to wait for the upstream code to merge (we'll inherit it anyway once we move to MAPO), however we need to make absolutely sure the API is stable and agreed on upstream. This is what we must implement in this patch.

With the introduction of new technologies, like SR-IOV, it is required that the user have the ability to configured trunked and not-trunked ports for the same machines using spec.ports.trunk. - Extended trunk support needed check to cover spec.port and spec.network - Makes ports inherit trunk setting from spec.Trunk if not specified. - Refactored trunking to its own function. Trunk name matches port name - Added trunking to spec.ports - Trunks created for instances are named after their parent port. OSASINFRA-2504 Co-authored-by: Martin André <m.andre@redhat.com>

adduarte · 2021-07-27T01:41:02Z

the current version of this patch mimics upstream:
trunks are created with trunkname=port.name,

iamemilio · 2021-07-27T16:00:02Z

Keep in mind we're talking about downstream CAPO and we want to retire this code soon anyway. We don't have to wait for the upstream code to merge (we'll inherit it anyway once we move to MAPO), however we need to make absolutely sure the API is stable and agreed on upstream. This is what we must implement in this patch.

true, as long as the upstream api is stable, I suppose its ok

iamemilio · 2021-07-28T14:31:42Z

little nit pick, but otherwise lgtm

Co-authored-by: Emilio Garcia <iamemilio@users.noreply.github.com>

iamemilio · 2021-07-29T16:42:05Z

/lgtm

openshift-ci · 2021-07-29T16:43:24Z

@adduarte: All pull requests linked via external trackers have merged:

openshift/cluster-api-provider-openstack#185

Bugzilla bug 1964540 has been moved to the MODIFIED state.

Details

In response to this:

Bug 1964540: Extend trunk configuration to port level in machineset

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.

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 7, 2021

openshift-ci Bot requested review from EmilienM and Fedosin July 7, 2021 09:57

adduarte force-pushed the OSASINFRA-2504 branch from 2caddc8 to 20e6fa3 Compare July 8, 2021 14:38

adduarte force-pushed the OSASINFRA-2504 branch 6 times, most recently from da7e047 to 6455359 Compare July 21, 2021 03:32

adduarte changed the title ~~[WIP] Extend trunk configuration to port level in machineset~~ Extend trunk configuration to port level in machineset Jul 21, 2021

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 21, 2021

openshift-ci Bot requested review from mandre and removed request for Fedosin July 21, 2021 15:21

openshift-ci Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 21, 2021

mandre reviewed Jul 22, 2021

View reviewed changes

adduarte force-pushed the OSASINFRA-2504 branch from 4a9414d to 032b736 Compare July 22, 2021 09:31

mandre reviewed Jul 22, 2021

View reviewed changes

Comment thread pkg/cloud/openstack/clients/machineservice.go Outdated

adduarte force-pushed the OSASINFRA-2504 branch 2 times, most recently from 6d8e493 to b012228 Compare July 23, 2021 04:27

adduarte force-pushed the OSASINFRA-2504 branch from 0833663 to 2bb2e9e Compare July 23, 2021 05:06

mandre changed the title ~~Extend trunk configuration to port level in machineset~~ Bug 1964540: Extend trunk configuration to port level in machineset Jul 23, 2021

openshift-ci Bot added bugzilla/severity-medium Referenced Bugzilla bug's severity is medium for the branch this PR is targeting. bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jul 23, 2021

openshift-ci Bot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. and removed bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. labels Jul 23, 2021

mandre mentioned this pull request Jul 23, 2021

Add SR-IOV notes specific to Kuryr openshift/installer#4957

Closed

adduarte force-pushed the OSASINFRA-2504 branch 2 times, most recently from 52b7473 to 530bc77 Compare July 25, 2021 06:29

adduarte force-pushed the OSASINFRA-2504 branch from 530bc77 to 00da405 Compare July 25, 2021 07:06

adduarte force-pushed the OSASINFRA-2504 branch from 00da405 to efb5628 Compare July 26, 2021 22:35

iamemilio reviewed Jul 28, 2021

View reviewed changes

Comment thread pkg/cloud/openstack/clients/machineservice.go Outdated

Update pkg/cloud/openstack/clients/machineservice.go

4803fe2

Co-authored-by: Emilio Garcia <iamemilio@users.noreply.github.com>

openshift-ci Bot assigned iamemilio Jul 29, 2021

openshift-ci Bot added the lgtm Indicates that a PR is ready to be merged. label Jul 29, 2021

openshift-merge-robot merged commit 065b0b4 into openshift:master Jul 29, 2021

pierreprinetti deleted the OSASINFRA-2504 branch March 29, 2022 12:45

Conversation

adduarte commented Jul 7, 2021 • edited by openshift-ci Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

adduarte commented Jul 8, 2021

Uh oh!

adduarte commented Jul 14, 2021

Uh oh!

adduarte commented Jul 14, 2021

Uh oh!

dulek commented Jul 21, 2021

Uh oh!

adduarte commented Jul 21, 2021

Uh oh!

adduarte commented Jul 21, 2021

Uh oh!

openshift-ci Bot commented Jul 21, 2021

Uh oh!

mandre left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

adduarte commented Jul 22, 2021

Uh oh!

Uh oh!

adduarte commented Jul 23, 2021

Uh oh!

openshift-ci Bot commented Jul 23, 2021

Uh oh!

mandre commented Jul 23, 2021

Uh oh!

openshift-ci Bot commented Jul 23, 2021

Uh oh!

openshift-ci Bot commented Jul 23, 2021

Uh oh!

adduarte commented Jul 25, 2021

Uh oh!

adduarte commented Jul 25, 2021

Uh oh!

iamemilio commented Jul 26, 2021

Uh oh!

mandre commented Jul 26, 2021

Uh oh!

adduarte commented Jul 27, 2021

Uh oh!

iamemilio commented Jul 27, 2021

Uh oh!

Uh oh!

iamemilio commented Jul 28, 2021

Uh oh!

iamemilio commented Jul 29, 2021

Uh oh!

openshift-ci Bot commented Jul 29, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

adduarte commented Jul 7, 2021 •

edited by openshift-ci Bot

Loading