Implement rollback stage for kubernetes plugin #5390

Warashi · 2024-12-04T06:23:43Z

What this PR does:

Implement executeK8sRollbackStage
refactor duplicates of executeK8sSyncStage and executeK8sRollbackStage
- There are some duplicates remaining, but these parts has unimplemented TODOs. so I didn't refactor all of them.

Why we need it:

We have to support rollback of k8s stages

Which issue(s) this PR fixes:

Part of #4980

Does this PR introduce a user-facing change?: No

How are users affected by this change:
Is this breaking change:
How to migrate (if breaking change):

…text cancellation handling Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

codecov · 2024-12-04T06:28:04Z

Codecov Report

Attention: Patch coverage is 8.65385% with 95 lines in your changes missing coverage. Please review.

Project coverage is 25.81%. Comparing base (5831f36) to head (02a4df2).
Report is 3 commits behind head on master.

Files with missing lines	Patch %	Lines
...app/pipedv1/plugin/kubernetes/deployment/server.go	0.00%	80 Missing ⚠️
pkg/rpc/server.go	0.00%	6 Missing and 1 partial ⚠️
pkg/rpc/signal_interceptor.go	0.00%	5 Missing ⚠️
pkg/plugin/signalhandler/handler.go	81.81%	2 Missing ⚠️
pkg/app/pipedv1/plugin/kubernetes/server.go	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #5390      +/-   ##
==========================================
- Coverage   25.82%   25.81%   -0.01%     
==========================================
  Files         448      450       +2     
  Lines       48281    48337      +56     
==========================================
+ Hits        12468    12480      +12     
- Misses      34841    34885      +44     
  Partials      972      972

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

ffjlabo · 2024-12-04T07:18:21Z

pkg/app/pipedv1/plugin/kubernetes/deployment/server.go

+		if !response.GetStatus().IsCompleted() && errors.Is(ctx.Err(), context.Canceled) {
+			return
+		}


@Warashi
[Ask]
I thought that we should not mark the log persisted as completed when stage is still running regardless of context cancel.
Also, we should not mark the log persister as completed when the context is canceled.
WDYT?

it means we should !response.GetStatus().IsCompleted() || errors.Is(ctx.Err(), context.Canceled) ?

I realized that we have to handle two cancellations, but we cannot determine the cause because the cancel is propagated beyond the gRPC layer.

cancellation occurred by graceful shutdown.

in this case, we should not mark the log persister as completed.

cancellation occurred by the user via the web UI.

in this case, we should return the status as canceled and mark the log persister as completed.

I realized the plugin receives SIGTERM or SIGINT when the graceful shutdown is going, so I implemented the handling of them.
When the cancellation occurs without receiving such signals, the log persister should be completed.

…tion Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

…ister as completed Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi · 2024-12-06T02:17:37Z

I realized that the plugin should return the StageStatus and should not return an error even if the error has occurred.
Because the plugin can return StageStatus_FAILURE if the error occurs.
I'll change my codes.

👉🏻 DONE

…eStageResponse Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

khanhtc1202

LGTM with small nits

pkg/app/pipedv1/plugin/kubernetes/deployment/server.go

Co-authored-by: Khanh Tran <[email protected]> Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

khanhtc1202

LGTM 👍

ffjlabo · 2024-12-06T05:21:37Z

pkg/rpc/signal_interceptor.go

+// SignalHandlingInterceptor is a gRPC interceptor that cancels the context when a termination signal is received.
+func SignalHandlingInterceptor(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp any, err error) {
+	ctx, cancel := signal.NotifyContext(ctx, syscall.SIGINT, syscall.SIGTERM)
+	defer cancel()
+
+	return handler(ctx, req)
+}


@Warashi Sorry, I noticed that we can't distinguish Graceful Shutdown and context cancel for now. piped still treats both as context canceled.

We might use signal.Notify like this.

Suggested change

// SignalHandlingInterceptor is a gRPC interceptor that cancels the context when a termination signal is received.

func SignalHandlingInterceptor(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp any, err error) {

ctx, cancel := signal.NotifyContext(ctx, syscall.SIGINT, syscall.SIGTERM)

defer cancel()

return handler(ctx, req)

}

// SignalHandlingInterceptor is a gRPC interceptor that cancels the context when a termination signal is received.

var terminated atomic.Bool

// SignalHandlingInterceptor is a gRPC interceptor that cancels the context when a termination signal is received.

func SignalHandlingInterceptor(ctx context.Context, req any, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (resp any, err error) {

sigCh := make(chan os.Signal, 1)

signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM)

ctx, cancel := context.WithCancel(ctx)

defer cancel()

go func() {

select {

case <-sigCh:

// received a termination signal

terminated.Store(true)

cancel()

}

}()

return handler(ctx, req)

}

@ffjlabo
I implemented the determination of the Graceful Shutdown in this file. Is it not enough?

pkg/plugin/signalhandler/handler.go

@Warashi I got it. Sorry, I missed it. 🙏

ffjlabo

Thank you for the fix! left a nit comment.

ffjlabo · 2024-12-06T05:52:46Z

pkg/plugin/signalhandler/handler.go

+func init() {
+	ctx, cancel := signal.NotifyContext(context.Background(), signals...)
+	go func() {
+		defer cancel()
+		<-ctx.Done()
+		terminated.Store(true)
+	}()
+}


[nit] We should add a comment to describe that this distinguishes context.Cancel and Graceful shutdown.

I added the comments. Please check whether the comments are enough.
02a4df2

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

ffjlabo

LGTM 👍

Warashi added 3 commits December 4, 2024 15:09

Refactor ExecuteStage method to include log persister and improve con…

9836cdf

…text cancellation handling Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Implement K8S_ROLLBACK stage

baa8504

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Remove done TODO comment

d671df0

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi requested review from nghialv, khanhtc1202, kentakozuka, ffjlabo and t-kikuc as code owners December 4, 2024 06:23

github-actions bot added area/go area/pipedv1 labels Dec 4, 2024

ffjlabo reviewed Dec 4, 2024

View reviewed changes

Warashi added 6 commits December 6, 2024 10:57

Add signal handler for graceful termination and gRPC context cancella…

98a81bd

…tion Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Handle termination signal in ExecuteStage to prevent marking log pers…

aa60a82

…ister as completed Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Move the interceptor

8c22f27

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Add signal handling unary interceptor option to Server

6051530

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Add signal handling unary interceptor to server initialization

3c38f36

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Add signal handling unary interceptor to server run method

de55937

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi added the do-not-merge/wip label Dec 6, 2024

Refactor ExecuteStage methods to return StageStatus instead of Execut…

28588af

…eStageResponse Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi removed the do-not-merge/wip label Dec 6, 2024

Warashi requested a review from ffjlabo December 6, 2024 02:23

khanhtc1202 reviewed Dec 6, 2024

View reviewed changes

pkg/app/pipedv1/plugin/kubernetes/deployment/server.go Show resolved Hide resolved

Update comments on ExecuteStage

3d81f7b

Co-authored-by: Khanh Tran <[email protected]> Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi requested a review from khanhtc1202 December 6, 2024 03:47

khanhtc1202 previously approved these changes Dec 6, 2024

View reviewed changes

ffjlabo reviewed Dec 6, 2024

View reviewed changes

Add comments to signal handler for clarity on termination signals

02a4df2

Signed-off-by: Shinnosuke Sawada-Dazai <[email protected]>

Warashi dismissed khanhtc1202’s stale review via 02a4df2 December 6, 2024 05:58

Warashi requested review from ffjlabo and khanhtc1202 December 6, 2024 06:00

ffjlabo approved these changes Dec 6, 2024

View reviewed changes

khanhtc1202 approved these changes Dec 6, 2024

View reviewed changes

Warashi merged commit 1d3b532 into master Dec 6, 2024
16 of 18 checks passed

Warashi deleted the k8s-plugin-rollback branch December 6, 2024 07:06

github-actions bot mentioned this pull request Jan 21, 2025

Release v0.50.1 #5498

Merged

github-actions bot mentioned this pull request Feb 17, 2025

update RELEASE to v0.50.2 #5571

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement rollback stage for kubernetes plugin #5390

Implement rollback stage for kubernetes plugin #5390

Warashi commented Dec 4, 2024

codecov bot commented Dec 4, 2024 •

edited

Loading

ffjlabo Dec 4, 2024

Warashi Dec 4, 2024

Warashi Dec 6, 2024

Warashi commented Dec 6, 2024 •

edited

Loading

khanhtc1202 left a comment

khanhtc1202 left a comment

ffjlabo Dec 6, 2024

Warashi Dec 6, 2024

ffjlabo Dec 6, 2024

ffjlabo left a comment

ffjlabo Dec 6, 2024

Warashi Dec 6, 2024

ffjlabo left a comment

Implement rollback stage for kubernetes plugin #5390

Implement rollback stage for kubernetes plugin #5390

Conversation

Warashi commented Dec 4, 2024

codecov bot commented Dec 4, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Warashi commented Dec 6, 2024 • edited Loading

khanhtc1202 left a comment

Choose a reason for hiding this comment

khanhtc1202 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffjlabo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ffjlabo left a comment

Choose a reason for hiding this comment

codecov bot commented Dec 4, 2024 •

edited

Loading

Warashi commented Dec 6, 2024 •

edited

Loading