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

Enable in-tree gcepd driver for e2e tests #26890

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

mattcary
Copy link
Contributor

See kubernetes/kubernetes#109541. Since cloud-provider-gcp installs the pd csi driver, it's safe to enable this now.

Tested manually, the best I could and it seemed to pass.

/assign @leiyiz

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. area/config Issues or PRs related to code in /config area/jobs area/provider/gcp Issues or PRs related to gcp provider sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jul 21, 2022
@leiyiz
Copy link
Contributor

leiyiz commented Jul 21, 2022

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 21, 2022
Copy link
Contributor

@leiyiz leiyiz left a comment

Choose a reason for hiding this comment

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

/assign @mikedanese

@jprzychodzen
Copy link
Contributor

Probably you want to add this to a much broader set of tests - presubmit and another periodic. In this case you probably should use preset. However, I have a strong feeling that this is a no-op change.

https://github.com/kubernetes-sigs/kubetest2/blob/master/kubetest2-gce/deployer/common.go#L89 does not pass environment variables as it's unsupported in kubetest2.

See #26760

@jprzychodzen
Copy link
Contributor

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jul 22, 2022
@mattcary
Copy link
Contributor Author

@jprzychodzen note these are tests that up until the beginning of the year ran with this e2e tests, and were disabled due to csi migration. So I'm not sure another periodic is necessary?

At any rate, the thing about environment variables and kubetest2 is annoying. I don't fully understand it, but I'll take your word for it. As you can see from kubernetes/kubernetes#109541 the test setup depends on environment variables being passed in.

Maybe we should fix kubetest2? Or change e2e.test to detect if the pd csi driver is installed.

@jprzychodzen
Copy link
Contributor

Ah, I see now - I was thinking that is passed to kubetest2 deployer part - gce deployer. This part does not support env variables. I don't know about ginko framework part. It might already support env variables so this change would do what you want.

In this case - could you create a preset with this env, add this preset to all e2e tests - there is presubmit that should have this env variable.

Regarding changing kubetest2 gce deployer - it seems like some kind of explicit decision from the comment, so I guess it would be to understand why it's this way before changing the code.

@mattcary
Copy link
Contributor Author

Hmm, looking at some internal GKE tests, it appears that indeed the env variable doesn't get plumbed through (we stop testing gcepd at 1.24 which is where kubernetes/kubernetes#109541 was backported to IIRC.

So I think we do need a command line flag or a test config change?

@mattcary
Copy link
Contributor Author

I've raised kubernetes-sigs/kubetest2#202 to start discussion and will work on adding a flag in the meantime.

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jul 29, 2022
@mattcary
Copy link
Contributor Author

I've updated this PR for the new flag, but it will need to be backported to 1.24 before it will be usable from the prow job (and wait for a patch release to come out? the prow job uses test version 1.24.2 explicitly, so we'll have to update the patch release as well I think)

@jprzychodzen
Copy link
Contributor

Sorry for the long time to respond, pre freeze time was intensive.

Probably the most reasonable approach forward with this flag is to migrate tests (and repository) to 1.25/master branch, which should happen ~soon, as a part of preparation to 1.25 release.

@mattcary
Copy link
Contributor Author

mattcary commented Aug 3, 2022

Makes sense. Are you planning on continuing to pin to a patch release, track release-1.25, or just track master?

@jprzychodzen
Copy link
Contributor

Short term - pin it to 1.25.

Medium/long term - I need to rework repository bumping/tracking, and this will be part of the problem to solve.

@mattcary
Copy link
Contributor Author

mattcary commented Aug 3, 2022

sg, thx

@k8s-ci-robot k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 1, 2022
@spiffxp
Copy link
Member

spiffxp commented Oct 7, 2022

/cc

@jprzychodzen
Copy link
Contributor

@mattcary can we close this PR?

Right now test jobs are using ginko version based on cloud-provider-gcp file (which now points to 1.25.3).

@mattcary
Copy link
Contributor Author

mattcary commented Nov 2, 2022

If we're now pinned to 1.25, I think we can submit this (when it was on 1.24, the flag would be ignored IIRC).

Let me rebase and test it again.

@jprzychodzen
Copy link
Contributor

In this case please first add this flag in a fork of current presubmit to prevent breaking presubmit (see #27618 and #27823 for a process)

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Nov 2, 2022
@mattcary
Copy link
Contributor Author

mattcary commented Nov 2, 2022

The recent push has forked out the jobs as you requested.

Although, I'm not able to test the kubetest2 commands any longer. I can add a --gcp-project flag to avoid boskos, but it fails trying to make release-tars (there's no release-tars rule). Maybe I have a bad combination of kubetest2 and k8s versions? I've tried kubetest2 at head and k8s at head and release-1.25, but no luck.

Anyway I guess since I've forked the presubmits it will probably be faster to submit this and let them run, than figure out kubetest :-/

@jprzychodzen
Copy link
Contributor

Thanks!

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 3, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jprzychodzen, leiyiz, mattcary

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 Nov 3, 2022
@mattcary
Copy link
Contributor Author

mattcary commented Nov 3, 2022

/remove hold

@jprzychodzen
Copy link
Contributor

/unhold

@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 Nov 4, 2022
@k8s-ci-robot k8s-ci-robot merged commit 03233a9 into kubernetes:master Nov 4, 2022
@k8s-ci-robot
Copy link
Contributor

@mattcary: Updated the job-config configmap in namespace default at cluster test-infra-trusted using the following files:

  • key cloud-provider-gcp-periodics.yaml using file config/jobs/kubernetes/cloud-provider-gcp/cloud-provider-gcp-periodics.yaml

In response to this:

See kubernetes/kubernetes#109541. Since cloud-provider-gcp installs the pd csi driver, it's safe to enable this now.

Tested manually, the best I could and it seemed to pass.

/assign @leiyiz

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.

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. area/config Issues or PRs related to code in /config area/jobs area/provider/gcp Issues or PRs related to gcp provider 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. sig/cloud-provider Categorizes an issue or PR as relevant to SIG Cloud Provider. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants