Break up and Refactor test/e2e/framework/util.go

[andrewsykim]

What would you like to be added:
There are a lot of methods in test/e2e/framework/util.go that can be cleaned up or moved into it's own package for easier consumption/discovery. In #76206 there were some discussions for how to break up this file. The proposed package structure was:

test/e2e/framework/

• kubectl/ (@MorrisLaw)
  - builder.go
  - runner.go
• log/ (@draveness)
  - logger.go
• pod/ (@alejandrox1)
  - wait.go
  - resource.go
• node/ (@jiatongw)
  - wait.go
  - resource.go
• service/ (@draveness)
  - resource.go
• endpoints/ (@draveness)
  - resource.go
• resource/ (@alejandrox1)
  - runtime.go
• lifecycle/ (@jiatongw)
  - versions.go
  - state.go
  - restart
• wrappers.go (@MorrisLaw)
• providers.go (@alejandrox1)

I think each package & top level file can be it's own task. Pick a package above, go through each method in test/e2e/framework/util.go and move it to that package/file if it makes sense. The above is just a guideline, feel free to introduce new packages/files where it makes sense.

Why is this needed:
This is a part of #76206 & #75601

[andrewsykim]

/sig testing
/area e2e-framework

[andrewsykim]

/area e2e-test-framework

[johnSchnake]

There are also ssh utilities that should probably move into their own package; I'll take that one as I think it is also a reasonable step in #69373 which is assigned to me (reducing reliance on ssh in tests). Step 1, figure out where it really bubbles up to, step 2, do something about it.

[draveness]

I'd like to take the files as below :)

service/*
endpoints/*
resource/*

[alejandrox1]

I’d like to take kubectl/, node/, and pod/* :)

[WanLinghao]

I would like to take cluster and log

[WanLinghao]

Hi, all. When I try handle cluster thing, I found there are many functions containing k8s-component. Like

CheckNodesReady
RestartControllerManager
WaitForApiserverUp

Should we combine node/ and cluster/, maybe something like:

• cluster/
  - kubelet.go
  - kube-proxy.go
  - controller-manager.go
  - api-server.go

What do you think?

[draveness]

Hi, all. When I try handle cluster thing, I found there are many functions containing k8s-component. Like

Maybe we could leave these for now and see what we could do after the major part of this is done.

[andrewsykim]

Should we combine node/ and cluster/, maybe something like:

test/e2e/framework/nodes should be for anything handling the node resource. So CheckNodesReady should go there if it gets and checks node objects from the apiserver.

RestartControllerManager and WaitForApiserverUp should go into test/e2e/framework/restart.go or test/e2e/framework/cluster/state.go IMO. What do you think?

[andrewsykim]

@WanLinghao updated test/e2e/framework/cluster to test/e2e/framework/lifecycle as per @jiatongw's suggestion and moved restart.go under lifecycle.

[draveness]

Hi @SataQiu, how is the process of refactoring networking_util going on?

I'm working on service related functions in utils.go #77155. Some of the functions have to import framework package since it calls some common functions like Logf. And networking_util.go is calling WaitForServiceEndpointsNum and WaitForService which means it could cause the import cycle problem.

Do you have the time to work on the networking_util? If you do not have the time, I could take it over from here, many thanks. :)

[MorrisLaw]

I can work on wrappers and providers if they’re still available.

[johnSchnake]

@draveness I have a WIP for extracting the logging into its own package. It is a very heavily used set of functions though so I am running into a few issues. All solvable though I think. I’ll let you know when that is done.

[MorrisLaw]

Should provider related things go into test/e2e/framework/providers? Or, is it actually intended for provider related methods to be broken into a test/e2e/framework/providers.go file despite there already being a test/e2e/framework/provider.go file? @andrewsykim

[andrewsykim]

Should provider related things go into test/e2e/framework/providers? Or, is it actually intended for provider related methods to be broken into a test/e2e/framework/providers.go file despite there already being a test/e2e/framework/provider.go file? @andrewsykim

I think everything in test/e2e/framework/providers should be the provider implementations and test/e2e/framework/providers.go should have methods like ProviderIsand variables like ProvidersWithSSH in it. We can consider moving that up to test/e2e/framework/providers/provider.go but a lot of other methods in test/e2e/framework depend on it so I think keeping it in the same package at least for now makes sense. What do you think?

[SataQiu]

Hi @SataQiu, how is the process of refactoring networking_util going on?

I'm working on service related functions in utils.go #77155. Some of the functions have to import framework package since it calls some common functions like Logf. And networking_util.go is calling WaitForServiceEndpointsNum and WaitForService which means it could cause the import cycle problem.

Do you have the time to work on the networking_util? If you do not have the time, I could take it over from here, many thanks. :)

@draveness I have tried to solve this problem before, but it was suspended for some reason. If you have the time, feel free to take it over. Thanks a lot.

[andrewsykim]

@SataQiu that's why we're breaking up test/e2e/framework/util.go first. If we can move methods like Logf and WaitForService into their own packages we would avoid import cycles. Correct me if I'm wrong though, maybe there's another import cycle I'm not seeing

[WanLinghao]

I think we need move log functions and ssh functions to their own packages first to avoid cycle import.
They are widely referred in test/e2e/framework package .
I have create PR #77181 to solve this.

[tanjunchen]

/cc

[oomichi]

Fixed a typo in the title

/retitle Break up and Refactor test/e2e/framework/util.go

[gavinfish]

It seems most node related work has been done. I will continue to follow up node related refactor. Share it here to avoid unnecessary conflicts.

[tanjunchen]

/help

[k8s-ci-robot]

@tanjunchen:
This request has been marked as needing help from a contributor.

Please ensure the request meets the requirements listed here.

If this request no longer meets these requirements, the label can be removed
by commenting with the /remove-help command.

In response to this:

/help

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.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[oomichi]

/remove-lifecycle stale