Evaluate Relationships #430

mklopp · 2018-01-22T02:32:28Z

Implement:

CheckModel
Prepare
VisitRelationships

codecov · 2018-01-22T02:44:24Z

Codecov Report

Merging #430 into master will increase coverage by 1.19%.
The diff coverage is 80.85%.

@@             Coverage Diff             @@
##             master    #430      +/-   ##
===========================================
+ Coverage        69%   70.2%   +1.19%     
  Complexity      114     114              
===========================================
  Files           299     304       +5     
  Lines          6602    6756     +154     
  Branches        452     458       +6     
===========================================
+ Hits           4556    4743     +187     
+ Misses         1859    1813      -46     
- Partials        187     200      +13

Impacted Files	Coverage Δ	Complexity Δ
...a/plugins/cloudformation/CloudFormationPlugin.java	`80.95% <0%> (+7.03%)`	`0 <0> (ø)`	⬇️
...gins/cloudformation/CloudFormationFileCreator.java	`85.52% <100%> (-6.58%)`	`0 <0> (ø)`
...lugins/cloudformation/mapper/CapabilityMapper.java	`56.47% <61.53%> (+30%)`	`0 <0> (ø)`	⬇️
.../cloudformation/visitor/CheckModelNodeVisitor.java	`66.66% <66.66%> (ø)`	`0 <0> (?)`
...loudformation/visitor/PrepareModelNodeVisitor.java	`72.22% <72.22%> (ø)`	`0 <0> (?)`
...rmation/visitor/CheckModelRelationshipVisitor.java	`75% <75%> (ø)`	`0 <0> (?)`
...mation/visitor/CloudFormationVisitorExtension.java	`80.76% <80.76%> (ø)`	`0 <0> (?)`
...lugins/cloudformation/CloudFormationLifecycle.java	`85.71% <81.25%> (+12.38%)`	`0 <0> (ø)`	⬇️
...udformation/visitor/TransformModelNodeVisitor.java	`83.8% <83.8%> (ø)`	`0 <0> (?)`
...ation/visitor/PrepareModelRelationshipVisitor.java	`90.9% <90.9%> (ø)`	`0 <0> (?)`
... and 21 more

c-mueller

Just some Points i want to adress

c-mueller · 2018-01-22T12:05:29Z

.../src/integration/java/org/opentosca/toscana/plugins/cloudformation/CloudFormationLampIT.java

@@ -31,7 +33,10 @@ protected void onSuccess(File outputDir) {

    @Override
    protected void onFailure(File outputDir, Exception e) {
-        fail();
+        //if the cause for transformation failure is amazonEC2Exception its fine because of the credentials 
+        if (!(e instanceof TransformationFailureException && e.getCause() instanceof AmazonEC2Exception)) {


Please consider using assumptions to prevnt a test from beeing executed if the credentials are not set.

you can use the check assumptions method in this test to do this.

c-mueller · 2018-01-22T12:06:23Z

server/src/main/java/org/opentosca/toscana/plugins/cloudformation/CloudFormationLifecycle.java

+            throw tfe;
+        } catch (Exception e) {
+            logger.error("Transformation to CloudFormation unsuccessful. Random Exception should not appear here.");
+            throw new TransformationFailureException("Random exception", e);


I dont like this wording. Unexpected error sonds much better.

Noting is really random here

c-mueller · 2018-01-22T12:11:35Z

server/src/main/java/org/opentosca/toscana/plugins/cloudformation/CloudFormationLifecycle.java

            logger.error("Transformation to CloudFormation unsuccessful. Please check the StackTrace for more Info.");
-            e.printStackTrace();
+            tfe.printStackTrace();


there is no need to print the stacktrace, because the ExecutionTask (the part of the core that handles the excution of transformation)

c-mueller · 2018-01-22T12:14:49Z

...ain/java/org/opentosca/toscana/plugins/cloudformation/visitor/CloudFormationNodeVisitor.java

+            }
+        } catch (Exception e) {
+            logger.error("Error while creating WebApplication");
+            throw new TransformationFailureException("Failed at WebApplication node", e);


Maybe condider adding the name of the node

… fulfiller of a capability

… the privateAddress of the compute to reference the database. We can do the same if the mysql is the only node hosted on a compute

…o-TOSCAna/TOSCAna into feature/aws/evaluate-relationships

hnicke · 2018-01-29T21:00:10Z

...rc/main/java/org/opentosca/toscana/plugins/cloudformation/visitor/CheckModelNodeVisitor.java

+
+    @Override
+    public void visit(MysqlDatabase node) {
+        logger.info("Check MysqlDatabase node {}.", node.getEntityName());


I adopted the habit to always quote variables when logging.

logger.info("Check MysqlDatabase node '{}'"),

This way you can detect null variables when reading the log:

INFO Check MysqlDatabase node ''

But taste varies.

hnicke · 2018-01-29T21:11:13Z

.../main/java/org/opentosca/toscana/plugins/cloudformation/visitor/PrepareModelNodeVisitor.java

+        // check if Mysql is the only node hosted on his compute node
+        Compute compute = getCompute(node);
+        if (topology.incomingEdgesOf(compute).size() == 1) {
+            // means our dbms is the only one hosted on this compute


This is incorrect. The graph's edges are a Set of all possible Relationships. HostedOn is only one of them.
A quick fix could be:

if ( topology.incomingEdgesOf(compute).stream().filter(p -> p instanceof HostedOn).collect(Collectors.toSet()).size() == 1)

Didn't try that code, though.

Actually I'm pretty sure this is correct. Sure it is a Set of all possible relationships but I already know that the mysqlDatabase node I am currently visiting is hosted on this specific compute (or mysqlDatabase is hosted on mysqlDbms is hosted on this compute node).
That means if the compute node has exactly one relationship it is that hosted on relation.

Imagine the tosca modeller has written a template that defines a timing dependency (DependsOn) from node 'otherNode' to 'server'.
Now,

if (topology.incomingEdgesOf(compute).size() == 1) {

..will evaluate to false, altough mysql dbms node is still the only node hosted on the server node.

hnicke · 2018-01-29T21:16:36Z

...va/org/opentosca/toscana/plugins/cloudformation/visitor/PrepareModelRelationshipVisitor.java

+            Compute computeWebApplication = getCompute(webApplication);
+            if (computeMysqlDatabase.equals(computeWebApplication)) {
+                // means we can set the private address as reference the database endpoint
+                //TODO only set privateAddress or also publicAddress?


Why not set the public address as well? Go for it.

hnicke · 2018-01-29T21:18:04Z

...va/org/opentosca/toscana/plugins/cloudformation/visitor/PrepareModelRelationshipVisitor.java

+            if (computeMysqlDatabase.equals(computeWebApplication)) {
+                // means we can set the private address as reference the database endpoint
+                //TODO only set privateAddress or also publicAddress?
+                computeMysqlDatabase.setPrivateAddress(Fn.fnGetAtt(toAlphanumerical(mysqlDatabase.getEntityName()),


Why is the private address set in both Visitor classes?

I try to cover two possibilities here.

PrepareModelNodeVisitor.java sets the private address of the compute node to the database endpoint exactly when the database is the only node hosted on the compute. That means this compute node completely belongs to the database and represents it (by providing the endpoint)

PrepareModelRelationshipVisitor.java is used in the case of our current lamp-model where both the webapplication and the database are hosted on one compute. I only feel safe to set the private address of a compute node to reference the database if there is a connects to relationship between the two nodes sitting on top of the compute node.
Here I see the problem tackled in the previous meeting. What if someone references the webapplication as well by calling the private address of compute node it is hosted on (which is valid). He would get the database endpoint instead.

I just had following alternative idea:

when handling the dbms node;

store the initial value of compute#privateAddress

apply the AWS Fn fix (compute.setPrivateAddress(Fn....))

write all mysql dbms related configuration to aws template file

restore value of compute#privateAddress

Could this work?

I will look into this

I don't think this will work. I actually need the privateAddress to be set to the database endpoint when I visit the webApplication node and not the dbms/database node.
I think the current solution is the best there is and will work for most cases.

stale

hnicke · 2018-01-31T15:59:35Z

...rc/main/java/org/opentosca/toscana/plugins/cloudformation/visitor/CheckModelNodeVisitor.java

+    @Override
+    public void visit(Compute node) {
+        logger.info("Check Compute node '{}'.", node.getEntityName());
+        List<OsCapability.Type> supportedTypes = new ArrayList<>();


There's a utility class that could help for better readibility here:

List<OsCapability.Type> supportedTypes = Lists.newArrayList(OsCapability.Type.LINUX);

hnicke · 2018-01-31T16:04:28Z

.../main/java/org/opentosca/toscana/plugins/cloudformation/visitor/PrepareModelNodeVisitor.java

+    }
+
+    @Override
+    public void visit(MysqlDatabase node) {


Maybe refactor this, as its a quite big method.

hnicke · 2018-01-31T16:10:23Z

server/src/main/java/org/opentosca/toscana/plugins/cloudformation/CloudFormationLifecycle.java

+            CheckModelNodeVisitor checkModelNodeVisitor = new CheckModelNodeVisitor(context);
+            logger.debug("Check nodes");
+            for (VisitableNode node : nodes) {
+                node.accept(checkModelNodeVisitor);


In the Visitor classes you always log which node you are visiting when entering the method. Why don't you just write it here once for all methods?

hnicke · 2018-01-31T16:10:33Z

server/src/main/java/org/opentosca/toscana/plugins/cloudformation/CloudFormationLifecycle.java

+            CheckModelRelationshipVisitor checkModelRelationshipVisitor = new CheckModelRelationshipVisitor(context);
+            logger.debug("Check relationships");
+            for (VisitableRelationship relationship : relationships) {
+                relationship.accept(checkModelRelationshipVisitor);


Same for relationships.

hnicke · 2018-01-31T16:12:35Z

...va/org/opentosca/toscana/plugins/cloudformation/visitor/PrepareModelRelationshipVisitor.java

+            // if they are hosted on the same compute --> we can set the compute private address to
+            Compute computeMysqlDatabase = getCompute(mysqlDatabase);
+            Compute computeWebApplication = getCompute(webApplication);
+            if (computeMysqlDatabase.equals(computeWebApplication)) {


What happens if that is not the case?

hnicke · 2018-01-31T16:13:15Z

...va/org/opentosca/toscana/plugins/cloudformation/visitor/PrepareModelRelationshipVisitor.java

+        logger.info("Prepare ConnectsTo relation '{}'.", relation.getEntityName());
+        RootNode source = topology.getEdgeSource(relation);
+        RootNode target = topology.getEdgeTarget(relation);
+        if (source instanceof WebApplication && target instanceof MysqlDatabase) {


What happens if that is not the case? Maybe log that you drop the relationship.

hnicke · 2018-01-31T16:15:20Z

.../src/test/java/org/opentosca/toscana/plugins/cloudformation/CloudFormationLifecycleTest.java

+        PluginFileAccess accessL = new PluginFileAccess(
+            new File(tmpdir, "sourceDir"),
+            new File(tmpdir, "targetDir"),
+            mock(Log.class));


you could simply use logMock() instead of mock(Log.class). The calls to getLogger() are already stubbed.

mmunozba

Some questions, some suggestions.

mmunozba · 2018-01-31T15:49:32Z

server/src/main/java/org/opentosca/toscana/plugins/cloudformation/CloudFormationModule.java

@@ -75,6 +83,7 @@ public CloudFormationModule(PluginFileAccess fileAccess, String awsRegion, AWSCr
            (KEYNAME_CONSTRAINT_DESCRIPTION);
        keyNameVar = template.ref(KEYNAME);
        cfnInitMap = new HashMap<>();
+        computeToEc2 = new HashSet<>();


Could you explain the purpose of this set?

We don't want to transform every compute node to an ec2 instance. For example if an database is the only node hosted on an compute we dont want to transform it.

mmunozba · 2018-01-31T15:57:16Z

...rc/main/java/org/opentosca/toscana/plugins/cloudformation/visitor/CheckModelNodeVisitor.java

+        //might grow but for now only linux
+        List<OsCapability.Distribution> supportedDistributions = new ArrayList<>();
+        supportedDistributions.add(OsCapability.Distribution.UBUNTU);
+        //might grow, but for now only ubuntu, maybe already work with others but not yet tested


Added an issue for this: #461.

mmunozba · 2018-01-31T16:02:44Z

...rc/main/java/org/opentosca/toscana/plugins/cloudformation/visitor/CheckModelNodeVisitor.java

+        logger.info("Check Compute node '{}'.", node.getEntityName());
+        List<OsCapability.Type> supportedTypes = new ArrayList<>();
+        supportedTypes.add(OsCapability.Type.LINUX);
+        //might grow but for now only linux


Added an issue for this: #462.

mmunozba · 2018-01-31T16:06:54Z

...ain/java/org/opentosca/toscana/plugins/cloudformation/visitor/CloudFormationNodeVisitor.java

+     @param webApplication the webApplication to find the host for
+     @return the underlying Compute node
+     */
+    protected static Compute getCompute(WebApplication webApplication) {


I feel like CloudFormationNodeVisitor is getting pretty big.
Maybe we should extract some of the utility methods like these getCompute methods?
We might need these for other future visitors, so it might be a good idea to extract them regardless of the size of this class.

I extracted these into a parent class

mmunozba · 2018-01-31T16:12:44Z

...ain/java/org/opentosca/toscana/plugins/cloudformation/visitor/CloudFormationNodeVisitor.java

-                .imageId(imageId)
-                .instanceType(instanceType);
+            logger.info("Visit Compute node '{}'.", node.getEntityName());
+            if (cfnModule.checkComputeToEc2(node)) {


Maybe I'm misunderstanding this, but doesn't this mean that a Compute node only gets transformed if an Apache is hosted on it? Since addComputeToEc2 only gets called in the visit() method of Apache in PrepareModelNodeVisitor.

Yeah this is wrong, I changed it

mmunozba · 2018-01-31T16:18:43Z

...ain/java/org/opentosca/toscana/plugins/cloudformation/visitor/CloudFormationNodeVisitor.java

@@ -296,4 +338,18 @@ private String getFileURL(String bucketName, String objectKey) {
    public CapabilityMapper createCapabilityMapper() {
        return new CapabilityMapper(cfnModule.getAWSRegion(), cfnModule.getAwsCredentials(), logger);
    }
+
+    private Set<Compute> getHostsOfConnectedTo(RootNode node) {


Maybe extract this method? See above.

mmunozba · 2018-01-31T16:21:11Z

.../main/java/org/opentosca/toscana/plugins/cloudformation/visitor/PrepareModelNodeVisitor.java

+            //password needs to be at least 8 characters long
+            String password = node.getPassword().get();
+            if (password.length() < minPWLength) {
+                logger.warn("Database password to short, creating new random password");


hnicke · 2018-01-31T16:33:38Z

server/src/test/resources/csars/yaml/valid/lamp-noinput/my_app/myphpapp.php

@@ -10,6 +10,19 @@
 }


+/**


Please also rezip the corresponding csar (run csar-make)

stale

mklopp added 2 commits January 22, 2018 02:08

Throw TransformationFailureExceptions

18b5025

Implement checkModel and handle more lifecycle operations of nodes

b7cd981

mklopp added the plugin/cloudformation label Jan 22, 2018

mklopp self-assigned this Jan 22, 2018

c-mueller previously requested changes Jan 22, 2018

View reviewed changes

mklopp added 17 commits January 22, 2018 13:54

Implement checkModel visitors and start prepare Visitors

74b7e97

Assume instead of expected failure

26027ba

Instead of fulfillers, use the new method getNode() for getting the…

8cb77b9

… fulfiller of a capability

Check the database's values and fill mandatory ones

7cb3ee0

Merge branch 'master' into feature/aws/evaluate-relationships

cfc25ee

If we find a connectsTo and nodes are on the same compute, we can set…

57ae2e1

… the privateAddress of the compute to reference the database. We can do the same if the mysql is the only node hosted on a compute

Merge branch 'feature/aws/evaluate-relationships' of github.com:StuPr…

4e749b9

…o-TOSCAna/TOSCAna into feature/aws/evaluate-relationships

Merge branch 'master' into feature/aws/evaluate-relationships

6706034

Fix merge

bb6b85d

Add properties from environment on test

a591c19

Enabling transform test again

c30f454

Fix test

170ccf1

Change file path artifects get dropped on ec2 machine

24bfb63

Add securityGroup ingress from connectsTo relationships

ad37980

Merge branch 'master' into feature/aws/evaluate-relationships

0c06a8a

Extract method

52e3855

Update cloudformation version

06930e0

mklopp requested a review from mmunozba January 29, 2018 14:36

mklopp changed the title ~~[WIP] Evaluate Relationships~~ Evaluate Relationships Jan 29, 2018

hnicke previously requested changes Jan 29, 2018

View reviewed changes

hnicke changed the title ~~Evaluate Relationships~~ [WIP] Evaluate Relationships Jan 29, 2018

mklopp added 3 commits January 30, 2018 12:11

Refactor logging for better readablity

4dd972b

Fix check for only hostedOn relation + also set public address

c098204

Merge branch 'master' into feature/aws/evaluate-relationships

d7c98c7

mklopp requested review from hnicke and mmunozba and removed request for mmunozba January 31, 2018 15:24

hnicke suggested changes Jan 31, 2018

View reviewed changes

hnicke changed the title ~~Evaluate Relationships~~ [WIP] Evaluate Relationships Jan 31, 2018

mmunozba previously requested changes Jan 31, 2018

View reviewed changes

hnicke previously requested changes Jan 31, 2018

View reviewed changes

mmunozba and others added 12 commits January 31, 2018 17:35

Merge branch 'master' into feature/aws/evaluate-relationships

3bfd21a

Get the right representation for referencing with the Fn class

188702b

Merge branch 'master' into feature/aws/evaluate-relationships

8f365da

Merge branch 'master' into feature/aws/evaluate-relationships

6e68982

Fix merge

04a98bb

Fix test

c20722e

Refactor lifecycle and logging

8c6bf46

Rename transform node visitor

08aeb18

Refactor: extract an abstract supermethod + pull up methodes

2661265

Adapt new changes

85b8522

Log if private address is not set

6c6c56d

Merge branch 'master' into feature/aws/evaluate-relationships

476767f

mklopp requested review from hnicke and mmunozba February 2, 2018 13:27

mmunozba approved these changes Feb 2, 2018

View reviewed changes

mklopp changed the title ~~[WIP] Evaluate Relationships~~ Evaluate Relationships Feb 2, 2018

Merge branch 'master' into feature/aws/evaluate-relationships

155fe31

mklopp merged commit 84d91f7 into master Feb 2, 2018

mklopp deleted the feature/aws/evaluate-relationships branch February 2, 2018 14:57

Evaluate Relationships #430

Evaluate Relationships #430

Conversation

mklopp commented Jan 22, 2018 • edited Loading

codecov bot commented Jan 22, 2018 • edited Loading

Codecov Report

c-mueller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mmunozba left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mklopp commented Jan 22, 2018 •

edited

Loading

codecov bot commented Jan 22, 2018 •

edited

Loading