Skip to content

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

Closed
soltysh wants to merge 1 commit intoopenshift:masterfrom
soltysh:set_owner
Closed

WIP: Set proper InvolvedObject when using library-go EventRecorder#2891
soltysh wants to merge 1 commit intoopenshift:masterfrom
soltysh:set_owner

Conversation

@soltysh
Copy link
Copy Markdown
Contributor

@soltysh soltysh commented Dec 22, 2021

You'll need to set a proper InvolvedObject such that you won't too many events like:

W1222 00:46:00.935946       1 recorder.go:190] Error creating event &Event{ObjectMeta:{.16c2ed25cd8773b4      0 0001-01-01 00:00:00 +0000 UTC <nil> <nil> map[] map[] [] []  []},InvolvedObject:ObjectReference{Kind:,Namespace:,Name:,UID:,APIVersion:,ResourceVersion:,FieldPath:,},Reason:ClusterRoleUpdated,Message:Updated ClusterRole.rbac.authorization.k8s.io/machine-config-controller-events -n openshift-machine-config-operator because it changed,Source:EventSource{Component:machine-config-operator,Host:,},FirstTimestamp:2021-12-22 00:46:00.93406098 +0000 UTC m=+8307.807549581,LastTimestamp:2021-12-22 00:46:00.93406098 +0000 UTC m=+8307.807549581,Count:1,Type:Normal,EventTime:0001-01-01 00:00:00 +0000 UTC,Series:nil,Action:,Related:nil,ReportingController:,ReportingInstance:,}: Event ".16c2ed25cd8773b4" is invalid: involvedObject.namespace: Invalid value: "": does not match event.namespace

as you can observe for example in https://gcsweb-ci.apps.ci.l2s4.p1.openshiftapps.com/gcs/origin-ci-test/pr-logs/pull/openshift_kubernetes/1087/pull-ci-openshift-kubernetes-master-k8s-e2e-gcp-serial/1473408603632177152/artifacts/k8s-e2e-gcp-serial/gather-extra/artifacts/pods/openshift-machine-config-operator_machine-config-controller-789f46f7dc-mwsn2_machine-config-controller.log
/assign @kikisdeliveryservice @sinnykumari

@openshift-ci openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Dec 22, 2021
@openshift-ci openshift-ci Bot requested review from jkyros and mkenigs December 22, 2021 16:44
@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Dec 22, 2021

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: soltysh
To complete the pull request process, please assign cgwalters after the PR has been reviewed.
You can assign the PR to them by writing /assign @cgwalters in a comment when ready.

The full list of commands accepted by this bot can be found here.

Details Needs approval from an approver in each of these files:

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

@mkenigs
Copy link
Copy Markdown
Contributor

mkenigs commented Dec 22, 2021

Thanks, I missed this in #2833

Comment thread pkg/operator/operator.go
libgoRecorder: events.NewRecorder(kubeClient.CoreV1().Events("openshift-machine-config-operator"), "machine-config-operator", &corev1.ObjectReference{}),
queue: workqueue.NewNamedRateLimitingQueue(workqueue.DefaultControllerRateLimiter(), "machineconfigoperator"),
libgoRecorder: events.NewRecorder(kubeClient.CoreV1().Events("openshift-machine-config-operator"), "machine-config-operator", &corev1.ObjectReference{
Kind: "Deployment",
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is Kind always supposed to be Deployment even if we're using it to sync eg Custom Resource Definition? https://github.com/openshift/machine-config-operator/blob/master/pkg/operator/sync.go#L382

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it can be any resource that exists in the cluster, we usually do that in Run method and try to read this one out from a running pod like here https://github.com/openshift/library-go/blob/624c91f4e514cb29f235478042f1ac2e9ef20b93/pkg/operator/staticpod/installerpod/cmd.go#L406 but it's to also hardcode a particular parent resource. Like I said, I not fluent with how your operator works 😉 so you can just treat that as a pointer.

@soltysh
Copy link
Copy Markdown
Contributor Author

soltysh commented Dec 22, 2021

Thanks, I missed this in #2833

Yeah, feel free to pick it up and update as necessary, I just put it here so you can push this forward, if you're happy with the current shape feel free to tag it, I'll attach k8s bump PR with it since it popped up during debugging other things.

@openshift-ci
Copy link
Copy Markdown
Contributor

openshift-ci Bot commented Dec 22, 2021

@soltysh: 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-single-node b578ebe link false /test e2e-aws-single-node
ci/prow/e2e-aws-upgrade-single-node b578ebe link false /test e2e-aws-upgrade-single-node
ci/prow/e2e-aws-disruptive b578ebe link false /test e2e-aws-disruptive
ci/prow/e2e-ovn-step-registry b578ebe link false /test e2e-ovn-step-registry
ci/prow/e2e-aws-workers-rhel7 b578ebe link false /test e2e-aws-workers-rhel7
ci/prow/e2e-vsphere-upgrade b578ebe link false /test e2e-vsphere-upgrade
ci/prow/e2e-aws-workers-rhel8 b578ebe link false /test e2e-aws-workers-rhel8
ci/prow/e2e-gcp-op b578ebe link true /test e2e-gcp-op
ci/prow/e2e-agnostic-upgrade b578ebe link true /test e2e-agnostic-upgrade

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.

@soltysh
Copy link
Copy Markdown
Contributor Author

soltysh commented Jan 5, 2022

Replaced with #2893

@soltysh soltysh closed this Jan 5, 2022
@soltysh soltysh deleted the set_owner branch January 5, 2022 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants