Quit all processes when "apiserver-boot run local" failed #255

interma · 2018-07-20T11:14:45Z

Glad to open my first PR, thanks for your review.

ghost · 2018-07-20T12:31:35Z

I can't review this, but I'd suggest you do the following:

Create a new context.Context
Create a cancel function from it
Give all processes the cancel function and context
In the run functions create a new <-chan error, run cmd.Run in a new goroutine handing over the channel and sending the return value of that to the channel as soon as it exits
outside the goroutine select from context.Done() and the channel you created.
- If your channel case is hit, cancel the context and return
- If the context is canceled by another process exiting kill the process and return
make sure your context also cancels when the process receives an Interrupt or Kill signal (Ctrl+C in console for instance)

func cancelWhenSignaled(parent context.Context) context.Context {
	ctx, cancel := context.WithCancel(parent)

	go func() {
		signalChannel := make(chan os.Signal)
		signal.Notify(signalChannel, os.Interrupt, os.Kill)
		<-signalChannel
		cancel()
	}()

	return ctx
}

interma · 2018-07-23T04:03:50Z

Thanks @damongant
I will update later.

interma · 2018-07-23T10:20:32Z

@damongant
I have refined quit logic as your advice (yes, it more makes sense). Did I catch your idea?

Thanks for your help.

ghost · 2018-07-23T10:45:40Z

cmd/apiserver-boot/boot/run/local.go

-			log.Printf("Failed to run controller-manager %v", err)
-			stopCh <- struct{}{}
+			log.Printf("Failed to run %s, error: %v\n", cmdName, err)
+			stopCh <- err


You can save this line and the else and just stopCh<-err - it'll be nil in the else case either way.

ok, I will update this.

ghost · 2018-07-23T10:46:42Z

cmd/apiserver-boot/boot/run/local.go

+		cancel()
+	case <-ctx.Done():
+		// other commands quited
+		cmd.Process.Kill()


do we need to check if cmd.Process is nil here? Unfortunate timing with 2 processes exiting could produce a segfault here afaik, but I'm not sure.

Yes, check nil is better, I will update them later.

ghost · 2018-07-23T10:48:12Z

LGTM overall, didn't have a chance to test yet.

interma · 2018-07-24T09:52:49Z

@pwittrock Could you help to review this?
Thanks.

fejta-bot · 2019-04-24T19:49:47Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

ghost · 2019-04-25T07:50:54Z

I'd hate for this contribution to go to waste because of inactive maintainers.

wink wink nudge nudge

/remove-lifecycle stale

yue9944882

apologies for delayed response, i'm having trouble finding out active threads in the pool 🧐

yue9944882 · 2019-04-25T12:33:35Z

cmd/apiserver-boot/boot/util/util.go

@@ -139,3 +142,16 @@ func CheckInstall() {
 			strings.Join(missing, ","))
 	}
 }
+
+func CancelWhenSignaled(parent context.Context) context.Context {


reusing existing signal handler from here seems better to me :

https://github.com/kubernetes/kubernetes/blob/7f23a743e8c23ac6489340bbb34fa6f1d392db9d/staging/src/k8s.io/sample-controller/pkg/signals/signal.go#L29

@interma any plan to bump the thread?

double-receiving should be a fine addition to this piece of code if that's what you mean, but the context is also used to support cancelling when one of the processes unexpectedly quits.

double-receiving should be a fine addition to this piece of code if that's what you mean

for now we're not catching signal in the code so there's no double-receiving. i mean the pull LGTM overall, but we should reuse the existing code from upstream as much as possible.

I've attempted to address this final code-review comment in #410

It'd be great to get this fixed in the next release.

Trouble is that importing SetupSignalHandler from k8s.io/apiserver causes a clash with the k8s.io/kube-openapi dependency.
So I copied the code across instead.

interma · 2019-04-26T06:16:13Z

I remember apiserver-builder is deprecated last year, reloads now?
What's the purpose and roadmap now?
Thanks.

yue9944882 · 2019-04-26T06:26:53Z

I remember apiserver-builder is deprecated last year, reloads now?

for now, we don't see a better replacement tooling/SDK for AA server so we're still delivering it. actually there were supposed to be a replacement for AA-builder, we planed and named it apiserver-runtime. but out of some reason, it's delayed and we don't have a clear roadmap for it yet.

What's the purpose and roadmap now?

generally, we're aiming at converging this project w/ kubebuilder (which is now actively maintained by phil and his team) to share the same framework like controller-runtime etc. fwiw the point of this project is reusing the wheels from kube-builder as much as possible and extending features beyond CRD's capability. CRD's going more light-weight and better integrated w/ openapi-schema and AA's going more extensible for customization.

fejta-bot · 2019-07-25T06:33:31Z

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

fejta-bot · 2019-08-24T06:34:41Z

Stale issues rot after 30d of inactivity.
Mark the issue as fresh with /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle rotten

wallrj · 2019-08-31T11:34:27Z

@interma I forked your branch and addressed the remaining code review feedback in #410 which has now been merged, so this can now be closed. I hope you don't mind.

interma · 2019-09-02T02:45:19Z

@wallrj Ok, no problem, thanks!

quit all processes when "apiserver-boot run local" failed

65f4c53

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Jul 20, 2018

interma mentioned this pull request Jul 20, 2018

Should quit all processes in "apiserver-boot run local" when one of them failed. #253

Closed

Refine quit logic via context.WithCancel

2098e37

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jul 23, 2018

ghost reviewed Jul 23, 2018

View reviewed changes

check process nil to prevent concurrent quit

b05e212

interma mentioned this pull request Dec 12, 2018

REQUEST: New membership for interma kubernetes/org#301

Closed

6 tasks

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 24, 2019

k8s-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 25, 2019

yue9944882 reviewed Apr 25, 2019

View reviewed changes

k8s-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jul 25, 2019

k8s-ci-robot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels Aug 24, 2019

wallrj mentioned this pull request Aug 30, 2019

Quit all processes when "apiserver-boot run local" failed #410

Merged

interma closed this Sep 2, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Quit all processes when "apiserver-boot run local" failed #255

Quit all processes when "apiserver-boot run local" failed #255

interma commented Jul 20, 2018

ghost commented Jul 20, 2018 •

edited by ghost

Loading

interma commented Jul 23, 2018

interma commented Jul 23, 2018

ghost Jul 23, 2018

interma Jul 24, 2018

ghost Jul 23, 2018

interma Jul 24, 2018

ghost commented Jul 23, 2018

interma commented Jul 24, 2018

fejta-bot commented Apr 24, 2019

ghost commented Apr 25, 2019

yue9944882 left a comment

yue9944882 Apr 25, 2019

yue9944882 Apr 25, 2019

ghost Apr 25, 2019

yue9944882 Apr 25, 2019

wallrj Aug 30, 2019

wallrj Aug 30, 2019

interma commented Apr 26, 2019

yue9944882 commented Apr 26, 2019

fejta-bot commented Jul 25, 2019

fejta-bot commented Aug 24, 2019

wallrj commented Aug 31, 2019

interma commented Sep 2, 2019

Quit all processes when "apiserver-boot run local" failed #255

Quit all processes when "apiserver-boot run local" failed #255

Conversation

interma commented Jul 20, 2018

ghost commented Jul 20, 2018 • edited by ghost Loading

interma commented Jul 23, 2018

interma commented Jul 23, 2018

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

ghost commented Jul 23, 2018

interma commented Jul 24, 2018

fejta-bot commented Apr 24, 2019

ghost commented Apr 25, 2019

yue9944882 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

interma commented Apr 26, 2019

yue9944882 commented Apr 26, 2019

fejta-bot commented Jul 25, 2019

fejta-bot commented Aug 24, 2019

wallrj commented Aug 31, 2019

interma commented Sep 2, 2019

ghost commented Jul 20, 2018 •

edited by ghost

Loading