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

[vmm]: add vmm-sandboxer as a systemd service #44

Merged
merged 1 commit into from
Jul 18, 2023

Conversation

flyflypeng
Copy link
Member

Fix: #4

reason: add vmm-sandboxer as a systemd service

@flyflypeng flyflypeng force-pushed the add-systemd-service branch from 50b6b8f to ea21513 Compare July 13, 2023 09:23
OPTIONS='--listen /run/vmm-sandboxer.sock --dir /run/kuasar-vmm'

# set vmm-sandboxer log level
RUST_LOG=info
Copy link
Collaborator

Choose a reason for hiding this comment

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

is this log level duplicated with the loglevel config added in the #36?

Copy link
Member Author

Choose a reason for hiding this comment

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

I want to provide two ways to configure the vmm-sandboxer process's log level. In this place, keep the original way to set the log level by RUST_LOG environment which is set by default.
Another way in the #36 provides to configure log level in the config_<hypervisor>.toml file which is more flexible for user to change the log level.

#36 supports log_level set in the config_<hypervisor>.toml file overrides the default log level set in the /etc/sysconfig/kuasar-vmm environment file.

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 the env of RUST_LOG is enough to set the log level and it is simple. keeping these to ways to set the log level makes things complicated, I am wondering if we have to revert the #36 ?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does seem a bit repetitive, and I think it's better to keep the support for configuring it in the config file rather than in an systemd service env file.
So, we don't need to revert the #36, just remove the RUST_LOG env option in the /etc/sysconfig/kuasar-vmm env file.


// notify the systemd about state changes
#[cfg(feature = "systemd")]
sd_notify::notify(false, &[sd_notify::NotifyState::Ready])?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

is it necssary to do this intrusive modification to support systemd?

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 the type field value is notify in the systemd service file, it is necessary for the daemon process to notify the systemd that the service is ready.

Copy link
Collaborator

Choose a reason for hiding this comment

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

So, can we just set the type of service to simple, this services seems not that complicated, only the sandboxer process is included, why do we need to set the service type to notify?

Copy link
Member Author

Choose a reason for hiding this comment

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

Nice suggestion, maybe the simple type is the simplest and enough for the kuasar-vmm service.
I used to reference the service file from the containerd.service which started as a notify type systemd service.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

The key point is whether or not someone is going to get the status of the kuasar-vmm service, if not, I think simple type is enough.

@flyflypeng flyflypeng force-pushed the add-systemd-service branch from fe5a68c to de2625a Compare July 14, 2023 02:03
Copy link
Member

@Burning1020 Burning1020 left a comment

Choose a reason for hiding this comment

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

Nice work. Please update README.md at the same time.


## Install and run kuasar-vmm systemd service

1. Run `make install-vmm` to install vmm-sandboxer related executable file, configuraiton files, and `kuasar-vmm` systemd service file into the destination directory.
Copy link
Member

Choose a reason for hiding this comment

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

Typo, configuration

Copy link
Member Author

Choose a reason for hiding this comment

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

Fix it!

@@ -158,6 +158,10 @@ async fn main() -> anyhow::Result<()> {
}
builder.init();

// notify the systemd about state changes
#[cfg(feature = "systemd")]
sd_notify::notify(false, &[sd_notify::NotifyState::Ready])?;
Copy link
Member

Choose a reason for hiding this comment

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

Should we ignore the error returned by notify and keep on running? Probably just record this error in logs.
So that we don't need the "systemd" feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've thought about this, but if the sd_notify execution fails to continue down the line, then systemctl start kuasar-vmm systemd service will be blocked forever and kuasar-vmm systemd service will abnormal.

Copy link
Member

Choose a reason for hiding this comment

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

I found both dockerd and containerd all ignored this error.

Copy link
Member Author

Choose a reason for hiding this comment

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

I found both dockerd and containerd all ignored this error.

If we just ignore the error with notify type systemd service, which will cause systemctl start command blocked until exceeding the timeout.

@flyflypeng flyflypeng force-pushed the add-systemd-service branch 2 times, most recently from 5825c23 to 63c12ef Compare July 14, 2023 09:12
@@ -45,4 +45,4 @@ rtnetlink = "0.12"
netlink-packet-route = "0.15"
netlink-packet-core = "0.5.0"
ttrpc = { version = "0.7", features = ["async"] }
protobuf = "3.2"
protobuf = "3.2"
Copy link
Collaborator

@abel-von abel-von Jul 17, 2023

Choose a reason for hiding this comment

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

is it an empty line that is not necessary?

Copy link
Member Author

Choose a reason for hiding this comment

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

protobuf this line is already exist in the Cargo.toml, this change maybe compared the pre commit in the current PR.

Makefile Outdated

.PHONY: vmm wasm quark clean all install-vmm install-wasm install-quark install

all: vmm quark wasm

bin/vmm-sandboxer:
ifeq ($(RUN_AS_SYSTEMD_SERVICE), true)
@cd vmm/sandbox && cargo build --release --features=${HYPERVISOR},systemd
Copy link
Collaborator

@abel-von abel-von Jul 17, 2023

Choose a reason for hiding this comment

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

we don't have a feature of systemd anymore if the service type is simple.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fix it!

@flyflypeng flyflypeng force-pushed the add-systemd-service branch 2 times, most recently from 5cc1c0d to d1931cf Compare July 18, 2023 02:47
Fix: kuasar-io#4

reason:
1. add vmm-sandboxer as a systemd service
2. minimising installation file permissions

Signed-off-by: flyflypeng <flyflyflypeng@gmail.com>
@flyflypeng flyflypeng force-pushed the add-systemd-service branch from d1931cf to 9f7270a Compare July 18, 2023 11:54
Copy link
Member

@Burning1020 Burning1020 left a comment

Choose a reason for hiding this comment

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

LGTM

@Burning1020 Burning1020 merged commit a08a8dd into kuasar-io:main Jul 18, 2023
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.

Support to run sandboxer process as a systemd service
3 participants