[CLOUDSTACK-10241] Duplicated file SRs being created in XenServer pools by rafaelweingartner · Pull Request #2414 · apache/cloudstack

rafaelweingartner · 2018-01-19T18:07:05Z

Due to a race condition between multiple management servers, in some rare cases, CloudStack is creating multiple file SRs to the same secondary folder. This causes a problem when introducing the SR to the XenServer pools, as “there will be VDIs with duplicated UUIDs“. The VDIs are the same, but they are seen in different SRs, and therefore cause an error.

The solution to avoid race conditions between management servers is to use a deterministic srUuid for the file SR to be created (we are leaving XenServer with the burden of managing race conditions). The UUID is based on the SR file path and is generated using UUID.nameUUIDFromBytes. Therefore, if there is an SR with the generated UUID, this means that some other management server has just created it. An exception will occur and it will contain a message saying 'Db_exn.Uniqueness_constraint_violation'. In these unlikely events, we catch the exception and use the method retrieveAlreadyConfiguredSrWithoutException to get the SR that has already been created for the given mount point.

DaanHoogland

sorry @rafaelweingartner , i've been trying to review this in between other things but the diff Xenserver625StorageProcessor.java won't load :( i'll restart when i have that fixed . (firefox on mac problem?)

DaanHoogland · 2018-01-30T12:26:12Z

@@ -17,7 +17,52 @@
 package com.cloud.hypervisor.xenserver.resource;


did anything actually change in this class? the file has +761 −214 but i cannot find a 'real' change. if it is only formatting maybe put it in a separate commit.

Sorry for that. It is a small change at lien 244. I removed a final keyword so I would be able to Spy an object to create unit test cases. I removed the formatting. It is my Eclipse, even though I configured it to format only edited lines, it keeps applying the standard on everything.

DaanHoogland · 2018-01-30T15:16:22Z

 @Before
 public void beforeTest() {
- super.citrixResourceBase = new Xenserver625Resource();
+ super.citrixResourceBase = Mockito.spy(new Xenserver625Resource());


i don't see this spy being used, why create it?

Oh, this one... It is because there is a hierarchy of test classes.. :(.. the property citrixResourceBase needs to be either a mock or spy object because of the testes executed in the super class. Therefore, needed to create a spy object whenever attributing an object to this property.

DaanHoogland · 2018-01-30T15:18:46Z

+
+ @Test
+ public void makeDirectoryTestCallHostPlugingReturningSomething() {
+ boolean makeDirectoryReturn = configureAndExecuteMakeDirectoryMethodForTest(pathMock, "Soemthing");


Soemthing on purpose or Something intended?

Ah this one. It can actually be any string. instead of writing a fictitious directory, which though it would be misleading. Therefore, I decided to write anything... I will try to change this...

rafaelweingartner · 2018-01-30T17:38:17Z

@DaanHoogland no problem! Thanks for the feedback so far.
Have you tried opening on Chrome? For me, it seems to work just fine.

Due to a race condition between multiple management servers, in some rare cases, CloudStack is creating multiple file SRs to the same secondary folder. This causes a problem when introducing the SR to the XenServer pools, as “there will be VDIs with duplicated UUIDs“. The VDIs are the same, but they are seen in different SRs, and therefore cause an error. The solution to avoid race conditions between management servers is to use a deterministic srUuid for the file SR to be created (we are leaving XenServer with the burden of managing race conditions). The UUID is based on the SR file path and is generated using UUID#nameUUIDFromBytes. Therefore, if there is an SR with the generated UUID, this means that some other management server has just created it. An exception will occur and it will contain a message saying 'Db_exn.Uniqueness_constraint_violation'. In these unlikely events, we catch the exception and use the method retrieveAlreadyConfiguredSrWithoutException to get the SR that has already been created for the given mount point.

rafaelweingartner · 2018-03-13T12:46:51Z

@DaanHoogland I managed to separate the formatting from the code change itself. Now it should be easier to review.

@khos2ow, @marcaurele and others, this might be of your interest as well (if you run XenServer).

DaanHoogland

lgtm

DaanHoogland · 2018-03-13T19:15:34Z

- remoteDir);
-
- if (results == null || results.isEmpty()) {
+ String result = hypervisorResource.callHostPluginAsync(conn, "cloud-plugin-storage", "mountNfsSecondaryStorage", 100 * 1000, "localDir", localDir, "remoteDir", remoteDir);


final not allowed?

Of course, they are. However, here and in a lot of places in ACS, there is an indiscriminate use of the final keyword. It does not harm, but it is something I have to read when reviewing/coding, which does not aggregate/help. So, I remove when they are not needed.

ok, short method with low cyclic complexity; makes sense

DaanHoogland · 2018-03-13T19:16:07Z


- protected boolean makeDirectory(final Connection conn, final String path) {
- final String result = hypervisorResource.callHostPlugin(conn, "cloud-plugin-storage", "makeDirectory", "path", path);
+ protected boolean makeDirectory(Connection conn, String path) {


why removing the final?

Because it is useless in this context.

DaanHoogland · 2018-03-13T19:20:13Z

@blueorangutan package

blueorangutan · 2018-03-13T19:20:23Z

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

blueorangutan · 2018-03-13T20:18:13Z

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

DaanHoogland · 2018-03-13T23:25:18Z

@blueorangutan test

blueorangutan · 2018-03-13T23:25:24Z

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

blueorangutan · 2018-03-14T04:54:48Z

Trillian test result (tid-2355)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 18301 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2414-t2355-kvm-centos7.zip
Smoke tests completed. 67 look OK, 0 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File

GabrielBrascher

Code LGTM, found just a typo in a Javadoc. Thanks for the work @rafaelweingartner !

GabrielBrascher · 2018-03-20T18:45:16Z


- final String msg = "createFileSR failed! due to the following: " + ex.toString();
+ /**
+ * This method will check if there is an already configured file SR for the given path. If by any chance we find more an one SR with the same name (mount point path) we throw a runtime exception because this situation should never happen.


If by any chance we find more an one SR with the same name -> more than one

Thanks @GabrielBrascher. Typo fixed.

GabrielBrascher

LGTM

yadvr · 2018-03-22T16:18:10Z

Thanks @rafaelweingartner since this is a bug fix, do you think it may be useful for 4.11 LTS?

rafaelweingartner · 2018-03-22T16:32:43Z

@rhtyd I would rather not. It is not a simple rebase to 4.11.
I just tried and a lot of conflicts appeared, which would require some extra work to solve.

yadvr · 2018-03-22T16:34:31Z

okay @rafaelweingartner, I'll review this soon.

yadvr · 2018-03-22T16:34:47Z

@blueorangutan test centos7 xenserver-65sp1

blueorangutan · 2018-03-22T16:35:03Z

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

rafaelweingartner · 2018-03-25T22:56:41Z

@rhtyd do you want me to wait a little longer on this one?

rafaelweingartner · 2018-03-28T15:35:25Z

@rhtyd I will wait more 24 hours before merging this one to give you some extra time in cause you have objections. Tests results are green (tests executed by @DaanHoogland 14 days ago), and the reviews are positive.

yadvr · 2018-05-04T08:14:29Z

@rafaelweingartner I scanned PRs merged on master but not 4.11 and came across this PR, since this is marked 'bug' do you think this would be useful for 4.11? If so, can you help create a backport PR for 4.11?

rafaelweingartner · 2018-05-04T10:00:45Z

I would leave it only for 4.12 due to its size. Only we seemed to have this issue so far.

rafaelweingartner force-pushed the CLOUDSTACK-10241 branch 9 times, most recently from 048a9cf to 0ffdb9aCompare January 26, 2018 15:53

rafaelweingartner self-assigned this Jan 29, 2018

rafaelweingartner added this to the 4.12 milestone Jan 29, 2018

rafaelweingartner requested review from DaanHoogland and yadvr January 29, 2018 23:56

rafaelweingartner added type:bug component:XenServer labels Jan 29, 2018

DaanHoogland reviewed Jan 30, 2018
View reviewed changes

rafaelweingartner force-pushed the CLOUDSTACK-10241 branch 2 times, most recently from dea3667 to 891a5c0Compare January 30, 2018 17:35

rafaelweingartner force-pushed the CLOUDSTACK-10241 branch from 891a5c0 to 1618cb6Compare March 13, 2018 12:45

rafaelweingartner force-pushed the CLOUDSTACK-10241 branch from 1618cb6 to a9f1f6fCompare March 13, 2018 13:13

Code formatting according to checkstyle
c7be953

rafaelweingartner force-pushed the CLOUDSTACK-10241 branch from a9f1f6f to c7be953Compare March 13, 2018 15:02

DaanHoogland reviewed Mar 13, 2018
View reviewed changes

DaanHoogland approved these changes Mar 13, 2018
View reviewed changes

rafaelweingartner requested a review from marcaurele March 14, 2018 11:28

GabrielBrascher reviewed Mar 22, 2018
View reviewed changes

fix typo
3a24daf

GabrielBrascher approved these changes Mar 22, 2018
View reviewed changes

rafaelweingartner merged commit 060715e into apache:masterMar 29, 2018

yadvr mentioned this pull request May 3, 2018
Backports for 4.11 branch #2621
Merged
12 tasks

		@@ -17,7 +17,52 @@
		package com.cloud.hypervisor.xenserver.resource;

Uh oh!

Conversation

rafaelweingartner commented Jan 19, 2018

Uh oh!

DaanHoogland left a comment

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!

rafaelweingartner commented Jan 30, 2018

Uh oh!

rafaelweingartner commented Mar 13, 2018

Uh oh!

DaanHoogland left a comment

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!

DaanHoogland commented Mar 13, 2018

Uh oh!

blueorangutan commented Mar 13, 2018

Uh oh!

blueorangutan commented Mar 13, 2018

Uh oh!

DaanHoogland commented Mar 13, 2018

Uh oh!

blueorangutan commented Mar 13, 2018

Uh oh!

blueorangutan commented Mar 14, 2018

Uh oh!

GabrielBrascher left a comment

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!

GabrielBrascher left a comment

Choose a reason for hiding this comment

Uh oh!

yadvr commented Mar 22, 2018

Uh oh!

rafaelweingartner commented Mar 22, 2018

Uh oh!

yadvr commented Mar 22, 2018

Uh oh!

yadvr commented Mar 22, 2018

Uh oh!

blueorangutan commented Mar 22, 2018

Uh oh!

rafaelweingartner commented Mar 25, 2018

Uh oh!

rafaelweingartner commented Mar 28, 2018

Uh oh!

yadvr commented May 4, 2018

Uh oh!

rafaelweingartner commented May 4, 2018

Uh oh!

Reviewers

Assignees