CLOUDSTACK-2998: Added two more device name patterns to valid bridge slaves (lo* and dummy*) by bwsw · Pull Request #3000 · apache/cloudstack

bwsw · 2018-11-04T22:56:10Z

Added dummy and lo devices to be treated as a normal bridge slave devs

Description

Changed device names list for bridge slave devices which are valid for networking.

#2998

Types of changes

Breaking change (fix or feature that would cause existing functionality to change)
New feature (non-breaking change which adds functionality)
Bug fix (non-breaking change which fixes an issue)
Enhancement (improves an existing feature and functionality)
Cleanup (Code refactoring and cleanup, that may add test cases)

Screenshots (if appropriate):

How Has This Been Tested?

The unit test was improved to handle new device names.

bwsw · 2018-11-05T00:02:35Z

Bug listed in the build
https://travis-ci.org/apache/cloudstack/jobs/450628551#L1047
Doesn't connected with changes and is related to datetime comparisons, happens from time to time. Probably introduced in another PR recently.

bwsw · 2018-11-05T02:15:53Z

@TestpublicvoidzonedTimeFormatIsoNoColonZMs() throwsParseException {
Datetime = newDate();
DateTimeFormatterformatter = DateTimeFormatter.ofPattern("yyyy-MM-dd'T'HH:mm:ss.SSSSSSZ");
Stringstr = OffsetDateTime.now().format(formatter);
DatedtParsed = DateUtil.parseTZDateString(str);
assertEquals(str, time.toString(), dtParsed.toString());
}

This unittest is obviously buggy. Because there are two times objects are used to get current DT, as a result, sometimes it fails.

bwsw · 2018-11-05T02:33:15Z

Bug introduced here: #2893

yadvr · 2018-11-05T02:41:33Z

Can you check Travis failure @bwsw, also drop the old styled s_ variable name.

bwsw · 2018-11-05T02:51:54Z

@rhtyd I have checked, please take a look at
PR: #2893
it's merged and the bug is introduced there.

bwsw · 2018-11-05T03:18:13Z

@rhtyd varname is fixed.

yadvr · 2018-11-05T03:23:08Z

@bwsw you're welcome to fix the unit test in this or a separate PR, otherwise someone eventually will.

wido · 2018-11-05T10:10:09Z

 if (vmTO.getType() != VirtualMachine.Type.User) {
 devices.addDevice(new ChannelDef(vmTO.getName() + ".vport", ChannelDef.ChannelType.UNIX,
-new File(_qemuSocketsPath + "/" + vmTO.getName() + ".agent")));
+ new File(_qemuSocketsPath + "/" + vmTO.getName() + ".agent")));


While you are busy with this, why not use File.seperator?

@wido it's not me. It's IDEA did that occasionally.

wido · 2018-11-05T10:10:18Z

 /* Add a VirtIO channel for the Qemu Guest Agent tools */
 devices.addDevice(new ChannelDef(_qemuGuestAgentSocketName, ChannelDef.ChannelType.UNIX,
-new File(_qemuSocketsPath + "/" + vmTO.getName() + "." + _qemuGuestAgentSocketName)));
+ new File(_qemuSocketsPath + "/" + vmTO.getName() + "." + _qemuGuestAgentSocketName)));


Same goes here

@wido same here

bwsw · 2018-11-05T19:29:09Z

@wido I addressed your comments by refactoring the code even further. Changed "+" to Paths.get(...) which does it safely. Please, review.

GabrielBrascher

Thanks, @bwsw.
Code LGTM and all checks have passed.

rafaelweingartner · 2018-11-07T12:17:38Z

 }

- String [] _ifNamePatterns = {
+static String [] ifNamePatterns = {


@bwsw, what if we invert the logic?
Let me explain myself. We already have 13! (that is too much; it is indeed a nice magic number though) patterns to tell which ones are the valid physical interfaces names that we should look for. However, we can do the opposite. So, we can say that the physical interface is anything that is not “^vnet*”. Then, if I decide to use a physical interface named “XYZ”, I will not need to change anything else in the code.
What do you think?

@rafaelweingartner I think it's a good extension, but suppose it's a conceptual change which requires voting from the project team. Don't you think so?
Personally, I like your approach, but people "potentially" may meet incompatibilities in their deployments if they have weird bridging configurations.
@rhtyd @PaulAngus @DaanHoogland @wido @GabrielBrascher can give your opinions on such a change, that @rafaelweingartner suggests?

Is it possible for people to configure/customize the "vnet" name of the interfaces? I thought that was something hard coded in KVM. That is why I thought doing the opposite would be easier.

@rafaelweingartner they can, but not sure that they can with CloudStack. Actually, your idea that only "vnet" devices must be excluded is good but just would like to get the opinion of other persons before changing that. In theory, it can break someone's deployment even if our tests work well.

@rafaelweingartner what I personally like is that people will no longer be limited by using "bond" or "team" names for physical interfaces, but can use other semantic names, that they like, the same is for single NICs like "public", "private" instead of eth0, eth1.

@bwsw I think that the proposal from @rafaelweingartner of inverting the logic is valid and gives us flexibility. I do not see any potential issue that such change would bring to us.
+1 on following that path.
Do you guys share the same thoughts @rhtyd @PaulAngus @DaanHoogland @wido @nvazquez?

I agree with @rafaelweingartner , @GabrielBrascher , but it would be a separate issue. I am not sure of the consequences so I would leave it out of scope for this PR.

@DaanHoogland +1

yadvr · 2018-11-12T06:06:57Z

@blueorangutan package

blueorangutan · 2018-11-12T06:07:18Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-11-12T06:48:56Z

Packaging result: ✔centos6 ✔centos7 ✖debian. JID-2424

yadvr · 2018-11-30T11:08:37Z

@bwsw can you fix the conflict, thanks.

Added two more device names (lo* and dummy*). Implemented tests. Code was refactored. Improved paths concatenation code from "+" to Paths.get.

bwsw · 2018-11-30T15:00:17Z

@rhtyd Conflicts fixed. Local build and tests passed.

yadvr · 2018-11-30T16:29:32Z

@blueorangutan package

blueorangutan · 2018-11-30T16:29:40Z

@rhtyd a Jenkins job has been kicked to build packages. I'll keep you posted as I make progress.

blueorangutan · 2018-11-30T16:49:46Z

Packaging result: ✔centos6 ✔centos7 ✔debian. JID-2475

yadvr · 2018-11-30T16:55:05Z

@blueorangutan test

blueorangutan · 2018-11-30T16:55:40Z

@rhtyd a Trillian-Jenkins test job (centos7 mgmt + kvm-centos7) has been kicked to run smoke tests

blueorangutan · 2018-12-01T00:58:34Z

Trillian test result (tid-3240)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 27502 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr3000-t3240-kvm-centos7.zip
Intermittent failure detected: /marvin/tests/smoke/test_multipleips_per_nic.py
Intermittent failure detected: /marvin/tests/smoke/test_ssvm.py
Intermittent failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Smoke tests completed. 68 look OK, 2 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
test_nic_secondaryip_add_remove	`Error`	41.56	test_multipleips_per_nic.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	271.79	test_vpc_redundant.py

bwsw changed the title ~~Added two more device names (lo* and dummy*).~~Added two more device names (lo* and dummy*)Nov 5, 2018

wido reviewed Nov 5, 2018
View reviewed changes

bwsw changed the title ~~Added two more device names (lo* and dummy*)~~2998: Added two more device names (lo* and dummy*)Nov 5, 2018

bwsw changed the title ~~2998: Added two more device names (lo* and dummy*)~~CLOUDSTACK-2998: Added two more device names (lo* and dummy*)Nov 5, 2018

wido approved these changes Nov 6, 2018
View reviewed changes

GabrielBrascher approved these changes Nov 6, 2018
View reviewed changes

bwsw changed the title ~~CLOUDSTACK-2998: Added two more device names (lo* and dummy*)~~CLOUDSTACK-2998: Added two more device name patterns to valid bridge slaves (lo* and dummy*)Nov 6, 2018

rafaelweingartner reviewed Nov 7, 2018
View reviewed changes

yadvr added this to the 4.12.0.0 milestone Nov 12, 2018

Feature ID:#2998
1ced317
Added two more device names (lo* and dummy*). Implemented tests. Code was refactored. Improved paths concatenation code from "+" to Paths.get.

yadvr approved these changes Dec 6, 2018
View reviewed changes

yadvr merged commit 9dce8a5 into apache:masterDec 6, 2018

Uh oh!

Conversation

bwsw commented Nov 4, 2018

Description

Types of changes

Screenshots (if appropriate):

How Has This Been Tested?

Uh oh!

bwsw commented Nov 5, 2018

Uh oh!

bwsw commented Nov 5, 2018

Uh oh!

bwsw commented Nov 5, 2018

Uh oh!

yadvr commented Nov 5, 2018

Uh oh!

bwsw commented Nov 5, 2018

Uh oh!

bwsw commented Nov 5, 2018

Uh oh!

yadvr commented Nov 5, 2018

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!

bwsw commented Nov 5, 2018

Uh oh!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

rafaelweingartnerNov 7, 2018• edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yadvr commented Nov 12, 2018

Uh oh!

blueorangutan commented Nov 12, 2018

Uh oh!

blueorangutan commented Nov 12, 2018

Uh oh!

yadvr commented Nov 30, 2018

Uh oh!

bwsw commented Nov 30, 2018

Uh oh!

yadvr commented Nov 30, 2018

Uh oh!

blueorangutan commented Nov 30, 2018

Uh oh!

blueorangutan commented Nov 30, 2018

Uh oh!

yadvr commented Nov 30, 2018

Uh oh!

blueorangutan commented Nov 30, 2018

Uh oh!

blueorangutan commented Dec 1, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

rafaelweingartnerNov 7, 2018•
edited
Loading