Bug 2045866: Set proper InvolvedObject when using library-go EventRecorder #2893

mkenigs · 2021-12-23T16:24:45Z

Currently we're getting the error:
Error creating event &Event...
Event ".16c2ed25cd8773b4" is invalid: involvedObject.namespace: Invalid value: "": does not match event.namespace

Related to Bug 2034364, overlooked in
#2833

Replaces #2891

openshift-ci · 2021-12-23T16:24:52Z

@mkenigs: This pull request references Bugzilla bug 2034364, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state ASSIGNED, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla ([email protected]), skipping review request.

Details

In response to this:

Bug 2034364: Set proper InvolvedObject when using library-go EventRecorder

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.

mkenigs · 2021-12-23T16:26:10Z

@soltysh do I have to have a different recorder for every kind like this?

soltysh · 2022-01-05T09:54:52Z

@soltysh do I have to have a different recorder for every kind like this?

theoretically - yes, but I'd suggest creating a single recorder and link that only with deployment and not with every possible resource.

soltysh · 2022-01-05T09:56:35Z

pkg/operator/operator.go

This is wrong, you're hardcoding APIVersion and you're using newRecorder for kinds outside apps/v1 group. As mentioned elsewhere, I'd suggest creating a single recorder tied with just the MCO deployment, which should be sufficient.

mkenigs · 2022-01-06T15:20:53Z

@soltysh do I have to have a different recorder for every kind like this?

theoretically - yes, but I'd suggest creating a single recorder and link that only with deployment and not with every possible resource.

So would you suggest passing an empty recorder in instances like https://github.com/openshift/machine-config-operator/pull/2893/files#diff-3e205577bec1a1d1711df8bfeff30e63f044b24cfbdc69bbd7d0052fdc881891L382 (where we need a recorder for a CustomResourceDefinition)?

Or am I misunderstanding and I can set the recorder's kind to Deployment and still use it for the CRD?

soltysh · 2022-01-11T11:43:17Z

@mkenigs I was suggesting one recorder pointing to MCO deployment and use that in most places, if not all.

openshift-ci · 2022-01-25T22:14:28Z

@mkenigs: This pull request references Bugzilla bug 2045866, which is invalid:

expected the bug to target the "4.10.0" release, but it targets "---" instead

Comment /bugzilla refresh to re-evaluate validity if changes to the Bugzilla bug are made, or edit the title of this pull request to link to a different bug.

Details

In response to this:

Bug 2045866: Set proper InvolvedObject when using library-go EventRecorder

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.

mkenigs · 2022-01-25T22:24:07Z

/bugzilla refresh

openshift-ci · 2022-01-25T22:24:14Z

@mkenigs: This pull request references Bugzilla bug 2045866, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state NEW, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla ([email protected]), skipping review request.

Details

In response to this:

/bugzilla refresh

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.

kikisdeliveryservice · 2022-01-25T23:12:56Z

/retest-required

yuqi-zhang · 2022-01-26T00:12:46Z

/test e2e-aws

yuqi-zhang

Tentatively approving the general code, some questions/concerns:

So looking at gcp-op (install cluster, run MCO e2e) it looks like we do have some additional event logging at the beginning that fail due to waiting for apiserver https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_machine-config-operator/2893/pull-ci-openshift-machine-config-operator-master-e2e-gcp-op/1486006930555015168/artifacts/e2e-gcp-op/gather-extra/artifacts/pods/openshift-machine-config-operator_machine-config-operator-69c597d945-66st6_machine-config-operator.log

Which seems fine since it does eventually resolve (and just log the pool syncs later). It is still a bit noisy but better than before. I assume those events are normal?

theoretically - yes, but I'd suggest creating a single recorder and link that only with deployment and not with every possible resource.

I'm not the most familiar with how this works out. In the MCO we currently use the recorder when applying:

secrets
clusterrole
rolebinding
clusterrolebinding

And interestingly not the daemonset/deployment since we still use MCO code which do not invoke the recorder yet. @soltysh are you saying that with the singular hardcoded Deployment recorder we can cover all of the above cases?

Currently we're getting the error: Error creating event &Event... Event ".16c2ed25cd8773b4" is invalid: involvedObject.namespace: Invalid value: "": does not match event.namespace Related to Bug 2034364, overlooked in openshift#2833

openshift-ci · 2022-01-26T21:59:22Z

@mkenigs: This pull request references Bugzilla bug 2045866, which is valid.

3 validation(s) were run on this bug

bug is open, matching expected state (open)
bug target release (4.10.0) matches configured target release for branch (4.10.0)
bug is in the state POST, which is one of the valid states (NEW, ASSIGNED, ON_DEV, POST, POST)

No GitHub users were found matching the public email listed for the QA contact in Bugzilla ([email protected]), skipping review request.

Details

In response to this:

Bug 2045866: Set proper InvolvedObject when using library-go EventRecorder

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.

yuqi-zhang · 2022-01-27T00:07:17Z

Given that:

This PR fixes the log spam
The PR does not as far as I can tell cause any other eventing issues
This PR passed all the required tests at least once

Going to go ahead and lgtm so we can try to herd this past CI before code freeze. We can follow up on whether we should be creating different recorders per-object type as a follow up.

/lgtm

openshift-bot · 2022-01-27T01:14:45Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-27T03:43:57Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-27T04:43:55Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-27T05:07:54Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-27T05:43:56Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-27T06:19:55Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-27T07:07:55Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

kikisdeliveryservice · 2022-01-27T08:39:05Z

/retest-required

openshift-bot · 2022-01-27T10:20:57Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-27T11:21:56Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-27T11:57:56Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-27T13:46:00Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-27T14:21:58Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-27T15:34:57Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-27T17:05:58Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-27T18:56:57Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

kikisdeliveryservice · 2022-01-27T19:28:12Z

/test e2e-agnostic-upgrade

openshift-bot · 2022-01-27T20:41:04Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-27T22:50:59Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-27T23:16:57Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-28T01:01:01Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-28T01:27:02Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-28T01:39:16Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2022-01-28T02:51:36Z

@mkenigs: The following tests failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/e2e-aws-workers-rhel7	`14c4c33`	link	false	`/test e2e-aws-workers-rhel7`
ci/prow/okd-e2e-aws	`14c4c33`	link	false	`/test okd-e2e-aws`
ci/prow/e2e-aws-workers-rhel8	`14c4c33`	link	false	`/test e2e-aws-workers-rhel8`
ci/prow/e2e-vsphere-upgrade	`14c4c33`	link	false	`/test e2e-vsphere-upgrade`
ci/prow/e2e-gcp-op-single-node	`14c4c33`	link	false	`/test e2e-gcp-op-single-node`
ci/prow/e2e-ovn-step-registry	`14c4c33`	link	false	`/test e2e-ovn-step-registry`
ci/prow/e2e-aws-disruptive	`14c4c33`	link	false	`/test e2e-aws-disruptive`
ci/prow/e2e-aws-upgrade-single-node	`14c4c33`	link	false	`/test e2e-aws-upgrade-single-node`

Full PR test history. Your PR dashboard.

Details

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.

openshift-bot · 2022-01-28T03:32:58Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-28T03:59:01Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-bot · 2022-01-28T04:11:55Z

/retest-required

Please review the full test history for this PR and help us cut down flakes.

openshift-ci · 2022-01-28T04:54:07Z

@mkenigs: All pull requests linked via external trackers have merged:

openshift/machine-config-operator#2893

Bugzilla bug 2045866 has been moved to the MODIFIED state.

Details

In response to this:

Bug 2045866: Set proper InvolvedObject when using library-go EventRecorder

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.

openshift-ci bot added bugzilla/severity-low Referenced Bugzilla bug's severity is low for the branch this PR is targeting. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. labels Dec 23, 2021

openshift-ci bot requested review from cheesesashimi and kikisdeliveryservice December 23, 2021 16:25

soltysh mentioned this pull request Jan 5, 2022

WIP: Set proper InvolvedObject when using library-go EventRecorder #2891

Closed

soltysh suggested changes Jan 5, 2022

View reviewed changes

mkenigs changed the title ~~Bug 2034364: Set proper InvolvedObject when using library-go EventRecorder~~ Bug 2045866: Set proper InvolvedObject when using library-go EventRecorder Jan 25, 2022

openshift-ci bot added the bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. label Jan 25, 2022

openshift-ci bot removed the bugzilla/invalid-bug Indicates that a referenced Bugzilla bug is invalid for the branch this PR is targeting. label Jan 25, 2022

yuqi-zhang approved these changes Jan 26, 2022

View reviewed changes

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 26, 2022

mkenigs force-pushed the set_owner branch from 29553a2 to c5bd891 Compare January 26, 2022 13:51

mkenigs force-pushed the set_owner branch from c5bd891 to 14c4c33 Compare January 26, 2022 21:58

openshift-merge-robot merged commit 3e9f2ca into openshift:master Jan 28, 2022

mkenigs deleted the set_owner branch March 18, 2022 13:48

Bug 2045866: Set proper InvolvedObject when using library-go EventRecorder #2893

Bug 2045866: Set proper InvolvedObject when using library-go EventRecorder #2893

Uh oh!

Conversation

mkenigs commented Dec 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci bot commented Dec 23, 2021

Uh oh!

mkenigs commented Dec 23, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soltysh commented Jan 5, 2022

Uh oh!

soltysh Jan 5, 2022

Choose a reason for hiding this comment

Uh oh!

mkenigs commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

soltysh commented Jan 11, 2022

Uh oh!

openshift-ci bot commented Jan 25, 2022

Uh oh!

mkenigs commented Jan 25, 2022

Uh oh!

openshift-ci bot commented Jan 25, 2022

Uh oh!

kikisdeliveryservice commented Jan 25, 2022

Uh oh!

yuqi-zhang commented Jan 26, 2022

Uh oh!

yuqi-zhang left a comment

Choose a reason for hiding this comment

Uh oh!

openshift-ci bot commented Jan 26, 2022

Uh oh!

yuqi-zhang commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

kikisdeliveryservice commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

kikisdeliveryservice commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 27, 2022

Uh oh!

openshift-bot commented Jan 28, 2022

mkenigs commented Dec 23, 2021 •

edited

Loading

mkenigs commented Dec 23, 2021 •

edited

Loading

mkenigs commented Jan 6, 2022 •

edited

Loading

openshift-ci bot commented Jan 28, 2022 •

edited

Loading