-
Notifications
You must be signed in to change notification settings - Fork 154
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 livestatereporter for the plugin architecture #5452
Add livestatereporter for the plugin architecture #5452
Conversation
Signed-off-by: Yoshiki Fujikane <[email protected]>
Signed-off-by: Yoshiki Fujikane <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #5452 +/- ##
==========================================
- Coverage 26.23% 26.09% -0.15%
==========================================
Files 452 457 +5
Lines 48872 49081 +209
==========================================
- Hits 12823 12808 -15
- Misses 35024 35251 +227
+ Partials 1025 1022 -3 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Yoshiki Fujikane <[email protected]>
return r | ||
} | ||
|
||
func (r *reporter) Run(ctx context.Context) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented it based on this code.
https://github.com/pipe-cd/pipecd/blob/336d96d501311103dd9921bcea55722e2c8e774a/pkg/app/piped/livestatereporter/reporter.go#L104C1-L127C2
logger *zap.Logger | ||
} | ||
|
||
func (pr *pluginReporter) Run(ctx context.Context) error { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented it based on this code.
https://github.com/pipe-cd/pipecd/blob/master/pkg/app/piped/livestatereporter/kubernetes/reporter.go#L79C1-L110C2
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost LGTM. I commented on one nitpick
// Avoid starting all reporters at the same time to reduce the API call burst. | ||
time.Sleep(time.Duration(i) * 10 * time.Second) | ||
r.logger.Info("starting app live state reporter for plugin") | ||
|
||
group.Go(func() error { | ||
return reporter.Run(ctx) | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there are N plugins, this code sleeps for N(N+1) * 10s
. It's a bit long and the expected duration seems N * 10s
. One more, the info log should contain the plugin name. So, I suggest 2 ways below.
1
// Avoid starting all reporters at the same time to reduce the API call burst. | |
time.Sleep(time.Duration(i) * 10 * time.Second) | |
r.logger.Info("starting app live state reporter for plugin") | |
group.Go(func() error { | |
return reporter.Run(ctx) | |
}) | |
// Avoid starting all reporters at the same time to reduce the API call burst. | |
group.Go(func() error { | |
time.Sleep(time.Duration(i) * 10 * time.Second) | |
reporter.logger.Info("starting app live state reporter for plugin"))) | |
return reporter.Run(ctx) | |
}) |
2
// Avoid starting all reporters at the same time to reduce the API call burst. | |
time.Sleep(time.Duration(i) * 10 * time.Second) | |
r.logger.Info("starting app live state reporter for plugin") | |
group.Go(func() error { | |
return reporter.Run(ctx) | |
}) | |
// Avoid starting all reporters at the same time to reduce the API call burst. | |
time.Sleep(10 * time.Second) | |
reporter.logger.Info("starting app live state reporter for plugin") | |
group.Go(func() error { | |
return reporter.Run(ctx) | |
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Signed-off-by: Yoshiki Fujikane <[email protected]>
Signed-off-by: Yoshiki Fujikane <[email protected]>
Signed-off-by: Yoshiki Fujikane <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Signed-off-by: Yoshiki Fujikane <[email protected]>
I noticed that we don't fix the control plane side's rpc. This is just put it as json format.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
What this PR does:
Implement livestatereporter for the plugin architecture.
It reports the application's live state and the sync state for every registered apps by the interval of 1 minute.
Why we need it:
We need to support livestate features on the pipedv1
Which issue(s) this PR fixes:
Part of #5363
Does this PR introduce a user-facing change?: