Skip to content
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.

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

[feat] add gke release testing #1618

Merged
merged 8 commits into from
Jul 30, 2021
Merged

[feat] add gke release testing #1618

merged 8 commits into from
Jul 30, 2021

Conversation

shaneutt
Copy link
Contributor

@shaneutt shaneutt commented Jul 30, 2021

What this PR does / why we need it:

This adds testing of conformant, production grade Kubernetes clusters as a pre-requisite for any release.

Which issue this PR fixes

Supports #1095

Special notes for your reviewer:

This is only one iteration, there are several related and follow-up items:

Do Not Merge:

There's some git history cleanup that the author needs to perform before merging, please let the author merge this when its ready.

@shaneutt shaneutt self-assigned this Jul 30, 2021
@Kong Kong deleted a comment from codecov bot Jul 30, 2021
@Kong Kong deleted a comment from github-actions bot Jul 30, 2021
@Kong Kong deleted a comment from codecov bot Jul 30, 2021
@shaneutt shaneutt temporarily deployed to gcloud July 30, 2021 15:11 Inactive
@shaneutt shaneutt temporarily deployed to gcloud July 30, 2021 15:11 Inactive
@shaneutt shaneutt temporarily deployed to gcloud July 30, 2021 15:11 Inactive
@shaneutt shaneutt temporarily deployed to gcloud July 30, 2021 15:11 Inactive
@shaneutt shaneutt temporarily deployed to gcloud July 30, 2021 15:20 Inactive
@shaneutt shaneutt temporarily deployed to gcloud July 30, 2021 15:20 Inactive
@shaneutt shaneutt temporarily deployed to gcloud July 30, 2021 15:20 Inactive
@shaneutt shaneutt temporarily deployed to gcloud July 30, 2021 15:20 Inactive
@Kong Kong deleted a comment from codecov bot Jul 30, 2021
@Kong Kong deleted a comment from codecov bot Jul 30, 2021
@shaneutt shaneutt temporarily deployed to gcloud July 30, 2021 16:49 Inactive
@shaneutt shaneutt temporarily deployed to gcloud July 30, 2021 16:49 Inactive
@shaneutt shaneutt temporarily deployed to gcloud July 30, 2021 16:49 Inactive
@shaneutt shaneutt disabled auto-merge July 30, 2021 16:49
@mflendrich
Copy link
Contributor

Is it a good idea to review before git history cleanup? These tend to introduce bugs.

@shaneutt
Copy link
Contributor Author

Is it a good idea to review before git history cleanup? These tend to introduce bugs.

In this case, it's literally just dropping the release workflow trigger on every PR (so that we can verify that the tests are passing before merge, but not leave them on for all prs).

Copy link
Contributor

@rainest rainest left a comment

Choose a reason for hiding this comment

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

For posterity, example run at https://github.com/Kong/kubernetes-ingress-controller/runs/3203219168?check_suite_focus=true, which looks fine.

It seems like we'd maybe just want to skip the bulk ingress test on GKE to avoid modifying it (I assume to deal with GKE resource quotas), as that's to stress the controller code internals and shouldn't have any difference compatibility-wise from the single-ingress tests. Doesn't look like it really runs slower though (3m random traditional run versus 4m for this), so 🤷

Do we have example failure runs or other documentation of the issues with the Knative and AC tests? The TODOs don't mention them and it seems like we'd definitely want to follow up on those.

@shaneutt
Copy link
Contributor Author

For posterity, example run at https://github.com/Kong/kubernetes-ingress-controller/runs/3203219168?check_suite_focus=true, which looks fine.

It seems like we'd maybe just want to skip the bulk ingress test on GKE to avoid modifying it (I assume to deal with GKE resource quotas), as that's to stress the controller code internals and shouldn't have any difference compatibility-wise from the single-ingress tests. Doesn't look like it really runs slower though (3m random traditional run versus 4m for this), so shrug

Do we have example failure runs or other documentation of the issues with the Knative and AC tests? The TODOs don't mention them and it seems like we'd definitely want to follow up on those.

The changes to the bulk ingress test were just modifying what were semi-arbitrary values: reducing the staggered deployment in half does still leaves the test ultimately verifying what it originally intended to verify. At this point if we want to get deep into the characteristics of performance at a specific scale, I would suggest we treat that as follow up in our upcoming performance testing features: #1197

@shaneutt shaneutt temporarily deployed to gcloud July 30, 2021 17:24 Inactive
@shaneutt shaneutt temporarily deployed to gcloud July 30, 2021 17:24 Inactive
@shaneutt shaneutt temporarily deployed to gcloud July 30, 2021 17:24 Inactive
@shaneutt shaneutt temporarily deployed to gcloud July 30, 2021 17:24 Inactive
@shaneutt
Copy link
Contributor Author

lgtm
Things are looking good. I'm going to remove the test trigger and mark this for a rebase merge once the remaining CI workflows settle. 👍

@shaneutt shaneutt enabled auto-merge (rebase) July 30, 2021 17:51
@shaneutt shaneutt merged commit 8a71a3f into next Jul 30, 2021
@shaneutt shaneutt deleted the e2e branch July 30, 2021 18:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci ci/license/changed do not merge let the author merge this, don't merge for them. priority/high
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants