WIP: Enable import-boss for e2e framework dependency #81246

oomichi · 2019-08-09T23:09:31Z

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

Core e2e test framework(test/e2e/framework) should not import sub e2e test frameworks (e.g. test/e2e/framework/auth).
This enables import-boss check for blocking such invalid dependency on e2e test framework.

Ref: #81245

Does this PR introduce a user-facing change?:

NONE

oomichi · 2019-08-09T23:09:44Z

/sig testing

oomichi · 2019-08-10T00:11:50Z

The errors are outputs as we expected:

Errors:
test/e2e/framework/create.go
test/e2e/framework/networking_utils.go
test/e2e/framework/perf/perf.go
test/e2e/framework/suites.go
test/e2e/framework/pods.go
test/e2e/framework/test_context.go
test/e2e/framework/psp/psp.go
test/e2e/framework/size.go
test/e2e/framework/google_compute.go
test/e2e/framework/get-kubemark-resource-usage.go
test/e2e/framework/resource_usage_gatherer.go
test/e2e/framework/rc_util.go
test/e2e/framework/log_size_monitoring.go
test/e2e/framework/util.go
test/e2e/framework/exec_util.go
test/e2e/framework/framework.go
test/e2e/framework/profile_gatherer.go
test/e2e/framework/pv_util.go
test/e2e/framework/flake_reporting_util.go
test/e2e/framework/nodes_util.go

The above files which are core e2e framework should not import sub e2e frameworks
(e.g. k8s.io/kubernetes/test/e2e/framework/foo)

oomichi · 2019-08-15T22:26:39Z

/area e2e-test-framework

alejandrox1

Overall +1
There is some more work to be done for #76206 and #77095 and due to the gigantic universe of imports it there may be new framework subpkgs and those may be imported by the framework itself (not the ideal case but it takes a lot of work to rewrite the framework to not use its subpkgs).
Would it be reasonable to update hack/.e2e_framework_import_failures with these new packages as they come up, clean them up (make sure the framework doesn't import these), and then remove the reference to them from hack/.e2e_framework_import_failures ?

/priority important-soon
/assign @timothysc @andrewsykim

spiffxp · 2019-08-20T21:38:40Z

Can we not use our existing ./hack/verify-import-boss.sh and/or ./hack/verify-imports.sh tools to enforce this? ref: #74352 (comment)

spiffxp · 2019-08-20T21:38:57Z

/hold

alejandrox1 · 2019-08-20T22:02:33Z

Can we not use our existing ./hack/verify-import-boss.sh and/or ./hack/verify-imports.sh tools to enforce this?

Ha forgot about #80496
relies on ./hack/verify-import-boss.sh

oomichi · 2019-08-20T22:21:32Z

@spiffxp @alejandrox1 Thanks for your advice.
I will update this after some investigation to see which is the best way for us.

Core e2e test framework(test/e2e/framework) should not import sub e2e test frameworks (e.g. test/e2e/framework/auth) for avoiding circular dependency. This adds e2e code check for avoiding such circular dependency.

k8s-ci-robot · 2019-08-20T23:28:30Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: oomichi

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:

~~test/e2e/framework/OWNERS~~ [oomichi]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

oomichi · 2019-08-20T23:28:49Z

I am adding some rule to test/e2e/framework/.import-restrictions for managing the rule in testing-common.

oomichi · 2019-08-21T01:07:01Z

I guessed this PR should fail on some gating job because current implementation is against the additional rule I expected, but it doesn't happen.
I need more investigation how to implement the rule.

alejandrox1 · 2019-08-21T15:30:54Z

I guessed this PR should fail on some gating job because current implementation is against the additional rule I expected, but it doesn't happen.
I need more investigation how to implement the rule.

Ha! I think I discovered an issue...
All the .import-restrictions files throughout k/k are "used" by ./hack/verify-import-boss.sh:

make -C "${KUBE_ROOT}" WHAT=vendor/k8s.io/code-generator/cmd/import-boss

$(kube::util::find-binary "import-boss") --verify-only

import-boss is a small script that uses the .import-restrictions files to validate directories. The comments within that script provide all existing documentation on how to use this sort of tooling, as far as I know.

Cming back to this, I don't think that /test/e2e/framework/.import-restrictions is "working". It seems that we have to explicitly tell import-boss to go into /test/e2e/framework by changing
https://github.com/kubernetes/kubernetes/blob/master/staging/src/k8s.io/code-generator/cmd/import-boss/main.go#L79-L83 into something like

arguments.InputDirs = []string{                                             
    "k8s.io/kubernetes/pkg/...",                                            
    "k8s.io/kubernetes/cmd/...",                                            
    "k8s.io/kubernetes/plugin/...",                                         
    "k8s.io/kubernetes/test/e2e/framework/...",                             
}
```.

oomichi · 2019-08-21T17:32:11Z

@alejandrox1

Great help, thanks!
I've added the line to staging/src/k8s.io/code-generator/cmd/import-boss/main.go as you said, and ./hack/verify-import-boss.sh outputs error like the following on my local env:

$ ./hack/verify-import-boss.sh

...

errors in package "k8s.io/kubernetes/test/e2e/framework/service":
import k8s.io/kubernetes/test/e2e/framework/kubelet has forbidden prefix k8s.io/kubernetes/test/e2e/framework/kubelet
import k8s.io/kubernetes/test/e2e/framework/psp has forbidden prefix k8s.io/kubernetes/test/e2e/framework/psp
import k8s.io/kubernetes/test/e2e/framework/auth has forbidden prefix k8s.io/kubernetes/test/e2e/framework/auth
import k8s.io/kubernetes/test/e2e/framework/ssh has forbidden prefix k8s.io/kubernetes/test/e2e/framework/ssh
import k8s.io/kubernetes/test/e2e/framework/testfiles has forbidden prefix k8s.io/kubernetes/test/e2e/framework/testfiles
import k8s.io/kubernetes/test/e2e/framework/metrics has forbidden prefix k8s.io/kubernetes/test/e2e/framework/metrics
import k8s.io/kubernetes/test/e2e/framework/pod has forbidden prefix k8s.io/kubernetes/test/e2e/framework/pod
import k8s.io/kubernetes/test/e2e/framework/resource has forbidden prefix k8s.io/kubernetes/test/e2e/framework/resource
import k8s.io/kubernetes/test/e2e/framework/node has forbidden prefix k8s.io/kubernetes/test/e2e/framework/node
import k8s.io/kubernetes/test/e2e/framework/ginkgowrapper has forbidden prefix k8s.io/kubernetes/test/e2e/framework/ginkgowrapper
import k8s.io/kubernetes/test/e2e/framework/log has forbidden prefix k8s.io/kubernetes/test/e2e/framework/log
import k8s.io/kubernetes/test/e2e/framework/config has forbidden prefix k8s.io/kubernetes/test/e2e/framework/config
the following imports did not match any allowed prefix:
  k8s.io/kubernetes/pkg/version
  k8s.io/kubernetes/test/e2e/framework
  k8s.io/kubernetes/test/e2e/framework/auth
  k8s.io/kubernetes/test/e2e/framework/config
  k8s.io/kubernetes/test/e2e/framework/config
  k8s.io/kubernetes/test/e2e/framework/ginkgowrapper
  k8s.io/kubernetes/test/e2e/framework/kubelet
  k8s.io/kubernetes/test/e2e/framework/kubelet
  k8s.io/kubernetes/test/e2e/framework/log
  k8s.io/kubernetes/test/e2e/framework/metrics
  k8s.io/kubernetes/test/e2e/framework/node
  k8s.io/kubernetes/test/e2e/framework/pod
  k8s.io/kubernetes/test/e2e/framework/psp
  k8s.io/kubernetes/test/e2e/framework/psp
  k8s.io/kubernetes/test/e2e/framework/resource
  k8s.io/kubernetes/test/e2e/framework/ssh
  k8s.io/kubernetes/test/e2e/framework/testfiles
  k8s.io/utils/io


!!! Error in ./hack/verify-import-boss.sh:28
  Error in ./hack/verify-import-boss.sh:28. '$(kube::util::find-binary "import-boss") --verify-only' exited with status 1
Call stack:
  1: ./hack/verify-import-boss.sh:28 main(...)
Exiting with status 1

Yep, we need to add the line.

timothysc · 2019-09-03T16:31:05Z

test/e2e/framework/.import-restrictions

+			"SelectorRegexp": "k8s[.]io/kubernetes/test/e2e/framework/",
+			"AllowedPrefixes": [],
+			"ForbiddenPrefixes": [
+				"k8s.io/kubernetes/test/e2e/framework/auth",


Do we need this? Wouldn't it by definition cause an import cycle?
All subpackages would import framework.

Sorry for my late response.

and you are right, all subpackages should be able to import the framework.
After understanding this verify-import-boss check way, I saw the above proposed change was invalid.
I will try to find the valid way to block circular import with this.

alejandrox1 · 2019-09-29T19:21:30Z

update on this: I'll work on fixing the current import restrictions on the e2e framework (ref #81246 (comment) ) so we can get on with this.
/milestone v1.17

josiahbjorgaard · 2019-10-26T08:38:08Z

Bug triage for 1.17 here. Code freeze is Nov. 14th. Will this PR be merged by then? If not, can we move it out of the milestone?

alejandrox1 · 2019-10-26T13:21:16Z

/milestone clear

timothysc · 2019-10-28T17:33:11Z

/assign @alejandrox1

oomichi · 2020-01-15T22:35:49Z

Sorry for leaving this in long time, I'd like to restart fixing this again because this is useful for maintaining e2e framework in long-term.
At first step, it is necessary to enable verify-import-boss check for e2e framework before this.
I created #87264 for managing it.

oomichi · 2020-03-20T03:57:21Z

/hold cancel

k8s-ci-robot added sig/testing Categorizes an issue or PR as relevant to SIG Testing. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Aug 9, 2019

k8s-ci-robot requested review from spiffxp and zmerlynn August 9, 2019 23:11

k8s-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 14, 2019

oomichi force-pushed the add-import-check-4-e2e-framework branch from 017cf9d to 3fafa10 Compare August 15, 2019 21:42

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Aug 15, 2019

k8s-ci-robot added the area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework label Aug 15, 2019

alejandrox1 reviewed Aug 20, 2019

View reviewed changes

k8s-ci-robot assigned andrewsykim and timothysc Aug 20, 2019

k8s-ci-robot added priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. and removed needs-priority Indicates a PR lacks a `priority/foo` label and requires one. labels Aug 20, 2019

k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Aug 20, 2019

oomichi changed the title ~~Check e2e code for avoiding circular dependency~~ WIP: Check e2e code for avoiding circular dependency Aug 20, 2019

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 20, 2019

Check e2e code for avoiding circular dependency

e82a0a0

Core e2e test framework(test/e2e/framework) should not import sub e2e test frameworks (e.g. test/e2e/framework/auth) for avoiding circular dependency. This adds e2e code check for avoiding such circular dependency.

oomichi force-pushed the add-import-check-4-e2e-framework branch from 3fafa10 to e82a0a0 Compare August 20, 2019 23:27

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 20, 2019

k8s-ci-robot added the area/test label Aug 20, 2019

alejandrox1 mentioned this pull request Aug 22, 2019

[e2e] e2e framework import-restrictions need to be fixed #81803

Closed

timothysc reviewed Sep 3, 2019

View reviewed changes

k8s-ci-robot added this to the v1.17 milestone Sep 29, 2019

k8s-ci-robot removed this from the v1.17 milestone Oct 26, 2019

k8s-ci-robot assigned alejandrox1 Oct 28, 2019

oomichi mentioned this pull request Jan 15, 2020

Enable verify-import-boss check for e2e framework #87264

Closed

k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 20, 2020

oomichi changed the title ~~WIP: Check e2e code for avoiding circular dependency~~ Enable import-boss for e2e framework dependency Mar 20, 2020

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 20, 2020

oomichi changed the title ~~Enable import-boss for e2e framework dependency~~ WIP: Enable import-boss for e2e framework dependency Mar 20, 2020

k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 20, 2020

oomichi closed this May 7, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Enable import-boss for e2e framework dependency #81246

WIP: Enable import-boss for e2e framework dependency #81246

oomichi commented Aug 9, 2019 •

edited

oomichi commented Aug 9, 2019

oomichi commented Aug 10, 2019

oomichi commented Aug 15, 2019

alejandrox1 left a comment

spiffxp commented Aug 20, 2019

spiffxp commented Aug 20, 2019

alejandrox1 commented Aug 20, 2019 •

edited

oomichi commented Aug 20, 2019

k8s-ci-robot commented Aug 20, 2019

oomichi commented Aug 20, 2019

oomichi commented Aug 21, 2019

alejandrox1 commented Aug 21, 2019

oomichi commented Aug 21, 2019

timothysc Sep 3, 2019

oomichi Jan 23, 2020

alejandrox1 commented Sep 29, 2019

josiahbjorgaard commented Oct 26, 2019 •

edited

alejandrox1 commented Oct 26, 2019

timothysc commented Oct 28, 2019

oomichi commented Jan 15, 2020

oomichi commented Mar 20, 2020

WIP: Enable import-boss for e2e framework dependency #81246

WIP: Enable import-boss for e2e framework dependency #81246

Conversation

oomichi commented Aug 9, 2019 • edited

oomichi commented Aug 9, 2019

oomichi commented Aug 10, 2019

oomichi commented Aug 15, 2019

alejandrox1 left a comment

Choose a reason for hiding this comment

spiffxp commented Aug 20, 2019

spiffxp commented Aug 20, 2019

alejandrox1 commented Aug 20, 2019 • edited

oomichi commented Aug 20, 2019

k8s-ci-robot commented Aug 20, 2019

oomichi commented Aug 20, 2019

oomichi commented Aug 21, 2019

alejandrox1 commented Aug 21, 2019

oomichi commented Aug 21, 2019

timothysc Sep 3, 2019

Choose a reason for hiding this comment

oomichi Jan 23, 2020

Choose a reason for hiding this comment

alejandrox1 commented Sep 29, 2019

josiahbjorgaard commented Oct 26, 2019 • edited

alejandrox1 commented Oct 26, 2019

timothysc commented Oct 28, 2019

oomichi commented Jan 15, 2020

oomichi commented Mar 20, 2020

oomichi commented Aug 9, 2019 •

edited

alejandrox1 commented Aug 20, 2019 •

edited

josiahbjorgaard commented Oct 26, 2019 •

edited