Create unit test cases for 'ConfigDriveBuilder' class by rafaelweingartner · Pull Request #2674 · apache/cloudstack

rafaelweingartner · 2018-05-24T11:25:38Z

Description

@rhtyd, @DaanHoogland, @wido, and @mike-tutkowski here is the unit tests to remove the dependency of "genisoimage" when building ACS in other operating systems (running maven).

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)

Issues and PRs fixed by this PR

This solves #2671
Also solves #2665
Fixes #2677

Checklist:

I have read the CONTRIBUTING document.
My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
Testing
I have added tests to cover my changes.
All relevant new and existing integration tests have passed.
A full integration testsuite with all test that can run on my environment has passed.

yadvr · 2018-05-24T11:35:42Z

...ne/storage/configdrive/src/org/apache/cloudstack/storage/configdrive/ConfigDriveBuilder.java

+    public static String fileToBase64String(File isoFile) throws IOException {
        byte[] encoded = Base64.encodeBase64(FileUtils.readFileToByteArray(isoFile));
-        return new String(encoded, StandardCharsets.US_ASCII);
+        return new String(encoded, com.cloud.utils.StringUtils.getPreferredCharset());


@rafaelweingartner for base64 encoding should n't we keep it to US_ASCII? Bear in mind that this encoded string is passed over network to agents.

That is a good question. I noticed that in some places we were using US_ASCII and others the com.cloud.utils.StringUtils.getPreferredCharset(). I asked @DaanHoogland about it.
I can revert this change though. What do you guys think?

I think let's stick with US_ASCII for both encoding and decoding?

yeah, i made the same comment

ok, so I will change that

yadvr · 2018-05-24T11:36:51Z

...ne/storage/configdrive/src/org/apache/cloudstack/storage/configdrive/ConfigDriveBuilder.java

+     */
+    static String generateAndRetrieveIsoAsBase64Iso(String isoFileName, String driveLabel, String tempDirName) throws IOException {
+        File tmpIsoStore = new File(tempDirName, isoFileName);
+        Script command = new Script("/usr/bin/genisoimage", Duration.standardSeconds(300), LOG);


Can you include Daan's change (or @DaanHoogland can you push your change?) around using genisoimage or mkisofs if either is available?

Is it in some other PR? I can take a look and include it here.

@rafaelweingartner yes, see Daan's comment here: #2665 (comment)

yadvr · 2018-05-24T11:37:41Z

Thanks @rafaelweingartner - I dislike however getting those final removed :)
@blueorangutan package

blueorangutan · 2018-05-24T11:37:53Z

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

rafaelweingartner · 2018-05-24T11:39:39Z

ah the final keyword. I am quite picky with things that do not seem to add value to the code.
I can add them back though.

yadvr · 2018-05-24T11:41:43Z

@rafaelweingartner I prefer keeping objects open to assignments/change only if necessary, so I keep adding final by default. I'm okay with your change, but generally prefer people adopting final often.

rafaelweingartner · 2018-05-24T11:46:00Z

That is interesting. I am used to do the opposite. I only restrict something, when I want this behavior. Otherwise, I leave them open.

blueorangutan · 2018-05-24T12:09:39Z

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

DaanHoogland · 2018-05-24T11:33:12Z

...ne/storage/configdrive/src/org/apache/cloudstack/storage/configdrive/ConfigDriveBuilder.java

+    public static String fileToBase64String(File isoFile) throws IOException {
        byte[] encoded = Base64.encodeBase64(FileUtils.readFileToByteArray(isoFile));
-        return new String(encoded, StandardCharsets.US_ASCII);
+        return new String(encoded, com.cloud.utils.StringUtils.getPreferredCharset());


the default charset might be different on the MS and the host. something that we might want to force in a test.

I must change org.apache.cloudstack.storage.configdrive.ConfigDriveBuilder.base64StringToFile(String, String, String) as well, right?

these must be symetrical and will be running on different host, so yes

This is another method that is causing Jenkins to fail for almost a month

rafaelweingartner · 2018-05-24T14:49:52Z

@khos2ow I am also fixing that MockServerTest.testIsMockServerCanUpgradeConnectionToSsl().

rafaelweingartner · 2018-05-24T16:25:00Z

@rhtyd and @DaanHoogland and finally Jenkins is Green!

rafaelweingartner · 2018-05-24T18:41:25Z

@blueorangutan package

blueorangutan · 2018-05-24T18:41:53Z

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

blueorangutan · 2018-05-24T19:13:44Z

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

GabrielBrascher

I could only raise a minor issue in the documentation.
All tests have passed and code LGTM.

GabrielBrascher · 2018-05-25T00:18:07Z

engine/storage/configdrive/src/org/apache/cloudstack/storage/configdrive/ConfigDrive.java

    /**
-     * This is the path to iso file relative to mount point
-     * @return config drive iso file path
+     * Created the path to ISO file relative to mount point.


What do you think of Creates the path to ... instead of Created?
Change formatt to format

Thanks. Fixed!

yadvr · 2018-05-25T06:03:55Z

@blueorangutan test

blueorangutan · 2018-05-25T06:04:54Z

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

DaanHoogland · 2018-05-25T09:33:26Z

jenkins is still red but I don't see this as config drive related. We should remove the infamous timeout test.

rafaelweingartner · 2018-05-25T10:29:35Z

Ahaha. Everything was green until i fixed a typo :(
Let me force a new build

rafaelweingartner · 2018-05-25T16:49:48Z

People, all green again!

blueorangutan · 2018-05-25T19:58:25Z

Trillian test result (tid-2717)
Environment: kvm-centos7 (x2), Advanced Networking with Mgmt server 7
Total time taken: 23960 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr2674-t2717-kvm-centos7.zip
Intermitten failure detected: /marvin/tests/smoke/test_deploy_virtio_scsi_vm.py
Intermitten failure detected: /marvin/tests/smoke/test_privategw_acl.py
Intermitten failure detected: /marvin/tests/smoke/test_ssvm.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_redundant.py
Intermitten failure detected: /marvin/tests/smoke/test_vpc_vpn.py
Smoke tests completed. 64 look OK, 3 have error(s)
Only failed tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestDeployVirtioSCSIVM>:setup	`Error`	0.00	test_deploy_virtio_scsi_vm.py
test_03_vpc_privategw_restart_vpc_cleanup	`Error`	117.12	test_privategw_acl.py
test_04_rvpc_network_garbage_collector_nics	`Failure`	268.69	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Failure`	318.98	test_vpc_redundant.py
test_05_rvpc_multi_tiers	`Error`	340.90	test_vpc_redundant.py

DaanHoogland · 2018-05-29T08:47:12Z

I will discuss with @PaulAngus before merging any more things in 4.11 while we have an RC out.

yadvr · 2018-06-04T07:53:58Z

Let's merge this @PaulAngus @DaanHoogland ?

rafaelweingartner added this to the 4.11.1.0 milestone May 24, 2018

Create unit test cases for 'ConfigDriveBuilder' class

0f3d324

rafaelweingartner force-pushed the createUnitTestCasesForConfigDriveBuilder branch from c3e73c4 to 0f3d324 Compare May 24, 2018 11:29

yadvr reviewed May 24, 2018

View reviewed changes

add method 'getProgramToGenerateIso' as suggested by rohit and Daan

28a3002

DaanHoogland reviewed May 24, 2018

View reviewed changes

fix encoding for base64 to StandardCharsets.US_ASCII

49923a6

rafaelweingartner self-assigned this May 24, 2018

rafaelweingartner added the type:enhancement label May 24, 2018

fix MockServerTest.testIsMockServerCanUpgradeConnectionToSsl()

69961dc

This is another method that is causing Jenkins to fail for almost a month

GabrielBrascher approved these changes May 25, 2018

View reviewed changes

yadvr approved these changes May 25, 2018

View reviewed changes

rafaelweingartner force-pushed the createUnitTestCasesForConfigDriveBuilder branch from 305b78e to ad3504a Compare May 25, 2018 10:33

fix typos

c92d1ba

rafaelweingartner force-pushed the createUnitTestCasesForConfigDriveBuilder branch from ad3504a to c92d1ba Compare May 25, 2018 14:23

rafaelweingartner mentioned this pull request May 26, 2018

package.sh -d centos7 ERROR #2677

Closed

DaanHoogland added status:testing status:ready-for-merge and removed type:enhancement labels May 29, 2018

yadvr mentioned this pull request Jun 4, 2018

configdrive: run unit test only for Linux #2665

Closed

12 tasks

DaanHoogland merged commit 9b83337 into apache:4.11 Jun 4, 2018

DaanHoogland removed the status:ready-for-merge label Dec 4, 2024

Conversation

rafaelweingartner commented May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Types of changes

Issues and PRs fixed by this PR

Checklist:

Uh oh!

yadvr May 24, 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 May 24, 2018

Uh oh!

blueorangutan commented May 24, 2018

Uh oh!

rafaelweingartner commented May 24, 2018

Uh oh!

yadvr commented May 24, 2018

Uh oh!

rafaelweingartner commented May 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

blueorangutan commented May 24, 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!

rafaelweingartner commented May 24, 2018

Uh oh!

rafaelweingartner commented May 24, 2018

Uh oh!

rafaelweingartner commented May 24, 2018

Uh oh!

blueorangutan commented May 24, 2018

Uh oh!

blueorangutan commented May 24, 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!

yadvr commented May 25, 2018

Uh oh!

blueorangutan commented May 25, 2018

Uh oh!

DaanHoogland commented May 25, 2018

Uh oh!

rafaelweingartner commented May 25, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rafaelweingartner commented May 25, 2018

Uh oh!

blueorangutan commented May 25, 2018

Uh oh!

DaanHoogland commented May 29, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rafaelweingartner commented May 24, 2018 •

edited

Loading

yadvr May 24, 2018 •

edited

Loading

rafaelweingartner commented May 24, 2018 •

edited

Loading

rafaelweingartner commented May 25, 2018 •

edited

Loading

DaanHoogland commented May 29, 2018 •

edited

Loading