Correct output when inspecting containers created with --ipc #17201
Conversation
openshift-ci
bot
added
the
do-not-merge/release-note-label-needed
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: 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
approved
|
LGTM |
Fixes: containers#17189 Signed-off-by: Daniel J Walsh <[email protected]>
|
@Luap99 Mine or yours, or should we merge. |
|
I don't care, both seem to do the same code and test wise, they are just using a bit different syntax. How about we let @edsantiago decide which test change he likes better? This PR or #17200? |
|
I've only skimmed these two (and restarted flakes). I will review in the next hour, but am OK if someone beats me to it. |
| } | ||
| } | ||
| case c.config.NoShm: | ||
| hostConfig.IpcMode = "none" | ||
| case c.config.NoShmShare: | ||
| hostConfig.IpcMode = "private" | ||
| } | ||
| if hostConfig.IpcMode == "" { | ||
| hostConfig.IpcMode = "shareable" | ||
| } |
There was a problem hiding this comment.
I prefer a switch to the if/elses in #17200, but I can't convince myself that this part of the change is equivalent. If a default case is possible, and none of the conditions trigger, won't IpcMode be left blank? (I don't know if this is possible)
Moving line 186 (initialization to "host") to line 177 (before the switch) would ease my mind, but perhaps it's obvious to those familiar with this code why that is not necessary.
| is "$output" "$hostipc" "HostIPC and container IPC should be same" | ||
| run_podman inspect IPC --format '{{ .HostConfig.IpcMode }}' | ||
| is "$output" "host" "host mode should be selected" |
There was a problem hiding this comment.
Just as a suggestion for future, not a blocker: I like my failure messages to make it clear why something is expected, not what is expected. (The "what" will be shown as part of the error message). Something like
is "$output" "host" ".HostConfig.IpcMode when --ipc=host"That makes the error message something like:
FAIL: .HostConfig.IpcMode when --ipc=host
expected: host
got: slaartibartfast
|
Ok I will merge mine, but I still think we can get some of @Luap99 improvements in a different PR. |
containers/pull/17186 and containers/pull/17201 have been merged at roughly the same time. Both work fine in isolation but the new kube test breaks in combination. Fix the IPC kube test to make CI healthy. Signed-off-by: Valentin Rothberg <[email protected]>
github-actions
bot
added
the
locked - please file new issue/PR
Fixes: #17189
Signed-off-by: Daniel J Walsh [email protected]
Does this PR introduce a user-facing change?