Trigger new deployment chain via piped api #2815
Conversation
|
The following files are not gofmt-ed. By commenting pkg/app/api/grpcapi/piped_api.go--- pkg/app/api/grpcapi/piped_api.go.orig
+++ pkg/app/api/grpcapi/piped_api.go
@@ -26,6 +26,7 @@
"google.golang.org/grpc/status"
"github.com/google/uuid"
+
"github.com/pipe-cd/pipe/pkg/app/api/analysisresultstore"
"github.com/pipe-cd/pipe/pkg/app/api/applicationlivestatestore"
"github.com/pipe-cd/pipe/pkg/app/api/commandstore"
|
|
Code coverage for golang is
|
|
The following files are not gofmt-ed. By commenting pkg/app/api/grpcapi/piped_api.go--- pkg/app/api/grpcapi/piped_api.go.orig
+++ pkg/app/api/grpcapi/piped_api.go
@@ -26,6 +26,7 @@
"google.golang.org/grpc/status"
"github.com/google/uuid"
+
"github.com/pipe-cd/pipe/pkg/app/api/analysisresultstore"
"github.com/pipe-cd/pipe/pkg/app/api/applicationlivestatestore"
"github.com/pipe-cd/pipe/pkg/app/api/commandstore"
|
1 similar comment
|
The following files are not gofmt-ed. By commenting pkg/app/api/grpcapi/piped_api.go--- pkg/app/api/grpcapi/piped_api.go.orig
+++ pkg/app/api/grpcapi/piped_api.go
@@ -26,6 +26,7 @@
"google.golang.org/grpc/status"
"github.com/google/uuid"
+
"github.com/pipe-cd/pipe/pkg/app/api/analysisresultstore"
"github.com/pipe-cd/pipe/pkg/app/api/applicationlivestatestore"
"github.com/pipe-cd/pipe/pkg/app/api/commandstore"
|
| string app_kind = 2; | ||
| map<string,string> app_labels = 3; | ||
| } | ||
|
|
There was a problem hiding this comment.
How about using the same Node as it is?
There was a problem hiding this comment.
tbh, I'm just want to rename the Node to ApplicationsFilter since those things are from the spec.postSync.chain configuration and at the time we load it from the configuration file, it's kind of filter used to find applications more than node or applications, the node word come when we create deployment chain, and applications come from users side, just to notify users that the filters written there used to find out applications for the chain.
There was a problem hiding this comment.
Ok. I get your points.
So it is time to find some good names for all those things: name in the config, name in gRPC request, name in chain model. 🤔
How do you think about this?
// config
type DeploymentChain struct {
Applications []ChainApplicationMatcher`json:"applications"`
// Conditions *ChainTriggerCondition `json:"conditions,omitempty"`
}
type ChainApplicationMatcher struct {
Name string `json:"name"`
Kind string `json:"kind"`
Labels map[string]string `json:"labels"`
}
// gRPC request
// use the same name with config
type CreateDeploymentChainRequest struct {
ApplicationMatcher {
}
ID string
Applications []ApplicationMatcher
}
// model in Proto format
type DeploymentChain struct {
ID string
Blocks []ChainBlock
}
type ChainBlock struct {
Nodes []ChainNode
}
type ChainNode struct {
Application *ChainApplicationRef
Deployment *ChainDeploymentRef
Runnable bool
}
// Just needed information to render on the web.
type ChainApplicationRef struct {
AppID string
AppName string
}
// Just needed information to render on the web.
// Basically we just need the "status" and ID to link to the actual one.
type ChainDeploymentRef struct {
DeploymentID string
...
CompletedAt int64
}There was a problem hiding this comment.
ChainBlock or ChainGroup (Maybe we should use ChainBlock to indicate that we are using Blockchain technology in our project. 🤣 )
There was a problem hiding this comment.
The control plane can run a simple check every time based on the deployment status to decide?
Yeh, the control-plane runs the check, but the pipedx who deploys the deployment just internally check does it should start triggering its deployment, so we need that runnable lock, I think
There was a problem hiding this comment.
I got your points. Looks nice.
Just a small thing, about where to put runnable field, if that field will be set by control-plne so I still prefer to put it in each node. By that way we still be abe to get what we want and beside that we can have more control on running order of each application. I mean controlling an application gives us more opportunity than controlling whole block.
(example, we allow apps in a block could be run serially.)
There was a problem hiding this comment.
okay, I get that. For now, it will be lock/unlock all nodes of a block at the same time, right?
There was a problem hiding this comment.
note: we may don't need that lock, since the control-plane knows when the previous block deployed successfully, one rpc call to ask control-plane whether the pipedx should start its deployment is enough 👍
| } | ||
|
|
||
| dc := model.DeploymentChain{ | ||
| Id: uuid.New().String(), |
There was a problem hiding this comment.
Maybe we should add this chain ID to the deployment model.
There was a problem hiding this comment.
You're right, we need that as a reference to display which chain this deployment belongs to in the deployment detailed page 👍
There was a problem hiding this comment.
The first deployment needs this ID as well. Do you think that we should create it by the first Piped?
There was a problem hiding this comment.
I see, in such case, the first piped will create deploymentChain object and register it to the control-plane just as we do in trigger.triggerDeployment currently, right?
There was a problem hiding this comment.
How do you think about adding a chain option to CreateDeploymentRequest and initializing chain commands in that API instead of creating this new CreateDeploymentChain?
In that way, we can have both chain ID and the first deployment ID at the same time.
There was a problem hiding this comment.
So you mean to make CreateDeploymentRequest has the ability to make chain commands to trigger all other applications in the chain and the RPC call to create deployment for the first application will do it? In that case, I wonder 2 things:
- we also need to provide matchers to filter out applications for chain block in that request body, right?
- though it's named
CreateDeployment, it may also createCommandin some conditions.
Besides, not sure what do you mean by "we can have both chain ID and the first deployment ID at the same time", but since we separate the RPC to create the first deployment of the chain with the deployment chain itself, we have to first: create the deployment chain and wait until it's succeeded, then: use the deployment chain id created to find the deployment chain object in the datastore and add the on going create deployment as the deploymentChain.block[0] item, that brings complicated to the CreateDeployment interface
Currently, that part is addressed here: https://github.com/pipe-cd/pipe/pull/2815/files#diff-dc8d28c3b055fcad0f022406b744ce756e3be8c630b6c40aef12e026c6a58dcfR1019-R1032
There was a problem hiding this comment.
we also need to provide matchers to filter out applications for chain block in that request body, right?
Yes, we will need those matchers.
not sure what do you mean by "we can have both chain ID and the first deployment ID at the same time"
I meant by that way, we can have chain ID, deployment objects, commands in the same API. Then the first deployment can have its needed chain ID seamlessly.
What I am concerned about is how to set the chain ID for the first deployment object. In the case of doing with separate RPCs, the flow will be like this, right?
- Piped generates chain ID locally
- Piped requests to create a new Deployment with the generated chain ID
- Piped requests to create a new Chain (commands, etc...)
IMO, generating the chain ID locally is not good. A Piped may send a chain ID of other projects (accidentally or intentionally).
So I think it would be better to avoid that. Another idea is reordering the above steps to
- Piped requests to create a new Chain (commands, etc..) and chain ID is returned
- Piped requests to create a new Deployment with the received chain ID
There was a problem hiding this comment.
Yeh, in that case, we have to use the chain id to find the chain and add deployment ref to chain: we have 2 different possible cases:
- the common case: the creating deployment come by pipedx when it's triggered by chain_sync_app command, and besides with the chain id, we also need block index to add the deployment to the right block
- the uncommon case: the creating deployment is the first deployment of the chain, in this case, we have to add a way to separate it with others, maybe by check for
matcherexisted in the request body or not, or specified block_index = 0 in the request body
Piped requests to create a new Chain (commands, etc..) and chain ID is returned
One minor point for this is, at the time when we make commands chain_sync_app for applications in the chain, the first application deployment is not yet in the block[0] of the deployment chain model, if pipedx get the command and ask (via RPC) controlplane should it handle its command, we don't have any "previous" stage to determine. To avoid that, we have to ensure commands only be created after the first deployment is available as chain block[0] by making it as currently or adding matcher to the CreateDeploymentRequest as optional. The new CreateDeploymentChain rpc only creates DeploymentChain model object with its Id, ProjectId, and an empty Blocks array, nothing else. All other stub on CreateDeploymentChain should be moved to CreateDeployment rpc. I'm okay with that, but just want to confirm 👍
If you're worried about the ChainId generated in the client (piped side), we have another way to avoid that. That is generate it in the controlplane (API side) and update this req.FirstDeployment here before actually create/add it to the datastore. How do you think about that?
|
Code coverage for golang is
|
|
/golinter fmt |
|
Code coverage for golang is
|
pkg/app/api/grpcapi/piped_api.go
Outdated
| Type: model.Command_SYNC_APPLICATION, | ||
| SyncApplication: &model.Command_SyncApplication{ | ||
| ApplicationId: app.Id, | ||
| SyncStrategy: model.SyncStrategy_AUTO, | ||
| }, |
There was a problem hiding this comment.
Maybe we need a way to distinguish from the normal SYNC command emitted via the web to have a better summary for its deployment.
https://github.com/pipe-cd/pipe/blob/master/pkg/app/piped/trigger/trigger.go#L259
There was a problem hiding this comment.
We are on the same page 👍 My plan is to make a new command type (such as IN_CHAIN_SYNC_APPLICATION) and filter out all commands with type prefix IN_CHAIN and handle those commands with OnChainDeterminer instead of OnCommandDeterminer. Also, the new command kind needs to have: deploymentChainId and NodeIndex at least so that the pipedx (who handles the command) will be able to update this deployment chain model its deployment state correctly. That is the thing I'm going to do with that TODO comment.
|
Code coverage for golang is
|
|
Looks like |
|
Code coverage for golang is
|
|
Code coverage for javascript is |
|
/hold |
**What this PR does / why we need it**: Rename as suggestion discussed in #2815 (comment) **Which issue(s) this PR fixes**: Fixes # **Does this PR introduce a user-facing change?**: <!-- If no, just write "NONE" in the release-note block below. --> ```release-note NONE ``` This PR was merged by Kapetanios.
|
Code coverage for golang is
|
| projectID, _, _, err := rpcauth.ExtractPipedToken(ctx) | ||
| if err != nil { | ||
| return nil, err | ||
| } |
There was a problem hiding this comment.
Add a check to ensure that the application of the requested deployment belongs to that this authenticated Piped.
|
Code coverage for javascript is |
pkg/app/api/grpcapi/piped_api.go
Outdated
| Type: model.Command_SYNC_APPLICATION, | ||
| SyncApplication: &model.Command_SyncApplication{ | ||
| ApplicationId: app.Id, | ||
| SyncStrategy: model.SyncStrategy_AUTO, |
There was a problem hiding this comment.
ChainID and BlockIndex will be added in separate PR, right?
pkg/app/piped/trigger/trigger.go
Outdated
| // Build deployment model and send a request to API to create a new deployment. | ||
| deployment, err := t.triggerDeployment(ctx, app, appCfg, branch, headCommit, commander, strategy, strategySummary) | ||
| // TODO: Add ability to get deployment chain id from CHAIN_SYNC_APPLICATION command. | ||
| var deploymentChainId string |
There was a problem hiding this comment.
| var deploymentChainId string | |
| var deploymentChainID string |
Co-authored-by: Le Van Nghia <[email protected]>
|
The golinter build is completed with FAILURE. The build will be triggered again when you push any other commits. Or you can trigger it manually by You can check the build log from here. |
|
/golinter trigger |
pipecd-bot
left a comment
pipecd-bot
left a comment
There was a problem hiding this comment.
Some issues were detected while linting go source files in your changes.
pkg/app/piped/trigger/deployment.go
Outdated
| strategySummary string, | ||
| now time.Time, | ||
| noti *config.DeploymentNotification, | ||
| deploymentChainId string, |
There was a problem hiding this comment.
func parameter deploymentChainId should be deploymentChainID
|
The following ISSUES will be created once got merged. If you want me to skip creating the issue, you can use Details1. Support find node apps by appKind and appLabels.This was created by todo plugin since "TODO:" was found in 604453f when #2815 was merged. cc: @khanhtc1202.2. Add ability to get deployment chain id from CHAIN_SYNC_APPLICATION command.This was created by todo plugin since "TODO:" was found in 604453f when #2815 was merged. cc: @khanhtc1202.3. Find a better way to ensure that the application should be updated correctlyThis was created by todo plugin since "TODO:" was found in 604453f when #2815 was merged. cc: @khanhtc1202. |
|
Great work! |
|
This pull request is APPROVED by Approvers can cancel the approval by writing |
|
Code coverage for golang is
|
|
Code coverage for javascript is |
**What this PR does / why we need it**: For deployment which be created to sync application in chain, we should add the ref to it in the chain model it belongs to, otherwise is keep as is for standalone deployment (deployment that does not belong to any chain called standalone deployment) **Which issue(s) this PR fixes**: Follow PR #2815 **Does this PR introduce a user-facing change?**: <!-- If no, just write "NONE" in the release-note block below. --> ```release-note NONE ``` This PR was merged by Kapetanios.
**What this PR does / why we need it**: This PR adds an RPC which be used by piped to determine whether to start planning a deployment that is triggered by the DeploymentChain model. **Which issue(s) this PR fixes**: Follow PR #2815 **Does this PR introduce a user-facing change?**: <!-- If no, just write "NONE" in the release-note block below. --> ```release-note NONE ``` This PR was merged by Kapetanios.
What this PR does / why we need it:
This PR provides:
spec.postSync.chainconfigurationWhich issue(s) this PR fixes:
Fixes #
Does this PR introduce a user-facing change?: