-
Notifications
You must be signed in to change notification settings - Fork 195
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
Conversation
/assign @BenTheElder |
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. |
|
/test pull-cloud-provider-gcp-e2e |
|
/test pull-cloud-provider-gcp-e2e |
5 similar comments
/test pull-cloud-provider-gcp-e2e |
/test pull-cloud-provider-gcp-e2e |
/test pull-cloud-provider-gcp-e2e |
/test pull-cloud-provider-gcp-e2e |
/test pull-cloud-provider-gcp-e2e |
it went through 👏 |
tools/run-e2e-test.sh
Outdated
if [[ ! -z "${PROJECT}" ]]; then | ||
echo "Cleaning up boskos..." | ||
cleanup_boskos | ||
echo | ||
fi | ||
|
||
echo "Cleaning up run dir: ${RUN_DIR}" | ||
rm -rf ${RUN_DIR} |
There was a problem hiding this comment.
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}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
d46c6da
to
55e3a96
Compare
/test pull-cloud-provider-gcp-e2e |
|
||
# Install bazel | ||
# | ||
BAZEL_VERSION="5.4.0" |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this 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]
test/e2e/suite_test.go
Outdated
|
||
func TestE2E(t *testing.T) { | ||
gomega.RegisterFailHandler(framework.Fail) | ||
repoDir := framework.TestContext.ReportDir |
There was a problem hiding this comment.
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]
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.
There was a problem hiding this comment.
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" |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
[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 |
55e3a96
to
76d177f
Compare
/test pull-cloud-provider-gcp-e2e |
/triage accepted |
/hold cancel |
/lgtm |
gce
e2e tests to out-of-treecloud-provider-gcp
repository.[sig-network] Firewall rule control plane should not expose well-known ports
[sig-network] LoadBalancers should be able to change the type and ports of a UDP service [Slow]
gce
provider code for the e2e tests.tools/run-e2e-test.sh
to run the e2e tests from the command line or in prow CI.e2e.test
binary by runninggo test -c
within thetest/e2e
directory.gce
provider and theginkgo
tester.Current Status
ginkgo
tester with the following output: