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.

Sign up for GitHub

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

Jump to bottom

implement podman update #15276

Merged
merged 1 commit into from
Sep 1, 2022
Merged

implement podman update #15276

merged 1 commit into from
Sep 1, 2022

Conversation

cdoern
Copy link
Contributor

@cdoern cdoern commented Aug 10, 2022 •
edited
Loading

podman update allows users to change the cgroup configuration of an existing container using the already defined resource limits flags from podman create/run. The supported flags in crun are:

  • –cpus
  • –cpuset-cpus
  • –cpuset-mems
  • –memory
  • –memory-swap
  • –memory-reservation
  • –cpu-shares
  • –cpu-quota
  • –cpu-period
  • –blkio-weight
  • –cpu-rt-period
  • –cpu-rt-runtime
  • --device-read-bps
  • --device-write-bps
  • --device-read-iops
  • --device-write-iops
  • --memory-swappiness
  • --blkio-weight-device

this command is also now supported in the libpod api via the /libpod/containers//update endpoint where
the resource limits are passed in the request body and follow the OCI resource spec format

resolves #15067

Signed-off-by: Charlie Doern cdoern@redhat.com

Does this PR introduce a user-facing change?

introduce podman update which allows users to modify an existing container's cgroup configuration after creation/run time

@cdoern cdoern force-pushed the update branch 3 times, most recently from 329fb1d to db31054 Compare August 10, 2022 19:03
@cdoern
Copy link
Contributor Author

cdoern commented Aug 10, 2022

@edsantiago I can no longer run the man page checker locally since the recent man page changes. Any quick tips on how to remedy this?
the man-page-checker always reports no errors and:

./hack/xref-helpmsgs-manpages
xref-helpmsgs-manpages: docs/source/markdown/podman-attach.1.md:58: invalid link podman-run(1) -> podman-run.1.md
xref-helpmsgs-manpages: docs/source/markdown/podman-auto-update.1.md:143: invalid link podman-run(1) -> podman-run.1.md
xref-helpmsgs-manpages: Cannot read /home/charliedoern/Documents/podman/hack/../docs/source/markdown/podman-build.1.md: No such file or directory

@edsantiago
Copy link
Member

make docs

@cdoern cdoern force-pushed the update branch 3 times, most recently from 697cbaa to e46e980 Compare August 10, 2022 22:11
giuseppe
giuseppe reviewed Aug 11, 2022
View reviewed changes
pkg/domain/infra/abi/containers.go Outdated Show resolved Hide resolved
libpod/oci_conmon_linux.go Outdated Show resolved Hide resolved
libpod/oci_conmon_linux.go Outdated Show resolved Hide resolved
@cdoern cdoern force-pushed the update branch 3 times, most recently from 055efc8 to 96ab9db Compare August 11, 2022 17:21
mheon
mheon reviewed Aug 11, 2022
View reviewed changes
libpod/oci.go Outdated Show resolved Hide resolved
mheon
mheon reviewed Aug 11, 2022
View reviewed changes
libpod/oci_conmon_linux.go Outdated Show resolved Hide resolved
@cdoern cdoern force-pushed the update branch 11 times, most recently from c7dbfc2 to 903ac04 Compare August 12, 2022 18:22
@rhatdan
Copy link
Member

rhatdan commented Aug 17, 2022

LGTM
Nice work @cdoern

edsantiago added a commit to edsantiago/libpod that referenced this pull request Aug 31, 2022
@edsantiago
Man pages: refactor common options: --device-X-Y
c6d91e1 
In all cases, went with the version from podman-run because
it conforms to our documentation guidelines. I then added
the FAQ-26 link to each one.

This does not review easily with hack/markdown-preprocess-review
because there are too many files involved. Sorry.

This will cause conflicts in containers#15276 - again, I'm sorry. On the bright
side, that PR still needs work anyway, and rebasing on top of
this should make it easier to review.

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago edsantiago mentioned this pull request Aug 31, 2022
Closed
@edsantiago
Copy link
Member

LGTM

@containers/podman-maintainers PTAL

vrothberg
vrothberg reviewed Sep 1, 2022
View reviewed changes
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.

/lgtm

Nice work, @cdoern !

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 1, 2022
@vrothberg
Copy link
Member

/hold

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2022
@vrothberg
Copy link
Member

Argh, there's a merge conflict.

One more round, @cdoern

@cdoern
implement podman update
050f329 
podman update allows users to change the cgroup configuration of an existing container using the already defined resource limits flags
from podman create/run. The supported flags in crun are:

this command is also now supported in the libpod api via the /libpod/containers/<CID>/update endpoint where
the resource limits are passed inthe request body and follow the OCI resource spec format

–memory
–cpus
–cpuset-cpus
–cpuset-mems
–memory-swap
–memory-reservation
–cpu-shares
–cpu-quota
–cpu-period
–blkio-weight
–cpu-rt-period
–cpu-rt-runtime
-device-read-bps
-device-write-bps
-device-read-iops
-device-write-iops
-memory-swappiness
-blkio-weight-device

resolves containers#15067

Signed-off-by: Charlie Doern <cdoern@redhat.com>
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 1, 2022
@cdoern
Copy link
Contributor Author

cdoern commented Sep 1, 2022

/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 1, 2022
@cdoern
Copy link
Contributor Author

cdoern commented Sep 1, 2022

@containers/podman-maintainers PTAL

@edsantiago
Copy link
Member

This is not a simple rebase; git range-diff shows WeightDevices() removed. Not something I can review, but reviewers might appreciate a short note explaining that change.

@cdoern
Copy link
Contributor Author

cdoern commented Sep 1, 2022 •
edited
Loading

This is not a simple rebase; git range-diff shows WeightDevices() removed. Not something I can review, but reviewers might appreciate a short note explaining that change.

that function was moved to utils_linux.go otherwise with its new usages it breaks cross builds

@edsantiago
Copy link
Member

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 1, 2022
@openshift-merge-robot openshift-merge-robot merged commit d0d1af3 into containers:main Sep 1, 2022
edsantiago added a commit to edsantiago/libpod that referenced this pull request Sep 1, 2022
@edsantiago
Man pages: refactor common options: --device-X-Y
3d09d47 
Followup from containers#15276: add the FAQ-26 link, and fix one
broken replacement.

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago edsantiago mentioned this pull request Sep 1, 2022
Merged
edsantiago added a commit to edsantiago/libpod that referenced this pull request Sep 6, 2022
@edsantiago
Man pages: refactor common options: --memory*
fb38838 
(memory-star, i.e., several memory options) that didn't get
included in containers#15276. Most of them are shoo-ins; the two in
container-clone and pod-clone deserve special attention
because of the "If unspecified" wording.

Signed-off-by: Ed Santiago <santiago@redhat.com>
@edsantiago edsantiago mentioned this pull request Sep 6, 2022
Merged
jamacku pushed a commit to jamacku/podman that referenced this pull request Sep 6, 2022
@edsantiago @jamacku
Man pages: refactor common options: --memory*
2a82367 
(memory-star, i.e., several memory options) that didn't get
included in containers#15276. Most of them are shoo-ins; the two in
container-clone and pod-clone deserve special attention
because of the "If unspecified" wording.

Signed-off-by: Ed Santiago <santiago@redhat.com>
@lukasmrtvy
Copy link

lukasmrtvy commented Nov 2, 2022 •
edited
Loading

@cdoern hey, I am trying to use API to update container, but seems it just does not work:

podman run -dt --name fooBar alpine
curl  --unix-socket /run/podman/podman.sock -X POST -H "Content-Type: application/json" 'http://d/v4/libpod/containers/fooBar/update' -d '{"Resources":{"cpu":{"quota":4321,"period":1234}}}'
podman exec  fooBar cat /sys/fs/cgroup/cpu.max
max 100000

podman run -dt --name fooBar alpine
podman update --cpu-quota 1234 --cpu-period 4321 fooBar
podman exec  fooBar cat /sys/fs/cgroup/cpu.max
1234 4321

Am I doing something wrong? Thanks

EDIT:

-d '{"cpu":{"quota":4321,"period":1234}}'

is correct payload format.

@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 20, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 20, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@vrothberg vrothberg vrothberg left review comments

@mheon mheon mheon left review comments

@edsantiago edsantiago edsantiago approved these changes

@giuseppe giuseppe giuseppe left review comments

Assignees

@edsantiago edsantiago

@vrothberg vrothberg

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. release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

add "podman container update" command to update the cgroup configuration of running containers
9 participants
@cdoern @edsantiago @rhatdan @TomSweeneyRedHat @giuseppe @vrothberg @lukasmrtvy @mheon @openshift-merge-robot