-
Notifications
You must be signed in to change notification settings - Fork 149
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 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
For sure
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 |
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.
Nice update.
pkg/app/api/grpcapi/web_api.go
Outdated
Apps: tidiedApps, | ||
}) | ||
} | ||
return repos |
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.
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
manifests
repo-id which is referring tomanifests-dev
repository - piped-prod also has a
manifests
repo-id which is referring tomanifests-prod
repository
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
|
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.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah okay I see, get the point here, thanks 👍
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.
Just created this issue for those things, lets think about it later 👍
#2865
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.
Nice 👍
Nice work |
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?: