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

[BUG] Update and Error events of old resources are skipped #147

Closed
codenio opened this issue Aug 8, 2019 · 8 comments · Fixed by #148
Closed

[BUG] Update and Error events of old resources are skipped #147

codenio opened this issue Aug 8, 2019 · 8 comments · Fixed by #148
Labels
Milestone

Comments

@codenio
Copy link
Contributor

@codenio codenio commented Aug 8, 2019

Describe the bug
botkube skips the latest update, error event notifications of old resources available in the cluster. i.e, resources that were already existing before starting botkube.

To Reproduce
Steps to reproduce the behavior:

  1. add config to watch update/error events in config.yaml
  2. start/restart botkube server
  3. change/modify the some old resources
  4. notice that update/error events are skipped.

Expected behavior
Irrespective of the object creating time, botkube should send update, error events on resources when configured by the user.

Snippet

DEBU[2019-08-08T20:12:22+05:30] Processing update to service/app-5-demo-dummy in default namespaces 
DEBU[2019-08-08T20:12:22+05:30] event.TimeStamp >>> 0001-01-01 00:00:00 +0000 UTC 
DEBU[2019-08-08T20:12:22+05:30] Skipping older events

// Add TimeStamps
if eventType == config.CreateEvent {
event.TimeStamp = objectMeta.CreationTimestamp.Time
}
if eventType == config.DeleteEvent {
if objectMeta.DeletionTimestamp != nil {
event.TimeStamp = objectMeta.DeletionTimestamp.Time
}
}

event.New() initialises event.time with 0001-01-01 00:00:00 +0000 UTC for events other than CreateEvent and DeleteEvent which is way long before startTime of botkube.

note :
This is my first valuable bug. Feeling awesome 🎉
I wish I get some bug-bounty 🤣

@codenio codenio added the bug label Aug 8, 2019
@codenio codenio changed the title [BUG] Latest Update and Error events of old resources are skipped [BUG] Update and Error events of old resources are skipped Aug 8, 2019
@PrasadG193
Copy link
Member

@PrasadG193 PrasadG193 commented Aug 8, 2019

Good catch @aananthraj :). But I believe this is already fixed as a part of #146

@codenio
Copy link
Contributor Author

@codenio codenio commented Aug 8, 2019

But I believe this is already fixed as a part of #146

Omg didnt see this comming through 👀

Is this completely fixed.?
I inspected for this bug and found it in #146 branch as well, before raising this issue and its corresponding fix.
can you check once by replicating the above mentioned steps..?

@PrasadG193
Copy link
Member

@PrasadG193 PrasadG193 commented Aug 9, 2019

@aananthraj I have tested this on #146 with mentioned steps and seems to be working as expected. Let's wait for #146 to get merged then try reproducing again.

@codenio
Copy link
Contributor Author

@codenio codenio commented Aug 9, 2019

sure, let me try now and update here.

@codenio
Copy link
Contributor Author

@codenio codenio commented Aug 9, 2019

@PrasadG193 , still the issue exists

DEBU[2019-08-09T15:53:53+05:30] Processing update to service/app-5-demo-dummy in default namespaces 
DEBU[2019-08-09T15:53:53+05:30] event.Timestamp >>> 0001-01-01 00:00:00 +0000 UTC 
DEBU[2019-08-09T15:53:53+05:30] Skipping older events                        

the following resources are still open to this issue

case *apiV1.Namespace:
event.Kind = "Namespace"
case *apiV1.PersistentVolume:
event.Kind = "PersistentVolume"

case *apiV1.Service:
event.Kind = "Service"
case *apiV1.Secret:
event.Kind = "Secret"
case *apiV1.ConfigMap:
event.Kind = "ConfigMap"
case *extV1beta1.DaemonSet:
event.Kind = "DaemonSet"
case *extV1beta1.Ingress:
event.Kind = "Ingress"

case *rbacV1.Role:
event.Kind = "Role"
case *rbacV1.RoleBinding:
event.Kind = "RoleBinding"
case *rbacV1.ClusterRole:
event.Kind = "ClusterRole"
case *rbacV1.ClusterRoleBinding:
event.Kind = "ClusterRoleBinding"

let's fix this by initializing event.Timestamp = time.Now() to all events, which is the overwritten later based on the type of the event or kind of the object.

@PrasadG193
Copy link
Member

@PrasadG193 PrasadG193 commented Aug 9, 2019

Oh yes, you are right, let's fix this. Thanks a lot 🙂

@codenio
Copy link
Contributor Author

@codenio codenio commented Aug 17, 2019

still, there are some issues with the event.New() implementation.

for instance, let's consider the following scenario

  • deployment object gets created and gets obj.Status.Conditions before botkube is started
  • botkube is started now
  • an update event follows

case *appsV1.Deployment:
event.Kind = "Deployment"
if eventType == config.UpdateEvent {
condLen := len(obj.Status.Conditions)
if condLen != 0 {
event.TimeStamp = obj.Status.Conditions[condLen-1].LastTransitionTime.Time
}

Problem:

  • since deployment object has obj.Status.Conditions[condLen-1].LastTransitionTime.Time as time before sarting of botkube server, recent update event will be skipped

we need to reopen this issue and fix all such issues.

@codenio
Copy link
Contributor Author

@codenio codenio commented Aug 18, 2019

since deployment object has obj.Status.Conditions[condLen-1].LastTransitionTime.Time as time before sarting of botkube server, recent update event will be skipped

This issue will be fixed by #158

@sanketsudake sanketsudake added this to the v0.9.0 milestone Aug 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

3 participants