Skip to content
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.

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

feat: run manager-api as an OS agnostic service #1667

Merged
merged 9 commits into from
Apr 30, 2021

Conversation

bisakhmondal
Copy link
Member

@bisakhmondal bisakhmondal commented Mar 24, 2021

Please answer these questions before submitting a pull request, or your PR will get closed.

Why submit this pull request?

  • Bugfix
  • New feature provided
  • Improve performance
  • Backport patches

What changes will this PR take into?
Currently to run manager-api as a background process is through nohup to avoid unintended SIGHUP from the shell used for initialization.
This PR focuses on running manager-api as an OS agnostic service.

to run,

./manager-api
./manager-api start
./manager-api stop
./manager-api status #(option to check service status from the binary itself without depending on os specific service manager)
./manager-api remove #(would be helpful deleting service test file without manually navigating to the filesystem)
./manager-api install #(reinstalling the service, if the binary location needs to be changed in future, the service must be reinstalled. Also in newer releases if we introduce extra flags, that should be passed as arguments for ExecStart of the service unit file i.e. need reinstallation)

Related issues
#842

Checklist:

  • Did you explain what problem does this PR solve? Or what new features have been added?
  • Have you added corresponding test cases?
  • Have you modified the corresponding document?
  • Is this PR backward compatible? If it is not backward compatible, please discuss on the mailing list first

@bisakhmondal
Copy link
Member Author

Backend CLI Test is failing for changing
log.Infof("The Manager API server receive %s and start shutting down", sig.String())
to
log.Infof("The Manager API server receives %s and starts acting up", sig.String())
hehe :)

@nic-chen
Copy link
Member

Backend CLI Test is failing for changing
log.Infof("The Manager API server receive %s and start shutting down", sig.String())
to
log.Infof("The Manager API server receives %s and starts acting up", sig.String())
hehe :)

hi
Can it be solved by modifying the test's assertion?

@bisakhmondal
Copy link
Member Author

ya sure :)

@bisakhmondal bisakhmondal marked this pull request as ready for review March 27, 2021 05:50
@bisakhmondal
Copy link
Member Author

bisakhmondal commented Mar 27, 2021

pinging @nic-chen for review when you have time.

ps. reverted all the changes made to manger-api back.

@codecov-io
Copy link

Codecov Report

Merging #1667 (636009a) into master (bec8f1b) will increase coverage by 1.48%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1667      +/-   ##
==========================================
+ Coverage   72.48%   73.96%   +1.48%     
==========================================
  Files         133       86      -47     
  Lines        5728     2612    -3116     
  Branches      666      666              
==========================================
- Hits         4152     1932    -2220     
+ Misses       1332      680     -652     
+ Partials      244        0     -244     
Flag Coverage Δ
backend-e2e-test ?
backend-e2e-test-ginkgo ?
backend-unit-test ?
frontend-e2e-test 73.96% <ø> (-0.08%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
web/src/helpers.tsx 77.04% <0.00%> (-3.28%) ⬇️
api/internal/core/entity/entity.go
api/internal/handler/schema/schema.go
api/internal/filter/request_id.go
api/internal/utils/closer.go
api/internal/handler/handler.go
api/internal/core/entity/format.go
api/internal/utils/consts/api_error.go
api/internal/core/store/validate.go
api/internal/core/storage/storage_mock.go
... and 37 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update bec8f1b...636009a. Read the comment docs.

@nic-chen
Copy link
Member

@bisakhmondal
I think we could use a library to automate, instead of writing a configuration for each platform. What do you think?

@bisakhmondal
Copy link
Member Author

@bisakhmondal
I think we could use a library to automate, instead of writing a configuration for each platform. What do you think?

Totally agree with you 👍 . A few days back I found https://github.com/kardianos/service. It seems it's gonna serve our purpose (I haven't tested it though). What do you think?

@nic-chen
Copy link
Member

@bisakhmondal

I think we could use a library to automate, instead of writing a configuration for each platform. What do you think?

Totally agree with you 👍 . A few days back I found https://github.com/kardianos/service. It seems it's gonna serve our purpose (I haven't tested it though). What do you think?

looks good. you could find some more libs and compare them

@bisakhmondal
Copy link
Member Author

Ok, I'll post it here :)

@membphis
Copy link
Member

membphis commented Apr 9, 2021

any news? @bisakhmondal

@bisakhmondal
Copy link
Member Author

Hii @membphis, @nic-chen, I have checked and found these few packages that look good and have cross-platform support. On the way, found a lot of packages that run as a bg process using Exec() and are obviously not immune to SIGHUP when its controlling terminal sends it.

  1. https://github.com/takama/daemon [supports linux, win, osx] (easy APIs, uses OS-specific service manager, under the hood, no nasty multiple forks)
  2. https://github.com/sevlyar/go-daemon [Doesn't supports windows till now]
  3. https://github.com/kardianos/service [more stars :)]

Let me know which one you like :)
If you ask me, I'd say going with option 1 is a good choice. It's really simple, and no overhead of keeping the package up-to-date as it's kind of an overlay on top of the os specific system managers. Tested with a gin server

package main

import (
	"fmt"
	"github.com/gin-gonic/gin"
	"github.com/takama/daemon"
	"net/http"
	"os"
	"os/signal"
	"syscall"
	"time"
)

type Service struct {
	daemon.Daemon
}

func (service* Service) Manage() (string, error){
	if len(os.Args) > 1 {
		command := os.Args[1]
		switch command {
		case "install":
			return service.Install()
		case "remove":
			return service.Remove()
		case "start":
			return service.Start()
		case "stop":
			return service.Stop()
		case "status":
			return service.Status()
		default:
			return "Usage: myservice install | remove | start | stop | status", nil
		}
	}

	r := gin.Default()

	fmt.Println("id: ",os.Getpid())
	fmt.Println("ppid: ", os.Getppid())

	r.GET("/", func(g *gin.Context) {
		g.JSON(http.StatusOK, gin.H{
			"message": "Server running successfully",
		})
	})


	go r.Run(":8080")

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

	signal.Notify(sig, syscall.SIGINT, syscall.SIGTERM, syscall.SIGKILL)

	sis := <- sig

	fmt.Println("signal received: ", sis.String())
	time.Sleep(2*time.Second)
	fmt.Println("Shutted down")
	return "Closed", nil
}
func main() {
	srv, err := daemon.New("my-gin-app", "my gin app", daemon.SystemDaemon)

	if err !=nil{
		panic(err)
	}
	service := &Service{srv}
	status, err := service.Manage()
	if err != nil{
		panic(err)
	}
	fmt.Println(status)
}

commands

go build main.go
sudo ./main install 
sudo ./main start

image

Thanks😄

@nic-chen
Copy link
Member

nic-chen commented Apr 9, 2021

hi, @bisakhmondal I think you could start a discussion on the mailing list: dev@apisix.apache.org, including which lib should be used and what our service name should be called, apisix-dashboard or manager-api

@bisakhmondal
Copy link
Member Author

Absolutely. No problem :)

@membphis
Copy link
Member

@tokers do you have time to look at this? #1667 (comment)

In memory, you are familiar with this.

Type=simple
Restart=on-failure
RestartSec=5
WorkingDirectory=<"Your Directory">/apisix-dashboard/output
Copy link
Contributor

Choose a reason for hiding this comment

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

Does the standard release have this directory? @nic-chen

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, if it is built from source. The build script takes care of that. However, for rpm tarball, the directory structure is slightly customized.

Copy link
Member

Choose a reason for hiding this comment

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

@tokers After the build, there will be an output directory.
The current problem is to choose a suitable library to implement this feature. Please check the mailing list when you have time to discuss it. @bisakhmondal has started a discussion. thanks.

Copy link
Member

Choose a reason for hiding this comment

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

do we have created an issue about this when we build an RPM package?

@bisakhmondal
Copy link
Member Author

Hii all, I am updating this PR with takama/daemon as @nic-chen suggested on the discussion thread.

@juzhiyuan juzhiyuan requested a review from tokers April 16, 2021 21:55
@nic-chen
Copy link
Member

don't see the update? @bisakhmondal

@netlify
Copy link

netlify bot commented Apr 19, 2021

Deploy preview for apisix-dashboard ready!

Built with commit 3c81afd

https://deploy-preview-1667--apisix-dashboard.netlify.app

@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2021

Codecov Report

Merging #1667 (3c81afd) into master (f146898) will decrease coverage by 0.59%.
The diff coverage is 37.76%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1667      +/-   ##
==========================================
- Coverage   71.62%   71.03%   -0.60%     
==========================================
  Files         172      173       +1     
  Lines        6119     6200      +81     
  Branches      711      711              
==========================================
+ Hits         4383     4404      +21     
- Misses       1487     1539      +52     
- Partials      249      257       +8     
Flag Coverage Δ
backend-e2e-test 45.42% <34.96%> (-0.69%) ⬇️
backend-e2e-test-ginkgo 48.55% <37.76%> (-0.52%) ⬇️
backend-unit-test 52.44% <ø> (ø)
frontend-e2e-test 72.07% <ø> (-0.07%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
api/cmd/service.go 13.51% <13.51%> (ø)
api/cmd/managerapi.go 45.92% <46.22%> (-2.43%) ⬇️
web/src/helpers.tsx 68.85% <0.00%> (-3.28%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f146898...3c81afd. Read the comment docs.

@bisakhmondal
Copy link
Member Author

don't see the update? @bisakhmondal

Updated :) Fully tested on Linux (ubuntu 20.04). I don't have macOS or windows. Please let me know if it is working or not when you have some time.
image

cc @nic-chen, @membphis, @tokers, @starsz

@bisakhmondal
Copy link
Member Author

image
new commands

@starsz
Copy link
Contributor

starsz commented Apr 21, 2021

how about ./manager-api start and ./manager-api stop,
when run ./manager-api start and the service is not installed, install it ?

Okay then, I will take care of checking and installing(if required) for ./manager-api start.
And yes, actual commands instead of flags would look nice, thanks for that. Regarding the commands I think, we could provide install, remove and status too.

  • ./manager-api start
  • ./manager-api stop
  • ./manager-api status (option to check service status from the binary itself without depending on os specific service manager)
  • ./manager-api remove (would be helpful deleting service test file without manually navigating to the filesystem)
  • ./manager-api install (reinstalling the service, if the binary location needs to be changed in future, service must be reinstalled. Also in newer releases if we introduce extra flags, that should be passed as arguments for ExecStart of the service unit file i.e. need reinstallation)

what do you think :)

LGTM too!.

@nic-chen nic-chen requested review from tokers and starsz April 21, 2021 03:24
@nic-chen
Copy link
Member

@tokers @starsz please have a look again when you have time.

#

[Unit]
Description=Manager-API service
Copy link
Member

Choose a reason for hiding this comment

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

I think we should add the keyword "Apache APISIX"

Copy link
Member Author

Choose a reason for hiding this comment

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

Hi, the binary takes care of running itself as a service. Unit file for each platform isn't required anymore. Thanks

api/manager-api.service Outdated Show resolved Hide resolved
Copy link
Member

@membphis membphis left a comment

Choose a reason for hiding this comment

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

missing test cases for the new code

@starsz
Copy link
Contributor

starsz commented Apr 21, 2021

missing test cases for the new code

Agree.
I think you can update the cli_test.sh now.
https://github.com/apache/apisix-dashboard/blob/master/api/test/shell/cli_test.sh

@bisakhmondal
Copy link
Member Author

Okay

var err error
service, err = createService()
if err != nil {
fmt.Fprintf(os.Stderr, "error occurred while initializing service: %s", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can quit after outputing this message.

Copy link
Member Author

@bisakhmondal bisakhmondal Apr 22, 2021

Choose a reason for hiding this comment

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

I avoided quitting because executing plain ./manager-api doesn't require service struct ref. If somehow createService returns a nil service and service-specific commands are used go runtime manager will throw an error of referencing a nil struct before any line of actual logic of manager-api being executed (slightly ungraceful though). Let me know what you think.

api/cmd/managerapi.go Outdated Show resolved Hide resolved
api/cmd/managerapi.go Show resolved Hide resolved
api/cmd/managerapi.go Show resolved Hide resolved
api/cmd/managerapi.go Outdated Show resolved Hide resolved
api/cmd/managerapi.go Outdated Show resolved Hide resolved
api/cmd/managerapi.go Outdated Show resolved Hide resolved
api/cmd/managerapi.go Show resolved Hide resolved
api/cmd/managerapi.go Outdated Show resolved Hide resolved
api/cmd/service.go Outdated Show resolved Hide resolved
@tokers
Copy link
Contributor

tokers commented Apr 22, 2021

@bisakhmondal I think we can change the title now, it's not so associated with systemd.

@bisakhmondal
Copy link
Member Author

Cool. Thank you.

@bisakhmondal bisakhmondal changed the title feat: run manager-api as a systemd daemon/service feat: run manager-api as an OS agnostic service Apr 22, 2021
@bisakhmondal
Copy link
Member Author

pinging @starsz to have a look, when you have some time.

@starsz
Copy link
Contributor

starsz commented Apr 27, 2021

pinging @starsz to have a look, when you have some time.

LGTM.Thank you very much.

# Conflicts:
#	api/cmd/managerapi.go
@bisakhmondal
Copy link
Member Author

Thank you @nic-chen for resolving the merge conflicts. All CI has passed. I guess it's now safe to merge into master :)

@tokers tokers merged commit b4f3720 into apache:master Apr 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants