-
Notifications
You must be signed in to change notification settings - Fork 807
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
Update hack/run-e2e-test to be more idempotent and pleasant to use #616
Conversation
/test migration-test-latest not sure if this works |
@wongma7: The specified target(s) for
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/test pull-aws-ebs-csi-driver-migration-test-latest |
8595420
to
6b396cf
Compare
/test pull-aws-ebs-csi-driver-migration-test-latest |
|
Pull Request Test Coverage Report for Build 1314
💛 - Coveralls |
6b396cf
to
81546db
Compare
invalid mode:
|
/test pull-aws-ebs-csi-driver-migration-test-latest |
|
Nov 23 21:01:36 ip-172-20-49-41 kubelet[5351]: I1123 21:01:36.141788 5351 operation_generator.go:1332] Controller attach succeeded for volume "ebs.csi.aws.com-aws://us-west-2a/vol-0a4910df43136ea3e" (UniqueName: "kubernetes.io/csi/ebs.csi.aws.com^vol-0a4910df43136ea3e") pod "exec-volume-test-aws-8r7l" (UID: "017390c5-0c21-404b-a850-228c4311dd36") device path: "" |
inline path looks wonky.
VS
|
I think the colon is messing with docker. |
I am pretty sure it is the colon, I will do more testing and see if a patch in kubelet fixes it. In-tree is not affected because instead of putting spec.volumeHandle/volumeID they use spec.volumeName which I think will never have a colon in it |
translation should prevent this
|
csi should be using spec.name too hmmm https://github.com/kubernetes/kubernetes/blob/6d5cb36d36f34cb4f5735b6adcd5ea8ebb4440ba/pkg/volume/csi/csi_plugin.go#L430 |
OK, volumeID is not translated and used as the PV name which is then used as mounter's spec.Name, that is how colon slips through. will fix in translation lib. |
81546db
to
cc0305f
Compare
@wongma7: The following test failed, say
Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here. |
KOPS_DOWNLOAD_URL=https://github.com/kubernetes/kops/releases/download/v${KOPS_VERSION}/kops-${OS_ARCH} | ||
curl -L -X GET "${KOPS_DOWNLOAD_URL}" -o "${INSTALL_PATH}"/kops | ||
chmod +x "${INSTALL_PATH}"/kops | ||
fi |
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.
Not critical, but it would be helpful to log something in the else such as kops is already installed, not reinstalling.
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.
the curl is not silent, so in a roundabout way it is still possible to tell if the install is being skipped. But yeah, the more logging the better.
|
||
KUBECONFIG=${KUBECONFIG:-"${HOME}/.kube/config"} | ||
ARTIFACTS=${ARTIFACTS:-"${TEST_DIR}/artifacts"} | ||
GINKGO_FOCUS=${GINKGO_FOCUS:-"\[ebs-csi-migration\]"} |
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.
Just to verify, we're changing from ebs-csi-e2e
to ebs-csi-migration
?
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.
yeah, just for the default. The non-migration tests currently go through the tester binary instead of this script anyway. (the theory behind the tester binary is that we can update it once to update all 3 aws csi drivers, so we don't need to maintain 3 almost-identical kops "run-e2e-test" scripts in all 3 repos, but in practice we need to do that anyway since the binary configuratoin yaml is effectively a script: https://github.com/kubernetes-sigs/aws-ebs-csi-driver/blob/master/tester/single-az-config.yaml)
/lgtm |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ayberk, wongma7 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 |
@ayberk: changing LGTM is restricted to collaborators In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
/retest cluster create failed ...but it is not using this script so it's not related |
/lgtm |
Is this a bug fix or adding new feature?
What is this PR about? / Why do we need it? Used this PR to debug migration, it is still broken pending kubernetes/kubernetes#96821 but there are various fixes I made to the script along the way .
What testing is done?
TEST_ID=18512 CLEAN=false KOPS_STATE_FILE=s3://aws-ebs-csi-driver-e2e AWS_REGION=us-west-2 AWS_AVAILABILITY_ZONES=us-west-2a GINKGO_FOCUS=Dynamic.*xfs.*should.store.data GINKGO_NODES=1 ./hack/run-e2e-test