chore: Fix flaky IT test cases #8260

lqiu96 · 2022-08-31T21:00:18Z

This PR should resolve a bunch of the flaky test cases in the monorepo.

Status:

Quota Metrics after VM cleanup (7 Day Graph):

.kokoro/build.sh

meltsufin · 2022-09-06T18:41:50Z

...loud-compute/src/test/java/com/google/cloud/compute/v1/integration/ITSmokeInstancesTest.java

@@ -72,6 +72,8 @@ public static void setUp() throws IOException {
    instances = new ArrayList<>();
    InstancesSettings instanceSettings = InstancesSettings.newBuilder().build();
    instancesClient = InstancesClient.create(instanceSettings);
+
+    Util.cleanUpComputeInstances(instancesClient, DEFAULT_PROJECT, DEFAULT_ZONE);


Isn't this a bit too aggressive to delete all compute instances? Why isn't the cleanup in tearDown() sufficient?

tearDown() cleanup is sufficient for 99% of the case. There was a significant amount of compute instances that were in there (my guess from previous runs that didn't have the cleanup or when the IT jobs were cancelled halfway).

This project is only used for ITs so it would have 24 hours to report the results before the previous instances are deleted.

I'm worried that someone runs the test on their own project and causes inadvertent deletion of instances. If we want to do this kind of cleanup, maybe we can have instance name prefix that is consistent and can accurately filter instances created by this test.

I'm worried that someone runs the test on their own project and causes inadvertent deletion of instances.

Actually, I was thinking that this would be fine for the ITs. I was thinking the ideal state for gcloud-devel would have 0 compute instances when there are no active ITs running. If Util.cleanUpComputeInstances (or more filtered version of that function) matches any existing instances, then those should be deleted. 24 hours is 12x as long as the longest GraalVM job.

maybe we can have instance name prefix that is consistent and can accurately filter instances created by this test.

Yep! I was planning on creating another PR that would update the instances for each module to have a more unique name instead test-instance or gapic-instance. i.e. Compute would be test-compute-instance-UUID, Container would be test-container-instance-UUID or something like that.

Ok nvm, I just re-read the comment.

test on their own project

meltsufin · 2022-09-06T18:46:17Z

...iner/google-cloud-container/src/test/java/com/google/cloud/container/v1/it/ITSystemTest.java

@@ -90,7 +91,13 @@ public static void beforeClass() throws Exception {

  @AfterClass
  public static void afterClass() throws Exception {
-    Thread.sleep(TimeUnit.MINUTES.toMillis(5));
+    Operation response = client.getOperation(PROJECT_ID, ZONE, operation.getName());
+    // Sleep for one minute until Cluster CREATE operation is complete


Why are we waiting for cluster creation completion in afterClass()? Shouldn't we wait for it right after creation in beforeClass()?

I was able to run operations of the client (get/ list) without the operation having to be complete. It was only when deleting the cluster, I got an error with Operation in progress. I think it would make sense to move this to wait when the operation was run.

I'll change this to to do the busy wait in beforeClass().

meltsufin · 2022-09-06T18:47:43Z

...tebooks/src/test/java/com/google/cloud/notebooks/v1beta1/it/ITNotebookServiceClientTest.java

@@ -66,6 +66,8 @@ public class ITNotebookServiceClientTest {
  public static void setUp() throws IOException, ExecutionException, InterruptedException {
    // Create Test Notebook Instance
    client = NotebookServiceClient.create();
+    Util.cleanUpNotebookInstances(client, PARENT);


Wouldn't this delete the one created above before it's even used?
Also, why isn't cleanup in tearDown() sufficient?

It would only delete notebooks that have a creation time more than 24 hours earlier.

Can we have a prefix for the notebook names or something to avoid accidentally deleting unrelated notebooks.

Yep, it's similar to the comment above about compute and container. I'll add in more specific names for the IT instances.

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

…oud-java into update_container_IT

lqiu96 · 2022-09-12T15:01:07Z

@meltsufin @suztomo Could you re-review this PR?

meltsufin · 2022-09-12T15:20:24Z

...compute/google-cloud-compute/src/test/java/com/google/cloud/compute/v1/integration/Util.java

+    for (Instance instance : listPagedResponse.iterateAll()) {
+      if (isCreatedBeforeThresholdTime(
+              ZonedDateTime.parse(instance.getCreationTimestamp()).toInstant())
+          && instance.getName().startsWith(BaseTest.COMPUTE_PREFIX)) {


For both Util classes, I think I would rather have the prefix passed in as an argument into this method, but I guess this is OK too.

On second thought, this seems better. I'll create a new PR for this.

lqiu96 added 12 commits August 31, 2022 20:38

chore: Busy wait java-container CREATE operation to finish

9fb64e6

chore: Delete any existing java-notebook instances

c3edca8

chore: Use ListInstanceRequest with parent

e6c548e

chore: Delete gapic compute instances

06d3f67

chore: Use ZonedDateTime for RFC3339 with Zone

7ad0c30

chore: Setup the correct Request object field

e77cefd

chore: Create new IT profile

b466d5f

chore: Move shared deps profile to root pom.xml

704cd3c

chore: Use test lifecycle phase for graalvm tests

9c21c07

chore: Use old pom configs

056b2cd

chore: Use correct path to vision resources

0ad9b28

Merge branch 'main' into update_container_IT

06c618d

lqiu96 requested review from suztomo and meltsufin September 2, 2022 19:23

lqiu96 marked this pull request as ready for review September 2, 2022 19:23

lqiu96 requested a review from ddixit14 September 2, 2022 19:35

chore: Remove unused pom.xml

c200945

meltsufin requested changes Sep 6, 2022

View reviewed changes

lqiu96 and others added 3 commits September 6, 2022 21:20

chore: Move busy wait after CREATE operation

3922ea7

chore: Add specific names for IT compute instances

08a5645

🦉 Updates from OwlBot post-processor

7782c59

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

lqiu96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 7, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 7, 2022

lqiu96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 7, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 7, 2022

lqiu96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 7, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 7, 2022

lqiu96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 7, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 7, 2022

chore: Clean up compute instances names

aaa73ba

lqiu96 and others added 7 commits September 7, 2022 23:35

chore: Clean up instances

7ac0434

🦉 Updates from OwlBot post-processor

ad03253

See https://github.com/googleapis/repo-automation-bots/blob/main/packages/owl-bot/README.md

chore: Add checks for deletion

2bfe213

Merge branch 'update_container_IT' of github.com:googleapis/google-cl…

24462f0

…oud-java into update_container_IT

chore: Add missing import

dd1bad5

chore: Add missing import for notebooks

a99d4dd

Merge branch 'main' into update_container_IT

b087f07

lqiu96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 9, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 9, 2022

lqiu96 added the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2022

yoshi-kokoro removed the kokoro:force-run Add this label to force Kokoro to re-run the tests. label Sep 12, 2022

meltsufin approved these changes Sep 12, 2022

View reviewed changes

lqiu96 merged commit 8e4378d into main Sep 12, 2022

lqiu96 deleted the update_container_IT branch September 12, 2022 15:42

This was referenced Sep 15, 2022

chore: GraalVM Job tests a few select modules #8343

Merged

chore: Pass prefix name to Utils for ITs #8420

Merged

emmileaf mentioned this pull request Feb 17, 2023

IT Failure(google-cloud-compute): ITPaginationTest.testPaginationIterating zone not found #9134

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chore: Fix flaky IT test cases #8260

chore: Fix flaky IT test cases #8260

lqiu96 commented Aug 31, 2022 •

edited

Loading

meltsufin Sep 6, 2022

lqiu96 Sep 6, 2022

meltsufin Sep 6, 2022

lqiu96 Sep 6, 2022 •

edited

Loading

lqiu96 Sep 6, 2022 •

edited

Loading

meltsufin Sep 6, 2022

lqiu96 Sep 6, 2022

meltsufin Sep 6, 2022

lqiu96 Sep 6, 2022

meltsufin Sep 6, 2022

lqiu96 Sep 6, 2022

lqiu96 commented Sep 12, 2022

meltsufin Sep 12, 2022

lqiu96 Sep 12, 2022

chore: Fix flaky IT test cases #8260

chore: Fix flaky IT test cases #8260

Conversation

lqiu96 commented Aug 31, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 Sep 6, 2022 • edited Loading

Choose a reason for hiding this comment

lqiu96 Sep 6, 2022 • 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

lqiu96 commented Sep 12, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lqiu96 commented Aug 31, 2022 •

edited

Loading

lqiu96 Sep 6, 2022 •

edited

Loading

lqiu96 Sep 6, 2022 •

edited

Loading