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

APIv2 add generate systemd endpoint #7329

Merged

Conversation

Luap99
Copy link
Member

@Luap99 Luap99 commented Aug 16, 2020

Add support for generating systemd units
via the api and podman-remote.

Change the GenerateSystemdReport type to return the
units as map[string]string with the unit name as key.

Add --format flag to podman generate systemd
to allow the output to be formatted as json.

@Luap99 Luap99 force-pushed the generate-systemd-remote branch 4 times, most recently from 19b0dfe to afb6608 Compare August 16, 2020 19:22
@Luap99
Copy link
Member Author

Luap99 commented Aug 16, 2020

@vrothberg PTAL

@zhangguanzhang
Copy link
Collaborator

you should add some tests in here https://github.com/containers/podman/tree/master/test/apiv2

Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, @Luap99!

I appreciate all the work that went into the changes but we don't expose this feature on the API or the remote client on purpose. Running a remote process in a systemd unit is not something we want to support. Also, the generated units are closely tied to a locally running Podman and conmon.

@Luap99
Copy link
Member Author

Luap99 commented Aug 17, 2020

@vrothberg The goal here is not to run the remote process inside a unit. Its just so you can create a unit via the api and deploy it to your remote system/s. Especially useful with --new

e.g

$ podman-remote create --name t alpine
ae1eb3aac1c45cf97dfa53dfbb66d6befd368f4a78cce39db6a888d0ca1db16b
$ podman-remote generate systemd t --new
# container-ae1eb3aac1c45cf97dfa53dfbb66d6befd368f4a78cce39db6a888d0ca1db16b.service
# autogenerated by Podman 2.1.0-dev
# Mon Aug 17 10:40:51 CEST 2020

[Unit]
Description=Podman container-ae1eb3aac1c45cf97dfa53dfbb66d6befd368f4a78cce39db6a888d0ca1db16b.service
Documentation=man:podman-generate-systemd(1)
Wants=network.target
After=network-online.target

[Service]
Environment=PODMAN_SYSTEMD_UNIT=%n
Restart=on-failure
ExecStartPre=/bin/rm -f %t/container-ae1eb3aac1c45cf97dfa53dfbb66d6befd368f4a78cce39db6a888d0ca1db16b.pid %t/container-ae1eb3aac1c45cf97dfa53dfbb66d6befd368f4a78cce39db6a888d0ca1db16b.ctr-id
ExecStart=/usr/local/bin/podman run --conmon-pidfile %t/container-ae1eb3aac1c45cf97dfa53dfbb66d6befd368f4a78cce39db6a888d0ca1db16b.pid --cidfile %t/container-ae1eb3aac1c45cf97dfa53dfbb66d6befd368f4a78cce39db6a888d0ca1db16b.ctr-id --cgroups=no-conmon -d --replace --name t alpine
ExecStop=/usr/local/bin/podman stop --ignore --cidfile %t/container-ae1eb3aac1c45cf97dfa53dfbb66d6befd368f4a78cce39db6a888d0ca1db16b.ctr-id -t 10
ExecStopPost=/usr/local/bin/podman rm --ignore -f --cidfile %t/container-ae1eb3aac1c45cf97dfa53dfbb66d6befd368f4a78cce39db6a888d0ca1db16b.ctr-id
PIDFile=%t/container-ae1eb3aac1c45cf97dfa53dfbb66d6befd368f4a78cce39db6a888d0ca1db16b.pid
KillMode=none
Type=forking

[Install]
WantedBy=multi-user.target default.target

@vrothberg
Copy link
Member

@vrothberg The goal here is not to run the remote process inside a unit. Its just so you can create a unit via the api and deploy it to your remote system/s. Especially useful with --new

If users wish to deploy systemd units on remote machines, they need to login those machines in any case. Exposing a feature on the remote client that cannot be run on the client machine is very confusing.

@Luap99
Copy link
Member Author

Luap99 commented Aug 17, 2020

If users wish to deploy systemd units on remote machines, they need to login those machines in any case. Exposing a feature on the remote client that cannot be run on the client machine is very confusing.

That's a good point. But when I work with podman via the api I do not want to call podman generate systemd directly. Otherwise I could just use podman create and so on. There would be no reason for me to use the api in the first place.

@vrothberg
Copy link
Member

That's a good point. But when I work with podman via the api I do not want to call podman generate systemd directly. Otherwise I could just use podman create and so on. There would be no reason for me to use the api in the first place.

We didn't envision any API user to have the need for generating unit files as the data can be meaningless on the client side. Can you elaborate a bit how you would use such an endpoit?

@Luap99
Copy link
Member Author

Luap99 commented Aug 17, 2020

If I get the unit content from the endpoint i would add them to a ansible playbook and deploy them like that. I might want to inspect and edit the unit files before deployment, so I do not want to run podman generate systemd on each machine.

@vrothberg
Copy link
Member

@rhatdan WDYT?

@Luap99's use case sounds legit. If we decide to add an endpoint and enable the remote client, I want the remote client to logrus.Warn that the units are not meant to be used on the client side. We also need to update the man pages to make it explicit.

@Luap99
Copy link
Member Author

Luap99 commented Aug 17, 2020

I already added this Note in the man page: Note: If you use this command with the remote client, you would still have to place the generated units on the remote system. Adding a logrus.Warn would also make sense to me.

Copy link
Member

@jwhonce jwhonce left a comment

Choose a reason for hiding this comment

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

One API improvement request

// required: true
// description: Name or ID of the container or pod.
// - in: query
// name: name
Copy link
Member

Choose a reason for hiding this comment

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

Having name as both a path and query parameter in one endpoint seems a point of future confusion. I understand why this was done, but would this be better as useName?

Copy link
Member Author

Choose a reason for hiding this comment

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

useName sounds good 👍

Add support for generating systemd units
via the api and podman-remote.

Change the GenerateSystemdReport type to return the
units as map[string]string with the unit name as key.

Add `--format` flag to `podman generate systemd`
to allow the output to be formatted as json.

Signed-off-by: Paul Holzinger <paul.holzinger@web.de>
@rhatdan
Copy link
Member

rhatdan commented Sep 3, 2020

LGTM
@vrothberg @jwhonce PTAL

@jwhonce
Copy link
Member

jwhonce commented Sep 3, 2020

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 3, 2020
@zhangguanzhang
Copy link
Collaborator

need approve

@rhatdan
Copy link
Member

rhatdan commented Sep 5, 2020

/approve

@openshift-ci-robot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, rhatdan

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2020
@openshift-merge-robot openshift-merge-robot merged commit f1323a9 into containers:master Sep 5, 2020
@Luap99 Luap99 deleted the generate-systemd-remote branch November 2, 2020 15:49
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 24, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 24, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants