podman update: Fix overwriting of LinuxResources structure
#24749
Conversation
test/system/280-update.bats
Outdated
| @@ -309,4 +309,22 @@ function nrand() { | |||
|
|
|||
| run_podman rm -t 0 -f $ctrname | |||
| } | |||
|
|
|||
| # bats test_tags=ci:parallel | |||
| @test "podman update - check resources over write Resources configuration" { | |||
There was a problem hiding this comment.
From the name it is not clear what you are testing. Maybe resources on update are not changed unless requested
test/system/280-update.bats
Outdated
| run_podman inspect $ctrname --format {{.HostConfig.Memory}} | ||
| assert "$output" == "104857600" ".HostConfig.Memory" | ||
|
|
||
| run_podman inspect $ctrname --format {{.HostConfig.PidsLimit}} | ||
| assert "$output" == "1024" ".HostConfig.PidsLimit" |
There was a problem hiding this comment.
you can combine these in one podman command
--format "{{.HostConfig.Memory}}\n{{.HostConfig.PidsLimit}}" then match assert "${lines[0]}" ... and "${lines[1]}" respectively. This safes us one inspect command so it is a bit faster.
libpod/container_internal.go
Outdated
| var oldResources *spec.LinuxResources | ||
| if c.config.Spec.Linux.Resources != nil { | ||
| if err := JSONDeepCopy(oldResources, c.config.Spec.Linux.Resources); err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
I don't get that, you are copying a nil pointer on that which mean this makes no sense and nothing is copied AFIACT.
If think you want to copy the current settings to oldResources in that case you must change the JSONDeepCopy argument order unless I am missing something.
test/system/280-update.bats
Outdated
| run_podman inspect $ctrname --format {{.HostConfig.Memory}} | ||
| assert "$output" == "104857600" ".HostConfig.Memory" | ||
|
|
||
| run_podman inspect $ctrname --format {{.HostConfig.PidsLimit}} |
There was a problem hiding this comment.
Add a link to the bug #24610 here and add a sentence to explain this pid check is done to ensure other resource setting are not unset.
test/system/280-update.bats
Outdated
| assert ${lines[0]%"${lines[0]##*[![:space:]]}"} == "104857600" ".HostConfig.Memory" | ||
| assert ${lines[1]%"${lines[1]##*[![:space:]]}"} == "1024" ".HostConfig.PidsLimit" |
There was a problem hiding this comment.
I not sure what you are trying to do there, this can just be
assert "${lines[0]}" == "104857600" ".HostConfig.Memory"
assert "${lines[1]}" == "1024" ".HostConfig.PidsLimit"
There was a problem hiding this comment.
Oh, I used $lines[0] and got 104857600\[0\]. I thought \[0\] were some weird characters for newlines. So I tried to trim string.
There was a problem hiding this comment.
$lines is a bash array so the index must be accessed via ${lines[X]} where X is the index or @ if you want to get all elements
$output is basically the full command output as simple string while $lines is the $output split at newlines. It makes matching certain lines easier for us. There is no strict reason to use $lines.
We could have also done {{.HostConfig.Memory}}--{{.HostConfig.PidsLimit}} and then match $output == 104857600--1024 ... as another example. But that has the downside of using one assert which means if it fails you manually need to compare which of the two values are wrong. This is a tiny bit more work when reading a failed log so using the newline split and matching then using $lines just makes it a bit easier.
with defaults values when changes configuration with podman update. The new LinuxResource structure does not represent the current unchanged configuration, which was not affected by the change. Fixes: https://issues.redhat.com/browse/RUN-2375 Signed-off-by: Jan Rodák <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Honny1, Luap99 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 |
openshift-ci
bot
added
the
approved
|
I like the use of JSONDeepCopy here, nice and simple. |
This PR fixes overwriting the
LinuxResourcesstructure in the database with default values when configuring container withpodman update.The new LinuxResource structure does not represent the current unchanged configuration, which was not affected by the change.
Reproducer:
podman run -dt --pids-limit 1024 --name hc1 quay.io/libpod/alpine:latest top podman inspect hc1 --format {{.HostConfig.PidsLimit}} # 1024 podman update hc1 --memory 100Mb (You can change any other resource of your choice) podman inspect hc1 --format {{.HostConfig.Memory}} # 104857600 podman inspect hc1 --format {{.HostConfig.PidsLimit}} # 0 <- This should print the value 1024Fixes: https://issues.redhat.com/browse/RUN-2375
Fixes: #24610
Does this PR introduce a user-facing change?