The cidfile should be created when the container is created #8094
Conversation
openshift-ci-robot
added
the
approved
pkg/util/utils.go
Outdated
|
|
||
| func OpenCidFile(cidfile string) (*os.File, error) { | ||
| if cidfile == "" { | ||
| return nil, nil |
There was a problem hiding this comment.
may be need to new an error when cidfile==""?
There was a problem hiding this comment.
Removed check, and just let it fall through to get error at next level.
|
There has to be another set of logic for creating the files, as it does
(sort of) work now. I think you need to delete the existing code else we
might error on trying to make a duplicate CID file.
…On Wed, Oct 21, 2020, 08:29 Daniel J Walsh ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/util/utils.go
<#8094 (comment)>:
> @@ -638,3 +638,17 @@ func ValidateSysctls(strSlice []string) (map[string]string, error) {
func DefaultContainerConfig() *config.Config {
return containerConfig
}
+
+func OpenCidFile(cidfile string) (*os.File, error) {
+ if cidfile == "" {
+ return nil, nil
Removed check, and just let it fall through to get error at next level.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#8094 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AB3AOCAGW7NC6S2HAHPVQATSL3H2TANCNFSM4SZTT3UQ>
.
|
|
@mheon I did remove the existing code, from run.go. This was working but only on exit. So podman run -d ... would work the way you expect. The issue was podman run -ti ... would only create the cidfile when you exited from the container. |
|
@mheon Sorry you were right, some of my changes did not make it into the original PR. |
|
A bunch of red marks on the tests @rhatdan |
pkg/util/utils.go
Outdated
| if _, err = cidFile.WriteString(id); err != nil { | ||
| logrus.Error(err) | ||
| } | ||
| cidFile.Sync() |
There was a problem hiding this comment.
the Sync() is not needed here. If the node crashes the PID is not valid on the next boot anyway
|
LGTM once tests start passing and comment from @giuseppe is addressed. |
rhatdan
force-pushed
the
cidfile
branch
2 times, most recently
from
34bc9ab to
4f8e514
Compare
openshift-ci-robot
added
the
needs-rebase
openshift-ci-robot
removed
the
needs-rebase
Currently if you run an interactive session of podman run and specifiy the --cidfile option, the cidfile will not get created until the container finishes running. If you run a detached container, it will get created right away. This Patch creates the cidfile as soon as the container is created. This could allow other tools to use the cidefile on all running containers. Signed-off-by: Daniel J Walsh <[email protected]>
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: giuseppe, 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 |
github-actions
bot
added
the
locked - please file new issue/PR
Currently if you run an interactive session of podman run and
specifiy the --cidfile option, the cidfile will not get created
until the container finishes running. If you run a detached
container, it will get created right away. This Patch creates
the cidfile as soon as the container is created. This could allow
other tools to use the cidefile on all running containers.
Fixes: #8091
Signed-off-by: Daniel J Walsh [email protected]