Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

govc: Add device protocol configuration for vm.network.add #3168

Merged
merged 1 commit into from
Jul 6, 2023

Conversation

lubronzhan
Copy link
Contributor

@lubronzhan lubronzhan commented Jun 30, 2023

Description

When user add network adapter type vmxnet3vrdma, they can configure the protocol rocev2/rocev1. This is only supported on adapter type vmxnet3vrdma.

Closes: ##3167

Type of change

Please mark options that are relevant:

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to
    not work as expected)
  • This change requires a documentation update
  • Build related change

How Has This Been Tested?

Please describe the tests that you ran to verify your changes. Provide
instructions so we can reproduce. If applicable, please also list any relevant
details for your test configuration.

  • Test Description 1
    Built on a linux machine. Example output

Correct help message

kubo@keDgLYFEehvWP:~/govmomi/govc$ ./govc vm.network.add  --help
Usage: ./govc vm.network.add [OPTIONS]

Add network adapter to VM.
...
  -net.address=                   Network hardware address
  -net.protocol=                  Network device protocol. Applicable to vmxnet3vrdma. Default to 'rocev2'

Attempt to add adapter with protocol

## ensure no regression
kubo@keDgLYFEehvWP:~/govmomi/govc$ ./govc vm.network.add -vm test -net "port-group-vlan-101" -net.adapter vmxnet3vrdma
kubo@keDgLYFEehvWP:~/govmomi/govc$ ./govc vm.network.add -vm test -net "port-group-vlan-101" -net.adapter e1000e
kubo@keDgLYFEehvWP:~/govmomi/govc$

## valid input

kubo@keDgLYFEehvWP:~/govmomi/govc$ ./govc vm.network.add -vm test -net "port-group-vlan-101" -net.adapter vmxnet3vrdma -net.protocol rocev2
kubo@keDgLYFEehvWP:~/govmomi/govc$ ./govc vm.network.add -vm test -net "port-group-vlan-101" -net.adapter vmxnet3vrdma -net.protocol rocev1

## invalid protocol
kubo@keDgLYFEehvWP:~/govmomi/govc$ ./govc vm.network.add -vm test -net "port-group-vlan-101" -net.adapter vmxnet3vrdma -net.protocol rocev0
./govc: invalid device protocol 'rocev0'

## invalid protocol&adapter combination
kubo@keDgLYFEehvWP:~/govmomi/govc$ ./govc vm.network.add -vm test -net "port-group-vlan-101" -net.adapter e1000e -net.protocol rocev1
./govc: device protocol is only suppported for vmxnet3vrdma at the moment
  • Test Description 2

Checklist:

  • My code follows the CONTRIBUTION
    guidelines of
    this project
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged

@lubronzhan lubronzhan changed the title govc: Add device protocol support for vm.network.add govc: Add device protocol configuration for vm.network.add Jun 30, 2023
@lubronzhan lubronzhan force-pushed the topic/lubron/device_protocol branch 6 times, most recently from 593fe94 to 716e25a Compare July 1, 2023 06:32
@lubronzhan
Copy link
Contributor Author

just noticed I didn't follow the issue-<> branch name

Copy link
Member

@dougm dougm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @lubronzhan
Just requesting a typo fix. But also cannot merge currently, unrelated to your change, but CI (goreleaser) needs fixing. They removed some deprecated options we're using: https://goreleaser.com/deprecations/#archivesreplacements
I've not done any goreleaser related work myself, other than bumping the go version once, but will try to look at that soon.

a.DeviceProtocol = flag.proto
}
} else if flag.proto != "" {
return nil, fmt.Errorf("device protocol is only suppported for vmxnet3vrdma at the moment")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

typo: supported

@lubronzhan
Copy link
Contributor Author

Thanks Doug.

@lubronzhan lubronzhan force-pushed the topic/lubron/device_protocol branch 2 times, most recently from ccb2ef6 to 521d0c6 Compare July 4, 2023 05:03
@lubronzhan
Copy link
Contributor Author

lubronzhan commented Jul 4, 2023

I fixed the release here #3170
It looks straightforward from the doc. Hopefully I didn't do it wrong
image

@dougm
Copy link
Member

dougm commented Jul 5, 2023

just noticed I didn't follow the issue-<> branch name

Branch name doesn't matter, but in the commit you have:

Closes: 3167
Missing the # needed by GH to auto-close and changelog generator:
Closes: #3167

Mentioning that since you'll need to rebase with your goreleaser change to merge anyhow (thanks again for that)

@lubronzhan lubronzhan force-pushed the topic/lubron/device_protocol branch from 521d0c6 to e34c721 Compare July 6, 2023 04:59
@lubronzhan
Copy link
Contributor Author

Ok let me try that thanks

Copy link
Member

@dougm dougm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks again @lubronzhan

@dougm dougm merged commit 3313033 into vmware:main Jul 6, 2023
10 checks passed
@lubronzhan lubronzhan deleted the topic/lubron/device_protocol branch July 6, 2023 06:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants