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

tests: try and get pipeline green again #2259

Merged
merged 3 commits into from
Dec 16, 2021
Merged

Conversation

ciarams87
Copy link
Member

@ciarams87 ciarams87 commented Dec 8, 2021

Proposed changes

(to be taken in conjunction with gitlab pipeline changes)

Changes:

  • Move running all Plus and AP nightly tests to GKE
  • Run AP tests on GKE only (including PR)
  • Add a skip for certain tests when the service is loadbalancer (the transport server load balancing tests need changes to work with an external loadbalancer service)
  • Allow the VSR external route test that regularly fails to fail (there will be a follow up task to try and fix this permanently, but I can't find an obvious solution rn)
  • Use flaky plugin to auto rerun a few flaky tests (again this is not a permanent solution)
  • Switch AP log tests to using a single syslog deployment, and wipe the logs between runs (a lot of failures were due to some IP caching or similar when deleting and recreating the syslog deployments in a short timeframe)
  • Update the rate limiting tests to 5r/s instead of 10, because 10 is too fast in GKE

In the latest run, we had a failure due to GKE quotas because we were testing two nightly pipelines at once - we won't have that in future. This isn't a fix for all our problems, but more of a "round-1" effort.

At the moment we are still running the PR Plus tests in both GitHub and GitLab, we can decide later which path to choose.

See https://gitlab.com/f5/nginx/kic/kubernetes-ingress/-/pipelines/430470455 for the green PR run and https://gitlab.com/f5/nginx/kic/kubernetes-ingress/-/pipelines/430330791 for the green nightly run.

Checklist

Before creating a PR, run through this checklist and mark each as complete.

  • I have read the CONTRIBUTING doc
  • I have added tests that prove my fix is effective or that my feature works
  • I have checked that all unit tests pass after adding my changes
  • I have updated necessary documentation
  • I have rebased my branch onto master
  • I will ensure my PR is targeting the master branch and pulling from my branch from my own fork

@github-actions github-actions bot added the tests Pull requests that update tests label Dec 8, 2021
@ciarams87 ciarams87 force-pushed the tests/stabilize-tests-round1 branch 4 times, most recently from 70370c8 to ad91a59 Compare December 15, 2021 14:44
@ciarams87 ciarams87 marked this pull request as ready for review December 15, 2021 18:25
@codecov-commenter
Copy link

codecov-commenter commented Dec 15, 2021

Codecov Report

Merging #2259 (ad91a59) into master (69ad025) will decrease coverage by 0.01%.
The diff coverage is n/a.

❗ Current head ad91a59 differs from pull request most recent head d65b0a8. Consider uploading reports for the commit d65b0a8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2259      +/-   ##
==========================================
- Coverage   53.39%   53.38%   -0.02%     
==========================================
  Files          43       43              
  Lines       13412    13409       -3     
==========================================
- Hits         7161     7158       -3     
  Misses       6022     6022              
  Partials      229      229              
Impacted Files Coverage Δ
internal/configs/virtualserver.go 95.20% <0.00%> (-0.01%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 69ad025...d65b0a8. Read the comment docs.

@nginx-bot nginx-bot force-pushed the tests/stabilize-tests-round1 branch 3 times, most recently from 976186e to 7cefd29 Compare December 16, 2021 05:29

RUN apt-get update && apt-get install -y curl git jq \
&& curl -LO https://storage.googleapis.com/kubernetes-release/release/$(curl -s https://storage.googleapis.com/kubernetes-release/release/stable.txt)/bin/linux/amd64/kubectl \
&& chmod +x ./kubectl \
&& mv ./kubectl /usr/local/bin \
&& curl -O https://dl.google.com/dl/cloudsdk/channels/rapid/downloads/google-cloud-sdk-${GCLOUD_VERSION}-linux-x86_64.tar.gz \
&& tar xvzf google-cloud-sdk-${GCLOUD_VERSION}-linux-x86_64.tar.gz \
&& mv google-cloud-sdk /usr/lib/
&& mv google-cloud-sdk /usr/lib/ \
&& curl -LO https://get.helm.sh/helm-v${HELM_VERSION}-linux-amd64.tar.gz \
Copy link
Member

Choose a reason for hiding this comment

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

I couldn't find changes related to this. Why do we need to install helm?

Copy link
Member Author

Choose a reason for hiding this comment

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

The helm tests run on GKE because of networking issues in gitlab. Now that we have moved the GKE setup to the before_script, the helm tests run on the same runner as the GKE setup runner, and I thought it made more sense to have everything use the same image rather than have an entire separate logic for just helm.

@ciarams87 ciarams87 merged commit c4a38b1 into master Dec 16, 2021
@ciarams87 ciarams87 deleted the tests/stabilize-tests-round1 branch December 16, 2021 11:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Pull requests that update tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants