Skip to content

feat(compute): extend compute hyperdisk pool create #9595

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

Merged
TetyanaYahodska merged 15 commits into from
Nov 7, 2024
Merged

feat(compute): extend compute hyperdisk pool create #9595

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
371273a
Added performance-provisioning-type, fixed test
TetyanaYahodska Oct 5, 2024
15c24c0
merged changes from main
TetyanaYahodska Oct 19, 2024
d810b7e
fixed test
TetyanaYahodska Oct 19, 2024
cef06ab
Enabled tests
TetyanaYahodska Oct 19, 2024
fbb8fa6
Fixed whitespaces
TetyanaYahodska Oct 19, 2024
81891b3
Merge branch 'main' into extend-compute_hyperdisk_pool_create
TetyanaYahodska Oct 28, 2024
55c7835
Added comments
TetyanaYahodska Oct 28, 2024
47ecd63
Merge branch 'main' into extend-compute_hyperdisk_pool_create
TetyanaYahodska Oct 30, 2024
8294ad9
Fixed comments and zone
TetyanaYahodska Oct 30, 2024
928be83
Fixed timeout and zone in test class
TetyanaYahodska Oct 30, 2024
fdbb0f2
Merge branch 'main' into extend-compute_hyperdisk_pool_create
TetyanaYahodska Oct 30, 2024
df550e2
Fixed naming, added variable CAPACITY_PROVISIONING_TYPE
TetyanaYahodska Oct 30, 2024
3efab67
Fixed storagePoolType
TetyanaYahodska Nov 4, 2024
9fd467d
Fixed lint issue
TetyanaYahodska Nov 4, 2024
6dbb870
Merge branch 'main' into extend-compute_hyperdisk_pool_create
TetyanaYahodska Nov 7, 2024
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
10 changes: 6 additions & 4 deletions compute/cloud-client/src/main/java/compute/disks/storagepool/CreateHyperdiskStoragePool.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,16 +48,17 @@ public static void main(String[] args)
long provisionedIops = 3000;
// the throughput in MBps to provision for the storage pool.
long provisionedThroughput = 140;
String performanceProvisioningType = "advanced";

createHyperdiskStoragePool(projectId, zone, storagePoolName, storagePoolType,
capacityProvisioningType, provisionedCapacity, provisionedIops, provisionedThroughput);
capacityProvisioningType, provisionedCapacity, provisionedIops,
provisionedThroughput, performanceProvisioningType);
}

// Creates a hyperdisk storagePool in a project
public static StoragePool createHyperdiskStoragePool(String projectId, String zone,
String storagePoolName, String storagePoolType,
String capacityProvisioningType, long capacity,
long iops, long throughput)
String storagePoolName, String storagePoolType, String capacityProvisioningType,
long capacity, long iops, long throughput, String performanceProvisioningType)
throws IOException, ExecutionException, InterruptedException, TimeoutException {
// Initialize client that will be used to send requests. This client only needs to be created
// once, and can be reused for multiple requests.
Expand All @@ -71,6 +72,7 @@ public static StoragePool createHyperdiskStoragePool(String projectId, String zo
.setPoolProvisionedCapacityGb(capacity)
.setPoolProvisionedIops(iops)
.setPoolProvisionedThroughput(throughput)
.setPerformanceProvisioningType(performanceProvisioningType)
.build();

InsertStoragePoolRequest request = InsertStoragePoolRequest.newBuilder()
Expand Down
12 changes: 6 additions & 6 deletions compute/cloud-client/src/test/java/compute/disks/HyperdisksIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@
import org.junit.FixMethodOrder;
import org.junit.jupiter.api.AfterAll;
import org.junit.jupiter.api.BeforeAll;
import org.junit.jupiter.api.Disabled;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.Timeout;
import org.junit.runner.RunWith;
Expand All @@ -48,6 +47,7 @@ public class HyperdisksIT {
private static String HYPERDISK_NAME;
private static String HYPERDISK_IN_POOL_NAME;
private static String STORAGE_POOL_NAME;
private static final String PERFORMANCE_PROVISIONING_TYPE = "advanced";

// Check if the required environment variables are set.
public static void requireEnvVar(String envVarName) {
Expand All @@ -73,9 +73,9 @@ public static void cleanup()
throws IOException, InterruptedException, ExecutionException, TimeoutException {
// Delete all disks created for testing.
DeleteDisk.deleteDisk(PROJECT_ID, ZONE, HYPERDISK_NAME);
//DeleteDisk.deleteDisk(PROJECT_ID, ZONE, HYPERDISK_IN_POOL_NAME);
DeleteDisk.deleteDisk(PROJECT_ID, ZONE, HYPERDISK_IN_POOL_NAME);

//Util.deleteStoragePool(PROJECT_ID, ZONE, STORAGE_POOL_NAME);
Util.deleteStoragePool(PROJECT_ID, ZONE, STORAGE_POOL_NAME);
}

@Test
Expand All @@ -96,15 +96,15 @@ public void stage1_CreateHyperdiskTest()
Assert.assertTrue(hyperdisk.getZone().contains(ZONE));
}

@Disabled
@Test
public void stage1_CreateHyperdiskStoragePoolTest()
Copy link
Contributor

@minherz minherz Oct 29, 2024

Choose a reason for hiding this comment

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

why the name of the method follows snake notation and not pascal notation?

Copy link
Contributor Author

@TetyanaYahodska TetyanaYahodska Oct 30, 2024

Choose a reason for hiding this comment

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

It's not a scope of this task. I've created a new task to fix code and will follow yours recommendations.

Copy link
Contributor Author

@TetyanaYahodska TetyanaYahodska Oct 30, 2024

Choose a reason for hiding this comment

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

Fixed

throws IOException, ExecutionException, InterruptedException, TimeoutException {
String poolType = String.format("projects/%s/zones/%s/storagePoolTypes/hyperdisk-balanced",
PROJECT_ID, ZONE);
StoragePool storagePool = CreateHyperdiskStoragePool
.createHyperdiskStoragePool(PROJECT_ID, ZONE, STORAGE_POOL_NAME, poolType,
"advanced", 10240, 10000, 10240);
"advanced", 10240, 10000, 10240,
Copy link
Contributor

@minherz minherz Oct 29, 2024

Choose a reason for hiding this comment

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

should the capacity to be 1024 and not 10240?

Copy link
Contributor Author

@TetyanaYahodska TetyanaYahodska Oct 30, 2024

Choose a reason for hiding this comment

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

It's not a scope of this task. I've created a new task to fix code and will follow yours recommendations.

Copy link
Contributor

@minherz minherz Oct 30, 2024

Choose a reason for hiding this comment

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

I do not understand what you mean by "creating a new task"? Do you propose to address code review comments made in this PR by submitting a separate PR?
If you do, I find this proposal very inconvenient and time consuming. Please, explain what do you mean.

Regarding my question about the value 10240, I looked into Cloud Console UI and it limits the range to be from 1 to 1024 TB. I do not know if client library limits the range but I think that making it matching UX in Console is preferred.

Copy link
Contributor Author

@TetyanaYahodska TetyanaYahodska Oct 30, 2024

Choose a reason for hiding this comment

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

Fixed

PERFORMANCE_PROVISIONING_TYPE);

Assert.assertNotNull(storagePool);
Assert.assertEquals(STORAGE_POOL_NAME, storagePool.getName());
Expand All @@ -113,10 +113,10 @@ public void stage1_CreateHyperdiskStoragePoolTest()
Assert.assertEquals(10240, storagePool.getPoolProvisionedCapacityGb());
Assert.assertTrue(storagePool.getStoragePoolType().contains("hyperdisk-balanced"));
Assert.assertTrue(storagePool.getCapacityProvisioningType().equalsIgnoreCase("advanced"));
Assert.assertTrue(storagePool.getPerformanceProvisioningType().equalsIgnoreCase("advanced"));
Copy link
Contributor

@minherz minherz Oct 24, 2024

Choose a reason for hiding this comment

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

please use PERFORMANCE_PROVISIONING_TYPE instead of "advanced" literal

Copy link
Contributor Author

@TetyanaYahodska TetyanaYahodska Oct 28, 2024

Choose a reason for hiding this comment

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

Thank you. Done

Assert.assertTrue(storagePool.getZone().contains(ZONE));
}

@Disabled
@Test
public void stage2_CreateHyperdiskStoragePoolTest()
throws IOException, ExecutionException, InterruptedException, TimeoutException {
Copy link
Contributor

@minherz minherz Oct 29, 2024

Choose a reason for hiding this comment

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

what is being testing in this method that was not tested in the previous method? Why changing the arguments to the call to CreateDiskInStoragePool.createDiskInStoragePool() should matter for testing this code sample?
The code sample method does not have any conditions, so testing Google API invocation with different parameters does not test code sample but the backend API which is unnecessary.

Copy link
Contributor Author

@TetyanaYahodska TetyanaYahodska Oct 30, 2024

Choose a reason for hiding this comment

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

It's not a scope of this task. I've created a new task to fix code and will follow yours recommendations.

Copy link
Contributor Author

@TetyanaYahodska TetyanaYahodska Oct 30, 2024

Choose a reason for hiding this comment

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

I've fixed naming for these tests. As I understand from code and tests

  1. the first testCreateHyperdiskStoragePool() tests that StoragePoll was created as expected
  2. the second testCreateDiskInStoragePool() takes the StoragePoolLink of the StoragePool created in the first test and tests it's parameters.

Copy link
Contributor

@minherz minherz Nov 6, 2024

Choose a reason for hiding this comment

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

according to your explanations the second test does not validate the code sample. the code sample shows how to create a storage pool. the test(s) should validate the code.

please, modify the test class to do one of the following, either to remove the second test because it does not add to the validation to code sample or perform all validations / asserts in the first test method.

Copy link
Contributor Author

@TetyanaYahodska TetyanaYahodska Nov 7, 2024

Choose a reason for hiding this comment

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

We have two classes - CreateDiskInStoragePool and CreateHyperdiskStoragePool. To test this code Svyatoslav created two test methods - testCreateDiskInStoragePool and testCreateHyperdiskStoragePool. As far as I can see, these tests are working properly

Expand Down