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

maintenance api server's default socket should be under root dir #280

Merged
merged 1 commit into from
Dec 13, 2022

Conversation

changweige
Copy link
Member

We should put all snapshotter's stuff under root dir which can be customized.

Signed-off-by: Changwei Ge gechangwei@bytedance.com

config/config.go Outdated
@@ -205,7 +205,11 @@ func SetStartupParameter(startupFlag *command.Args, cfg *Config) error {
cfg.GCPeriod = d

cfg.Address = startupFlag.Address
cfg.APISocket = startupFlag.APISocket

if startupFlag.APISocket == "" {
Copy link
Collaborator

Choose a reason for hiding this comment

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

What will happen when startupFlag.APISocket != "" ?

config/config.go Outdated
cfg.APISocket = startupFlag.APISocket

if startupFlag.APISocket == "" {
cfg.APISocket = path.Join(cfg.RootDir, "api.sock")
Copy link
Collaborator

Choose a reason for hiding this comment

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

It seems we should allow setting the APISocket to empty.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do you mean completely shutdown api server?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I mean how do users set it to empty?

@imeoer
Copy link
Collaborator

imeoer commented Dec 12, 2022

It seems the default grpc address is /run/containerd-nydus/containerd-nydus-grpc.sock, should we also put the system management api into /run/containerd-nydus dir ?

@changweige
Copy link
Member Author

It seems the default grpc address is /run/containerd-nydus/containerd-nydus-grpc.sock, should we also put the system management api into /run/containerd-nydus dir ?

Yes. It sounds a good idea

if cfg.APISocket != "" {
systemController, err := system.NewSystemController(manager, cfg.APISocket)
if cfg.EnableSystemController {
systemController, err := system.NewSystemController(manager, path.Join(cfg.RootDir, "system_controller.sock"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe name it supervisor.sock ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Um. We already have supervisors for nydusd daemon. Or just rename it to system.sock ?snapshotter.sock ?

Copy link
Collaborator

Choose a reason for hiding this comment

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

system.sock is ok.

Copy link
Member Author

Choose a reason for hiding this comment

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

updated

We should put all snapshotter's stuff under root dir which can
be customized.

Signed-off-by: Changwei Ge <gechangwei@bytedance.com>
@changweige changweige merged commit f1f0cff into containerd:main Dec 13, 2022
@changweige changweige deleted the fix-default-api-socket branch January 18, 2023 01:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants