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

Initial e2e test migration from in-tree to gcp cloud provider #683

Merged
merged 1 commit into from
May 21, 2024

Conversation

seans3
Copy link
Contributor

@seans3 seans3 commented May 6, 2024

  • Initial migration of in-tree gce e2e tests to out-of-tree cloud-provider-gcp repository.
  • Creates e2e test infrastructure, validating e2e tests by running them from the command line or in prow CI.
  • Currently only migrated two network tests
      1. A simple firewall test [sig-network] Firewall rule control plane should not expose well-known ports
      1. A load-balancer test [sig-network] LoadBalancers should be able to change the type and ports of a UDP service [Slow]
  • More tests will be migrated in future PR's.
  • Migrate some gce provider code for the e2e tests.
  • Includes script tools/run-e2e-test.sh to run the e2e tests from the command line or in prow CI.
    • Builds the e2e.test binary by running go test -c within the test/e2e directory.
    • Runs the e2e test using the gce provider and the ginkgo tester.

Current Status

  • Sucessfully runs the e2e tests using the ginkgo tester with the following output:
Running Suite: Cloud Provider GCP End-to-End Tests - /home/sean/go/src/k8s.io/cloud-provider-gcp/_rundir/sean-test
==================================================================================================================
Random Seed: 1715403389 - will randomize all specs

Will run 2 of 2 specs
Running in parallel across 30 processes
••

Ran 2 of 2 Specs in 186.584 seconds
SUCCESS! -- 2 Passed | 0 Failed | 0 Pending | 0 Skipped

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 6, 2024
@seans3
Copy link
Contributor Author

seans3 commented May 6, 2024

/assign @BenTheElder
/assign @aojea

@BenTheElder
Copy link
Member

First pass: this is generally looking good, we'll want to stand up a job to actually run this in CI (we can make that manually run at first) and sort out what to do re: Bazel.

@seans3 seans3 changed the title [WIP] Initial e2e test migration from in-tree to gcp cloud provider Initial e2e test migration from in-tree to gcp cloud provider May 11, 2024
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 11, 2024
tools/run-e2e-test.sh Outdated Show resolved Hide resolved
@aojea
Copy link
Member

aojea commented May 13, 2024

First pass: this is generally looking good, we'll want to stand up a job to actually run this in CI (we can make that manually run at first) and sort out what to do re: Bazel.

yeah kubernetes/test-infra#32603

@aojea
Copy link
Member

aojea commented May 13, 2024

/test pull-cloud-provider-gcp-e2e

@aojea
Copy link
Member

aojea commented May 13, 2024

/home/prow/go/src/cloud-provider-gcp
cp: cannot stat '/root/go/bin/ginkgo': No such file or directory
+ EXIT_VALUE=1
+ set +o xtrace
Cleaning up after docker in docker.

@seans3
Copy link
Contributor Author

seans3 commented May 18, 2024

/test pull-cloud-provider-gcp-e2e

5 similar comments
@seans3
Copy link
Contributor Author

seans3 commented May 18, 2024

/test pull-cloud-provider-gcp-e2e

@seans3
Copy link
Contributor Author

seans3 commented May 18, 2024

/test pull-cloud-provider-gcp-e2e

@seans3
Copy link
Contributor Author

seans3 commented May 18, 2024

/test pull-cloud-provider-gcp-e2e

@seans3
Copy link
Contributor Author

seans3 commented May 21, 2024

/test pull-cloud-provider-gcp-e2e

@seans3
Copy link
Contributor Author

seans3 commented May 21, 2024

/test pull-cloud-provider-gcp-e2e

@aojea
Copy link
Member

aojea commented May 21, 2024

it went through 👏

Comment on lines 163 to 170
if [[ ! -z "${PROJECT}" ]]; then
echo "Cleaning up boskos..."
cleanup_boskos
echo
fi

echo "Cleaning up run dir: ${RUN_DIR}"
rm -rf ${RUN_DIR}
Copy link
Member

Choose a reason for hiding this comment

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

todo

trap cleanup INT TERM EXIT 
cleanup() {
if [[ ! -z "${PROJECT}" ]]; then
    echo "Cleaning up boskos..."
    cleanup_boskos
    echo
fi

echo "Cleaning up run dir: ${RUN_DIR}"
rm -rf ${RUN_DIR}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

@seans3
Copy link
Contributor Author

seans3 commented May 21, 2024

/test pull-cloud-provider-gcp-e2e


# Install bazel
#
BAZEL_VERSION="5.4.0"
Copy link
Member

Choose a reason for hiding this comment

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

TODO: setup .bazel-version in this repo (and/or remove bazel) and respect that here (it's like GOTOOLCHAIN)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed.

Copy link
Member

@BenTheElder BenTheElder left a comment

Choose a reason for hiding this comment

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

let's merge and iterate, I don't have any major blocking comments and we want to start letting people collaborate on expanding the migrated tests soon

/lgtm
/approve
/hold
[we'll either do some quick nits or /hold cancel shortly]


func TestE2E(t *testing.T) {
gomega.RegisterFailHandler(framework.Fail)
repoDir := framework.TestContext.ReportDir
Copy link
Member

Choose a reason for hiding this comment

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

[non-blocking nit, could be a follow-up]

Suggested change
repoDir := framework.TestContext.ReportDir
reportDir := framework.TestContext.ReportDir

there are a lot of other places in testing where we get something like KUBE_ROOT or REPO_DIR that is the root of the source directory, so scanning this is a little confusing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

@@ -253,6 +253,7 @@ function copy-to-staging() {
fi
fi

rm -f "${tar}.sha512"
Copy link
Member

Choose a reason for hiding this comment

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

we shouldn't need to do this? > below should already overwrite, >> would be append? what am I missing?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I ran into an issue when running the e2e tests locally. If the file already exists from a previous run, it is sometimes created without write permissions. So the following command fails: echo "${hash}" > "${tar}.sha512" . I think it had to do with my umask. I can remove this if you want.

Copy link
Member

Choose a reason for hiding this comment

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

ooh, we should defend against umask issues when manipulating tarballs etc.

I'm not sure what the best answer is here. in some other places where we're containerized we've explicitly set the permission bits.

TODO: follow-up on umask fun and these scripts

@@ -1,5 +1,24 @@
#!/usr/bin/env bash

# Copyright 2024 The Kubernetes Authors.
Copy link
Member

Choose a reason for hiding this comment

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

we should be able to leave this whole thing to the kubetest2 deployer? but let's leave that to a follow-up

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I had problems leaving it up to kubetest2 to create the boskos project, because I needed to pass the project name on to the ginkgo-tester test args.

# - Packages e2e test binary with "ginkgo" and "kubectl" binaries.
# - Starts a cluster with kubetest2, using the "gce" deployer.
# - Cluster started in either passed cluster (GCP_CLUSTER env var),
# or "boskos" cluster in prow CI.
Copy link
Member

Choose a reason for hiding this comment

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

TODO: revisit this versus letting kubetest2 boskos client handle this from go?

Copy link
Contributor Author

@seans3 seans3 May 21, 2024

Choose a reason for hiding this comment

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

I don't think that works. We need to pass the GCE_PROJECT into the ginkgo-tester test args. But if we let kubetest2 create the boskos project, there doesn't seem to be a way to get the project name and then pass it into the test args. Please let me know if there is a way to do this.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think that works. We need to pass the GCE_PROJECT into the ginkgo-tester test args. But if we let kubetest2 create the boskos project, there doesn't seem to be a way to get the project name and then pass it into the test args. Please let me know if there is a way to do this.

IMHO the deployer should either set it as the current project (in gcloud?) and we depend on that in the test as the default or we should modify kubetest2 to support passing through information like this.

We could maybe smuggle it in the kubeconfig.

something TODO later, this is super unweildy and we should find a better answer.

@k8s-ci-robot k8s-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm "Looks good to me", indicates that a PR is ready to be merged. labels May 21, 2024
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: BenTheElder, seans3

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 21, 2024
@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2024
@seans3
Copy link
Contributor Author

seans3 commented May 21, 2024

/test pull-cloud-provider-gcp-e2e

@seans3
Copy link
Contributor Author

seans3 commented May 21, 2024

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 21, 2024
@seans3
Copy link
Contributor Author

seans3 commented May 21, 2024

/hold cancel

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 21, 2024
@BenTheElder
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2024
@k8s-ci-robot k8s-ci-robot merged commit ab93615 into kubernetes:master May 21, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants