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 7 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
12 changes: 8 additions & 4 deletions compute/cloud-client/src/main/java/compute/disks/storagepool/CreateHyperdiskStoragePool.java
View file
Open in desktop
Comment thread
minherz marked this conversation as resolved.
Original file line number Diff line number Diff line change
Expand Up @@ -48,16 +48,19 @@ public static void main(String[] args)
long provisionedIops = 3000;
// the throughput in MBps to provision for the storage pool.
long provisionedThroughput = 140;
// Optional: the performance provisioning type of the storage pool.
Comment thread
minherz marked this conversation as resolved.
Outdated
// The allowed values are advanced and standard. If not specified, the value advanced is used.
Comment thread
minherz marked this conversation as resolved.
Outdated
String performanceProvisioningType = "advanced";
Comment thread
TetyanaYahodska marked this conversation as resolved.

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 +74,7 @@ public static StoragePool createHyperdiskStoragePool(String projectId, String zo
.setPoolProvisionedCapacityGb(capacity)
.setPoolProvisionedIops(iops)
.setPoolProvisionedThroughput(throughput)
.setPerformanceProvisioningType(performanceProvisioningType)
.build();

InsertStoragePoolRequest request = InsertStoragePoolRequest.newBuilder()
Expand Down
13 changes: 7 additions & 6 deletions compute/cloud-client/src/test/java/compute/disks/HyperdisksIT.java
View file
Open in desktop
Comment thread
minherz marked this conversation as resolved.
Comment thread
minherz marked this conversation as resolved.
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);
Comment thread
minherz marked this conversation as resolved.

//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()

@minherz minherz Oct 29, 2024

Copy link
Copy Markdown
Contributor

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?

@TetyanaYahodska TetyanaYahodska Oct 30, 2024

Copy link
Copy Markdown
Contributor Author

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.

@TetyanaYahodska TetyanaYahodska Oct 30, 2024

Copy link
Copy Markdown
Contributor Author

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",
Comment thread
minherz marked this conversation as resolved.
PROJECT_ID, ZONE);
StoragePool storagePool = CreateHyperdiskStoragePool
.createHyperdiskStoragePool(PROJECT_ID, ZONE, STORAGE_POOL_NAME, poolType,
"advanced", 10240, 10000, 10240);
"advanced", 10240, 10000, 10240,
Comment thread
minherz marked this conversation as resolved.
Outdated

@minherz minherz Oct 29, 2024

Copy link
Copy Markdown
Contributor

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?

@TetyanaYahodska TetyanaYahodska Oct 30, 2024

Copy link
Copy Markdown
Contributor Author

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.

@minherz minherz Oct 30, 2024

Copy link
Copy Markdown
Contributor

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.

@TetyanaYahodska TetyanaYahodska Oct 30, 2024

Copy link
Copy Markdown
Contributor Author

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,11 @@ 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(PERFORMANCE_PROVISIONING_TYPE));
Assert.assertTrue(storagePool.getZone().contains(ZONE));
}

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

@minherz minherz Oct 29, 2024

Copy link
Copy Markdown
Contributor

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.

@TetyanaYahodska TetyanaYahodska Oct 30, 2024

Copy link
Copy Markdown
Contributor Author

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.

@TetyanaYahodska TetyanaYahodska Oct 30, 2024

Copy link
Copy Markdown
Contributor Author

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.

@minherz minherz Nov 6, 2024

Copy link
Copy Markdown
Contributor

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.

@TetyanaYahodska TetyanaYahodska Nov 7, 2024

Copy link
Copy Markdown
Contributor Author

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