OCPBUGS-14396: Set controller-runtime logger to a null logger for E2E #946
Conversation
openshift-ci-robot
added
jira/severity-low
|
@gcs278: This pull request references Jira Issue OCPBUGS-14396, which is valid. The bug has been moved to the POST state. 3 validation(s) were run on this bug
Requesting review from QA contact: The bug has been updated to refer to the pull request using the external bug tracker. 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. |
|
/lgtm To test the approver's lgtm. |
|
/lgtm cancel |
|
/assign @alebedev87 |
|
failures are unrelated or known failures (such as TestAWSELBConnectionIdleTimeout that has been recently fixed) |
pkg/operator/client/client.go
Outdated
| // This is required because controller-runtime expects its consumers to | ||
| // set a logger through log.SetLogger within 30 seconds of the program's | ||
| // initalization. | ||
| ctrlruntimelog.SetLogger(logr.New(ctrlruntimelog.NullLogSink{})) |
There was a problem hiding this comment.
pkg/operator/client package is used in the operator and e2e test binaries. The e2e binary was missing the logger so this change makes the required migration for it. However the operator binary already sets the controller runtime logger here so this change doesn't cause any conflict (because client's init runs first) but is redundant.
The alternative would be to add SetLogger somewhere in test/e2e.
There was a problem hiding this comment.
I don't see where pkg/operator/client is used by the operator, hence it seemed safe putting here. Am I misunderstanding the usage of pkg/operator/client?
There was a problem hiding this comment.
pkg/operator/client is imported in pkg/operator/operator.go, that is, its init function is invoked.
There was a problem hiding this comment.
Oh whoops, you are right. I didn't give Goland enough time to finish loading all of the references...
Yea, I see, it's not-problematic, but redundant. I'm going to find a different place for it in test/e2e.
|
/hold |
openshift-ci
bot
added
the
do-not-merge/hold
test/e2e/all_test.go
Outdated
| // This is required because controller-runtime expects its consumers to | ||
| // set a logger through log.SetLogger within 30 seconds of the program's | ||
| // initalization. | ||
| ctrlruntimelog.SetLogger(logr.New(ctrlruntimelog.NullLogSink{})) |
There was a problem hiding this comment.
I think that the better place for this is operator_test.go, in a init() function. Because here the logger will be set at the test runtime. Some setup may take longer (TestMain) or some other test can be run before TestAll (I believe it's alphabetically ordered but not guaranteed).
There was a problem hiding this comment.
Yea that's fair, I didn't realize TestAll was really just a test itself, it wasn't as special as I thought. I also goofed up the imports.
Good suggestion, seems reasonable. I put it in an init() function in operator_test.go.
|
/lgtm |
Due to controller-runtime v0.15.0 bump, you must set a logger for controller-runtime and E2E tests were not initializing the logger. `test/e2e/operator_test.go`: Initialize controller-runtime logger in init() function
|
@gcs278: This pull request references Jira Issue OCPBUGS-14396, which is valid. 3 validation(s) were run on this bug
Requesting review from QA contact: 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. |
|
LGTM, just the title and the description need an update. |
gcs278
changed the title
Done. |
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: alebedev87 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 |
openshift-ci
bot
added
the
approved
|
Lots of E2E failures, but none look related. I confirmed we don't get the SetLogger error in https://storage.googleapis.com/origin-ci-test/pr-logs/pull/openshift_cluster-ingress-operator/946/pull-ci-openshift-cluster-ingress-operator-master-e2e-aws-operator/1674059716902260736/build-log.txt /retest |
|
/unhold |
openshift-ci
bot
removed
the
do-not-merge/hold
|
@gcs278: Jira Issue OCPBUGS-14396: All pull requests linked via external trackers have merged: Jira Issue OCPBUGS-14396 has been moved to the MODIFIED state. 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. |
|
@gcs278: The following test failed, say
Full PR test history. Your PR dashboard. 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. |
Initialise controller-runtime logger in E2E tests for v0.15.0 compliance. This is the same fix as openshift/cluster-ingress-operator#946.
Due to controller-runtime v0.15.0 bump and changes made in kubernetes-sigs/controller-runtime#2317, you must set a logger for controller-runtime and client.go was not initializing the logger.
test/e2e/operator_test.go: Initialize controller-runtime logger in init() function