configdrive: run unit test only for Linux

[yadvr]

This runs a platform dependent test only on Linux, otherwise skips.

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)

GitHub Issue/PRs

Screenshots (if appropriate):

How Has This Been Tested?

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.

[DaanHoogland]

this should not be a unit test on linux either. mkisofs is available for macos as well. I don't like the trend this is setting. Let's find another solution.

[wido]

I am with this on @DaanHoogland

This could only lead to Mac developers thinking everything is just fine and they merge a PR with broken code.

[DaanHoogland]

jenkins is failing with this as well now (on #2664):
testConfigDriveBuild(org.apache.cloudstack.storage.configdrive.ConfigDriveBuilderTest) Time elapsed: 0.206 sec <<< ERROR!
com.cloud.utils.exception.CloudRuntimeException: Unable to create iso file: i-x-y.iso due to java.io.IOException: Cannot run program "/usr/bin/genisoimage": error=2, No such file or directory
at java.lang.ProcessBuilder.start(ProcessBuilder.java:1048)
at com.cloud.utils.script.Script.execute(Script.java:215)
at com.cloud.utils.script.Script.execute(Script.java:183)

[yadvr]

@wido @DaanHoogland I've removed the integration like unit test. @borisstoyanov can you help create a different PR that fixes the configdrive in future?

[borisstoyanov]

I'm not sure if I can for unit tests @rhtyd, if you're referring to the component tests maybe we can write them from scratch since they are bond with some nuage utilities and test data, which we don't get with normal marvin installation

[yadvr]

@borisstoyanov I was referring to the marvin based integration test (I'm okay if it's a new one or fixing the existing one).

[borisstoyanov]

Okay @rhtyd

[DaanHoogland]

either jenkins didn't restart or it is still failing on the lack of /usr/bin/genisoimage, whuich seems strange.

[DaanHoogland]

testing locally

[DaanHoogland]

by the looks of it testAddPasswordAndUserData() has now become an integration type test, consequentially.

[yadvr]

@DaanHoogland @wido are you both okay with removal of test and submission of proper integration test in future? I've tried to rekick Jenkins using close+open of the PR but it does do not look like it worked (but Travis got re-kicked).

[DaanHoogland]

@rhtyd as a consequence of the original change testAddPasswordAndUserData() is now an integration test as well. Can we change that to an integration test as well?

[wido]

Well, I would like to see this tested ofcourse, but I'm ok with removal for now.

[yadvr]

@DaanHoogland I see, I can do some mocking tomorrow on static methods that should get rid of the integration side of things.

[yadvr]

Since this only affects developers (non_linux envs) I've moved it to 4.11.2.0 milestone. I'll get back on this soon (but if I've time, will focus on 4.11.1.0 related issues first).

[DaanHoogland]

jenkins fails because of this so I'd say this is high urgency, even when not a blocker.

[rafaelweingartner]

Folks I am almost finished with unit test for ConfigDriveBuilder class. It is taking a little longer than I expected because of static methods mocking.

[yadvr]

Okay @rafaelweingartner, feel free to push your changes to this PR. Thanks.

[yadvr]

@DaanHoogland we don't need to remove this unit test, instead just mock the call that addPasswordAndUserdata makes to the ConfigDriveBuilder static method which is tricky but do-able.

[DaanHoogland]

whatever solution is fine by me. for now I want to see jenkins pass. We can revert when we have a better solution.

[DaanHoogland]

@fmaximus there is a nuage specific config drive test suite. Can you verify that these tstill pass? And do those all need to be nuage specific? Maybe we can generify the config drives ones to run as generic regression tests.

[DaanHoogland]

as for running on macos, i think the following might do:

diff --git a/engine/storage/configdrive/src/org/apache/cloudstack/storage/configdrive/ConfigDriveBuilder.java b/engine/storage/configdrive/src/org/apache/cloudstack/storage/configdrive/ConfigDriveBuilder.java
index d847aa1d1d..b975e12154 100644
--- a/engine/storage/configdrive/src/org/apache/cloudstack/storage/configdrive/ConfigDriveBuilder.java
+++ b/engine/storage/configdrive/src/org/apache/cloudstack/storage/configdrive/ConfigDriveBuilder.java
@@ -136,7 +136,18 @@ public class ConfigDriveBuilder {
             }

             File tmpIsoStore = new File(tempDirName, new File(isoFileName).getName());
-            Script command = new Script("/usr/bin/genisoimage", Duration.standardSeconds(300), LOG);
+            File isoCreator = new File("/usr/bin/genisoimage");
+            if (!isoCreator.exists()) {
+                isoCreator = new File("/usr/local/bin/mkisofs"); // are these all the paths we search?
+                if(!isoCreator.exists()) {
+                    throw new CloudRuntimeException("cannot create iso for config drive using any know tool.");
+                }
+            }
+            if(!isoCreator.canExecute()) {
+                throw new CloudRuntimeException("cannot create iso for config drive using " + isoCreator.getCanonicalPath());
+            }
+
+            Script command = new Script(isoCreator.getCanonicalPath(), Duration.standardSeconds(300), LOG);
             command.add("-o", tmpIsoStore.getAbsolutePath());
             command.add("-ldots");
             command.add("-allow-lowercase");

I'll test and add to this PR (or separately if we feel it needs to be.

[DaanHoogland]

As for @rhtyd 's comment above, after jenkins passes I will revert the removal of the test and extract line 350 of ConfigDriveNetworkElement,

        final String isoData = ConfigDriveBuilder.buildConfigDrive(profile.getVmData(), ConfigDrive.CONFIGDRIVEFILENAME, profile.getConfigDriveLabel());

, into a separate method to be mocked. That hopefully will do the trick.

now involved in the code, other reviewers wanted

[rafaelweingartner]

@DaanHoogland I am almost finished with the complete set of unit tests for the static methods as you originally designed this code. Could you just wait a bit more? Therefore, there would be no need to do the changes you are doing here that are intended to facilitate your mocking.

I know writing unit tests for static method is a pain, that is why I started doing yesterday, but I was not able to finish yesterday.

[fmaximus]

@DaanHoogland @borisstoyanov The test in the component directory is a test which can run without any need for Nuage. It started as a copy of the Nuage specific test in plugins/nuagevsp, and recently I have moved the common code to a separate class in the component test, so I can reuse it from the nuage test. PR #2673 has some more fixes for issues I encountered when trying to run it.

[yadvr]

Closing this as #2674 addresses the issues.

[yadvr]

@DaanHoogland I just saw you had made some additional changes on top of my PR, can you move/check those changes in Rafael's PR in #2674 .

[DaanHoogland]

check

[DaanHoogland]

https://github.com/apache/cloudstack/pull/2674/files#diff-6e8787853d95373fe884e1d08021d57cR167