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

qemu: Don't use deprecated/removed vlan option for multinet #1103

Merged
merged 1 commit into from
Feb 27, 2019

Conversation

Martchus
Copy link
Contributor

Instead, just increase the number of network devices to be added to get another device (with different MAC address).

See https://progress.opensuse.org/issues/29419

@richiejp Am I'm missing something or is it really that easy? At least ip addr now shows the additional device with IP.

@codecov
Copy link

codecov bot commented Feb 13, 2019

Codecov Report

Merging #1103 into master will increase coverage by 1.7%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #1103     +/-   ##
=========================================
+ Coverage   38.51%   40.21%   +1.7%     
=========================================
  Files          40       40             
  Lines        4798     4794      -4     
  Branches      805      803      -2     
=========================================
+ Hits         1848     1928     +80     
+ Misses       2626     2541     -85     
- Partials      324      325      +1
Impacted Files Coverage Δ
backend/qemu.pm 32.45% <ø> (+1.54%) ⬆️
needle.pm 54.43% <0%> (+1.26%) ⬆️
consoles/VNC.pm 44.15% <0%> (+1.51%) ⬆️
consoles/console.pm 55% <0%> (+2.5%) ⬆️
consoles/vnc_base.pm 42.7% <0%> (+3.12%) ⬆️
backend/baseclass.pm 58.89% <0%> (+8.97%) ⬆️
consoles/network_console.pm 90.9% <0%> (+9.09%) ⬆️
consoles/sshIucvconn.pm 45% <0%> (+20%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 307c430...cb3f677. Read the comment docs.

@richiejp
Copy link
Contributor

It is not clear to me whether the two NICs will be on the same VLAN. @asmorodskyi says he will look into it. I'm not familiar with this code so I don't have much useful to say without doing a lot of research.

@Martchus
Copy link
Contributor Author

I don't think those would be in the same VLAN. Since the previous code explicitly used vlan=1 I thought that this might be what's wanted.

@asmorodskyi
Copy link
Member

in this case I suggest to drop this variable at all. I don't see actual use case where such combination could be tested. Basically if you got two NIC which not able to connect each other this means that you can only cover same thing which you could cover with "normal" one NIC machine.
Also after reading code I realized that same thing should be achievable by use of NICMAC or TAPDEV variables

It was using obsolete qemu options and is likely not used anyways.

The same can be achieved using NICMAC and TAPDEV variables.

See https://progress.opensuse.org/issues/29419
@Martchus
Copy link
Contributor Author

It is true, the change my $num_networks = $vars->{MULTINET} ? 2 : 1; would be just a shortcut for what could be achieved by passing multiple values via NICMAC or TAPDEV variables.

So I removed the support for that variable at all and updated the documentation to use NICMAC or TAPDEV variables. I don't think anybody is using that MULTINET code path containing obsolete and likely wrong code anyways, right?

@Martchus
Copy link
Contributor Author

So I assume nobody has any objections?

@Martchus Martchus merged commit 488a332 into os-autoinst:master Feb 27, 2019
@Martchus Martchus deleted the multinet branch February 27, 2019 12:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants