From 6bb3a264c517da064a3587333e4ea472dea808e5 Mon Sep 17 00:00:00 2001 From: Dipesh Patel Date: Mon, 8 Jul 2013 15:46:53 +0100 Subject: [PATCH 1/8] Introduced a SoleInstance cluster and VPC checks - Introduced a cluster which contains all the instances in a region - Added a Rule to check if instances are in the Virtual Private Cloud --- .../conformity/crawler/AWSClusterCrawler.java | 32 +++++++--- .../aws/conformity/rule/InstanceInVPC.java | 61 +++++++++++++++++++ .../simianarmy/conformity/Cluster.java | 24 ++++++++ 3 files changed, 108 insertions(+), 9 deletions(-) create mode 100644 src/main/java/com/netflix/simianarmy/aws/conformity/rule/InstanceInVPC.java diff --git a/src/main/java/com/netflix/simianarmy/aws/conformity/crawler/AWSClusterCrawler.java b/src/main/java/com/netflix/simianarmy/aws/conformity/crawler/AWSClusterCrawler.java index 92281c09..32450982 100644 --- a/src/main/java/com/netflix/simianarmy/aws/conformity/crawler/AWSClusterCrawler.java +++ b/src/main/java/com/netflix/simianarmy/aws/conformity/crawler/AWSClusterCrawler.java @@ -94,21 +94,35 @@ public List clusters(String... clusterNames) { } } Cluster cluster = new Cluster(asg.getAutoScalingGroupName(), region, conformityAsg); + updateCluster(cluster); list.add(cluster); - updateExcludedConformityRules(cluster); - cluster.setOwnerEmail(getOwnerEmailForCluster(cluster)); - String prop = String.format("simianarmy.conformity.cluster.%s.optedOut", cluster.getName()); - if (cfg.getBoolOrElse(prop, false)) { - LOGGER.info(String.format("Cluster %s is opted out of Conformity Monkey.", cluster.getName())); - cluster.setOptOutOfConformity(true); - } else { - cluster.setOptOutOfConformity(false); - } } + //Cluster containing all instances + List instances = Lists.newArrayList(); + for (com.amazonaws.services.ec2.model.Instance awsInstance : awsClient.describeInstances()) { + //May need to exclude those in an asg but not a straightforward api for it. + LOGGER.info(String.format("Found instance %s.", awsInstance.getInstanceId())); + instances.add(awsInstance.getInstanceId()); + } + Cluster cluster = new Cluster("SoleInstances", region, instances); + updateCluster(cluster); + list.add(cluster); } return list; } + private void updateCluster(Cluster cluster) { + updateExcludedConformityRules(cluster); + cluster.setOwnerEmail(getOwnerEmailForCluster(cluster)); + String prop = String.format("simianarmy.conformity.cluster.%s.optedOut", cluster.getName()); + if (cfg.getBoolOrElse(prop, false)) { + LOGGER.info(String.format("Cluster %s is opted out of Conformity Monkey.", cluster.getName())); + cluster.setOptOutOfConformity(true); + } else { + cluster.setOptOutOfConformity(false); + } + } + /** * Gets the owner email from the monkey configuration. * @param cluster diff --git a/src/main/java/com/netflix/simianarmy/aws/conformity/rule/InstanceInVPC.java b/src/main/java/com/netflix/simianarmy/aws/conformity/rule/InstanceInVPC.java new file mode 100644 index 00000000..097e7d18 --- /dev/null +++ b/src/main/java/com/netflix/simianarmy/aws/conformity/rule/InstanceInVPC.java @@ -0,0 +1,61 @@ +package com.netflix.simianarmy.aws.conformity.rule; + +import com.google.common.collect.Lists; +import com.netflix.simianarmy.client.aws.AWSClient; +import com.netflix.simianarmy.conformity.AutoScalingGroup; +import com.netflix.simianarmy.conformity.Cluster; +import com.netflix.simianarmy.conformity.Conformity; +import com.netflix.simianarmy.conformity.ConformityRule; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + +import java.util.Collection; + +/** + * The class implements a conformity rule to check an instance is in a virtual private cloud. + */ +public class InstanceInVPC implements ConformityRule { + + private static final Logger LOGGER = LoggerFactory.getLogger(InstanceInVPC.class); + + private static final String RULE_NAME = "InstanceInVPC"; + private static final String REASON = "VPC_ID not defined"; + + @Override + public Conformity check(Cluster cluster) { + AWSClient awsClient = new AWSClient(cluster.getRegion()); + Collection failedComponents = Lists.newArrayList(); + //check all instances + checkInstancesInVPC(cluster.getSoleInstances(), awsClient, failedComponents); + //check asg instances ( there will be some overlap) + for (AutoScalingGroup asg : cluster.getAutoScalingGroups()) { + if (asg.isSuspended()) { + continue; + } + checkInstancesInVPC(asg.getInstances(), awsClient, failedComponents); + } + return new Conformity(getName(), failedComponents); + } + + private void checkInstancesInVPC(Collection instances, AWSClient awsClient, + Collection failedComponents) { + for (String instanceID : instances) { + for (com.amazonaws.services.ec2.model.Instance awsInstance : awsClient.describeInstances(instanceID)) { + if (awsInstance.getVpcId() == null) { + LOGGER.info(String.format("Instance %s is not in a virtual private cloud", instanceID)); + failedComponents.add(instanceID); + } + } + } + } + + @Override + public String getName() { + return RULE_NAME; + } + + @Override + public String getNonconformingReason() { + return REASON; + } +} \ No newline at end of file diff --git a/src/main/java/com/netflix/simianarmy/conformity/Cluster.java b/src/main/java/com/netflix/simianarmy/conformity/Cluster.java index e823ca1e..47a4603b 100644 --- a/src/main/java/com/netflix/simianarmy/conformity/Cluster.java +++ b/src/main/java/com/netflix/simianarmy/conformity/Cluster.java @@ -55,6 +55,7 @@ public class Cluster { private final Collection excludedConformityRules = Sets.newHashSet(); private boolean isConforming; private boolean isOptOutOfConformity; + private Collection soleInstances = Lists.newArrayList(); /** * Constructor. @@ -74,6 +75,24 @@ public Cluster(String name, String region, AutoScalingGroup... autoScalingGroups } } + /** + * Constructor. + * @param name + * the name of the cluster + * @param soleInstances + * the list of all instances + */ + public Cluster(String name, String region, List soleInstances) { + Validate.notNull(name); + Validate.notNull(region); + Validate.notNull(autoScalingGroups); + this.name = name; + this.region = region; + for (String soleInstance : soleInstances) { + this.soleInstances.add(soleInstance); + } + } + /** * Gets the name of the cluster. * @return @@ -294,4 +313,9 @@ private static void putToMapIfNotNull(Map map, String key, Strin map.put(key, value); } } + + public Collection getSoleInstances() { + return Collections.unmodifiableCollection(soleInstances); + } + } From 0c128c00784330bc6555f109129e7329ba47e042 Mon Sep 17 00:00:00 2001 From: Dipesh Patel Date: Mon, 8 Jul 2013 17:11:50 +0100 Subject: [PATCH 2/8] Added VPC check into the SimianArmy Context. --- .../basic/conformity/BasicConformityMonkeyContext.java | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/main/java/com/netflix/simianarmy/basic/conformity/BasicConformityMonkeyContext.java b/src/main/java/com/netflix/simianarmy/basic/conformity/BasicConformityMonkeyContext.java index 27e73281..7628e6c1 100644 --- a/src/main/java/com/netflix/simianarmy/basic/conformity/BasicConformityMonkeyContext.java +++ b/src/main/java/com/netflix/simianarmy/basic/conformity/BasicConformityMonkeyContext.java @@ -29,6 +29,7 @@ import com.netflix.simianarmy.aws.conformity.rule.InstanceHasHealthCheckUrl; import com.netflix.simianarmy.aws.conformity.rule.InstanceHasStatusUrl; import com.netflix.simianarmy.aws.conformity.rule.InstanceInSecurityGroup; +import com.netflix.simianarmy.aws.conformity.rule.InstanceInVPC; import com.netflix.simianarmy.aws.conformity.rule.InstanceIsHealthyInEureka; import com.netflix.simianarmy.aws.conformity.rule.InstanceTooOld; import com.netflix.simianarmy.aws.conformity.rule.SameZonesInElbAndAsg; @@ -143,6 +144,11 @@ public BasicConformityMonkeyContext() { ruleEngine().addRule(new SameZonesInElbAndAsg()); } + if (configuration().getBoolOrElse( + "simianarmy.conformity.rule.InstanceInVPC.enabled", false)) { + ruleEngine.addRule(new InstanceInVPC()); + } + regionToAwsClient.put(region(), new AWSClient(region())); clusterCrawler = new AWSClusterCrawler(regionToAwsClient, configuration()); sesClient = new AmazonSimpleEmailServiceClient(); From fe463f6004d51242cd2478c1f7c6cd3043e5f8f1 Mon Sep 17 00:00:00 2001 From: Dipesh Patel Date: Mon, 8 Jul 2013 19:53:14 +0100 Subject: [PATCH 3/8] Exclude instances already in an asg cluster from SoloInstances cluster. --- .../conformity/crawler/AWSClusterCrawler.java | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/main/java/com/netflix/simianarmy/aws/conformity/crawler/AWSClusterCrawler.java b/src/main/java/com/netflix/simianarmy/aws/conformity/crawler/AWSClusterCrawler.java index 32450982..af3cb395 100644 --- a/src/main/java/com/netflix/simianarmy/aws/conformity/crawler/AWSClusterCrawler.java +++ b/src/main/java/com/netflix/simianarmy/aws/conformity/crawler/AWSClusterCrawler.java @@ -22,6 +22,7 @@ import com.amazonaws.services.autoscaling.model.SuspendedProcess; import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import com.google.common.collect.Sets; import com.netflix.simianarmy.MonkeyConfiguration; import com.netflix.simianarmy.client.aws.AWSClient; import com.netflix.simianarmy.conformity.Cluster; @@ -31,6 +32,7 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; +import java.util.HashSet; import java.util.List; import java.util.Map; @@ -75,11 +77,13 @@ public List clusters(String... clusterNames) { for (Map.Entry entry : regionToAwsClient.entrySet()) { String region = entry.getKey(); AWSClient awsClient = entry.getValue(); + HashSet asgInstances = Sets.newHashSet(); LOGGER.info(String.format("Crawling clusters in region %s", region)); for (AutoScalingGroup asg : awsClient.describeAutoScalingGroups(clusterNames)) { List instances = Lists.newArrayList(); for (Instance instance : asg.getInstances()) { instances.add(instance.getInstanceId()); + asgInstances.add(instance.getInstanceId()); } com.netflix.simianarmy.conformity.AutoScalingGroup conformityAsg = new com.netflix.simianarmy.conformity.AutoScalingGroup( @@ -97,14 +101,17 @@ public List clusters(String... clusterNames) { updateCluster(cluster); list.add(cluster); } - //Cluster containing all instances + //Cluster containing all solo instances List instances = Lists.newArrayList(); for (com.amazonaws.services.ec2.model.Instance awsInstance : awsClient.describeInstances()) { - //May need to exclude those in an asg but not a straightforward api for it. - LOGGER.info(String.format("Found instance %s.", awsInstance.getInstanceId())); - instances.add(awsInstance.getInstanceId()); + LOGGER.debug(String.format("Found instance %s.", awsInstance.getInstanceId())); + if (!asgInstances.contains(awsInstance.getInstanceId())) { + LOGGER.info(String.format("Adding instance %s to soloInstances cluster.", + awsInstance.getInstanceId())); + instances.add(awsInstance.getInstanceId()); + } } - Cluster cluster = new Cluster("SoleInstances", region, instances); + Cluster cluster = new Cluster("SoloInstances", region, instances); updateCluster(cluster); list.add(cluster); } From a8052f0748fe19d353382b020f6febd87ceb82c3 Mon Sep 17 00:00:00 2001 From: Dipesh Patel Date: Mon, 8 Jul 2013 23:00:32 +0100 Subject: [PATCH 4/8] Added test for InstanceInVPC. Refactored code to make test easier to run. --- .../aws/conformity/rule/InstanceInVPC.java | 56 ++++++++++----- .../simianarmy/conformity/Cluster.java | 14 ++-- .../conformity/rule/TestInstanceInVPC.java | 70 +++++++++++++++++++ 3 files changed, 117 insertions(+), 23 deletions(-) create mode 100644 src/test/java/com/netflix/simianarmy/aws/conformity/rule/TestInstanceInVPC.java diff --git a/src/main/java/com/netflix/simianarmy/aws/conformity/rule/InstanceInVPC.java b/src/main/java/com/netflix/simianarmy/aws/conformity/rule/InstanceInVPC.java index 097e7d18..76a6eb30 100644 --- a/src/main/java/com/netflix/simianarmy/aws/conformity/rule/InstanceInVPC.java +++ b/src/main/java/com/netflix/simianarmy/aws/conformity/rule/InstanceInVPC.java @@ -1,6 +1,8 @@ package com.netflix.simianarmy.aws.conformity.rule; +import com.amazonaws.services.ec2.model.Instance; import com.google.common.collect.Lists; +import com.google.common.collect.Maps; import com.netflix.simianarmy.client.aws.AWSClient; import com.netflix.simianarmy.conformity.AutoScalingGroup; import com.netflix.simianarmy.conformity.Cluster; @@ -10,6 +12,8 @@ import org.slf4j.LoggerFactory; import java.util.Collection; +import java.util.List; +import java.util.Map; /** * The class implements a conformity rule to check an instance is in a virtual private cloud. @@ -18,37 +22,25 @@ public class InstanceInVPC implements ConformityRule { private static final Logger LOGGER = LoggerFactory.getLogger(InstanceInVPC.class); + private final Map regionToAwsClient = Maps.newHashMap(); private static final String RULE_NAME = "InstanceInVPC"; private static final String REASON = "VPC_ID not defined"; @Override public Conformity check(Cluster cluster) { - AWSClient awsClient = new AWSClient(cluster.getRegion()); Collection failedComponents = Lists.newArrayList(); //check all instances - checkInstancesInVPC(cluster.getSoleInstances(), awsClient, failedComponents); - //check asg instances ( there will be some overlap) + checkInstancesInVPC(cluster.getRegion(), cluster.getSoloInstances(), failedComponents); + //check asg instances for (AutoScalingGroup asg : cluster.getAutoScalingGroups()) { if (asg.isSuspended()) { continue; } - checkInstancesInVPC(asg.getInstances(), awsClient, failedComponents); + checkInstancesInVPC(cluster.getRegion(), asg.getInstances(), failedComponents); } return new Conformity(getName(), failedComponents); } - private void checkInstancesInVPC(Collection instances, AWSClient awsClient, - Collection failedComponents) { - for (String instanceID : instances) { - for (com.amazonaws.services.ec2.model.Instance awsInstance : awsClient.describeInstances(instanceID)) { - if (awsInstance.getVpcId() == null) { - LOGGER.info(String.format("Instance %s is not in a virtual private cloud", instanceID)); - failedComponents.add(instanceID); - } - } - } - } - @Override public String getName() { return RULE_NAME; @@ -58,4 +50,36 @@ public String getName() { public String getNonconformingReason() { return REASON; } + + private AWSClient getAwsClient(String region) { + AWSClient awsClient = regionToAwsClient.get(region); + if (awsClient == null) { + awsClient = new AWSClient(region); + regionToAwsClient.put(region, awsClient); + } + return awsClient; + } + + private void checkInstancesInVPC(String region, Collection instances, + Collection failedComponents) { + for (String instanceId : instances) { + for (Instance awsInstance : getAWSInstances(region, instanceId)) { + if (awsInstance.getVpcId() == null) { + LOGGER.info(String.format("Instance %s is not in a virtual private cloud", instanceId)); + failedComponents.add(instanceId); + } + } + } + } + + /** + * Gets the list of AWS instances. Can be overridden + * @param region the region + * @param instanceId the instance id. + * @return the list of the AWS instances with the given id. + */ + protected List getAWSInstances(String region, String instanceId) { + AWSClient awsClient = getAwsClient(region); + return awsClient.describeInstances(instanceId); + } } \ No newline at end of file diff --git a/src/main/java/com/netflix/simianarmy/conformity/Cluster.java b/src/main/java/com/netflix/simianarmy/conformity/Cluster.java index 47a4603b..e0afe89a 100644 --- a/src/main/java/com/netflix/simianarmy/conformity/Cluster.java +++ b/src/main/java/com/netflix/simianarmy/conformity/Cluster.java @@ -55,7 +55,7 @@ public class Cluster { private final Collection excludedConformityRules = Sets.newHashSet(); private boolean isConforming; private boolean isOptOutOfConformity; - private Collection soleInstances = Lists.newArrayList(); + private Collection soloInstances = Lists.newArrayList(); /** * Constructor. @@ -79,17 +79,17 @@ public Cluster(String name, String region, AutoScalingGroup... autoScalingGroups * Constructor. * @param name * the name of the cluster - * @param soleInstances + * @param soloInstances * the list of all instances */ - public Cluster(String name, String region, List soleInstances) { + public Cluster(String name, String region, List soloInstances) { Validate.notNull(name); Validate.notNull(region); Validate.notNull(autoScalingGroups); this.name = name; this.region = region; - for (String soleInstance : soleInstances) { - this.soleInstances.add(soleInstance); + for (String soleInstance : soloInstances) { + this.soloInstances.add(soleInstance); } } @@ -314,8 +314,8 @@ private static void putToMapIfNotNull(Map map, String key, Strin } } - public Collection getSoleInstances() { - return Collections.unmodifiableCollection(soleInstances); + public Collection getSoloInstances() { + return Collections.unmodifiableCollection(soloInstances); } } diff --git a/src/test/java/com/netflix/simianarmy/aws/conformity/rule/TestInstanceInVPC.java b/src/test/java/com/netflix/simianarmy/aws/conformity/rule/TestInstanceInVPC.java new file mode 100644 index 00000000..99141f12 --- /dev/null +++ b/src/test/java/com/netflix/simianarmy/aws/conformity/rule/TestInstanceInVPC.java @@ -0,0 +1,70 @@ +// CHECKSTYLE IGNORE Javadoc +/* +* +* Copyright 2013 Netflix, Inc. +* +* Licensed under the Apache License, Version 2.0 (the "License"); +* you may not use this file except in compliance with the License. +* You may obtain a copy of the License at +* +* http://www.apache.org/licenses/LICENSE-2.0 +* +* Unless required by applicable law or agreed to in writing, software +* distributed under the License is distributed on an "AS IS" BASIS, +* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +* See the License for the specific language governing permissions and +* limitations under the License. +* +*/ +package com.netflix.simianarmy.aws.conformity.rule; + +import com.amazonaws.services.ec2.model.Instance; +import com.google.common.collect.Lists; +import com.netflix.simianarmy.conformity.Cluster; +import com.netflix.simianarmy.conformity.Conformity; +import junit.framework.Assert; +import org.mockito.MockitoAnnotations; +import org.mockito.Spy; +import org.testng.annotations.BeforeMethod; +import org.testng.annotations.Test; + +import java.util.List; + +import static org.mockito.Mockito.doReturn; + + +public class TestInstanceInVPC { + private static final String VPC_INSTANCE_ID = "abc-123"; + private static final String INSTANCE_ID = "zxy-098"; + private static final String REGION = "eu-west-1"; + + @Spy + private InstanceInVPC instanceInVPC = new InstanceInVPC(); + + @BeforeMethod + public void setUp() throws Exception { + MockitoAnnotations.initMocks(this); + List instanceList = Lists.newArrayList(); + Instance instance = new Instance().withInstanceId(VPC_INSTANCE_ID).withVpcId("12345"); + instanceList.add(instance); + doReturn(instanceList).when(instanceInVPC).getAWSInstances(REGION, VPC_INSTANCE_ID); + List instanceList2 = Lists.newArrayList(); + Instance instance2 = new Instance().withInstanceId(INSTANCE_ID); + instanceList2.add(instance2); + doReturn(instanceList2).when(instanceInVPC).getAWSInstances(REGION, INSTANCE_ID); + + } + + @Test + public void testCheckSoloInstances() throws Exception { + List list = Lists.newArrayList(); + list.add(VPC_INSTANCE_ID); + list.add(INSTANCE_ID); + Cluster cluster = new Cluster("SoloInstances", REGION, list); + Conformity result = instanceInVPC.check(cluster); + Assert.assertNotNull(result); + Assert.assertEquals(result.getRuleId(), instanceInVPC.getName()); + Assert.assertEquals(result.getFailedComponents().size(), 1); + Assert.assertEquals(result.getFailedComponents().iterator().next(), INSTANCE_ID); + } +} From 67f4dbf91ab891f06a20a606863067022a6a2559 Mon Sep 17 00:00:00 2001 From: Dipesh Patel Date: Tue, 9 Jul 2013 11:12:25 +0100 Subject: [PATCH 5/8] Added test for Asg clusters. --- .../conformity/rule/TestInstanceInVPC.java | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/src/test/java/com/netflix/simianarmy/aws/conformity/rule/TestInstanceInVPC.java b/src/test/java/com/netflix/simianarmy/aws/conformity/rule/TestInstanceInVPC.java index 99141f12..129b9b66 100644 --- a/src/test/java/com/netflix/simianarmy/aws/conformity/rule/TestInstanceInVPC.java +++ b/src/test/java/com/netflix/simianarmy/aws/conformity/rule/TestInstanceInVPC.java @@ -20,6 +20,7 @@ import com.amazonaws.services.ec2.model.Instance; import com.google.common.collect.Lists; +import com.netflix.simianarmy.conformity.AutoScalingGroup; import com.netflix.simianarmy.conformity.Cluster; import com.netflix.simianarmy.conformity.Conformity; import junit.framework.Assert; @@ -67,4 +68,22 @@ public void testCheckSoloInstances() throws Exception { Assert.assertEquals(result.getFailedComponents().size(), 1); Assert.assertEquals(result.getFailedComponents().iterator().next(), INSTANCE_ID); } + + @Test + public void testAsgInstances() throws Exception { + AutoScalingGroup autoScalingGroup = new AutoScalingGroup("Conforming", VPC_INSTANCE_ID); + Cluster conformingCluster = new Cluster("Conforming", REGION, autoScalingGroup); + Conformity result = instanceInVPC.check(conformingCluster); + Assert.assertNotNull(result); + Assert.assertEquals(result.getRuleId(), instanceInVPC.getName()); + Assert.assertEquals(result.getFailedComponents().size(), 0); + + autoScalingGroup = new AutoScalingGroup("NonConforming", INSTANCE_ID); + Cluster nonConformingCluster = new Cluster("NonConforming", REGION, autoScalingGroup); + result = instanceInVPC.check(nonConformingCluster); + Assert.assertNotNull(result); + Assert.assertEquals(result.getRuleId(), instanceInVPC.getName()); + Assert.assertEquals(result.getFailedComponents().size(), 1); + Assert.assertEquals(result.getFailedComponents().iterator().next(), INSTANCE_ID); + } } From 4923ed0cbe6bfaa2112b62964fa67533a12503fa Mon Sep 17 00:00:00 2001 From: Dipesh Patel Date: Tue, 9 Jul 2013 16:20:31 +0100 Subject: [PATCH 6/8] Only create solo instances cluster if we have solo instances. --- .../aws/conformity/crawler/AWSClusterCrawler.java | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/netflix/simianarmy/aws/conformity/crawler/AWSClusterCrawler.java b/src/main/java/com/netflix/simianarmy/aws/conformity/crawler/AWSClusterCrawler.java index af3cb395..cdbdbac5 100644 --- a/src/main/java/com/netflix/simianarmy/aws/conformity/crawler/AWSClusterCrawler.java +++ b/src/main/java/com/netflix/simianarmy/aws/conformity/crawler/AWSClusterCrawler.java @@ -104,16 +104,18 @@ public List clusters(String... clusterNames) { //Cluster containing all solo instances List instances = Lists.newArrayList(); for (com.amazonaws.services.ec2.model.Instance awsInstance : awsClient.describeInstances()) { - LOGGER.debug(String.format("Found instance %s.", awsInstance.getInstanceId())); if (!asgInstances.contains(awsInstance.getInstanceId())) { LOGGER.info(String.format("Adding instance %s to soloInstances cluster.", awsInstance.getInstanceId())); instances.add(awsInstance.getInstanceId()); } } - Cluster cluster = new Cluster("SoloInstances", region, instances); - updateCluster(cluster); - list.add(cluster); + //Only create cluster if we have solo instances. + if (!instances.isEmpty()) { + Cluster cluster = new Cluster("SoloInstances", region, instances); + updateCluster(cluster); + list.add(cluster); + } } return list; } From 22910812f63e565cd4379223ed20dbebee6d2c55 Mon Sep 17 00:00:00 2001 From: Dipesh Patel Date: Wed, 10 Jul 2013 12:00:24 +0100 Subject: [PATCH 7/8] Change from List to Set hence being more specific about the nature of the instances collection. --- .../aws/conformity/crawler/AWSClusterCrawler.java | 6 +++--- .../aws/conformity/rule/InstanceInVPC.java | 15 ++++++++++----- .../netflix/simianarmy/conformity/Cluster.java | 9 +++++---- .../aws/conformity/rule/TestInstanceInVPC.java | 4 +++- 4 files changed, 21 insertions(+), 13 deletions(-) diff --git a/src/main/java/com/netflix/simianarmy/aws/conformity/crawler/AWSClusterCrawler.java b/src/main/java/com/netflix/simianarmy/aws/conformity/crawler/AWSClusterCrawler.java index cdbdbac5..73343516 100644 --- a/src/main/java/com/netflix/simianarmy/aws/conformity/crawler/AWSClusterCrawler.java +++ b/src/main/java/com/netflix/simianarmy/aws/conformity/crawler/AWSClusterCrawler.java @@ -32,9 +32,9 @@ import org.slf4j.Logger; import org.slf4j.LoggerFactory; -import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; /** * The class implementing a crawler that gets the auto scaling groups from AWS. @@ -77,7 +77,7 @@ public List clusters(String... clusterNames) { for (Map.Entry entry : regionToAwsClient.entrySet()) { String region = entry.getKey(); AWSClient awsClient = entry.getValue(); - HashSet asgInstances = Sets.newHashSet(); + Set asgInstances = Sets.newHashSet(); LOGGER.info(String.format("Crawling clusters in region %s", region)); for (AutoScalingGroup asg : awsClient.describeAutoScalingGroups(clusterNames)) { List instances = Lists.newArrayList(); @@ -102,7 +102,7 @@ public List clusters(String... clusterNames) { list.add(cluster); } //Cluster containing all solo instances - List instances = Lists.newArrayList(); + Set instances = Sets.newHashSet(); for (com.amazonaws.services.ec2.model.Instance awsInstance : awsClient.describeInstances()) { if (!asgInstances.contains(awsInstance.getInstanceId())) { LOGGER.info(String.format("Adding instance %s to soloInstances cluster.", diff --git a/src/main/java/com/netflix/simianarmy/aws/conformity/rule/InstanceInVPC.java b/src/main/java/com/netflix/simianarmy/aws/conformity/rule/InstanceInVPC.java index 76a6eb30..8d45b895 100644 --- a/src/main/java/com/netflix/simianarmy/aws/conformity/rule/InstanceInVPC.java +++ b/src/main/java/com/netflix/simianarmy/aws/conformity/rule/InstanceInVPC.java @@ -3,6 +3,7 @@ import com.amazonaws.services.ec2.model.Instance; import com.google.common.collect.Lists; import com.google.common.collect.Maps; +import com.google.common.collect.Sets; import com.netflix.simianarmy.client.aws.AWSClient; import com.netflix.simianarmy.conformity.AutoScalingGroup; import com.netflix.simianarmy.conformity.Cluster; @@ -14,6 +15,7 @@ import java.util.Collection; import java.util.List; import java.util.Map; +import java.util.Set; /** * The class implements a conformity rule to check an instance is in a virtual private cloud. @@ -30,13 +32,15 @@ public class InstanceInVPC implements ConformityRule { public Conformity check(Cluster cluster) { Collection failedComponents = Lists.newArrayList(); //check all instances - checkInstancesInVPC(cluster.getRegion(), cluster.getSoloInstances(), failedComponents); + Set failedInstances = checkInstancesInVPC(cluster.getRegion(), cluster.getSoloInstances()); + failedComponents.addAll(failedInstances); //check asg instances for (AutoScalingGroup asg : cluster.getAutoScalingGroups()) { if (asg.isSuspended()) { continue; } - checkInstancesInVPC(cluster.getRegion(), asg.getInstances(), failedComponents); + Set asgFailedInstances = checkInstancesInVPC(cluster.getRegion(), asg.getInstances()); + failedComponents.addAll(asgFailedInstances); } return new Conformity(getName(), failedComponents); } @@ -60,16 +64,17 @@ private AWSClient getAwsClient(String region) { return awsClient; } - private void checkInstancesInVPC(String region, Collection instances, - Collection failedComponents) { + private Set checkInstancesInVPC(String region, Collection instances) { + Set failedInstances = Sets.newHashSet(); for (String instanceId : instances) { for (Instance awsInstance : getAWSInstances(region, instanceId)) { if (awsInstance.getVpcId() == null) { LOGGER.info(String.format("Instance %s is not in a virtual private cloud", instanceId)); - failedComponents.add(instanceId); + failedInstances.add(instanceId); } } } + return failedInstances; } /** diff --git a/src/main/java/com/netflix/simianarmy/conformity/Cluster.java b/src/main/java/com/netflix/simianarmy/conformity/Cluster.java index e0afe89a..2974a118 100644 --- a/src/main/java/com/netflix/simianarmy/conformity/Cluster.java +++ b/src/main/java/com/netflix/simianarmy/conformity/Cluster.java @@ -30,6 +30,7 @@ import java.util.Date; import java.util.List; import java.util.Map; +import java.util.Set; /** * The class implementing clusters. Cluster is the basic unit of conformity check. It can be a single ASG or @@ -55,7 +56,7 @@ public class Cluster { private final Collection excludedConformityRules = Sets.newHashSet(); private boolean isConforming; private boolean isOptOutOfConformity; - private Collection soloInstances = Lists.newArrayList(); + private final Set soloInstances = Sets.newHashSet(); /** * Constructor. @@ -82,7 +83,7 @@ public Cluster(String name, String region, AutoScalingGroup... autoScalingGroups * @param soloInstances * the list of all instances */ - public Cluster(String name, String region, List soloInstances) { + public Cluster(String name, String region, Set soloInstances) { Validate.notNull(name); Validate.notNull(region); Validate.notNull(autoScalingGroups); @@ -314,8 +315,8 @@ private static void putToMapIfNotNull(Map map, String key, Strin } } - public Collection getSoloInstances() { - return Collections.unmodifiableCollection(soloInstances); + public Set getSoloInstances() { + return Collections.unmodifiableSet(soloInstances); } } diff --git a/src/test/java/com/netflix/simianarmy/aws/conformity/rule/TestInstanceInVPC.java b/src/test/java/com/netflix/simianarmy/aws/conformity/rule/TestInstanceInVPC.java index 129b9b66..c9a5e528 100644 --- a/src/test/java/com/netflix/simianarmy/aws/conformity/rule/TestInstanceInVPC.java +++ b/src/test/java/com/netflix/simianarmy/aws/conformity/rule/TestInstanceInVPC.java @@ -20,6 +20,7 @@ import com.amazonaws.services.ec2.model.Instance; import com.google.common.collect.Lists; +import com.google.common.collect.Sets; import com.netflix.simianarmy.conformity.AutoScalingGroup; import com.netflix.simianarmy.conformity.Cluster; import com.netflix.simianarmy.conformity.Conformity; @@ -30,6 +31,7 @@ import org.testng.annotations.Test; import java.util.List; +import java.util.Set; import static org.mockito.Mockito.doReturn; @@ -58,7 +60,7 @@ public void setUp() throws Exception { @Test public void testCheckSoloInstances() throws Exception { - List list = Lists.newArrayList(); + Set list = Sets.newHashSet(); list.add(VPC_INSTANCE_ID); list.add(INSTANCE_ID); Cluster cluster = new Cluster("SoloInstances", REGION, list); From c811f8c1dc0a5ebffeedf5ec334c3da3d4cb870f Mon Sep 17 00:00:00 2001 From: codiverse Date: Thu, 18 Jul 2013 11:01:21 +0100 Subject: [PATCH 8/8] Fix typo in Constructor to correctly validate soloInstances - in response to pull request comment --- src/main/java/com/netflix/simianarmy/conformity/Cluster.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/com/netflix/simianarmy/conformity/Cluster.java b/src/main/java/com/netflix/simianarmy/conformity/Cluster.java index 2974a118..ba709e5e 100644 --- a/src/main/java/com/netflix/simianarmy/conformity/Cluster.java +++ b/src/main/java/com/netflix/simianarmy/conformity/Cluster.java @@ -86,7 +86,7 @@ public Cluster(String name, String region, AutoScalingGroup... autoScalingGroups public Cluster(String name, String region, Set soloInstances) { Validate.notNull(name); Validate.notNull(region); - Validate.notNull(autoScalingGroups); + Validate.notNull(soloInstances); this.name = name; this.region = region; for (String soleInstance : soloInstances) {