Add support to ShmSize in Pods with Quadlet #24899
Conversation
Odilhao
force-pushed
the
pod-shm-size-issue
branch
3 times, most recently
from
8307a36 to
a083f93
Compare
This closes containers#22915 Signed-off-by: Odilon Sousa <[email protected]>
Odilhao
force-pushed
the
pod-shm-size-issue
branch
from
a083f93 to
9c9f20a
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: Odilhao, 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 |
openshift-ci
bot
added
the
approved
pkg/systemd/quadlet/quadlet.go
Outdated
| @@ -1650,6 +1651,11 @@ func ConvertPod(podUnit *parser.UnitFile, name string, unitsInfoMap map[string]* | |||
| execStartPre.add("--infra-name", fmt.Sprintf("%s-infra", podName)) | |||
| execStartPre.add("--name", podName) | |||
|
|
|||
| shmSize := podUnit.LookupAll(PodGroup, KeyShmSize) | |||
There was a problem hiding this comment.
IIUC you are looking for all the occurrences of KeyShmSize and then use the first one. This does not align with the expected behaviour in which the last value takes precedence.
Instead of calling LookupAll and using the first element of the array, you should use Lookup (which returns the last occurrence) and use the returned value if exists.
Alternatively, you can create a map and use the lookupAndAddString function. Yes, it is an overkill for a single key, but it's the start for supporting more keys
There was a problem hiding this comment.
I changed from LookupAll to Lookup, I tried a to implement the map using the stringsKeys that is already created and it failed on my e2e assertions tests, tried also to create a new map just for sanity and it failed as well, if we really need to use map I can give a try later this week.
There was a problem hiding this comment.
I added a comment on the new code. We'll change the code to use a map later
There was a problem hiding this comment.
The map problem was in my e2e test, instead of using assert-podman-pre-args "--shm-size=5g" I'm now using assert-podman-pre-args "--shm-size" "5g" this made the test pass.
I also changed the stringKeys function from lookupAndAddAllStrings to lookupAndAddString.
Signed-off-by: Odilon Sousa <[email protected]>
Odilhao
force-pushed
the
pod-shm-size-issue
branch
from
9808bee to
bf3acc1
Compare
pkg/systemd/quadlet/quadlet.go
Outdated
| @@ -1650,6 +1651,11 @@ func ConvertPod(podUnit *parser.UnitFile, name string, unitsInfoMap map[string]* | |||
| execStartPre.add("--infra-name", fmt.Sprintf("%s-infra", podName)) | |||
| execStartPre.add("--name", podName) | |||
|
|
|||
| shmSize, exists := podUnit.Lookup(PodGroup, KeyShmSize) | |||
| if exists { | |||
There was a problem hiding this comment.
To align with other checks, please also check that there's a value:
| if exists { | |
| if exists && len(shmSize) > 0 { |
Odilhao
force-pushed
the
pod-shm-size-issue
branch
from
bec03ee to
2cd9f72
Compare
Signed-off-by: Odilon Sousa <[email protected]>
Odilhao
force-pushed
the
pod-shm-size-issue
branch
from
2cd9f72 to
d33ed1f
Compare
This closes #22915
Add support for ShmSize in Pods created with Quadlet.
Add e2e tests.
Add new Docs entry.
Does this PR introduce a user-facing change?