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

jitendra-12113 · 2021-01-05T20:08:47Z

Heading:
[#5919] Platform: change default storage option for AWS provider to 1x250GB
Description:
The empty volume list will be stored in the db in that case we will be using default value(volume_count, volume_size configured in refernce.conf.
Testing:
When we select aws provider while creating universe, volume count was showing more than one for some instances types starting with ""c5./c4.". Now with this fix it will show only one.
Will have to reset instanceTypeDetailsJson to empty for older YW
instances.
Added UT's as well. sbt test run results:

[info] Test com.yugabyte.yw.commissioner.TaskGarbageCollectorTest.testPurge_noneStale started
[info] Test com.yugabyte.yw.commissioner.TaskGarbageCollectorTest.testIgnorePromError started
[info] Test com.yugabyte.yw.commissioner.TaskGarbageCollectorTest.testPurge_invalidData started
[info] Test run finished: 0 failed, 0 ignored, 7 total, 0.589s
[info] Passed: Total 1092, Failed 0, Errors 0, Passed 1088, Skipped 4
[success] Total time: 575 s, completed 18 Jan, 2021 12:33:44 PM
hasher@LAP-LIN-908:~/yugabyte-db/managed$

[#5919] Platform: change default storage option for AWS provider to 1x250GB Description: We wanted to show default volume count to 1 on the UI for all the instance types starting with "c5./c4." Testing: When we select aws provider while creating universe, volume count was showing more than one for some instances types starting with ""c5./c4.". Now with this fix it will show only one. Will have to reset instanceTypeDetailsJson to empty for older YW instances.

CLAassistant · 2021-01-05T20:08:51Z

Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ jitendra-12113
❌ Jitendra Kumar

Jitendra Kumar seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
_{You have signed the CLA already but the status is still pending? Let us recheck it.}

sb-yb · 2021-01-05T22:21:09Z

managed/src/main/java/com/yugabyte/yw/models/InstanceType.java

@@ -44,6 +49,13 @@
    NVME
  }

+  @Inject
+  public InstanceType(ConfigFactory config) {


Config not ConfigFactory
Like I said use TaskGarbageCollector as example.

With Config, always getting NLP(Null pointers), hence used ConfigFactory instead of.

Thats odd. In tests or when running server? May be some ordering issue during binding.
Inject
private final Provider<Config> config
@Inject
InstanceType(Provider config) {
...
}`

then where you use it do

config.get()

This delays the resolution to runtime.

Let me know if that fixes the problem. If not will patch and see whats wrong.

The config factory is not an option here. That code is buggy.
Another way is to get to typesafe config through play but lets try this first.

Its happening while testing the code at runtime.
Getting below error:
Caused by: com.google.inject.CreationException: Unable to create injector, see the following errors:

Error injecting constructor, java.lang.NullPointerException
at AppInit.(AppInit.java:47)
at Module.configure(Module.java:48) (via modules: com.google.inject.util.Modules$OverrideModule -> Module)
while locating AppInit

Tried above things as well but that didn't work out.
Updated a patch, using config only. Please suggest! Thanks!

So I saw a provider for Config in Module. But looks like that is a different Config type not the typesafe.Config.
So it is not going to work this way. Provider trick also does not delay the resolution because we need it during AppInit.

So way to do this gracefully is

Inject Application in InstanceType

Then you can get to typesafe config using app.config() (NOT app.configuration() which is deprecated)
this.config = app.config()

Let me know if that helps.

Just noticed that InstanceType is a Entity bean. So it wont use guice injection. It is using the default ctor (that you added) followed by the field mapping by the ORM and thus config remains null.
So you can just pass the config value into findByProvider as parameter. No config field.

Sorry for confusion. Right way to do this is not use so much static factory methods. Usually there is a singleton helper class like InstanceTypeHelper. That will have non-static findByProvider method. then you can actually do proper guice injection in such class.

Done. Added this for now, later we will refactor the code once we have Helper method in place as you suggested above. Thanks!

sb-yb · 2021-01-05T22:21:28Z

managed/src/main/java/com/yugabyte/yw/models/InstanceType.java

@@ -29,6 +31,9 @@

  public static List<String> AWS_INSTANCE_PREFIXES_SUPPORTED = ImmutableList.of(
    "m3.", "c5.", "c5d.", "c4.", "c3.", "i3.");
+  static final String YB_VOLUME_INFO_VOLUME_COUNT = "yb.volumeInfo.volume_count";
+  static final String YB_VOLUME_INFO_VOLUME_SIZE_GB = "yb.volumeInfo.volume_size_gb";
+  private static ConfigFactory config;


why static?

changed config var to non-static, others kept as it is, since those are being used in static block.

yep. my comment was only about config.

sb-yb · 2021-01-05T22:22:09Z

managed/src/main/java/com/yugabyte/yw/models/InstanceType.java

+      for (InstanceType instanceType : entries) {
+        if (instanceType.instanceTypeDetailsJson.isEmpty()) {
+          instanceType.instanceTypeDetails.setVolumeDetailsList(
+            config.load().getInt(YB_VOLUME_INFO_VOLUME_COUNT),


Inject config. Then no need to load.

If this is the only config value then pass a parameter in to this method awsDefaultVolumeCount
Do config lookup in caller.
I will do a refactor later which should fix this DI fail as described above - using singleton helper classes.

sb-yb · 2021-01-05T22:23:26Z

managed/src/main/java/com/yugabyte/yw/models/InstanceType.java

@@ -29,6 +31,9 @@

  public static List<String> AWS_INSTANCE_PREFIXES_SUPPORTED = ImmutableList.of(
    "m3.", "c5.", "c5d.", "c4.", "c3.", "i3.");
+  static final String YB_VOLUME_INFO_VOLUME_COUNT = "yb.volumeInfo.volume_count";


YB_VOLUME_INFO_VOLUME_COUNT_KEY

sb-yb · 2021-01-05T22:36:53Z

Also bug description should say what we are doing here.
Basically the fact that empty volume list is stored in db when we want to use config default.
Also Fix the "Heading"
It should be following format
[#1234] Platform: One line description of change

sb-yb · 2021-01-05T22:50:51Z

managed/src/main/java/com/yugabyte/yw/models/InstanceType.java

@@ -179,6 +191,13 @@ public static void deleteInstanceTypesForProvider(Provider provider) {
      entries = entries.stream()
        .filter(supportedInstanceTypes(AWS_INSTANCE_PREFIXES_SUPPORTED))
        .collect(Collectors.toList());
+      for (InstanceType instanceType : entries) {


extract method populateDefaultIfEmpty()

…e-db into ENG-5919_latest

sb-yb

Few more changes.

sb-yb · 2021-01-07T07:15:08Z

managed/src/main/resources/reference.conf

@@ -13,4 +13,12 @@ yb {
    # For how long do we let the task be in database after it has completed
    task_retention_duration = 120 days
  }
+
+  volumeInfo {
+      # Volume count needed for AWS instance types


# default volume count for aws instance types with EBS Only storage info

sb-yb · 2021-01-07T07:15:48Z

managed/src/main/resources/reference.conf

+
+  volumeInfo {
+      # Volume count needed for AWS instance types
+      volume_count = 1


sb-yb · 2021-01-07T07:17:00Z

managed/src/main/resources/reference.conf

+      volume_count = 1
+
+      # volume size in GB
+      volume_size_gb = 250


sb-yb · 2021-01-07T07:17:30Z

managed/src/main/resources/reference.conf

@@ -13,4 +13,12 @@ yb {
    # For how long do we let the task be in database after it has completed
    task_retention_duration = 120 days
  }
+
+  volumeInfo {


aws { ... }

Thanks! Done.

…e-db into ENG-5919_latest

sb-yb

Add unit tests

sb-yb · 2021-01-11T19:20:47Z

managed/src/main/java/com/yugabyte/yw/controllers/InstanceTypeController.java

@@ -26,13 +27,20 @@
 public class InstanceTypeController extends AuthenticatedController {

  public static final Logger LOG = LoggerFactory.getLogger(InstanceTypeController.class);
+  private Config config;


sb-yb · 2021-01-11T19:25:07Z

managed/src/main/java/com/yugabyte/yw/controllers/InstanceTypeController.java

@@ -26,13 +27,20 @@
 public class InstanceTypeController extends AuthenticatedController {

  public static final Logger LOG = LoggerFactory.getLogger(InstanceTypeController.class);
+  private Config config;

  @Inject
  FormFactory formFactory;


Private final

Remove Inject

Add ctor param and assign.

sb-yb · 2021-01-11T19:25:22Z

managed/src/main/java/com/yugabyte/yw/controllers/InstanceTypeController.java

@@ -26,13 +27,20 @@
 public class InstanceTypeController extends AuthenticatedController {

  public static final Logger LOG = LoggerFactory.getLogger(InstanceTypeController.class);
+  private Config config;

  @Inject
  FormFactory formFactory;

  @Inject


Private final

Remove Inject

Add ctor param and assign.

Here we will have to use Inject, Without inject we are getting below error.

No implementation for com.yugabyte.yw.controllers.InstanceTypeController (with no qualifier annotation) was bound, and could not find an injectable constructor. Injectable classes must have either one (and only one) constructor annotated with @Inject or a zero-argument constructor that is not private.

sb-yb · 2021-01-11T19:45:40Z

managed/src/main/java/com/yugabyte/yw/models/InstanceType.java

@@ -185,6 +196,22 @@ public static void deleteInstanceTypesForProvider(Provider provider) {
      entry.getInstanceTypeCode())).collect(Collectors.toList());
  }

+  /**


The comment is not capturing the plan correctly. Just remove the comment.

Updated the same method instead of having overloaded method.

sb-yb · 2021-01-11T20:04:38Z

managed/src/main/java/com/yugabyte/yw/models/InstanceType.java

+   * TODO: Please remove this method when we have helperMethod in place to get config details
+   * Query Helper to find supported instance types for a given cloud provider.
+   */
+  public static List<InstanceType> findByProvider(Provider provider, Config config) {


Do not make separate method. Just update same method.

Updated! Thank you!

…e-db into ENG-5919_latest

sb-yb

@jitendra-12113 anything holding up this change? Do you need help with any part?

jitendra-12113 · 2021-01-14T12:39:34Z

@jitendra-12113 anything holding up this change? Do you need help with any part?

@sb-yb Have updated the patch. I think we are good to go ahead and merge this change. Need your approval though. Thanks!

sb-yb

Final lap:
small code change and missing unit test

sb-yb · 2021-01-14T23:05:32Z

managed/src/main/java/com/yugabyte/yw/controllers/InstanceTypeController.java

+  // TODO: Remove this when we have HelperMethod in place to get Config details
+  @Inject
+  public InstanceTypeController(Config config) {
+    this.config = config;


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.

Done. Thanks!

sb-yb · 2021-01-14T23:08:16Z

managed/src/main/java/com/yugabyte/yw/models/InstanceType.java

@@ -179,6 +190,9 @@ public static void deleteInstanceTypesForProvider(Provider provider) {
      entries = entries.stream()


Instead consider moving the whole code (line 190 to 196) in populateDefaultIfEmpty()
And name the method populateDefaultsIfEmpty (plural)

sb-yb · 2021-01-14T23:12:56Z

managed/src/test/java/com/yugabyte/yw/models/InstanceTypeTest.java

@@ -29,6 +31,9 @@
  private Customer defaultCustomer;
  private InstanceType.InstanceTypeDetails defaultDetails;

+  @Mock


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

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!

…e-db into ENG-5919_latest

sb-yb

Given detailed instructions inline. LMK if it helps.

sb-yb · 2021-01-16T00:39:57Z

managed/src/test/java/com/yugabyte/yw/models/InstanceTypeTest.java

@@ -90,7 +94,7 @@ public void testFindByProvider() {
    instanceType.setActive(false);
    instanceType.save();
    InstanceType.upsert(newProvider.code, "bar", 2, 10.0, defaultDetails);


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

Revert to using mock.Config

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.

Like line 88 upsert another instancetype such that its details have volumeList empty.

Now the test will fail complaining that you have not mocked config.getInt or whatever you use.

Add mock setup. For example volume count config return 3 and size returns 100GB.

assert that the volume list for this instance as returned by findPyProvider is size 3 and each one is 100GB.

Hope it helps.

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:)

💯 for right attitude!

…e-db into ENG-5919_latest

sb-yb

This looks great now!
Important tips:
Make sure to run the unittests before submit. If any fail take it seriously and rerun failed tests.
If you run sbt shell then you can do testQuick and most of the times that only re-runs failed tests. Or you can use testOnly for specific tests. There are few flaky tests so you can ignore if they pass in subsequent run. If not then take it seriously and post on slack channel for any debugging help. Sometime there are broken tests at HEAD. Not ideal situation, I know. But given that it is easier to not spend too much time debugging and just ask around (I learned painfully :( ..)

sb-yb · 2021-01-17T22:47:59Z

managed/src/test/java/com/yugabyte/yw/models/InstanceTypeTest.java

@@ -90,7 +94,7 @@ public void testFindByProvider() {
    instanceType.setActive(false);
    instanceType.save();
    InstanceType.upsert(newProvider.code, "bar", 2, 10.0, defaultDetails);


💯 for right attitude!

[yugabyte#5919] Platform: change default storage option for AWS provider to 1x250GB Description: We wanted to show default volume count to 1 on the UI for all the instance types starting with "c5./c4." Testing: When we select aws provider while creating universe, volume count was showing more than one for some instances types starting with ""c5./c4.". Now with this fix it will show only one. Will have to reset instanceTypeDetailsJson to empty for older YW instances. Co-authored-by: Jitendra Kumar <[email protected]>

jitendra-12113 mentioned this pull request Jan 5, 2021

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

Closed

sb-yb reviewed Jan 5, 2021

View reviewed changes

sb-yb suggested changes Jan 5, 2021

View reviewed changes

sb-yb reviewed Jan 5, 2021

View reviewed changes

Merge branch 'master' into ENG-5919_latest

bc1a948

jitendra-12113 changed the title ~~Heading:~~ [#5919] Platform: change default storage option for AWS provider to 1x250GB Jan 6, 2021

jitendra-12113 and others added 2 commits January 6, 2021 22:57

Merge branch 'master' into ENG-5919_latest

2a02efb

Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…

8e3e7ff

…e-db into ENG-5919_latest

sb-yb suggested changes Jan 7, 2021

View reviewed changes

Jitendra Kumar added 2 commits January 8, 2021 22:59

Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…

4922831

…e-db into ENG-5919_latest

Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…

bd16e96

…e-db into ENG-5919_latest

sb-yb mentioned this pull request Jan 11, 2021

[Platform] Get rid of static methods in Entity beans. #6832

Open

Jitendra Kumar added 4 commits January 11, 2021 23:32

Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…

d761c16

…e-db into ENG-5919_latest

Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…

1a54cfa

…e-db into ENG-5919_latest

Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…

8202218

…e-db into ENG-5919_latest

Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…

b8bc59e

…e-db into ENG-5919_latest

sb-yb suggested changes Jan 11, 2021

View reviewed changes

Jitendra Kumar added 2 commits January 12, 2021 12:20

Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…

b98f2cc

…e-db into ENG-5919_latest

Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…

fd70c2d

…e-db into ENG-5919_latest

sb-yb suggested changes Jan 14, 2021

View reviewed changes

Jitendra Kumar added 4 commits January 16, 2021 03:08

Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…

0300b33

…e-db into ENG-5919_latest

Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…

ad53844

…e-db into ENG-5919_latest

Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…

8093bb6

…e-db into ENG-5919_latest

Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…

afac6f2

…e-db into ENG-5919_latest

sb-yb suggested changes Jan 16, 2021

View reviewed changes

Jitendra Kumar added 2 commits January 16, 2021 13:49

Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…

da5d7a2

…e-db into ENG-5919_latest

Merge branch 'ENG-5919_latest' of https://github.com/yugabyte/yugabyt…

f9895eb

…e-db into ENG-5919_latest

sb-yb approved these changes Jan 17, 2021

View reviewed changes

jitendra-12113 merged commit 47122ef into master Jan 19, 2021

jitendra-12113 deleted the ENG-5919_latest branch January 19, 2021 09:57

jitendra-12113 mentioned this pull request Jan 19, 2021

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

Closed

		@@ -179,6 +190,9 @@ public static void deleteInstanceTypesForProvider(Provider provider) {
		entries = entries.stream()

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

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

Conversation

jitendra-12113 commented Jan 5, 2021 • edited Loading

CLAassistant commented Jan 5, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jitendra-12113 Jan 8, 2021 • edited Loading

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

sb-yb commented Jan 5, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sb-yb 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

sb-yb 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

sb-yb left a comment

Choose a reason for hiding this comment

jitendra-12113 commented Jan 14, 2021

sb-yb 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

sb-yb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jitendra-12113 Jan 16, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sb-yb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jitendra-12113 commented Jan 5, 2021 •

edited

Loading

CLAassistant commented Jan 5, 2021 •

edited

Loading

jitendra-12113 Jan 8, 2021 •

edited

Loading

jitendra-12113 Jan 16, 2021 •

edited

Loading