Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add EventRecorder to Helper #320

Closed
wants to merge 1 commit into from
Closed

Conversation

@fao89
Copy link
Contributor Author

fao89 commented Aug 11, 2023

@stuggi @gibizer what do you think about this?

@fao89 fao89 requested review from gibizer and stuggi August 11, 2023 10:23
@gibizer
Copy link
Contributor

gibizer commented Aug 11, 2023

@stuggi @gibizer what do you think about this?

I don't have much context about events in k8s. I think it worth to create a doc where you propose what will be the use of the events in openstack-k8s-operators. I.e. what step during a reconciliation worth emitting an event. What information an event should always contain, etc. A bit similar to the definition of the condition usage https://github.com/openstack-k8s-operators/docs/blob/main/conditions.md

@stuggi
Copy link
Contributor

stuggi commented Aug 14, 2023

@stuggi @gibizer what do you think about this?

I don't have much context about events in k8s. I think it worth to create a doc where you propose what will be the use of the events in openstack-k8s-operators. I.e. what step during a reconciliation worth emitting an event. What information an event should always contain, etc. A bit similar to the definition of the condition usage https://github.com/openstack-k8s-operators/docs/blob/main/conditions.md

I agree with @gibizer , I think we should not just create events, first we'd have to agree/define when there should be one and the content.

@fao89 fao89 closed this Dec 4, 2023
@fao89 fao89 reopened this Apr 11, 2024
@fao89
Copy link
Contributor Author

fao89 commented Apr 11, 2024

I think this can help us to detect race conditions, we can sort events and see the chronological order things were added/updated in the cluster

@fao89 fao89 force-pushed the event branch 2 times, most recently from 0b59b50 to b23188e Compare April 11, 2024 15:39
@fao89 fao89 changed the title Should we start publishing events? Add EventRecorder to Helper Apr 11, 2024
@fao89 fao89 force-pushed the event branch 2 times, most recently from 5c1988f to 5271d47 Compare April 11, 2024 18:31
fao89 added a commit to fao89/dataplane-operator that referenced this pull request Apr 11, 2024
Depends-On: openstack-k8s-operators/lib-common#320
Signed-off-by: Fabricio Aguiar <[email protected]>
fao89 added a commit to fao89/dataplane-operator that referenced this pull request Apr 11, 2024
Depends-On: openstack-k8s-operators/lib-common#320
Signed-off-by: Fabricio Aguiar <[email protected]>
fao89 added a commit to fao89/dataplane-operator that referenced this pull request Apr 11, 2024
Depends-On: openstack-k8s-operators/lib-common#320
Signed-off-by: Fabricio Aguiar <[email protected]>
fao89 added a commit to fao89/dataplane-operator that referenced this pull request Apr 11, 2024
Depends-On: openstack-k8s-operators/lib-common#320
Signed-off-by: Fabricio Aguiar <[email protected]>
@fao89 fao89 force-pushed the event branch 6 times, most recently from c7345be to ab9c8a7 Compare April 11, 2024 23:26
fao89 added a commit to fao89/openstack-ansibleee-operator that referenced this pull request Apr 12, 2024
Depends-On: openstack-k8s-operators/lib-common#320

Signed-off-by: Fabricio Aguiar <[email protected]>
@fao89 fao89 marked this pull request as ready for review April 15, 2024 11:11
fao89 added a commit to fao89/dataplane-operator that referenced this pull request Apr 15, 2024
Depends-On: openstack-k8s-operators/lib-common#320

Signed-off-by: Fabricio Aguiar <[email protected]>
fao89 added a commit to fao89/dataplane-operator that referenced this pull request Apr 15, 2024
Depends-On: openstack-k8s-operators/lib-common#320

Signed-off-by: Fabricio Aguiar <[email protected]>
fao89 added a commit to fao89/openstack-ansibleee-operator that referenced this pull request Apr 15, 2024
Depends-On: openstack-k8s-operators/lib-common#320

Signed-off-by: Fabricio Aguiar <[email protected]>
@@ -105,8 +105,12 @@ func createOrPatchConfigMap(
return nil
})
if err != nil {
h.GetRecorder().Event(obj, corev1.EventTypeWarning, "ConfigMapError", fmt.Sprintf("error create/updating configmap: %s", cm.Name))
Copy link
Contributor

Choose a reason for hiding this comment

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

re ConfigMapError and other events, its probably good that we create a central reason collection which then gets used in all the operators. similar to how we do it with the conditions. Otherwise I guess if events are used in the service operators, we'll see different reasons used. I could see two ways. Either we create an events pkg and have the consts for reasons in there, or they are part of the already existing pkgs, like ConfigMap reason in configmap. but then how to handle reasons for stuff we do not yet have. create extra pkgs, or better the one events pkg. thoughts?

return ctrl.Result{}, err
}
if op == controllerutil.OperationResultCreated {
h.GetRecorder().Event(h.GetBeforeObject(), corev1.EventTypeNormal, "DaemonSetCreated", fmt.Sprintf("daemonset %s created", d.daemonset.Name))
}
if op != controllerutil.OperationResultNone {
util.LogForObject(h, fmt.Sprintf("DaemonSet: %s", op), daemonset)
Copy link
Contributor

Choose a reason for hiding this comment

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

should there be an updated event when the object got updated?

@stuggi stuggi requested review from dprince and abays April 17, 2024 15:08
@gibizer
Copy link
Contributor

gibizer commented Apr 17, 2024

@stuggi @gibizer what do you think about this?

I don't have much context about events in k8s. I think it worth to create a doc where you propose what will be the use of the events in openstack-k8s-operators. I.e. what step during a reconciliation worth emitting an event. What information an event should always contain, etc. A bit similar to the definition of the condition usage https://github.com/openstack-k8s-operators/docs/blob/main/conditions.md

I'm worried that this PR is moving forward without agreeing on the scope of it.

@stuggi
Copy link
Contributor

stuggi commented Apr 18, 2024

@stuggi @gibizer what do you think about this?

I don't have much context about events in k8s. I think it worth to create a doc where you propose what will be the use of the events in openstack-k8s-operators. I.e. what step during a reconciliation worth emitting an event. What information an event should always contain, etc. A bit similar to the definition of the condition usage https://github.com/openstack-k8s-operators/docs/blob/main/conditions.md

I'm worried that this PR is moving forward without agreeing on the scope of it.

agreed, @fao89 do you plan to work on some definition doc as we have for conditions?

@fao89
Copy link
Contributor Author

fao89 commented Apr 18, 2024

yes, I started it here: openstack-k8s-operators/dev-docs#105
but I had to stop to work on another thing, I'll change all the PRs to draft

@fao89 fao89 marked this pull request as draft April 18, 2024 08:36
@fao89
Copy link
Contributor Author

fao89 commented Apr 23, 2024

Actually, after sorting the events on must-gather(openstack-k8s-operators/openstack-must-gather#47), I can see the events I was interested in are already there (pod/job creation and job completion):

18m         Normal    Scheduled              pod/nova-custom-edpm-deployment-ipam-openstack-edpm-ipam-b6pcd        Successfully assigned openstack/nova-custom-edpm-deployment-ipam-openstack-edpm-ipam-b6pcd to crc-pjmnl-master-0
18m         Normal    Completed              job/libvirt-edpm-deployment-ipam-openstack-edpm-ipam                  Job completed
18m         Normal    SuccessfulCreate       job/nova-custom-edpm-deployment-ipam-openstack-edpm-ipam              Created pod: nova-custom-edpm-deployment-ipam-openstack-edpm-ipam-b6pcd
18m         Normal    Pulling                pod/nova-custom-edpm-deployment-ipam-openstack-edpm-ipam-b6pcd        Pulling image "quay.io/openstack-k8s-operators/openstack-ansibleee-runner:latest"
18m         Normal    AddedInterface         pod/nova-custom-edpm-deployment-ipam-openstack-edpm-ipam-b6pcd        Add eth0 [10.217.1.0/23] from openshift-sdn
18m         Normal    AddedInterface         pod/nova-custom-edpm-deployment-ipam-openstack-edpm-ipam-b6pcd        Add ctlplane [192.168.122.30/24] from openstack/ctlplane
18m         Normal    Started                pod/nova-custom-edpm-deployment-ipam-openstack-edpm-ipam-b6pcd        Started container openstackansibleee
18m         Normal    Pulled                 pod/nova-custom-edpm-deployment-ipam-openstack-edpm-ipam-b6pcd        Successfully pulled image "quay.io/openstack-k8s-operators/openstack-ansibleee-runner:latest" in 310.677684ms (310.689134ms including waiting)
18m         Normal    Created                pod/nova-custom-edpm-deployment-ipam-openstack-edpm-ipam-b6pcd        Created container openstackansibleee
16m         Normal    Completed              job/nova-custom-edpm-deployment-ipam-openstack-edpm-ipam              Job completed
16m         Normal    SuccessfulCreate       job/telemetry-edpm-deployment-ipam-openstack-edpm-ipam                Created pod: telemetry-edpm-deployment-ipam-openstack-edpm-ipam-mffxd
16m         Normal    Scheduled              pod/telemetry-edpm-deployment-ipam-openstack-edpm-ipam-mffxd          Successfully assigned openstack/telemetry-edpm-deployment-ipam-openstack-edpm-ipam-mffxd to crc-pjmnl-master-0
16m         Normal    AddedInterface         pod/telemetry-edpm-deployment-ipam-openstack-edpm-ipam-mffxd          Add eth0 [10.217.1.1/23] from openshift-sdn
16m         Normal    Created                pod/telemetry-edpm-deployment-ipam-openstack-edpm-ipam-mffxd          Created container openstackansibleee
16m         Normal    AddedInterface         pod/telemetry-edpm-deployment-ipam-openstack-edpm-ipam-mffxd          Add ctlplane [192.168.122.30/24] from openstack/ctlplane
16m         Normal    Started                pod/telemetry-edpm-deployment-ipam-openstack-edpm-ipam-mffxd          Started container openstackansibleee
16m         Normal    Pulled                 pod/telemetry-edpm-deployment-ipam-openstack-edpm-ipam-mffxd          Successfully pulled image "quay.io/openstack-k8s-operators/openstack-ansibleee-runner:latest" in 330.564864ms (330.577015ms including waiting)
16m         Normal    Pulling                pod/telemetry-edpm-deployment-ipam-openstack-edpm-ipam-mffxd          Pulling image "quay.io/openstack-k8s-operators/openstack-ansibleee-runner:latest"
15m         Normal    Completed              job/telemetry-edpm-deployment-ipam-openstack-edpm-ipam                Job completed

https://logserver.rdoproject.org/44/844/b8765a14d4d5aeb7d2ce04d812bcb92d3b019f3e/github-check/podified-multinode-edpm-deployment-crc/9bee51c/controller/ci-framework-data/logs/openstack-k8s-operators-openstack-must-gather/namespaces/openstack/events.log

@fao89
Copy link
Contributor Author

fao89 commented Apr 23, 2024

by the community guide:

Events are expected to provide important insights for the application developer/operator on the state
of their application. Events relevant to cluster administrators are acceptable, as well, though they usually
also have the option of looking at component logs. Events are much more expensive than logs, thus they're not
expected to provide in-depth system debugging information. Instead concentrate on things that are important 
from the application developer's perspective. Events need to be either actionable, or be useful to understand 
past or future system's behavior. Events are not intended to drive automation. Watching resource status should 
be sufficient for controllers.

https://github.com/kubernetes/community/blob/master/contributors/devel/sig-instrumentation/event-style-guide.md#when-to-emit-an-event

I think we can close this PR

@fao89 fao89 closed this Apr 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants