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

Tansfer only application's manifests to cmp-server for plugin-based applications #17951

Open
jsolana opened this issue Apr 24, 2024 · 2 comments · May be fixed by #18054
Open

Tansfer only application's manifests to cmp-server for plugin-based applications #17951

jsolana opened this issue Apr 24, 2024 · 2 comments · May be fixed by #18054
Labels
enhancement New feature or request

Comments

@jsolana
Copy link

jsolana commented Apr 24, 2024

Summary

Reduce the volume of files transferred to the cmp-server during GenerateManifests op to optimize the manifest generation process for plugin-based applications.

Motivation

In monorepos, excessive data transfer during GenerateManifests can create bottlenecks by sending more information than the plugin actually requires to generate the manifest for an application. This inefficiency significantly impacts performance, especially with repeated operations.

The full repository (excluding globs) is being sent in each gRPC request to the cmp-server. For each call to the cmp-server, the repo-server creates a tar.gz of the entire repo and sends it, then the cmp-server has to decompress it and create all the files on the disk, run the generate command, and delete it. Most of the time is spent waiting for command executions, creating and deleting directories and files.

image

Proposal

Transmit only the necessary information to the cmp-server.

As a quick win, we can use appPath instead of repoPath in generateManifestsCMP for plugin-based applications.

reposerver/repository/repository.go:

// generateManifestsCMP will send the appPath files to the cmp-server over a gRPC stream.
// The cmp-server will generate the manifests. Returns a response object with the generated
// manifests.
func generateManifestsCMP(ctx context.Context, appPath string, env []string, cmpClient pluginclient.ConfigManagementPluginServiceClient, tarDoneCh chan<- bool, tarExcludedGlobs []string) (*pluginclient.ManifestResponse, error) {
	generateManifestStream, err := cmpClient.GenerateManifest(ctx, grpc_retry.Disable())
	if err != nil {
		return nil, fmt.Errorf("error getting generateManifestStream: %w", err)
	}
	opts := []cmp.SenderOption{
		cmp.WithTarDoneChan(tarDoneCh),
	}

	// In this case, we using use appPath instead of repoPath to only send app resources
	err = cmp.SendRepoStream(generateManifestStream.Context(), appPath, appPath, generateManifestStream, env, tarExcludedGlobs, opts...)
	if err != nil {
		return nil, fmt.Errorf("error sending file to cmp-server: %s", err)
	}

	return generateManifestStream.CloseAndRecv()
}

The screenshot below represents time duration of GenerataeManifests op before and after apply this quick win:

image

Note: A more general solution could involve leveraging the value described in the annotation argocd.argoproj.io/manifest-generate-paths. This approach can be extended to other solutions as kustomize or helm (to discuss maybe in another issue).

@jsolana
Copy link
Author

jsolana commented Apr 24, 2024

If it is ok, I can create the MR :)

@Oded-B
Copy link

Oded-B commented Apr 25, 2024

I'd use this.
But even if argocd.argoproj.io/manifest-generate-paths is set, and we use that value to copy files we would want to have this behavior off by default as some people might "include/import" out-of-app-folder files during manifest rendering, even if they didn't configure ArgoCD to trigger sync on those file changes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants