Add log.go back to core of e2e test framework

[oomichi]

What type of PR is this?

/kind cleanup

What this PR does / why we need it:

We tried to separate logger functionality as subpackage of e2e test
framework, but we've recognized that should be core functionality
and we should keep it as core of e2e test framework after facing
circular dependency issues.
So this adds log.go back to core of e2e test framework. In addition,
this makes volume sub package use the core logger as a sample.

Ref: #81245

Does this PR introduce a user-facing change?:

NONE

/sig testing

[oomichi]

/assign @timothysc @andrewsykim @johnSchnake

[oomichi]

/test pull-kubernetes-integration

[johnSchnake]

• I may have missed the clear consensus on this; I thought we were still going to talk about it at our next meeting before executing on this. To me this seems like such a simple package that it could exist outside of the framework entirely rather than in/under it. In that case it would:
  • avoid having the framework import a subpackage (framework/log)
  • still take LOC out of framework
  • be easy to update the import lines to reference wherever we want to put it
• Can we not just revert the changes that moved away from the framework to e2elog?
• It would be a large PR but can we just update all the logging at one time? I just dont recall; did we split up the other PRs (moving to e2elog) into multiple? Again, why not just revert those?

/hold

[oomichi]

@johnSchnake Thank you for your comments.

Can we not just revert the changes that moved away from the framework to e2elog?

That could be a nice idea, it would be easy to be created/reviewed if these PRs are just for reverting.
However we've done much work in parallel for improving e2e tests with not only this framework separation but also adding new framework functions and encouraging these functions.
For example, the latest PR which is related to e2elog also cannot be reverted as #78934
If pushing 'revert' button, you can see Sorry, this pull request couldn’t be reverted automatically. It may have already been reverted, or the content may have changed since it was merged. error message.

avoid having the framework import a subpackage (framework/log)

I cannot follw above your point. The PR makes e2elog into the core framework itself, not into sub package framework/log.

[johnSchnake]

@oomichi

The PR makes e2elog into the core framework itself, not into sub package framework/log.

So we have 3 options: put this outside of the framework entirely, put it in the framework, or under the framework as a "subpackage".

This PR puts it back into core, which prevents some cyclical deps but also means that if you just want to import the logging, you have to import all of framework

Putting it under the framework as a subpackage is exactly what you're wanting to move away from.

The last option, which I'm at least curious about, is moving it out of the framework entirely. It would be as if some 3rd party wrapped ginkgo and provided pass/fail/log messages and we just vendored that. It seems like that solves the concerns you have but also keeps it separate from the framework.

The framework would import it so the "size" of importing the framework wouldn't get smaller even though the code got removed, but it would enable you to vendor the logging without the framework if you wanted. I was just curious if we might prefer that?

[oomichi]

@johnSchnake

The framework would import it so the "size" of importing the framework wouldn't get smaller even though the code got removed, but it would enable you to vendor the logging without the framework if you wanted. I was just curious if we might prefer that?

That is a nice point.
I am guessing we are expecting different future of core framework.
I expect core framework will become small by moving non-core functions from it and it will just keep fundamental functions. Then sub packages of test frameworks and many e2e tests will import it, but the core framework will(should) not need to import the sub packages at all.
In the above situation, it can be fine to keep e2elog in core framework because logger is always necessary and essential for debugging test failures I think.
This thinking came from the slack conversation as https://kubernetes.slack.com/archives/C9NK9KFFW/p1565378690093600
TBH I don't have strong opinion for this at this time. I just want to solve this complex dependency issue.

[fejta]

/lgtm
/approve

[oomichi]

Sorry, I misunderstood the following error message

test/e2e/framework/log/logger.go:17:1: package comment should be of the form "Package log ..."

I should remove TODO(oomichi): also..

[oomichi]

/retest

[fejta]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: fejta, 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 [fejta,oomichi]

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

[johnSchnake]

I don't have any reason to hold now; it seemed like there might still be some conversations ongoing about this though (I saw a few random messages in slack)? I'm OK if others want to remove the hold and merge though.

[oomichi]

It seems fine to merge this at this time.

/hold cancel