Add RPCs to list unregistered apps #2847
Conversation
|
Code coverage for golang is
|
pkg/app/api/grpcapi/piped_api.go
Outdated
| @@ -58,13 +58,15 @@ type PipedAPI struct { | |||
| deploymentPipedCache cache.Cache | |||
| envProjectCache cache.Cache | |||
| pipedStatCache cache.Cache | |||
| // A map from projectID to map["repo-id"][]*model.ApplicationInfo | |||
| unregisteredAppsCache cache.Cache | |||
There was a problem hiding this comment.
Grouping by project ID like this could cause a problem where data is overridden between multiple Pipeds of the same project. (a kind of race condition).
For example, Piped-1 is watching Repo-1, Piped-2 is watching Repo-2, Piped-1 and Piped-2 send their list to the control plane for the same project. So in some conditions, Piped-1's data will be overridden by Piped'2 data.
Besides that, I think the data structure map["repo-id"][]*model.ApplicationInfo inside each project key could not work well since multiple Pipeds can watch the same repository and all of them are sending an unregistered list to the control plane.
There was a problem hiding this comment.
That's the case. Let me re-think what structure is for us.
cmd/pipecd/server.go
Outdated
| @@ -188,6 +189,7 @@ func (s *server) run(ctx context.Context, input cli.Input) error { | |||
| is := insightstore.NewStore(fs) | |||
| cmdOutputStore := commandoutputstore.NewStore(fs, input.Logger) | |||
| statCache := rediscache.NewHashCache(rd, defaultPipedStatHashKey) | |||
| unregisteredAppsCache := memorycache.NewTTLCache(ctx, 24*time.Hour, 3*time.Hour) | |||
There was a problem hiding this comment.
Memory cache will not work for us in this case so we have to use redis cache instead.
Request from Piped and Web can be routed and handled by a random server instance so memory cache is useless.
|
The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use Details1. Refactor and fall into independent functions and write testsThis was created by todo plugin since "FIXME:" was found in 589f433 when #2847 was merged. cc: @nakabonne. |
|
Code coverage for golang is
|
|
Code coverage for golang is
|
|
@nghialv Thank you for pointing it out! I made a couple of changes. First, I settled on caching them in completely separate keys so that there would be no cache conflicts between Piped. I also settled on using the Redis Hash type to hierarchize the cache keys and enumerate all the applications associated with the Project when listing them. Please have a look again when you have time (obviously fine next week). |
|
Code coverage for golang is
|
|
Nice. Let me check them. 👍 |
pkg/app/api/grpcapi/piped_api.go
Outdated
| projectID, pipedID, _, err := rpcauth.ExtractPipedToken(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| key := makeUnregisteredAppsCacheKey(projectID) | ||
| c := rediscache.NewHashCache(a.redis, key) | ||
| // Cache a slice of *model.ApplicationInfo. | ||
| if err := c.Put(pipedID, req.Applications); err != nil { | ||
| return nil, status.Error(codes.Internal, "failed to put the unregistered apps to the cache") | ||
| } | ||
|
|
||
| return &pipedservice.ReportUnregisteredApplicationConfigurationsResponse{}, nil |
pkg/app/api/grpcapi/web_api.go
Outdated
| Apps: tidiedApps, | ||
| }) | ||
| } | ||
| return repos |
There was a problem hiding this comment.
There are some concerns around the way we are grouping applications by repo-id and eliminating duplicates by GitPath.
Since we don't have any guide or restriction on the repository ID across Piped currently, so users can use the same repo ID in different Pipeds for different repositories.
For example:
- piped-dev has a
manifestsrepo-id which is referring tomanifests-devrepository - piped-prod also has a
manifestsrepo-id which is referring tomanifests-prodrepository
That will result in a not-correct list.
So I think we can return the raw data from the cache that includes PipedID in the response.
There was a problem hiding this comment.
For sure. Obviously repo-id isn't a project-wide concept. Apparently It's enough to give back directly grouped by Piped.
There was a problem hiding this comment.
The response structure depends on how to show the list on the adding form. Of course it will be able to change, but tell me a little bit more about your thoughts.
I'm guessing the unregistered list on the web is going to be kind of like:
- piped-1
- app-1 (repo: "repo-1", path: "path/to", cfgFilename: ".pipe.yaml")
- app-2 (repo: "repo-2", path: "path/to", cfgFilename: ".pipe.yaml")
- piped-2
- ...
Therefore, the response of ListUnregisteredApplications would be like:
message ListUnregisteredApplicationsResponse {
message Piped {
string id = 1;
repeated model.ApplicationInfo apps = 2;
}
repeated Piped pipeds = 1;
}But if it's enough to show a flat list like:
- app-1 (piped: "piped-1", repo: "repo-1", path: "path/to", cfgFilename: ".pipe.yaml")
- app-2 (piped: "piped-2", repo: "repo-1", path: "path/to", cfgFilename: ".pipe.yaml")
just add a field called PipedId to ApplicationInfo and return a list of ApplicationInfo.
|
I made a change based on the assumption that it's enough to show a flat list like (the second idea). If you have other ideas, I will kindly revert them. |
|
Code coverage for golang is
|
pkg/app/api/grpcapi/web_api.go
Outdated
| // Collect all apps that belong to the project. | ||
| key := makeUnregisteredAppsCacheKey(claims.Role.ProjectId) | ||
| c := rediscache.NewHashCache(a.redis, key) | ||
| // pipedToApps assumes to be a map["piped-id"][]*model.ApplicationInfo |
There was a problem hiding this comment.
The returned data will not be decoded to this *model.ApplicationInfo automatically to be ready to use since Redis doesn't have any way to know how the data was formatted. It just stored binary data.
So I think we have to encode (e.g. by JSON) the list before storing into Redis and decode them after getting back from Redis.
There was a problem hiding this comment.
Seriously? I am sorry, I had neglected to do my research. I assumed that redigo encodes/decodes using some kind of ways (such as gob). Let me look into how redigo encodes and re-apply new encoding way for us.
|
The following files are not gofmt-ed. By commenting pkg/app/piped/appconfigreporter/appconfigreporter_test.go--- pkg/app/piped/appconfigreporter/appconfigreporter_test.go.orig
+++ pkg/app/piped/appconfigreporter/appconfigreporter_test.go
@@ -118,7 +118,7 @@
{
name: "app changed",
reporter: &Reporter{
- config: &config.PipedSpec{PipedID: "piped-1"},
+ config: &config.PipedSpec{PipedID: "piped-1"},
applicationLister: &fakeApplicationLister{apps: []*model.Application{
{Id: "id-1", Name: "app-1", Labels: map[string]string{"key-1": "value-1"}, GitPath: &model.ApplicationGitPath{Repo: &model.ApplicationGitRepository{Id: "repo-1"}, Path: "app-1", ConfigFilename: ".pipe.yaml"}},
}},
@@ -145,7 +145,7 @@
RepoId: "repo-1",
Path: "app-1",
ConfigFilename: ".pipe.yaml",
- PipedId: "piped-1",
+ PipedId: "piped-1",
},
},
wantErr: false,
@@ -228,7 +228,7 @@
{
name: "valid app config that is unregistered",
reporter: &Reporter{
- config: &config.PipedSpec{PipedID: "piped-1"},
+ config: &config.PipedSpec{PipedID: "piped-1"},
applicationLister: &fakeApplicationLister{},
fileSystem: fstest.MapFS{
"path/to/repo-1/app-1/.pipe.yaml": &fstest.MapFile{Data: []byte(`
@@ -253,7 +253,7 @@
RepoId: "repo-1",
Path: "app-1",
ConfigFilename: ".pipe.yaml",
- PipedId: "piped-1",
+ PipedId: "piped-1",
},
},
wantErr: false,
@@ -261,7 +261,7 @@
{
name: "valid app config that name isn't default",
reporter: &Reporter{
- config: &config.PipedSpec{PipedID: "piped-1"},
+ config: &config.PipedSpec{PipedID: "piped-1"},
applicationLister: &fakeApplicationLister{},
fileSystem: fstest.MapFS{
"path/to/repo-1/app-1/dev.pipecd.yaml": &fstest.MapFile{Data: []byte(`
@@ -286,7 +286,7 @@
RepoId: "repo-1",
Path: "app-1",
ConfigFilename: "dev.pipecd.yaml",
- PipedId: "piped-1",
+ PipedId: "piped-1",
},
},
wantErr: false,
|
|
Code coverage for golang is
|
|
@nghialv As you mentioned,
Therefore, I settled on employing the gob format which can be encoded in a self-descriptive manner and is tiny in size compared to JSON, which tends to be more redundant than necessary in text encoding. |
|
Code coverage for golang is
|
|
It is unlikely that there will be a large number of unregistered applications, so to be honest, JSON is fine as well, or rather it could be better as it can be written more simply. |
Co-authored-by: Le Van Nghia <nghialv2607@gmail.com>
|
Nice. |
|
Code coverage for golang is
|
| @@ -82,6 +87,7 @@ func NewPipedAPI(ctx context.Context, ds datastore.DataStore, sls stagelogstore. | |||
| deploymentPipedCache: memorycache.NewTTLCache(ctx, 24*time.Hour, 3*time.Hour), | |||
| envProjectCache: memorycache.NewTTLCache(ctx, 24*time.Hour, 3*time.Hour), | |||
| pipedStatCache: hc, | |||
| redis: rd, | |||
There was a problem hiding this comment.
should we use the redis explicitly here or just pass pipedCfgCache as we do for pipedStatCache? 🤔
Since, for instance, we may want to support not just redis but memcached as shared cache in the future, should keep all of those specified things out of our logic.
There was a problem hiding this comment.
I feel you. Honestly I also want to abstract it in such a way. But we want to generate HashCache with different keys for each project in the method, that's why PipedAPI needs to have a connection to the redis server.
There was a problem hiding this comment.
ah okay I see, get the point here, thanks 👍
There was a problem hiding this comment.
Just created this issue for those things, lets think about it later 👍
#2865
|
Nice work |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
What this PR does / why we need it:
This PR primarily adds two RPCs that:
I'm looking to show unregistered apps on the web like:That's why the cached structure is a bit nested.The structure has been slightly changed, see #2847 (comment) for more details.
Which issue(s) this PR fixes:
Fixes #2804
Does this PR introduce a user-facing change?: