Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[#5919] Platform: change default storage option for AWS provider to 1x250GB #6792

Merged
merged 18 commits into from
Jan 19, 2021
Merged

[#5919] Platform: change default storage option for AWS provider to 1x250GB #6792

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
18 commits
Select commit Hold shift + click to select a range
1f79e0c
Heading:
Jan 5, 2021
bc1a948
Merge branch 'master' into ENG-5919_latest
jitendra-12113 Jan 6, 2021
2a02efb
Merge branch 'master' into ENG-5919_latest
jitendra-12113 Jan 6, 2021
8e3e7ff
Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…
Jan 6, 2021
4922831
Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…
Jan 6, 2021
bd16e96
Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…
Jan 8, 2021
d761c16
Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…
Jan 8, 2021
1a54cfa
Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…
Jan 11, 2021
8202218
Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…
Jan 11, 2021
b8bc59e
Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…
Jan 11, 2021
b98f2cc
Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…
Jan 11, 2021
fd70c2d
Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…
Jan 12, 2021
0300b33
Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…
Jan 12, 2021
ad53844
Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…
Jan 15, 2021
8093bb6
Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…
Jan 15, 2021
afac6f2
Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…
Jan 15, 2021
da5d7a2
Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…
Jan 15, 2021
f9895eb
Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…
Jan 16, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions managed/src/main/java/AppInit.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,10 +90,10 @@ public AppInit(Environment environment, Application application,
List<Provider> providerList = Provider.find.query().where().findList();
for (Provider provider : providerList) {
if (provider.code.equals("aws")) {
for (InstanceType instanceType : InstanceType.findByProvider(provider)) {
for (InstanceType instanceType : InstanceType.findByProvider(provider,
application.config())) {
if (instanceType.instanceTypeDetails != null &&
(instanceType.instanceTypeDetails.volumeDetailsList == null ||
instanceType.instanceTypeDetails.volumeDetailsList.isEmpty())) {
(instanceType.instanceTypeDetails.volumeDetailsList == null)) {
awsInitializer.initialize(provider.customerUUID, provider.uuid);
break;
}
Expand Down
2 changes: 1 addition & 1 deletion managed/src/main/java/com/yugabyte/yw/cloud/AWSInitializer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -517,7 +517,7 @@ private void storeInstanceTypeInfoToDB() {
throw new UnsupportedOperationException(msg);
} else {
// TODO: hardcode me not?
volumeCount = 2;
volumeCount = 0;
volumeSizeGB = 250;
volumeType = VolumeType.EBS;
}
Expand Down
17 changes: 12 additions & 5 deletions managed/src/main/java/com/yugabyte/yw/controllers/InstanceTypeController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

import com.fasterxml.jackson.databind.node.ObjectNode;
import com.google.inject.Inject;
import com.typesafe.config.Config;
import com.yugabyte.yw.cloud.CloudAPI;
import com.yugabyte.yw.cloud.PublicCloudConstants;
import com.yugabyte.yw.commissioner.Common;
Expand All @@ -26,12 +27,18 @@
public class InstanceTypeController extends AuthenticatedController {

public static final Logger LOG = LoggerFactory.getLogger(InstanceTypeController.class);
private final Config config;
private final FormFactory formFactory;
private final CloudAPI.Factory cloudAPIFactory;

// TODO: Remove this when we have HelperMethod in place to get Config details
@Inject
FormFactory formFactory;

@Inject
CloudAPI.Factory cloudAPIFactory;
public InstanceTypeController(Config config, FormFactory formFactory,
CloudAPI.Factory cloudAPIFactory) {
this.config = config;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do not mix ctor injections and field injections in same class.
Either fix all existing fields to be parameters here or make the new field use field injection.

FWIW, using ctor injection with private fina fields is the best practice. But i ll leave it to you which option you want for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. Thanks!

this.formFactory = formFactory;
this.cloudAPIFactory = cloudAPIFactory;
}

/**
* GET endpoint for listing instance types
Expand All @@ -48,7 +55,7 @@ public Result list(UUID customerUUID, UUID providerUUID, List<String> zoneCodes)
}
Map<String, InstanceType> instanceTypesMap;
try {
instanceTypesMap = InstanceType.findByProvider(provider).stream()
instanceTypesMap = InstanceType.findByProvider(provider, config).stream()
.collect(toMap(InstanceType::getInstanceTypeCode, identity()));
} catch (Exception e) {
LOG.error("Unable to list Instance types {}:{} in DB.", providerUUID, e.getMessage());
Expand Down
38 changes: 27 additions & 11 deletions managed/src/main/java/com/yugabyte/yw/models/InstanceType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import com.google.common.collect.ImmutableList;
import com.yugabyte.yw.cloud.PublicCloudConstants;
import com.yugabyte.yw.commissioner.Common;
import com.typesafe.config.Config;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

Expand All @@ -29,6 +30,8 @@ public class InstanceType extends Model {

public static List<String> AWS_INSTANCE_PREFIXES_SUPPORTED = ImmutableList.of(
"m3.", "c5.", "c5d.", "c4.", "c3.", "i3.");
static final String YB_AWS_DEFAULT_VOLUME_COUNT_KEY = "yb.aws.default_volume_count";
static final String YB_AWS_DEFAULT_VOLUME_SIZE_GB_KEY = "yb.aws.default_volume_size_gb";

public enum VolumeType {
@EnumValue("EBS")
Expand Down Expand Up @@ -156,8 +159,8 @@ public static void resetInstanceTypeDetailsForProvider(Common.CloudType provider
/**
* Delete Instance Types corresponding to given provider
*/
public static void deleteInstanceTypesForProvider(Provider provider) {
for (InstanceType instanceType : findByProvider(provider)) {
public static void deleteInstanceTypesForProvider(Provider provider, Config config) {
for (InstanceType instanceType : findByProvider(provider, config)) {
instanceType.delete();
}
}
Expand All @@ -166,23 +169,37 @@ private static Predicate<InstanceType> supportedInstanceTypes(List<String> suppo
return p -> supportedPrefixes.stream().anyMatch(prefix -> p.getInstanceTypeCode().startsWith(prefix));
}

private static List<InstanceType> populateDefaultsIfEmpty(List<InstanceType> entries,
Config config) {
// For AWS, we would filter and show only supported instance prefixes
entries = entries.stream()
.filter(supportedInstanceTypes(AWS_INSTANCE_PREFIXES_SUPPORTED))
.collect(Collectors.toList());
for (InstanceType instanceType : entries) {
instanceType.instanceTypeDetails =
Json.fromJson(Json.parse(instanceType.instanceTypeDetailsJson), InstanceTypeDetails.class);
if (instanceType.instanceTypeDetails.volumeDetailsList.isEmpty()) {
instanceType.instanceTypeDetails.setVolumeDetailsList(
config.getInt(YB_AWS_DEFAULT_VOLUME_COUNT_KEY),
config.getInt(YB_AWS_DEFAULT_VOLUME_SIZE_GB_KEY), VolumeType.EBS);
}
}
return entries;
}

/**
* Query Helper to find supported instance types for a given cloud provider.
*/
public static List<InstanceType> findByProvider(Provider provider) {
public static List<InstanceType> findByProvider(Provider provider, Config config) {
List<InstanceType> entries = InstanceType.find.query().where()
.eq("provider_code", provider.code)
.eq("active", true)
.findList();
if (provider.code.equals("aws")) {
// For AWS, we would filter and show only supported instance prefixes
entries = entries.stream()
.filter(supportedInstanceTypes(AWS_INSTANCE_PREFIXES_SUPPORTED))
.collect(Collectors.toList());
entries = populateDefaultsIfEmpty(entries, config);
}

return entries.stream().map(entry -> InstanceType.get(entry.getProviderCode(),
entry.getInstanceTypeCode())).collect(Collectors.toList());

return entries;
}

public static InstanceType createWithMetadata(Provider provider, String instanceTypeCode,
Expand Down Expand Up @@ -251,6 +268,5 @@ public static InstanceTypeDetails createGCPInstanceTypeDetails(VolumeType volume
volumeType);
return instanceTypeDetails;
}

}
}
9 changes: 9 additions & 0 deletions managed/src/main/resources/reference.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,15 @@ yb {
task_retention_duration = 120 days
}


aws {
# default volume count for aws instance types with EBS Only storage info
default_volume_count = 1

# default volume size for aws instance types with EBS Only storage info
default_volume_size_gb = 250
}

metrics.host="localhost"
metrics.url = "http://"${yb.metrics.host}":9090/api/v1"
storage.path="/opt/yugabyte"
Expand Down
45 changes: 38 additions & 7 deletions managed/src/test/java/com/yugabyte/yw/models/InstanceTypeTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import static org.hamcrest.CoreMatchers.notNullValue;
import static org.hamcrest.core.AllOf.allOf;
import static org.junit.Assert.*;
import static org.mockito.Mockito.when;

import java.util.ArrayList;
import java.util.List;
Expand All @@ -15,20 +16,28 @@

import com.fasterxml.jackson.databind.JsonNode;
import com.fasterxml.jackson.databind.node.ObjectNode;
import com.typesafe.config.Config;
import com.yugabyte.yw.common.ModelFactory;
import com.yugabyte.yw.models.InstanceType.VolumeType;

import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;

import com.yugabyte.yw.common.FakeDBApplication;
import play.libs.Json;

@RunWith(MockitoJUnitRunner.class)
public class InstanceTypeTest extends FakeDBApplication {
private Provider defaultProvider;
private Customer defaultCustomer;
private InstanceType.InstanceTypeDetails defaultDetails;

@Mock
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

where is a test where you have to setup this mock?
when(mockConfig.get(..._COUNT)).thenReturn(1);
when(mockConfig.get(..._GB)).thenReturn(250);

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah! Didn't realise this is for entity class. We can't mock config in this file. Most of the values are being read from in memory.
Instead used ConfigFactory.load() just to test our code. I think, this should be fine if not please suggest!
Also due to static method used in Entity class, getting difficulties in writing Unit test cases, need to refactor the code(Entity class). But for now I think this should be fine.

Thanks!

Config mockConfig;

@Before
public void setUp() {
defaultCustomer = ModelFactory.testCustomer();
Expand All @@ -48,7 +57,7 @@ public void testCreate() {
assertEquals("aws", i1.getProviderCode());
assertEquals("foo", i1.getInstanceTypeCode());
}

@Test
public void testGetNonDefaultInstanceTypeDetails() {
int volumeCount = 3;
Expand All @@ -64,7 +73,7 @@ public void testGetNonDefaultInstanceTypeDetails() {
assertEquals(String.format("/mnt/d%d", i), v.mountPath);
}
}

@Test
public void testGetDefaultInstanceTypeDetails() {
InstanceType.InstanceTypeDetails itDetails =
Expand All @@ -90,7 +99,7 @@ public void testFindByProvider() {
instanceType.setActive(false);
instanceType.save();
InstanceType.upsert(newProvider.code, "bar", 2, 10.0, defaultDetails);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is incorrect. Not sure what is your confusion.
Here is what you have to do in great detail:

  1. Revert to using mock.Config
  2. See what is in defaultDetails - line 44 populates it. But we do not want to test that code path. To excercise our code path the defaultDetails has to be empty. Thats when the populateEmpty will do something and try to lookup config values.
  3. Like line 88 upsert another instancetype such that its details have volumeList empty.
  4. Now the test will fail complaining that you have not mocked config.getInt or whatever you use.
  5. Add mock setup. For example volume count config return 3 and size returns 100GB.
  6. assert that the volume list for this instance as returned by findPyProvider is size 3 and each one is 100GB.

Hope it helps.

Copy link
Contributor Author

@jitendra-12113 jitendra-12113 Jan 16, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Had to refactor the code lil bit. Forgot to apply @RunWith(MockitoJUnitRunner.class) on top to use mock otherwise we will get null. Now its working fine. I really learnt a lot while working on this change. Please let me know if you still think, some changes are required. Thanks for all your suggestions:)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💯 for right attitude!

List<InstanceType> instanceTypeList = InstanceType.findByProvider(defaultProvider);
List<InstanceType> instanceTypeList = InstanceType.findByProvider(defaultProvider, mockConfig);
assertNotNull(instanceTypeList);
assertEquals(2, instanceTypeList.size());
Set<String> possibleTypes = new HashSet<>();
Expand All @@ -105,19 +114,41 @@ public void testFindByProvider() {
public void testFindByProviderWithUnSupportedInstances() {
InstanceType.upsert(defaultProvider.code, "t2.medium", 3, 10.0, defaultDetails);
InstanceType.upsert(defaultProvider.code, "c3.medium", 2, 10.0, defaultDetails);
List<InstanceType> instanceTypeList = InstanceType.findByProvider(defaultProvider);
List<InstanceType> instanceTypeList = InstanceType.findByProvider(defaultProvider, mockConfig);
assertNotNull(instanceTypeList);
assertEquals(1, instanceTypeList.size());
assertThat(instanceTypeList.get(0).getInstanceTypeCode(),
allOf(notNullValue(), equalTo("c3.medium")));
}


@Test
public void testFindByProviderWithEmptyInstanceTypeDetails() {
InstanceType.upsert(defaultProvider.code, "c5.medium", 3, 10.0,
new InstanceType.InstanceTypeDetails());
when(mockConfig.getInt(InstanceType.YB_AWS_DEFAULT_VOLUME_COUNT_KEY))
.thenReturn(1);
when(mockConfig.getInt(InstanceType.YB_AWS_DEFAULT_VOLUME_SIZE_GB_KEY))
.thenReturn(250);
List<InstanceType> instanceTypeList = InstanceType.findByProvider(defaultProvider, mockConfig);
assertNotNull(instanceTypeList);
InstanceType.VolumeDetails volumeDetails = instanceTypeList
.get(0)
.instanceTypeDetails
.volumeDetailsList
.get(0);
assertEquals(250, volumeDetails.volumeSizeGB.intValue());
assertEquals(InstanceType.VolumeType.EBS, volumeDetails.volumeType);
assertEquals(String.format("/mnt/d%d", 0), volumeDetails.mountPath);
assertThat(instanceTypeList.get(0).getInstanceTypeDetails().getVolumeDetailsList().size(),
allOf(notNullValue(), equalTo(1)));
}

@Test
public void testDeleteByProvider() {
Provider newProvider = ModelFactory.gcpProvider(defaultCustomer);
InstanceType.upsert(newProvider.code, "bar", 2, 10.0, defaultDetails);
InstanceType.deleteInstanceTypesForProvider(newProvider);
List<InstanceType> instanceTypeList = InstanceType.findByProvider(newProvider);
InstanceType.deleteInstanceTypesForProvider(newProvider, mockConfig);
List<InstanceType> instanceTypeList = InstanceType.findByProvider(newProvider, mockConfig);
assertEquals(0, instanceTypeList.size());
}

Expand Down