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

Implemented the backend of Metadata APIs as MetadataStoreRegistry #5471

Merged
merged 18 commits into from
Jan 21, 2025

Conversation

t-kikuc
Copy link
Member

@t-kikuc t-kikuc commented Jan 8, 2025

What this PR does:

  1. Created MetadataStoreRegistry
    • It contains metadataStores for deployments.
    • It's the backend of MetadataStore methods in the PluginAPI
    • It adds a new metadataStore for a deployment in controller by Register().
  2. Simplified reading metadata in planner & scheduler (5e3eea7)
    • They don't need metadataStore

Overview of New MetadataStore:
metadata-store-detail drawio

Why we need it:

  • This is the server-side implementation of MetadataStore methods of PluginServiceServer.
  • MetadataStoreRegistry is needed because PluginServiceServer does not exist per Deployment.

Note:

I want to refactor some points in pkg/app/pipedv1/metadatastore/store.go after this PR.

  • Convert some methods to be private (e.g. NewMetadataStore())
  • Use only deploymentID instead of the whole deployment (here)

I won't make drastic changes in the store.

Which issue(s) this PR fixes:

Part of #4980
Follows #5469

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):

Signed-off-by: t-kikuc <[email protected]>
Copy link
Contributor

@Warashi Warashi left a comment

Choose a reason for hiding this comment

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

The overview is good, and deleting the metadata store from the controller/planner is nice.

I have one concern about deployment metadata.
We will face a weird bug when multiple plugins use the same key to store deployment metadata.
I think it's good to append the plugin-name prefix to the deployment metadata key, WDYT?

@Warashi
Copy link
Contributor

Warashi commented Jan 9, 2025

Oops, we don't have a way to determine the plugin that requests piped to store deployment metadata. It's difficult to append the plugin-name prefix...

Warashi
Warashi previously approved these changes Jan 9, 2025
Copy link
Contributor

@Warashi Warashi left a comment

Choose a reason for hiding this comment

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

It's okay to start with this implementation.
Let's re-think this when we implement a way to determine which plugin sends requests to the piped service.

@t-kikuc
Copy link
Member Author

t-kikuc commented Jan 10, 2025

@Warashi
Thank you, in the next PR, I'll update to separate Deployment Metadata into (1)plugin-specific and (2)shared across piped and plugins.

A plugin will

  • a. explicitly specify the name of another plugin to access its metadata
  • b. use another method (GetDeploymentSharedMetadata()) for (2)

@t-kikuc
Copy link
Member Author

t-kikuc commented Jan 10, 2025

in the next PR, I'll update to separate Deployment Metadata into (1)plugin-specific and (2)shared across piped and plugins.

After merging this PR, I'll create a PR with this branch:
https://github.com/pipe-cd/pipecd/tree/plugin/metadatastore-v5

@t-kikuc
Copy link
Member Author

t-kikuc commented Jan 14, 2025

I solved the conflict: 3f448d0

Copy link

codecov bot commented Jan 14, 2025

Codecov Report

Attention: Patch coverage is 66.02564% with 53 lines in your changes missing coverage. Please review.

Project coverage is 26.31%. Comparing base (58fda21) to head (4c90308).
Report is 19 commits behind head on master.

Files with missing lines Patch % Lines
pkg/app/pipedv1/cmd/piped/grpcapi/plugin_api.go 0.00% 20 Missing ⚠️
pkg/app/pipedv1/metadatastore/registry.go 77.92% 12 Missing and 5 partials ⚠️
pkg/app/pipedv1/controller/controller.go 0.00% 11 Missing ⚠️
pkg/app/pipedv1/cmd/piped/piped.go 0.00% 3 Missing ⚠️
pkg/app/pipedv1/controller/planner.go 0.00% 1 Missing ⚠️
pkg/app/pipedv1/controller/scheduler.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5471      +/-   ##
==========================================
+ Coverage   26.24%   26.31%   +0.07%     
==========================================
  Files         458      462       +4     
  Lines       49364    49535     +171     
==========================================
+ Hits        12954    13036      +82     
- Misses      35383    35466      +83     
- Partials     1027     1033       +6     

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

@t-kikuc t-kikuc enabled auto-merge (squash) January 15, 2025 09:07
Warashi
Warashi previously approved these changes Jan 16, 2025
Copy link
Member

@ffjlabo ffjlabo left a comment

Choose a reason for hiding this comment

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

Almost all LGTM! I just left a comment for test.

Comment on lines 108 to 112
fmt.Printf("[DEBUG] store.shared: %+v\n", store.shared)
fmt.Printf("[DEBUG] store.plugins: %+v\n", store.plugins)
fmt.Printf("[DEBUG] store.plugins[plugin-1]: %+v\n", store.plugins["plugin-1"])
fmt.Printf("[DEBUG] store.stages: %+v\n", store.stages)
fmt.Printf("[DEBUG] store.stages[stage-1]: %+v\n", store.stages["stage-1"])
Copy link
Member

Choose a reason for hiding this comment

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

[nit] It would be nice to use t.Logf instead of fmt.Printf

Copy link
Member Author

Choose a reason for hiding this comment

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

@ffjlabo
Thank you, I fixed it: 4c90308.

Copy link
Member

@ffjlabo ffjlabo left a comment

Choose a reason for hiding this comment

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

LGTM!

Copy link
Contributor

@Warashi Warashi left a comment

Choose a reason for hiding this comment

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

LGTM

@t-kikuc t-kikuc merged commit 7f7d7ec into master Jan 21, 2025
18 checks passed
@t-kikuc t-kikuc deleted the plugin/metadatastore-v4 branch January 21, 2025 02:40
@github-actions github-actions bot mentioned this pull request Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants