Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Appearance settings
This repository was archived by the owner on Feb 24, 2020. It is now read-only.

stage0/status: fix failure when systemd never runs in stage1 - #3713

#3713
Merged
lucab merged 1 commit into
rkt:masterrkt/rkt:masterfrom
fabiokung:status-pid-bugfabiokung/rkt:status-pid-bugCopy head branch name to clipboard
Jul 25, 2017
Merged

stage0/status: fix failure when systemd never runs in stage1#3713
lucab merged 1 commit into
rkt:masterrkt/rkt:masterfrom
fabiokung:status-pid-bugfabiokung/rkt:status-pid-bugCopy head branch name to clipboard

Conversation

@fabiokung

@fabiokung fabiokung commented Jun 16, 2017

Copy link
Copy Markdown
Contributor

A pid file never gets written if systemd never gets to run in stage1. This can happen if the image had a bad command, i.e.: not in $PATH.

Steps to reproduce:

$ sudo rkt --insecure-options=image run --uuid-file-save=/tmp/id docker://alpine --exec=bad
stage1: cannot initialize immutable environment: unable to find "bad" in "/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin"

$ sudo rkt status $(cat /tmp/id)
state=exited
created=2017-06-16 16:34:27.38 -0700 PDT
status: unable to print status: unable to get PID for pod "e6ccefea-1cdf-459a-b98d-7da95373a7d2": <nil>

Signed-off-by: Fabio Kung fabio.kung@gmail.com

@ghost

ghost commented Jun 16, 2017

Copy link
Copy Markdown

Can one of the admins verify this patch?

@lucab

lucab commented Jun 19, 2017

Copy link
Copy Markdown
Member

ok to test

@lucab lucab added this to the 1.28.0 milestone Jun 19, 2017
@fabiokung

fabiokung commented Jun 19, 2017

Copy link
Copy Markdown
Contributor Author

The CI failures seems unrelated, some flakiness on a TTY test.

@lucab

lucab commented Jun 19, 2017

Copy link
Copy Markdown
Member

@fabiokung I'll review this after today release, but you could please format your commit titles so that they include an area/component prefix?

@fabiokung

fabiokung commented Jun 19, 2017

Copy link
Copy Markdown
Contributor Author

@lucab done.

lucab
lucab suggested changes Jun 20, 2017
View reviewed changes
Comment thread rkt/status.go
}

stdout.Printf("pid=%d\nexited=%t", pid, (state == pkgPod.Exited || state == pkgPod.ExitedGarbage))
if pid, err := p.Pid(); err == nil {

@lucab lucab Jun 20, 2017

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this change may re-introduce the race documented in the comment below, which was previously addressed by the goroutine+timeout. I would suggest to special-case the running case, block on it until pid exists or we hit the timeout, and then proceed with your logic.

@lucab

lucab commented Jun 20, 2017

Copy link
Copy Markdown
Member

I understand what you are experiencing but I think the current PR is a bit too aggressive and may bring back the previous racing issue.

@lucab

lucab commented Jun 20, 2017

Copy link
Copy Markdown
Member

On the other hand, this completely skips printing pid in the error and racing case, so it may actually be ok and push the retrying logic to the consumer side. I'm not sure what is the best behavior, but this approach may be more coherent.

@fabiokung

fabiokung commented Jun 20, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

On the other hand, this completely skips printing pid in the error and racing case, so it may actually be ok and push the retrying logic to the consumer side. I'm not sure what is the best behavior, but this approach may be more coherent.

Exactly my thoughts. There is already rkt status --wait that does the retrying/polling for users, so I'd be inclined to point people that need to guarantee a pid exists (and the container is up) to that.

The sleep code is also racy, nothing guarantees that the pid file will be written in 1s. I really dislike that approach.

@lucab lucab changed the title stage0: status fails when systemd never runs in stage1 rkt/status: fix failure when systemd never runs in stage1 Jun 26, 2017
@lucab

lucab commented Jun 26, 2017

Copy link
Copy Markdown
Member

@fabiokung I think I can agree with that.

@s-urbaniak @squeed any second opinion on this discussion?

s-urbaniak
s-urbaniak reviewed Jun 26, 2017
View reviewed changes

@s-urbaniak s-urbaniak left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I do agree with this change. Given we already have two separate --wait-... instructions this implicit wait does not fit into the semantics.

It is not guaranteed that a rkt run invocation "happened-before" a subsequent rkt status invocation which the current code tries to overcome. The reality is that the user should retry rkt status after invoking rkt run himself.

The only nit I'd have is that we should make the above fact more clear in the documentation of rkt status.

@fabiokung

fabiokung commented Jun 28, 2017

Copy link
Copy Markdown
Contributor Author

@s-urbaniak @lucab I added some bits to the doc (on rkt status) about pid being sometimes not available. PTAL

@fabiokung fabiokung changed the title rkt/status: fix failure when systemd never runs in stage1 stage0/status: fix failure when systemd never runs in stage1 Jun 28, 2017
lucab
lucab approved these changes Jul 10, 2017
View reviewed changes

@lucab lucab left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lucab

lucab commented Jul 18, 2017

Copy link
Copy Markdown
Member

@fabiokung it looks like the CI was flaking a bit at the time this PR was last pushed. Do you mind rebasing once more on top of current master? It should be ready to go then.

@lucab lucab mentioned this pull request Jul 18, 2017
Closed
@fabiokung

fabiokung commented Jul 22, 2017

Copy link
Copy Markdown
Contributor Author

Will do.

@fabiokung
stage0: status only prints a PID when there is one
35dae38 
A pid file never gets written if systemd never gets to run in stage1.
This can happen if the image had a bad command, i.e.: not in $PATH.

In that case, rkt status will constantly error with:

status: unable to print status: unable to get PID for pod ...

Signed-off-by: Fabio Kung <fabio.kung@gmail.com>
@fabiokung

fabiokung commented Jul 24, 2017

Copy link
Copy Markdown
Contributor Author

@lucab all green!

lucab
lucab approved these changes Jul 25, 2017
View reviewed changes

@lucab lucab left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lucab
lucab merged commit 5d5f72f into rkt:master Jul 25, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

2 more reviewers
@s-urbaniak s-urbaniak s-urbaniak left review comments
@lucab lucab lucab approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

component/stage0 kind/bugfix

Projects

None yet

Milestone

1.28.0

Development

Successfully merging this pull request may close these issues.

3 participants

@fabiokung @lucab @s-urbaniak
Morty Proxy This is a proxified and sanitized view of the page, visit original site.