-
Notifications
You must be signed in to change notification settings - Fork 110
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
[Node] Add node-e2e tester #103
Conversation
FYI @SergeyKanzhelev |
ping |
/cc @ehashman |
/cc |
@bobbypage: GitHub didn't allow me to request PR reviews from the following users: bobbypage. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. 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. |
What is this? |
kubetest2 replaces https://github.com/kubernetes/test-infra/tree/master/kubetest (deprecated). |
) | ||
|
||
// Name is the name of the deployer | ||
const Name = "noop" |
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.
this should probably be a seperate PR, so folks from SIG node can focus on the e2e_node specific parts.
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.
Ack
gceProjectResourceType = "gce-project" | ||
) | ||
|
||
type Tester struct { |
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.
how do we pass in --image-config-file
? https://github.com/kubernetes/kubernetes/blob/master/test/e2e_node/runner/remote/run_remote.go#L55
Many tests pass in custom image file, e.g. https://sourcegraph.com/search?q=repo:%5Egithub%5C.com/kubernetes/test-infra%24+file:%5Econfig/jobs+--image-config-file%3D&patternType=literal
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.
So this PR isn't the full migration, I anticipate we'll port over specific functionality as and when needed. Infact I'd prefer that so we don't just blindly port over all the flags.
I chose the subset of things used by pull-kubernetes-node-e2e
as a start
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.
Got it, makes sense, would be good clarify that in the PR description that goal here is just pull-kubernetes-node-e2e
for now.
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.
Ack, updated PR description
argsFromFlags := []string{ | ||
"SKIP=" + t.SkipRegex, | ||
"FOCUS=" + t.FocusRegex, | ||
"RUNTIME=" + t.Runtime, |
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'm not sure if just exposing RUNTIME
is enough. For example, take this job: https://github.com/kubernetes/test-infra/blob/master/config/jobs/kubernetes/sig-node/crio.yaml#L52
- '--node-test-args=--container-runtime=remote --container-runtime-endpoint=unix:///var/run/crio/crio.sock --container-runtime-process-name=/usr/local/bin/crio --container-runtime-pid-file= --kubelet-flags="--cgroup-driver=systemd --cgroups-per-qos=true --cgroup-root=/ --runtime-cgroups=/system.slice/crio.service --non-masquerade-cidr=0.0.0.0/0" --extra-log="{\"name\": \"crio.log\", \"journalctl\": [\"-u\", \"crio\"]}"'
I'm not clear how that translates into equivalent args here...
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.
Note that power users can still set the corrsponding ENVs before invoking kubetest2 (until kubetest2 supports them as flags (if at all))
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.
Note that power users can still set the corrsponding ENVs before invoking kubetest2
How does that work? Doesn't the make cmd execution need to inherit the bash env variables, i.e. something like cmd.Env = os.Environ()
?
Additionally, I think the challenge is it's not clear what exact envs you need set because the current jobs using kubetest(1) node e2e runner took in more or less same args as run_remote.go. However since this is based on test-e2e-node.sh
, for all those jobs, you'll need to translate those args to the equivalent envs from test-e2e-node.sh
which isn't super straightforward.
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.
https://golang.org/pkg/os/exec/#Cmd
cmd will inherit all exported envs from parent process by default.
I think the challenge is it's not clear what exact envs you need set
Ack, but I think that is orthogonal to the kubetest2 tester invocation. i.e. they come more from https://github.com/kubernetes/kubernetes/blob/96be00df69390ed41b8ec22facc43bcbb9c88aae/build/root/Makefile#L206-L271
Ideally, we will port over all relevant functionality as flags over time.
I think the main question at hand is whether or not sig-node
thinks make test-e2e-node
is a better common entrypoint for node testing as opposed what we have currently https://github.com/kubernetes/test-infra/blob/4be647a393b6b491a2c0f00145d8bb9f8416b082/kubetest/e2e.go#L556-L611
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.
ack got it.
I think the main question at hand is whether or not sig-node thinks make test-e2e-node is a better common entrypoint for node testing as opposed what we have currently
Yup, this is the main question.
As I mentioned the earlier, the problem I see is if the goal is for us is to migrate all e2e node jobs to kubetest2 eventually this will basically require rewriting all of the job configs since the current kubetest(1) interface doesn't rely on make test-e2e-node
. On other side standardizing on make test-e2e-node
is maybe a good move eventually :) but worth to discuss.
Folks in the sig-node-ci group would be candidates to ask about this.
cc a couple folks to get their thoughts on usage of make test-e2e-node
vs existing kubetest node e2e interface.
/cc @SergeyKanzhelev @ehashman @fromanirh @knabben @harche
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.
+1 for standardizing on make test-e2e-node
@bobbypage: GitHub didn't allow me to request PR reviews from the following users: fromanirh, knabben. Note that only kubernetes-sigs members and repo collaborators can review this PR, and authors cannot review their own PRs. 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. |
FYI kubetest is definitely deprecated, so we would still appreciate eyes on the migration, given node e2e is a bit different from the swaths of cluster e2e jobs SIG Testing tends to be most familiar with. |
/assign @ehashman @SergeyKanzhelev @derekwaynecarr |
/cc will take a look in the next couple of days! |
Ran this:
got this:
There are definitely some rough edges around credentials, ssh keys, etc that we will eventually need to work through. Let's merge and iterate! |
/approve thanks @amwat |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amwat, dims 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 |
@amwat: The following tests 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. |
/lgtm |
Also add a noop deployer to go with it.
Started with
REMOTE=true
only since most testing uses that.fixes: #45
ref:
https://github.com/kubernetes/community/blob/master/contributors/devel/sig-node/e2e-node-tests.md#delete-instance-after-tests-run
ref: https://github.com/kubernetes/kubernetes/blob/96be00df69390ed41b8ec22facc43bcbb9c88aae/build/root/Makefile#L206-L271
ref: https://github.com/kubernetes/kubernetes/blob/master/hack/make-rules/test-e2e-node.sh
xref: kubernetes/enhancements#2464
Currently fully relies on
make test-e2e-node
which itself does a build https://github.com/kubernetes/kubernetes/blob/cea1d4e20b4a7886d8ff65f34c6d4f95efcb4742/test/e2e_node/builder/build.go,might be possible to breakout the build part separately into a dedicated node deployer in the future.
Also sadly, local runs don't stream the test logs everything is dumped when the tests finish
tested locally with
started with porting over functionality needed by
pull-kubernetes-node-e2e
/cc @BenTheElder @spiffxp @dims