Add ops the ability to clean orphan commands #1434

sanposhiho · 2021-01-14T14:12:47Z

What this PR does / why we need it:

Create orphan commands cleaner. It periodically find orphan commands and mark them with TIMEOUT status

Which issue(s) this PR fixes:

Does this PR introduce a user-facing change?:

NONE

…ts status TIMEOUT

pipecd-bot · 2021-01-14T14:13:44Z

The following files are not gofmt-ed. By commenting /golinter fmt, the formatted one will be appended to this pull request automatically.

pkg/app/ops/orphancommandcleaner/orphancommandcleaner.go

--- pkg/app/ops/orphancommandcleaner/orphancommandcleaner.go.orig
+++ pkg/app/ops/orphancommandcleaner/orphancommandcleaner.go
@@ -4,9 +4,10 @@
 	"context"
 	"time"
 
+	"go.uber.org/zap"
+
 	"github.com/pipe-cd/pipe/pkg/datastore"
 	"github.com/pipe-cd/pipe/pkg/model"
-	"go.uber.org/zap"
 )
 
 var (

sanposhiho · 2021-01-14T14:14:48Z

/golinter fmt

pipecd-bot · 2021-01-14T14:20:58Z

Code coverage for golang is 33.96%. This pull request does not change code coverage.

…pipe into orphan-commands-timeout

pipecd-bot · 2021-01-14T14:56:13Z

Code coverage for golang is 33.96%. This pull request does not change code coverage.

sanposhiho · 2021-01-14T14:58:29Z

pkg/app/ops/orphancommandcleaner/orphancommandcleaner.go

+	commandTimeOut = 7 * 24 * time.Hour
+	interval       = 24 * time.Hour


What values should be set?

commandTimeOut = 24 * time.Hour
interval = 6 * time.Hour

sanposhiho · 2021-01-14T15:00:18Z

Is there a better place to create orphan commands cleaner instead of ops?

nghialv · 2021-01-15T01:52:21Z

cmd/pipecd/ops.go


+	// Starting orphan commands cleaner
+	cleaner := orphancommandcleaner.NewOrphanCommandCleaner(ds, t.Logger)
+	cleaner.Run(ctx)


This should be wrapped by errgroup to terminate ops when cleaner exit because of an error.

group.Go(func() error { return cleaner.Run(ctx) })

IMO, there was no need to stop ops because errors that occur in cleaner is probably caused by a network error when communicating with datastore.

Of course, it is necessary to output the error log to show that an error has occurred. However, I don't think it is necessary to stop ops because updateOrphanCommandsStatus will include the update that should have been done with the previous process if the previous process failed.

Or how about stopping ops if it fails several times in a row?

What do you think about it? 🤔

Nice comment. I got your points.
Agree with you that the cleaner may have some errors while executing its jobs.

But that is the problem of cleaner, not this run function.
This run should not care about what each component is doing, it just cares that they are fine or not. If any of them had a problem, ops should be stopped to notify that there was a problem from one of its components.
In other words, run should see all of its components (cleaner, http-server, insightcollector...) in the same way.

About the network problem or something that can cause the error while cleaner executing the job,
cleaner can ignore them, just log the error message, and does not return.
By that way, the ops will not be stopped by those errors and we can keep run to be nice too.

About the network problem or something that can cause the error while cleaner executing the job,
cleaner can ignore them, just log the error message, and does not return.
By that way, the ops will not be stopped by those errors and we can keep run to be nice too.

OK, I see 👍

nghialv · 2021-01-15T01:56:03Z

pkg/app/ops/orphancommandcleaner/orphancommandcleaner.go

+) *OrphanCommandCleaner {
+	return &OrphanCommandCleaner{
+		commandstore: datastore.NewCommandStore(ds),
+		logger:       logger.Named("orphan-command-finder"),


orphan-command-cleaner

nghialv · 2021-01-15T01:56:37Z

pkg/app/ops/orphancommandcleaner/orphancommandcleaner.go

+		if err := o.updateOrphanCommandsStatus(ctx); err != nil {
+			o.logger.Error("failed to update orphan commands", zap.Error(err))
+		}
+		time.Sleep(interval)


Let's use time.Ticker instead of sleep.

nghialv · 2021-01-15T01:57:09Z

pkg/app/ops/orphancommandcleaner/orphancommandcleaner.go

+	}
+}
+
+func (o *OrphanCommandCleaner) Run(ctx context.Context) {


nit: func(c * because it is a cleaner.

nghialv · 2021-01-15T01:59:06Z

pkg/app/ops/orphancommandcleaner/orphancommandcleaner.go

+}
+
+func (o *OrphanCommandCleaner) updateOrphanCommandsStatus(ctx context.Context) error {
+	timeoutedTime := time.Now().Add(-commandTimeOut).Unix()


nit: timedout or just timeout

nghialv · 2021-01-15T02:00:14Z

pkg/app/ops/orphancommandcleaner/orphancommandcleaner.go

+	}
+
+	for _, c := range commands {
+		o.commandstore.UpdateCommand(ctx, c.Id, func(c *model.Command) error {


Can you add a log to show that the command was successfully updated or not?

nghialv · 2021-01-15T02:01:06Z

Is there a better place to create orphan commands cleaner instead of ops?

I think the current place is fine.

nghialv · 2021-01-15T02:01:39Z

@sanposhiho Hi, Thank you for your great effort.
I have just left some comments. Please take a look.

pipecd-bot · 2021-01-15T13:13:59Z

Code coverage for golang is 33.96%. This pull request does not change code coverage.

nghialv · 2021-01-15T13:17:52Z

pkg/app/ops/orphancommandcleaner/orphancommandcleaner.go

+
+	t := time.NewTicker(interval)
+	for {
+		select {


Please check the context.Done to exit when ops was terminated.

for { select { case <- ctx.Done(): return nil case <- t.C: ... } }

Oops, thanks

nghialv · 2021-01-15T13:18:50Z

pkg/app/ops/orphancommandcleaner/orphancommandcleaner.go

+			cmd.Status = model.CommandStatus_COMMAND_TIMEOUT
+			return nil
+		}); err != nil {
+			c.logger.Error("failed to update orphan commands", zap.Error(err))


nit: commands - > command
And can you add the command ID and Type to the log message?

nghialv · 2021-01-15T13:21:43Z

pkg/app/ops/orphancommandcleaner/orphancommandcleaner.go

+				c.logger.Error("failed to update orphan commands", zap.Error(err))
+				consecutiveFailuresCount++
+				if consecutiveFailuresCount == maxConsecutiveFailuresCount {
+					return err


As I explained here, we can ignore this error.
Don't have to return. And we don't need the error log too because it is already logged inside the updateOrphanCommandsStatus function.

Btw, It would be nice if you can add a log to show the elapsed time of this execution.

…errors happened

sanposhiho · 2021-01-15T14:47:39Z

@nghialv
Thanks for your reviews, fixed them :D

pipecd-bot · 2021-01-15T14:53:13Z

Code coverage for golang is 33.96%. This pull request does not change code coverage.

nghialv · 2021-01-15T14:59:08Z

@sanposhiho Thank you very much for your great work! ❤️
/approve

pipecd-bot · 2021-01-15T14:59:11Z

This pull request is APPROVED by nghialv.

Approvers can cancel the approval by writing /approve cancel in a comment. Any additional commits also will change this pull request to be not-approved.

Create orphan commands cleaner that find orphan commands and update i…

dea8bbc

…ts status TIMEOUT

pipecd-bot added release-note-none do-not-merge/wip size/M area/ops area/server labels Jan 14, 2021

pipecd-bot requested review from khanhtc1202 and nakabonne January 14, 2021 14:12

Format unfmt-ed files

fabf13a

sanposhiho added 2 commits January 14, 2021 23:48

Fix BUILD.bazel

c7d7148

Merge branch 'orphan-commands-timeout' of https://github.com/pipe-cd/…

cf8d0dc

…pipe into orphan-commands-timeout

sanposhiho marked this pull request as ready for review January 14, 2021 14:56

pipecd-bot removed the do-not-merge/wip label Jan 14, 2021

sanposhiho commented Jan 14, 2021

View reviewed changes

nghialv reviewed Jan 15, 2021

View reviewed changes

sanposhiho added 3 commits January 15, 2021 17:32

Fix commandTimeout and interval

04a5385

Fix typo

105bbaa

Handle errors correctly

c0aaaf5

pipecd-bot added size/L and removed size/M labels Jan 15, 2021

nghialv reviewed Jan 15, 2021

View reviewed changes

sanposhiho added 3 commits January 15, 2021 22:24

Add context.Done

bd2af05

Add some information on error message

0512b9f

Logging only when successfuly processed, and not return even if some …

68021e7

…errors happened

pipecd-bot added the approved label Jan 15, 2021

nghialv changed the title ~~Create orphan commands cleaner~~ Add ops the ability to clean orphan commands Jan 15, 2021

pipecd-bot merged commit 2f95230 into master Jan 15, 2021

pipecd-bot deleted the orphan-commands-timeout branch January 15, 2021 15:06

pipecd-bot mentioned this pull request Jan 25, 2021

Update feature status #1474

Merged

		commandTimeOut = 7 * 24 * time.Hour
		interval = 24 * time.Hour

Add ops the ability to clean orphan commands #1434

Add ops the ability to clean orphan commands #1434

Uh oh!

Conversation

sanposhiho commented Jan 14, 2021

Uh oh!

pipecd-bot commented Jan 14, 2021

Uh oh!

sanposhiho commented Jan 14, 2021

Uh oh!

pipecd-bot commented Jan 14, 2021

Uh oh!

pipecd-bot commented Jan 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanposhiho commented Jan 14, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanposhiho Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nghialv Jan 15, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nghialv commented Jan 15, 2021

Uh oh!

nghialv commented Jan 15, 2021

Uh oh!

pipecd-bot commented Jan 15, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanposhiho commented Jan 15, 2021

Uh oh!

pipecd-bot commented Jan 15, 2021

Uh oh!

nghialv commented Jan 15, 2021

Uh oh!

pipecd-bot commented Jan 15, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

sanposhiho Jan 15, 2021 •

edited

Loading

nghialv Jan 15, 2021 •

edited

Loading