Skip to content

Fixing container restore issue due to different IDs #284

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
rajasec wants to merge 1 commit into from
Closed

Fixing container restore issue due to different IDs #284

rajasec wants to merge 1 commit into from

Conversation

@rajasec
Copy link
Contributor

@rajasec rajasec commented Sep 21, 2015

@crosbymichael @LK4D4 @mrunalp
With latest runc I started the container using runc start, post that in a different terminal (/home/raj)
I have done the checkpoint using runc --id=runc checkpoint. Checkpoint was successfull as expected.
checkpoint directory created in /home/raj.
From /home/raj While I tried to restore, I gave "runc restore" ( forget to mention the --id=runc), so it picked up the current working directory and started the container ( as checkpoint directory exists in current folder)

Here is the problem:
Under /run/opencontainers/containers you can see two folders, one is with runc and another is with raj. Both containers contains the same cgroup.img so if you to try stop either one of the container, cgroup path failed to remove.
This fix the above problem, if an container does not exists or already destroyed. if user does "restore", do not start the create any new containers just by looking into checkpoint folder alone.

Signed-off-by: rajasec [email protected]

crosbymichael
crosbymichael reviewed Sep 23, 2015
View reviewed changes
restore.go Outdated
Copy link
Member

@crosbymichael crosbymichael Sep 23, 2015

Choose a reason for hiding this comment

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

this defer will fire after handleContainerstatus() returns, is this intentional? if so, why not just call Destroy() without the defer?

@rajasec
Copy link
Contributor Author

rajasec commented Sep 24, 2015

@crosbymichael
Thanks for the comment. This defer was not intentional. I'll remove and commit back.

@rajasec
Copy link
Contributor Author

rajasec commented Sep 24, 2015

@crosbymichael
Eliminated the defer statement as per the review comment, tested and committed again.

crosbymichael
crosbymichael reviewed Sep 25, 2015
View reviewed changes
restore.go Outdated
Copy link
Member

@crosbymichael crosbymichael Sep 25, 2015

Choose a reason for hiding this comment

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

this is should have multiple return results, because you are not using the first one.

@rajasec
Copy link
Contributor Author

rajasec commented Sep 26, 2015

@crosbymichael
I got your point, it will be weird in calling destroyed on destroyed container state. I'm calling destroy to clean up the directory/run/opencontainers/containers/ which got created during the initial stage.
I'll try to remove via normal os.RemoveAll to clean up the directory ( that would be cleaner than calling Destroy)

@rajasec
Fixing container restore issue due to different IDs
c19ea71 
Signed-off-by: rajasec <[email protected]>

Fixed review comment

Signed-off-by: Rajasekaran <[email protected]>

Fixing review comments

Signed-off-by: Rajasekaran <[email protected]>
@rajasec
Copy link
Contributor Author

rajasec commented Sep 28, 2015

@crosbymichael
Eliminated the unwanted return condition variables which is not used. In Destroyed section, I'm calling RemoveAll to remove the container ID instead of "Destroy" function. Let me know your comments.

Somehow jenkins failed due to hugetlb issues in localtest. Even I've seen in other PR where it failed in hugetlb.

@crosbymichael
Copy link
Member

crosbymichael commented Oct 8, 2015

@rajasec i think my state pr #311 will resolve this issue as well because it makes all these things more robust and removes the correct state files when they should no longer exist.

@rajasec
Copy link
Contributor Author

rajasec commented Oct 9, 2015

@crosbymichael
I agree. I dont think this PR required anymore.

@mrunalp mrunalp closed this Oct 9, 2015
@xiaochenshen xiaochenshen mentioned this pull request Feb 14, 2016
Closed
stefanberger pushed a commit to stefanberger/runc that referenced this pull request Sep 8, 2017
@vbatts
Merge pull request opencontainers#284 from wking/single-config
9017a6c 
config: Single, unified config file
stefanberger pushed a commit to stefanberger/runc that referenced this pull request Sep 8, 2017
@wking
specs-go/config: Drop platform-independent comment
1fa80d2 
This has been stale since cb2da54 (config: Single, unified config
file, 2015-12-28, opencontainers#284), when we dropped the attempt to distinguish
between platform-independent and platform-dependent configuration.

Signed-off-by: W. Trevor King <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

status/0-triage

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@rajasec @crosbymichael @mrunalp @GordonTheTurtle