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

chore: merge same events for kbagent #8445

Merged
merged 13 commits into from
Dec 2, 2024
Merged

Conversation

Ursasi
Copy link
Contributor

@Ursasi Ursasi commented Nov 13, 2024

  1. Merge same events.
  2. Eliminate unnecessary logs.

Current issues [solved]

Expectation

47s (x12 over 47s) => 47s (x12 over xxx not 47 s)
Should not be displayed as two identical times, because each time newEvent is executed.

Reason

  1. Use EventTime.UnixMicro() of event as the version number in event_controller.go
  2. Every time the event is updated, EventTime will be updated. This results in a new version number being generated with each update, rendering version checking ineffective.

@github-actions github-actions bot added the size/M Denotes a PR that changes 30-99 lines. label Nov 13, 2024
Copy link

codecov bot commented Nov 13, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 62.64%. Comparing base (33bb8c8) to head (0497a48).
Report is 31 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8445      +/-   ##
==========================================
+ Coverage   60.34%   62.64%   +2.30%     
==========================================
  Files         357      378      +21     
  Lines       42589    48004    +5415     
==========================================
+ Hits        25699    30073    +4374     
- Misses      14566    15335     +769     
- Partials     2324     2596     +272     
Flag Coverage Δ
unittests 62.64% <100.00%> (+2.30%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Ursasi Ursasi marked this pull request as ready for review November 13, 2024 12:25
@Ursasi Ursasi requested a review from a team as a code owner November 13, 2024 12:25
@Ursasi Ursasi requested review from zjx20 and leon-inf November 14, 2024 01:17
pkg/kbagent/service/command.go Outdated Show resolved Hide resolved
pkg/kbagent/util/event.go Outdated Show resolved Hide resolved
pkg/kbagent/util/event.go Outdated Show resolved Hide resolved
pkg/kbagent/util/event.go Outdated Show resolved Hide resolved
pkg/kbagent/util/event.go Outdated Show resolved Hide resolved
pkg/kbagent/util/event.go Outdated Show resolved Hide resolved
eventsClient := clientSet.CoreV1().Events(namespace())

eventName := generateEventName(reason, message)
event, err := eventsClient.Get(context.Background(), eventName, metav1.GetOptions{})
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the Get() call should be part of the retry logic. For example, the call may fail due to temporary network failures, retry can help in this case.

@zjx20
Copy link
Contributor

zjx20 commented Nov 18, 2024

I must say I prefer the previous implementation, which packages the get and create/update operations together as a single retry unit, rather than retrying them separately. Update conflicts are a common type of error in the Kubernetes world, and re-getting the latest version of the data and then updating is the standard pattern for resolving such issues.

@Ursasi
Copy link
Contributor Author

Ursasi commented Nov 18, 2024

I must say I prefer the previous implementation, which packages the get and create/update operations together as a single retry unit, rather than retrying them separately. Update conflicts are a common type of error in the Kubernetes world, and re-getting the latest version of the data and then updating is the standard pattern for resolving such issues.

So actually, get and create or get and update should be considered as a retry unit, instead of retrying update or create operations separately.

@apecloud-bot apecloud-bot added the approved PR Approved Test label Nov 19, 2024
Copy link
Contributor

@leon-inf leon-inf left a comment

Choose a reason for hiding this comment

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

The strategy by which kbagent sends events has changed, so the event handling should also be adjusted accordingly.

@apecloud-bot apecloud-bot removed the approved PR Approved Test label Nov 21, 2024
@Ursasi
Copy link
Contributor Author

Ursasi commented Nov 22, 2024

The strategy by which kbagent sends events has changed, so the event handling should also be adjusted accordingly.

The code logic has been adjusted based on whether synchronous event sending is needed.

"time"

"github.com/go-logr/logr"
"github.com/pkg/errors"
corev1 "k8s.io/api/core/v1"
k8serrors "k8s.io/apimachinery/pkg/api/errors"
Copy link
Contributor

Choose a reason for hiding this comment

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

k8serrors -> apierrors

@apecloud-bot apecloud-bot added the approved PR Approved Test label Dec 2, 2024
@leon-inf leon-inf merged commit d9fda81 into main Dec 2, 2024
35 checks passed
@leon-inf leon-inf deleted the support/merge-event-kbagent branch December 2, 2024 07:44
@leon-inf
Copy link
Contributor

leon-inf commented Dec 2, 2024

/cherry-pick release-1.0-beta

@github-actions github-actions bot added this to the Release 0.9.2 milestone Dec 2, 2024
Copy link

github-actions bot commented Dec 2, 2024

🤖 says: cherry pick action finished successfully 🎉!
See: https://github.com/apecloud/kubeblocks/actions/runs/12114822536

github-actions bot pushed a commit that referenced this pull request Dec 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved PR Approved Test size/M Denotes a PR that changes 30-99 lines.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants