kubernetes
kubernetes copied to clipboard
kubernetes
e2e: sub package refactoring
What type of PR is this?
/kind cleanup
What this PR does / why we need it:
The goal of https://github.com/kubernetes/kubernetes/issues/81232 and https://github.com/kubernetes/kubernetes/issues/81245 (duplicates?) was to refactor the framework so that test/e2e/framework is smaller, with less dependency on features and packages that might not be needed. The intention was to then publish it for external consumption.
That is no longer a goal. However, refactoring the framework like that still has benefits:
- cleaner architecture
- API easier to read and discover
That makes it worthwhile to finish the original effort.
Another motivation is that the original effort got stuck halfway through and left the framework in limbo: the actual dependencies became the opposite (framework depends on sub packages) of what they should be (sub packages depend on framework). This made it impossible to add new core functionality that can be used by both core and sub packages. We either would have to revert or finish the work.
Which issue(s) this PR fixes:
Fixes #https://github.com/kubernetes/kubernetes/issues/81245
Special notes for your reviewer:
The approach taken with this PR was to move code incrementally out of core framework. Putting that code into the existing sub packages would have immediately led to import cycles, so instead test/e2e/framework/todo was created with sub packages that mirror the "real" ones. Those "todo" sub packages may depend on the core framework without breaking the compilation.
As a result, go build ./test/e2e/framework/... always passes for each commit. However, other code was not updated, so the full e2e test suite doesn't build (yet).
Once dependencies were as expected, the code under "todo" was moved into the real sub packages in the final commit.
Does this PR introduce a user-facing change?
The test/e2e/framework was refactored so that the core framework is smaller. Optional functionality like resource monitoring, log size monitoring, metrics gathering and debug information dumping must be imported by specific e2e test suites. Init packages are provided which can be imported to re-enable the functionality that traditionally was in the core framework.
/assign @aojea @spiffxp
Aaron is probably the person with more historical context, we'll need to have this context :smile:
I already discussed it with him on Slack: https://kubernetes.slack.com/archives/C09QZ4DQB/p1661356920849319
This is no longer WIP: I went ahead and fixed all imports, plus adapted some paths elsewhere.
However, https://github.com/kubernetes/kubernetes/pull/111998 should get merged first because this PR is based on it.
This PR currently includes changes at the beginning that were needed but have been merged in some slightly different form through different PRs.
Only the commits starting with "e2e framework: support callbacks during framework creation and before…" need to be reviewed.
I can rebase once it is certain that the PR then will be reviewed relatively soon.
(btw thank you for breaking this up into commits the way you did, that really helped make this more digestible)
I know this will get caught in rebase if it hangs out too long.
Yes, I had to rebase again today :sweat_smile: This makes a comparison against the revision that you reviewed harder, too. I changed one commit (e2e framework: move resource gathering into framework/debug) and add three new commits at the end.
I think this is mostly a move in the right direction, but I'm wary of immediately slapping approve on this without hearing feedback from other consumers/maintainers of the e2e framework.
I asked on #sig-testing for reviewers (not just this PR, but in general), but it looks like only @aojea and myself are active at the moment. I am also a consumer of the E2E framework outside of Kubernetes.
@mythi: you are also using the E2E framework in several projects. What do you think about this change?
@aojea: any comments?
Moving all of these functions around will break compilation of downstream test suites. It's worth calling out that the script from the commit message in https://github.com/kubernetes/kubernetes/pull/112043/commits/9c755e01db11a31f40283c221627e8afd9ea505c can be used to automatically fix up also downstream tests.
I was tempted to put that into a script inside the repo, but where, and how long should we keep it there?
I think this is mostly a move in the right direction, but I'm wary of immediately slapping approve on this without hearing feedback from other consumers/maintainers of the e2e framework.
I asked on #sig-testing for reviewers (not just this PR, but in general), but it looks like only @aojea and myself are active at the moment. I am also a consumer of the E2E framework outside of Kubernetes.
@mythi: you are also using the E2E framework in several projects. What do you think about this change?
Yes, we are pulling the k8s e2e framework run our automated device plugins tests. This PR make a lot of sense to me. Quoting the README:
The advantages of splitting the code like this are:
- leaner go doc packages by grouping related functions together
as an external user I often need to look for functionality xyz (e.g., get pod logs, wait for pods) in framework code and I find that cumbersome. This proposal seems to help with that by moving things in more logical places.
- not forcing all E2E suites to import all functionality
I see this as big benefit (although it remains to be seen how much it really helps to reduce the dependencies in our e2e suite).
@aojea: do you agree that this is going in the right direction?
I think @spiffxp wanted to get some additional opinions.
test/e2e/kubectl/kubectl.go:1948:14: undefined: framework.RunKubectlOrDie
test/e2e/kubectl/kubectl.go:1958:24: undefined: framework.RunKubectlOrDie
test/e2e/kubectl/kubectl.go:1965:23: undefined: framework.RunKubectlOrDie
Looks like this will need another rebase
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.
This bot retests PRs for certain kubernetes repos according to the following rules:
- The PR does have any
do-not-merge/*labels - The PR does not have the
needs-ok-to-testlabel - The PR is mergeable (does not have a
needs-rebaselabel) - The PR is approved (has
cncf-cla: yes,lgtm,approvedlabels) - The PR is failing tests required for merge
You can:
- Review the full test history for this PR
- Prevent this bot from retesting with
/lgtm cancelor/hold - Help make our tests less flaky by following our Flaky Tests Guide
/retest
The Kubernetes project has merge-blocking tests that are currently too flaky to consistently pass.
This bot retests PRs for certain kubernetes repos according to the following rules:
- The PR does have any
do-not-merge/*labels - The PR does not have the
needs-ok-to-testlabel - The PR is mergeable (does not have a
needs-rebaselabel) - The PR is approved (has
cncf-cla: yes,lgtm,approvedlabels) - The PR is failing tests required for merge
You can:
- Review the full test history for this PR
- Prevent this bot from retesting with
/lgtm cancelor/hold - Help make our tests less flaky by following our Flaky Tests Guide
/retest
Rebased - quick, someone please LGTM it while it is still fresh :sweat_smile:
[APPROVALNOTIFIER] This PR is APPROVED
This pull-request has been approved by: michelle192837, pohly, spiffxp
The full list of commands accepted by this bot can be found here.
The pull request process is described here
- ~~build/OWNERS~~ [spiffxp]
- ~~test/OWNERS~~ [spiffxp]
Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment
Just to give PR authors a heads up in case this caused a merge-conflict, this is a naive "do they touch the same files" list of open PR's that might need to rename / import a few things (see https://github.com/kubernetes/kubernetes/commit/9c755e01db11a31f40283c221627e8afd9ea505c):
- https://github.com/kubernetes/kubernetes/pull/86139
- https://github.com/kubernetes/kubernetes/pull/98307
- https://github.com/kubernetes/kubernetes/pull/100335
- https://github.com/kubernetes/kubernetes/pull/102025
- https://github.com/kubernetes/kubernetes/pull/107514
- https://github.com/kubernetes/kubernetes/pull/107760
- https://github.com/kubernetes/kubernetes/pull/107958
- https://github.com/kubernetes/kubernetes/pull/109924
- https://github.com/kubernetes/kubernetes/pull/109944
- https://github.com/kubernetes/kubernetes/pull/110159
- https://github.com/kubernetes/kubernetes/pull/110318
- https://github.com/kubernetes/kubernetes/pull/110863
- https://github.com/kubernetes/kubernetes/pull/110991
- https://github.com/kubernetes/kubernetes/pull/110994
- https://github.com/kubernetes/kubernetes/pull/111124
- https://github.com/kubernetes/kubernetes/pull/111184
- https://github.com/kubernetes/kubernetes/pull/111204
- https://github.com/kubernetes/kubernetes/pull/111211
- https://github.com/kubernetes/kubernetes/pull/111552
- https://github.com/kubernetes/kubernetes/pull/111735
- https://github.com/kubernetes/kubernetes/pull/111905
- https://github.com/kubernetes/kubernetes/pull/111930
- https://github.com/kubernetes/kubernetes/pull/111997
- https://github.com/kubernetes/kubernetes/pull/112019
- https://github.com/kubernetes/kubernetes/pull/112255
- https://github.com/kubernetes/kubernetes/pull/112546
I think this is causing https://github.com/kubernetes/kubernetes/issues/112996