chore: merge the same kbagent events #8044
Conversation
Ursasi
requested review from
nayutah,
ldming,
heng4fun,
free6om,
wangyelei,
Y-Rookie and
weicao
as code owners
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8044 +/- ##
==========================================
+ Coverage 62.38% 62.51% +0.12%
==========================================
Files 334 334
Lines 40745 40745
==========================================
+ Hits 25419 25470 +51
+ Misses 13054 13000 -54
- Partials 2272 2275 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -44,7 +44,7 @@ type EventReconciler struct { | |||
| } | |||
|
|
|||
| // events API only allows ready-only, create, patch | |||
| // +kubebuilder:rbac:groups=core,resources=events,verbs=get;list;watch;create;patch | |||
| // +kubebuilder:rbac:groups=core,resources=events,verbs=get;list;watch;create;patch;update; | |||
There was a problem hiding this comment.
The controller has no need to the update permission.
pkg/kbagent/util/event.go
Outdated
| "time" | ||
|
|
||
| "github.com/apecloud/kubeblocks/pkg/constant" |
pkg/kbagent/util/event.go
Outdated
| if !errors.IsNotFound(err) { | ||
| return fmt.Errorf("error getting event: %v", err) | ||
| } | ||
| event = createEvent(reason, message, eventName) |
There was a problem hiding this comment.
Rename these functions like: createEvent -> newEvent, sendEvent -> createEvent.
pkg/kbagent/util/event.go
Outdated
| event, err := clientSet.CoreV1().Events(baseInfo.namespace).Get(context.TODO(), eventName, metav1.GetOptions{}) | ||
| if err != nil { | ||
| if !errors.IsNotFound(err) { | ||
| return fmt.Errorf("error getting event: %v", err) |
There was a problem hiding this comment.
Should back-off to create a new Event, or the event may be lost.
pkg/kbagent/util/event.go
Outdated
| namespace := os.Getenv(constant.KBEnvNamespace) | ||
| func sendOrUpdateEvent(reason string, message string) error { | ||
| suffix := hashReasonNMessage(reason, message) | ||
| eventName := fmt.Sprintf("%s.%s.%s", baseInfo.podName, baseInfo.podUID, suffix) |
There was a problem hiding this comment.
It's a little weird that kinds of event with same reason and message will have an unique Event object.
pkg/kbagent/util/event.go
Outdated
| event = createEvent(reason, message, eventName) | ||
| return sendEvent(clientSet, event) | ||
| } | ||
| return updateEvent(clientSet, event) |
There was a problem hiding this comment.
Should there be some other conditions to determine whether to update the exist Event or create a new Event? For example, the time interval between events, or consecutive events of the same type, etc., rather than just depending on the existence of the Event.
pkg/kbagent/util/event.go
Outdated
| event := createEvent(reason, message) | ||
| err := sendEvent(event) | ||
| once.Do(func() { | ||
| baseInfo = eventBaseInfo{ |
There was a problem hiding this comment.
The eventBaseInfo struct is not necessary.
pkg/kbagent/util/event.go
Outdated
| podUID := os.Getenv(constant.KBEnvPodUID) | ||
| nodeName := os.Getenv(constant.KBEnvNodeName) | ||
| namespace := os.Getenv(constant.KBEnvNamespace) | ||
| func sendOrUpdateEvent(reason string, message string) error { |
There was a problem hiding this comment.
Should add some test cases for different behaviors.
| @@ -44,7 +44,7 @@ type EventReconciler struct { | |||
| } | |||
|
|
|||
| // events API only allows ready-only, create, patch | |||
| // +kubebuilder:rbac:groups=core,resources=events,verbs=get;list;watch;create;patch | |||
| // +kubebuilder:rbac:groups=core,resources=events,verbs=get;list;watch;create;patch;update; | |||
pkg/kbagent/util/event.go
Outdated
| @@ -22,94 +22,115 @@ package util | |||
| import ( | |||
| "context" | |||
| "fmt" | |||
| "github.com/apecloud/kubeblocks/pkg/constant" | |||
There was a problem hiding this comment.
Adjust the import order, move to last.
pkg/kbagent/util/event.go
Outdated
| metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" | ||
| "k8s.io/apimachinery/pkg/types" | ||
| "k8s.io/apimachinery/pkg/util/rand" | ||
| v1 "k8s.io/apimachinery/pkg/apis/meta/v1" |
No description provided.