Skip to content

Add check plannable deployment logic #2844

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.

Sign up for GitHub

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

Merged
pipecd-bot merged 3 commits into from
Nov 25, 2021
Merged

Add check plannable deployment logic #2844

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d849f88
Add check plannable deployment logic
khanhtc1202 Nov 25, 2021
6693a82
Update check plannable logic
khanhtc1202 Nov 25, 2021
87054ac
Only submit required fields on check plannable deployment
khanhtc1202 Nov 25, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 5 additions & 5 deletions pkg/app/api/grpcapi/piped_api.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1136,28 +1136,28 @@ func (a *PipedAPI) InChainDeploymentPlannable(ctx context.Context, req *pipedser
if err != nil {
return nil, err
}
if err := a.validateDeploymentBelongsToPiped(ctx, req.Deployment.Id, pipedID); err != nil {
if err := a.validateDeploymentBelongsToPiped(ctx, req.DeploymentId, pipedID); err != nil {
return nil, err
}

dc, err := a.deploymentChainStore.GetDeploymentChain(ctx, req.Deployment.DeploymentChainId)
dc, err := a.deploymentChainStore.GetDeploymentChain(ctx, req.DeploymentChainId)
if err != nil {
return nil, status.Error(codes.InvalidArgument, "unable to find the deployment chain which this deployment belongs to")
}

// Deployment of blocks[0] in the chain means it's the first deployment of the chain;
// hence it should be processed without any lock.
if req.Deployment.DeploymentChainBlockIndex == 0 {
if req.DeploymentChainBlockIndex == 0 {
return &pipedservice.InChainDeploymentPlannableResponse{
Plannable: true,
}, nil
}

if req.Deployment.DeploymentChainBlockIndex >= uint32(len(dc.Blocks)) {
if req.DeploymentChainBlockIndex >= uint32(len(dc.Blocks)) {
return nil, status.Error(codes.InvalidArgument, "invalid deployment with chain block index provided")
}

prevBlock := dc.Blocks[req.Deployment.DeploymentChainBlockIndex-1]
prevBlock := dc.Blocks[req.DeploymentChainBlockIndex-1]
plannable := true
for _, node := range prevBlock.Nodes {
// TODO: Consider add deployment status to the deployment ref in the deployment chain model
Expand Down
4 changes: 3 additions & 1 deletion pkg/app/api/service/pipedservice/service.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -501,7 +501,9 @@ message CreateDeploymentChainResponse {
}

message InChainDeploymentPlannableRequest {
pipe.model.Deployment deployment = 1 [(validate.rules).message.required = true];
string deployment_id = 1 [(validate.rules).string.min_len = 1];
string deployment_chain_id = 2 [(validate.rules).string.min_len = 1];
uint32 deployment_chain_block_index = 3;
}

message InChainDeploymentPlannableResponse {
Expand Down
43 changes: 43 additions & 0 deletions pkg/app/piped/controller/controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,6 +54,8 @@ type apiClient interface {
SaveStageMetadata(ctx context.Context, req *pipedservice.SaveStageMetadataRequest, opts ...grpc.CallOption) (*pipedservice.SaveStageMetadataResponse, error)
ReportStageLogs(ctx context.Context, req *pipedservice.ReportStageLogsRequest, opts ...grpc.CallOption) (*pipedservice.ReportStageLogsResponse, error)
ReportStageLogsFromLastCheckpoint(ctx context.Context, in *pipedservice.ReportStageLogsFromLastCheckpointRequest, opts ...grpc.CallOption) (*pipedservice.ReportStageLogsFromLastCheckpointResponse, error)

InChainDeploymentPlannable(ctx context.Context, in *pipedservice.InChainDeploymentPlannableRequest, opts ...grpc.CallOption) (*pipedservice.InChainDeploymentPlannableResponse, error)
}

type gitClient interface {
Expand Down Expand Up @@ -369,6 +371,32 @@ func (c *controller) syncPlanners(ctx context.Context) error {
}

for appID, d := range pendingByApp {
plannable, err := c.shouldStartPlanningDeployment(ctx, d)
if err != nil {
c.logger.Error("failed to check deployment plannability",
zap.String("deployment", d.Id),
zap.String("app", d.ApplicationId),
zap.Error(err),
)
continue
}

if !plannable {
if d.IsInChainDeployment() {
c.logger.Info("unable to start planning deployment, probably locked by the previous block in its deployment chain",
zap.String("deployment_chain", d.DeploymentChainId),
zap.String("deployment", d.Id),
zap.String("app", d.ApplicationId),
)
} else {
c.logger.Info("unable to start planning deployment, try again next sync interval",
zap.String("deployment", d.Id),
zap.String("app", d.ApplicationId),
)
}
continue
}

Copy link
Member

@nghialv nghialv Nov 25, 2021

Choose a reason for hiding this comment

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

Do we have any way to show the reason why a deployment has not been planned on the web?
Should we set that message by trigger or planner?

Copy link
Member Author

@khanhtc1202 khanhtc1202 Nov 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Currently, we don't have, since the state of the deployment is not changed, the message for that keep PENDING may be redundant to me. If we want to show kind of reason why the deployment keeps pending, we may need a new RPC like UpdateDeploymentStatusReason or such. How do you think?

Copy link
Member

@nghialv nghialv Nov 25, 2021

Choose a reason for hiding this comment

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

I just wanted to ask to know did you have any plan for that thing yet.
Could you create a new issue to ensure that it could not be flushed out of our memory over time?
Btw, I will approve this PR since that is out of the scope of this PR.

Copy link
Member Author

@khanhtc1202 khanhtc1202 Nov 25, 2021

Choose a reason for hiding this comment

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

I see, in that case

Should we set that message by trigger or planner?

Probably not the trigger since after triggering the deployment to start (PENDING state) the trigger is out of action, and this check is plannable logic run in syncPlaner phase but before a planner starts running, so the one who submits that message should be controller itself, via kind of UpdateDeploymentStatusReason RPC (the deployment state is unchanged but update the desc?) that is in my image. Let's me think about that in detail later (will create issue for that)

Copy link
Member Author

@khanhtc1202 khanhtc1202 Nov 25, 2021

Choose a reason for hiding this comment

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

created #2848 🙏

planner, err := c.startNewPlanner(ctx, d)
if err != nil {
c.logger.Error("failed to start a new planner",
Expand Down Expand Up @@ -643,6 +671,21 @@ func (c *controller) getMostRecentlySuccessfulDeployment(ctx context.Context, ap
return nil, err
}

func (c *controller) shouldStartPlanningDeployment(ctx context.Context, d *model.Deployment) (bool, error) {
if !d.IsInChainDeployment() {
return true, nil
}
resp, err := c.apiClient.InChainDeploymentPlannable(ctx, &pipedservice.InChainDeploymentPlannableRequest{
DeploymentId: d.Id,
DeploymentChainId: d.DeploymentChainId,
DeploymentChainBlockIndex: d.DeploymentChainBlockIndex,
})
if err != nil {
return false, err
}
return resp.Plannable, nil
}

type appLiveResourceLister struct {
lister liveResourceLister
cloudProvider string
Expand Down
6 changes: 6 additions & 0 deletions pkg/model/deployment.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -219,3 +219,9 @@ func (d *Deployment) ContainLabels(labels map[string]string) bool {
}
return true
}

// IsInChainDeployment returns true if the current deployment belongs
// to a deployment chain.
func (d *Deployment) IsInChainDeployment() bool {
return d.DeploymentChainId != ""
}