Fix livestatereporter to consider multiple plugins in one app by ffjlabo · Pull Request #5457 · pipe-cd/pipecd

ffjlabo · 2024-12-26T08:56:38Z

What this PR does:

~~Fix to start the livestatereporter when starting pipedv1.~~

~~Also, I added a plugin name field for the application model to list apps by it.~~

I fixed the livestatereporter to consider the multiple plugins situation.

Modify to use DeploySource

Added deploy_source to GetLivestateRequest.

Modify to merge Livestate

LivestateState

Add plugin name to the ResourceState
HealthStatus: Total sum of statuses across all plugins

SyncState

Status: Total sum of statuses across all plugins
- Priority is InvalidConfig > OutOfSync > Sync > Unknown
ShortReason:
- Combine results of each plugin separated by new lines
Reason:
- Combine results of each plugin separated by new lines

Note: the current Web UI for each field above.

Why we need it:

~~These fixes are needed to start the internal reporter for each plugin.~~

Since multiple plugins will be supported for one app, each plugin will be able to support its own Livestate.
Therefore, I retrieved all the Livestates and merged the results to display them.

Which issue(s) this PR fixes:

Part of #5363

Does this PR introduce a user-facing change?:

How are users affected by this change:
Is this breaking change:
How to migrate (if breaking change):

codecov · 2024-12-26T09:15:02Z

Codecov Report

Attention: Patch coverage is 56.85279% with 85 lines in your changes missing coverage. Please review.

Project coverage is 26.62%. Comparing base (b6fbc71) to head (c5284f3).
Report is 7 commits behind head on master.

Files with missing lines	Patch %	Lines
...app/pipedv1/livestatereporter/livestatereporter.go	43.57%	67 Missing and 12 partials ⚠️
pkg/model/application.go	90.38%	4 Missing and 1 partial ⚠️
pkg/app/pipedv1/controller/planner.go	50.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5457      +/-   ##
==========================================
+ Coverage   26.29%   26.62%   +0.33%     
==========================================
  Files         464      464              
  Lines       49626    49737     +111     
==========================================
+ Hits        13047    13241     +194     
+ Misses      35546    35437     -109     
- Partials     1033     1059      +26

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

khanhtc1202 · 2024-12-26T11:47:33Z

pkg/model/application.proto

    // The names of deploy taget where to deploy this application.
    repeated string deploy_targets = 16;
+    // The name of plugin used to deploy this application.
+    string plugin = 17;


it's not a single plugin which deploys an application but it's a list of plugins do

@khanhtc1202
Thanks, fixed it on 0c4fc0d

pkg/app/pipedv1/apistore/applicationstore/store.go

khanhtc1202 · 2024-12-27T02:44:20Z

@ffjlabo go lint error found, please fix it

ffjlabo · 2024-12-27T03:05:47Z

Thanks. I don't know it. I'll fix it.

lint job is succeed, but the result of the golangci-lint is in the labeler 🤔

ffjlabo · 2024-12-27T03:15:16Z

@khanhtc1202 Fixed for the lint

Warashi

LGTM

ffjlabo · 2025-01-06T00:46:32Z

📝 We decided to determine the plugin used on the application from app.pipecd.yaml. So I will fix not to add plugin name to the application model.

ffjlabo · 2025-01-17T05:41:42Z

pkg/app/pipedv1/livestatereporter/livestatereporter.go

+	// TODO: Set the limit based on the piped config
+	limit := runtime.GOMAXPROCS(0) / 2

-		// Report the application live state to the control plane.
-		snapshot := &model.ApplicationLiveStateSnapshot{
-			ApplicationId:        app.Id,
-			PipedId:              app.PipedId,
-			ProjectId:            app.ProjectId,
-			Kind:                 app.Kind,
-			ApplicationLiveState: res.GetApplicationLiveState(),
+	appsPerReporter := len(apps) / limit
+	if appsPerReporter == 0 {
+		appsPerReporter = 1
+	}
+
+	appsGrouped := make([][]*model.Application, 0)
+	for i := 0; i < len(apps); i += appsPerReporter {
+		end := i + appsPerReporter
+		if end > len(apps) {
+			end = len(apps)
 		}
-		snapshot.DetermineApplicationHealthStatus()
-
-		if _, err := pr.apiClient.ReportApplicationLiveState(ctx, &pipedservice.ReportApplicationLiveStateRequest{
-			Snapshot: snapshot,
-		}); err != nil {
-			pr.logger.Error("failed to report application live state",
-				zap.String("application-id", app.Id),
-				zap.Error(err),
-			)
+		appsGrouped = append(appsGrouped, apps[i:end])
+	}
+
+	eg, ctx := errgroup.WithContext(ctx)
+	eg.SetLimit(limit)
+
+	r.logger.Info("start flushing snapshots", zap.Int("application-count", len(apps)), zap.Int("concurrency", limit))
+	for _, apps := range appsGrouped {
+		eg.Go(func() error {
+			r.flushAll(ctx, apps, repoMap)
+			return nil
+		})
+	}


At first, I tried to implement the livestate reporter to process the apps grouped by plugins because the livestatereporter for pipedv0 processes the apps grouped by platform providers.

But it finds the plugin name of the app based on the app config.
It takes some more time and resource to prepare the deploy sources for all of the apps in one time.
So I implemented it in paralell.

ffjlabo · 2025-01-17T05:45:17Z

pkg/app/pipedv1/livestatereporter/livestatereporter.go

+	dir, err := os.MkdirTemp(r.workingDir, fmt.Sprintf("app-%s-*", app.Id))
+	if err != nil {
+		r.logger.Error("failed to create temporary directory", zap.Error(err))
+		return err
+	}
+
+	dsp := deploysource.NewProvider(dir, deploysource.NewLocalSourceCloner(repo, "target", "HEAD"), app.GitPath, r.secretDecrypter)
+	ds, err := dsp.Get(ctx, io.Discard)
+	if err != nil {
+		r.logger.Error("failed to get deploy source", zap.String("application-id", app.Id), zap.Error(err))
+		return err
+	}
+
+	cfg, err := config.DecodeYAML[*config.GenericApplicationSpec](ds.ApplicationConfig)
+	if err != nil {
+		r.logger.Error("unable to parse application config", zap.Error(err))
+		return err
+	}


We use the Deploy Source for two reasons.

to use app.pipecd.yaml for the app

to pass it when calling GetLivestate. It expects to use it do do drift detection.

ffjlabo · 2025-01-17T06:27:56Z

pkg/model/application_live_state.proto

    // The target where this resource is deployed.
    string deploy_target = 8;
+    // The plugin name that manages this resource.
+    string plugin_name = 9;


We need it to show livestates by grouping with plugin

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

khanhtc1202 · 2025-01-22T09:24:46Z

pkg/plugin/api/v1alpha1/common/common.proto

+option go_package = "github.com/pipe-cd/pipecd/pkg/plugin/api/v1alpha1/common";
+
+import "validate/validate.proto";
+import "pkg/model/common.proto";


Looks like we don't use this 👀

Thanks, removed them

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

Warashi · 2025-01-23T00:29:10Z

pkg/app/pipedv1/livestatereporter/livestatereporter.go

-	logger                *zap.Logger
-}
+	// TODO: Set the limit based on the piped config
+	limit := runtime.GOMAXPROCS(0) / 2


When runtime.GOMAXPROCS(0) returns 1, limit becomes 0.
This causes the division-by-zero error.
Please set the limit 1 if it is 0.

Thanks, fixed on 5ea0571

Warashi · 2025-01-23T00:32:15Z

pkg/app/pipedv1/livestatereporter/livestatereporter.go

-		case <-snapshotTicker.C:
-			pr.flushSnapshots(ctx)
+	eg, ctx := errgroup.WithContext(ctx)
+	eg.SetLimit(limit)


We use the appsGrouped to limit concurrency, so we don't need this SetLimit.
Or, we can use only eg.SetLimit and simply call eg.Go with all apps.

Thanks, you're right. Also there is no reason to use errgroup, so I fixed to use wait group instead of it.
af57c01

Warashi · 2025-01-23T00:42:16Z

pkg/plugin/api/v1alpha1/common/common.proto

+// See the License for the specific language governing permissions and
+// limitations under the License.
+
+syntax = "proto3";


How about adding comments on where to use this package?
I imagine this common package to be used in the deployment/livestate package as a dependency, and I want to clarify it.

Sorry, I forgot to save the file. Added it
c7df424

How about this long comment?

"// package common defines common messages which are depended on by messages in packages under pkg/plugin/api/v1alpha1."

As Sawada-san mentioned, I want to clarify what common means because common package will be also visible to plugin developers.

Thanks, fixed it.
c5284f3

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

Warashi

Sorry, I forgot to review the test codes.
I commented on it.

Warashi · 2025-01-23T02:51:02Z

pkg/app/pipedv1/livestatereporter/livestatereporter_test.go

+		logger:          zaptest.NewLogger(b),
+	}
+
+	pr.flushSnapshots(context.Background())


There is no use of b *testing.B; what is this benchmark to measure?

I wanted to measure the time to execute it in parallel for reference.
I left it because It will be helpful to check the performance. WDYT?

Okay, I got it.
Then you should use testing.B as below.

for range b.N { pr.flushSnapshots(context.Background()) }

ref; https://pkg.go.dev/testing#hdr-Benchmarks

OK, I added it.
3adea7e

Warashi · 2025-01-23T02:52:41Z

pkg/app/pipedv1/livestatereporter/livestatereporter_test.go

+		logger:          zaptest.NewLogger(t),
+	}
+
+	pr.flushSnapshots(context.Background())


There are no assertion statements.
Does this test only assert that no panic occurred? If so, it's better to comment on that.

Thanks. I added a comment and a new test for flush to check the logic. (currently added only the success case)
8b8f4c6

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

Warashi

LGTM

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

Warashi

👍🏻

github-actions bot added area/web area/go area/pipedv1 labels Dec 26, 2024

ffjlabo force-pushed the start-livestatereporter-on-pipedv1 branch from fa22a0f to a6603eb Compare December 26, 2024 09:10

ffjlabo marked this pull request as ready for review December 26, 2024 09:33

ffjlabo requested review from Warashi, kentakozuka, khanhtc1202, nghialv and t-kikuc as code owners December 26, 2024 09:33

khanhtc1202 reviewed Dec 26, 2024

View reviewed changes

ffjlabo force-pushed the start-livestatereporter-on-pipedv1 branch from c1654db to 0c4fc0d Compare December 27, 2024 01:16

ffjlabo requested a review from khanhtc1202 December 27, 2024 01:24

ffjlabo enabled auto-merge (squash) December 27, 2024 01:24

Warashi previously approved these changes Dec 27, 2024

View reviewed changes

khanhtc1202 added the do-not-merge/wip label Dec 27, 2024

ffjlabo mentioned this pull request Jan 7, 2025

Add plugin registry #5467

Merged

ffjlabo dismissed Warashi’s stale review via 205c8fd January 17, 2025 04:37

ffjlabo force-pushed the start-livestatereporter-on-pipedv1 branch 2 times, most recently from 0e3e607 to cd2b7f1 Compare January 17, 2025 05:06

ffjlabo commented Jan 17, 2025

View reviewed changes

ffjlabo force-pushed the start-livestatereporter-on-pipedv1 branch from 08a4aa9 to c951ab9 Compare January 17, 2025 05:56

ffjlabo commented Jan 17, 2025

View reviewed changes

ffjlabo removed the do-not-merge/wip label Jan 17, 2025

ffjlabo added 4 commits January 22, 2025 18:16

Fix to store git repo in reporter

793c113

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

Disable livestatereporter

af0022c

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

Add comment

9fdd2d9

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

Fix not to use repo.Clean()

7eec825

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

ffjlabo force-pushed the start-livestatereporter-on-pipedv1 branch from e40079e to d14e25d Compare January 22, 2025 09:21

khanhtc1202 reviewed Jan 22, 2025

View reviewed changes

Refactor to put DeploySource in common package

11a9a14

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

ffjlabo force-pushed the start-livestatereporter-on-pipedv1 branch from d14e25d to 11a9a14 Compare January 22, 2025 10:08

Warashi reviewed Jan 23, 2025

View reviewed changes

ffjlabo added 3 commits January 23, 2025 10:36

Refactor: consider 0 case

5ea0571

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

Use waitGroup instead of errgroup

af57c01

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

Add comment for common proto

c7df424

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

ffjlabo requested review from Warashi and khanhtc1202 January 23, 2025 02:27

Warashi reviewed Jan 23, 2025

View reviewed changes

ffjlabo added 2 commits January 23, 2025 13:35

Add new test Test_reporter_flush

8b8f4c6

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

Add b.N

3adea7e

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

ffjlabo requested a review from Warashi January 23, 2025 04:46

Warashi previously approved these changes Jan 23, 2025

View reviewed changes

Warashi reviewed Jan 23, 2025

View reviewed changes

Fix comment

c5284f3

Signed-off-by: Yoshiki Fujikane <ffjlabo@gmail.com>

ffjlabo dismissed Warashi’s stale review via c5284f3 January 23, 2025 09:24

ffjlabo requested a review from Warashi January 23, 2025 09:25

Warashi approved these changes Jan 24, 2025

View reviewed changes

t-kikuc approved these changes Jan 24, 2025

View reviewed changes

ffjlabo merged commit f50cc80 into master Jan 24, 2025
21 checks passed

ffjlabo deleted the start-livestatereporter-on-pipedv1 branch January 24, 2025 05:14

github-actions bot mentioned this pull request Feb 17, 2025

update RELEASE to v0.50.2 #5571

Merged

github-actions bot mentioned this pull request Mar 14, 2025

Release version v0.50.3 #5665

Closed

github-actions bot mentioned this pull request Mar 25, 2025

Release v0.51.0 #5695

Merged

Conversation

ffjlabo commented Dec 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Dec 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

khanhtc1202 commented Dec 27, 2024

Uh oh!

ffjlabo commented Dec 27, 2024

Uh oh!

ffjlabo commented Dec 27, 2024

Uh oh!

Warashi left a comment

Choose a reason for hiding this comment

Uh oh!

ffjlabo commented Jan 6, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Warashi left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Warashi left a comment

Choose a reason for hiding this comment

Uh oh!

Warashi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ffjlabo commented Dec 26, 2024 •

edited

Loading

codecov bot commented Dec 26, 2024 •

edited

Loading