Wrap K8s client in E2E with retry logic #3366

mociarain · 2024-01-23T13:27:57Z

Which issue this PR addresses:

Fixes https://issues.redhat.com/browse/ARO-4489#L67 and

What this PR does / why we need it:

This PR addresses the first part of initiative 1 outlined here.

TODO:

(Assuming the approach is okay'd) Apply expand this PR to fully cover the k8s API calls.
~~(Assuming the approach is okay'd) Start another PR/PR's for the other clients.~~ Not doing. See here for details.
Bump ginkgo to 2.8.x (We're on 2.7.0) at least to get the GinkoHelper and apply that to the helper methods. ~~Ideally, I would do this here as part of this PR. Is bumping the version of a dependency a big deal? AFAIK it's used (nearly) entirely in the E2E tests so I'm hoping it will be smooth.~~ Will do in a subsequent PR. This one is meaty enough.

Test plan for issue:

Run them all.

Is there any documentation that needs to be updated for this PR?

Very possibly

mociarain · 2024-01-23T13:34:16Z

test/e2e/helpers.go

+var (
+	DefaultTimeout  = 5 * time.Second
+	PollingInterval = 250 * time.Millisecond
+)


Unclear to me at this point what these numbers should be and if we should expose them in the API for users to mess with

kimorris27

I like this approach!

Re: your concern about where/when to bump the dependency, I don't see any problem with doing it in the same PR as this work. I think that's typically what we do unless it's an especially scary/large set of dependency bumps.

tsatam

I'm a big fan of this approach and implementation, I love the usage of generics here.

I'm in favor of having this PR, if acceptable to the wider team, merge as-is, and to make the effort of rolling this out across all our E2E tests the work of future stories, that are spread across the team. It'd be slower, but it would get more people involved in the change to hopefully normalize this approach across future tests as well.

test/e2e/helpers.go

mociarain · 2024-01-25T11:42:50Z

I'm in favour of having this PR, if acceptable to the wider team, merge as-is, and to make the effort of rolling this out across all our E2E tests the work of future stories, that are spread across the team. It'd be slower, but it would get more people involved in the change to hopefully normalize this approach across future tests as well.

I like this approach but I would suggest that splitting the work by client would be a more natural division. I can expand this PR to cover the k8s client and version bump and then people could pick up each of the other clients?

What do you think?

(I actually have a second proposal that builds on this concept that I thought would be useful to spread around for the same reasons you outline but I think you're right and we can start introducing it from here)

tsatam · 2024-01-25T14:53:15Z

I think that works too, my goal is to split work on this across the team, whichever way we split it is fine by me as long as it gets enough eyes on the change.

tsatam

👍🏼

mociarain · 2024-01-31T11:45:53Z

test/util/project/project.go

@@ -1,90 +0,0 @@
-package project


This was only being used by cluster.go so I just co-located it.

github-actions · 2024-01-31T15:54:21Z

Please rebase pull request.

tsatam

LGTM - one minor nit but it's not a blocker

test/e2e/cluster.go

test/e2e/adminapi_cluster_getlogs.go

hawkowl · 2024-02-05T01:49:47Z

/azp rerun e2e

azure-pipelines · 2024-02-05T01:49:52Z

Command 'rerun' is not supported by Azure Pipelines. Supported commands help: Get descriptions, examples and documentation about supported commands Example: help "command_name" list: List all pipelines for this repository using a comment. Example: "list" run: Run all pipelines or specific pipelines for this repository using a comment. Use this command by itself to trigger all related pipelines, or specify specific pipelines to run. Example: "run" or "run pipeline_name, pipeline_name, pipeline_name" where: Report back the Azure DevOps orgs that are related to this repository and org Example: "where" See additional documentation.

mociarain · 2024-02-05T10:21:29Z

/azp run e2e

azure-pipelines · 2024-02-05T10:21:40Z

Azure Pipelines successfully started running 1 pipeline(s).

AldoFusterTurpin

LGTM. Thank you for this @mociarain. This is a good improvement.

I just left some optional suggestion in case you find any useful.

test/e2e/adminapi_csr.go

test/e2e/adminapi_redeployvm.go

test/e2e/cluster.go

test/e2e/helpers.go

AldoFusterTurpin

LGTM. Thanks!

tsatam · 2024-02-07T14:05:17Z

Marking this PR as high priority - I believe this change will go a long way towards fixing known flakes and currently-broken e2e tests such as the one being debugged in #3384

cadenmarchese

Thanks a lot! I left one comment over on the other PR, it's more of a question, but we can get this merged now to help resolve e2e failures. #3388 (comment)

mociarain force-pushed the refactor-e2e-expect-not-to branch from 99a8cf1 to c03370a Compare January 23, 2024 13:31

mociarain commented Jan 23, 2024

View reviewed changes

mociarain requested review from SudoBrendan and tiguelu January 23, 2024 16:04

kimorris27 reviewed Jan 24, 2024

View reviewed changes

tsatam previously approved these changes Jan 24, 2024

View reviewed changes

test/e2e/helpers.go Outdated Show resolved Hide resolved

mociarain dismissed tsatam’s stale review via 5ba1557 January 30, 2024 10:00

mociarain requested review from kimorris27 and tsatam January 30, 2024 11:25

tsatam previously approved these changes Jan 30, 2024

View reviewed changes

kimorris27 previously approved these changes Jan 30, 2024

View reviewed changes

mociarain dismissed stale reviews from kimorris27 and tsatam via dc03987 January 31, 2024 11:44

mociarain commented Jan 31, 2024

View reviewed changes

github-actions bot added the needs-rebase branch needs a rebase label Jan 31, 2024

mociarain added 12 commits January 31, 2024 17:23

Add initial helpers.go

9189b9e

Apply to one file

f40d93f

Update doc comment

4a68234

Remove unneeded import

fd86d27

Leverage the type system for drier code

899d815

Actually tease out a MVP

adbc182

Tidy up the helpers

bfb9e3e

Attempt to conform to Go conventions

a53ee31

Update adminapi_delete_managedresources.go

727c6cd

Add list function

9ced6d8

Update adminapi_drainnode

05af225

Update adminapi_kubernetesobjects.go

00572ec

mociarain requested review from hlipsig and dofinn as code owners February 2, 2024 09:19

mociarain changed the title ~~Refactor e2e expect not to~~ Wrap K8s client in E2E with retry logic Feb 2, 2024

tsatam previously approved these changes Feb 2, 2024

View reviewed changes

test/e2e/cluster.go Show resolved Hide resolved

test/e2e/adminapi_cluster_getlogs.go Outdated Show resolved Hide resolved

Reuse the same variable name for the same thing

67dc78c

mociarain dismissed tsatam’s stale review via 67dc78c February 2, 2024 16:29

mociarain requested a review from tsatam February 5, 2024 12:16

AldoFusterTurpin previously approved these changes Feb 5, 2024

View reviewed changes

mociarain added 3 commits February 5, 2024 16:38

s/(.*)Call/$1Func

a9bef33

Bind repeated string to a constant

0cf4271

Tidy up naming conventions and comments

da49ae8

mociarain dismissed AldoFusterTurpin’s stale review via da49ae8 February 5, 2024 15:50

mociarain requested a review from AldoFusterTurpin February 5, 2024 15:50

mociarain mentioned this pull request Feb 6, 2024

Clearer E2E stack traces #3382

Closed

AldoFusterTurpin approved these changes Feb 6, 2024

View reviewed changes

mociarain mentioned this pull request Feb 7, 2024

Remove unused old code #3383

Merged

tsatam approved these changes Feb 7, 2024

View reviewed changes

tsatam added priority-high High priority issue or pull request ready-for-review labels Feb 7, 2024

This was referenced Feb 7, 2024

Add explicit step to wait for job completion in ARO cluster DNS e2e test #3388

Merged

[DONOTMERGE] debugging Azure host servicing E2E test failures in CI #3384

Closed

cadenmarchese approved these changes Feb 7, 2024

View reviewed changes

cadenmarchese merged commit b359efd into Azure:master Feb 7, 2024
18 checks passed

mociarain deleted the refactor-e2e-expect-not-to branch February 8, 2024 07:46

mociarain mentioned this pull request Feb 8, 2024

Make NSG Monitor run every 10 minutes #3343

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Wrap K8s client in E2E with retry logic #3366

Wrap K8s client in E2E with retry logic #3366

mociarain commented Jan 23, 2024 •

edited by niontive

Loading

mociarain Jan 23, 2024

kimorris27 left a comment

tsatam left a comment

mociarain commented Jan 25, 2024 •

edited

Loading

tsatam commented Jan 25, 2024

tsatam left a comment

mociarain Jan 31, 2024

github-actions bot commented Jan 31, 2024

tsatam left a comment

hawkowl commented Feb 5, 2024

azure-pipelines bot commented Feb 5, 2024

mociarain commented Feb 5, 2024

azure-pipelines bot commented Feb 5, 2024

AldoFusterTurpin left a comment

AldoFusterTurpin left a comment

tsatam commented Feb 7, 2024

cadenmarchese left a comment

Wrap K8s client in E2E with retry logic #3366

Wrap K8s client in E2E with retry logic #3366

Conversation

mociarain commented Jan 23, 2024 • edited by niontive Loading

Which issue this PR addresses:

What this PR does / why we need it:

Test plan for issue:

Is there any documentation that needs to be updated for this PR?

mociarain Jan 23, 2024

Choose a reason for hiding this comment

kimorris27 left a comment

Choose a reason for hiding this comment

tsatam left a comment

Choose a reason for hiding this comment

mociarain commented Jan 25, 2024 • edited Loading

tsatam commented Jan 25, 2024

tsatam left a comment

Choose a reason for hiding this comment

mociarain Jan 31, 2024

Choose a reason for hiding this comment

github-actions bot commented Jan 31, 2024

tsatam left a comment

Choose a reason for hiding this comment

hawkowl commented Feb 5, 2024

azure-pipelines bot commented Feb 5, 2024

mociarain commented Feb 5, 2024

azure-pipelines bot commented Feb 5, 2024

AldoFusterTurpin left a comment

Choose a reason for hiding this comment

AldoFusterTurpin left a comment

Choose a reason for hiding this comment

tsatam commented Feb 7, 2024

cadenmarchese left a comment

Choose a reason for hiding this comment

mociarain commented Jan 23, 2024 •

edited by niontive

Loading

mociarain commented Jan 25, 2024 •

edited

Loading