Skip to content
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.

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

Use existing interface to request IP address during restore #2061

Merged
merged 3 commits into from
Jan 9, 2019

Conversation

adrianreber
Copy link
Collaborator

The initial implementation to request the same IP address for a
container during a restore was based on environment variables
influencing CNI.

With this commit the IP address selection switches to Podman's internal
static IP API.

Signed-off-by: Adrian Reber [email protected]

@baude
Copy link
Member

baude commented Dec 29, 2018

can you add an integration test for this please

@rhatdan
Copy link
Member

rhatdan commented Dec 30, 2018

/approve

@openshift-ci-robot
Copy link
Collaborator

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Dec 30, 2018
@rhatdan
Copy link
Member

rhatdan commented Jan 3, 2019

@adrianreber Can you answer @baude

@adrianreber
Copy link
Collaborator Author

@baude I will add a test to verify that the IP address is the same before and after checkpoint/restore.

I have to re-work this anyway to not modify c.config.

@adrianreber
Copy link
Collaborator Author

I also added a test to verify that the IP does not change after restoring from checkpoint and I re-enabled the test with established TCP connections.

@mheon
Copy link
Member

mheon commented Jan 8, 2019

Code looks fine over here, just a small comment from me

@adrianreber
Copy link
Collaborator Author

Is /bin/sh: easyjson: command not found a known CI failure or something I broke?

@mheon
Copy link
Member

mheon commented Jan 8, 2019

make .install.easyjson should grab the tool

@mheon
Copy link
Member

mheon commented Jan 8, 2019

Oh, it's failing in CI. Hmmm.

Try a rm libpod/container_easyjson.go; make easyjson_generate locally

@adrianreber
Copy link
Collaborator Author

Oh, it's failing in CI. Hmmm.

Yes, it is a CI failure.

Try a rm libpod/container_easyjson.go; make easyjson_generate locally

Doesn't change anything locally except removing pkg/sysinfo/sysinfo_linux.go~, which looks like an unrelated temporary file. The removal of that file does not sound like it will fix my CI easyjson problem.

@mheon
Copy link
Member

mheon commented Jan 8, 2019

Try squashing your commits down into one - it looks like it's failing trying to build an individual commit

@adrianreber adrianreber force-pushed the static-ip branch 2 times, most recently from 59e72f0 to 8d432a7 Compare January 8, 2019 17:08
@adrianreber
Copy link
Collaborator Author

Same CI failure (/bin/sh: easyjson: command not found) with different patch ordering as well as with squashing all commits together. Will have another look at it tomorrow.

@rhatdan
Copy link
Member

rhatdan commented Jan 8, 2019

Run the make locally to see if it creates a new json file.

@mheon
Copy link
Member

mheon commented Jan 8, 2019

Alright, going to take a shot at nuking easyjson from orbit. #2105

Restoring a container from a checkpoint should give the container the
same IP as before checkpointing. This adds a test to make sure the IP
stays the same.

Signed-off-by: Adrian Reber <[email protected]>
The initial implementation to request the same IP address for a
container during a restore was based on environment variables
influencing CNI.

With this commit the IP address selection switches to Podman's internal
static IP API.

This commit does a comment change in libpod/container_easyjson.go to
avoid unnecessary re-generation of libpod/container_easyjson.go during
build as this fails in CI. The reason for this is that make sees that
libpod/container_easyjson.go needs to be re-created. The commit,
however, only changes a part of libpod/container.go which is marked as
'ffjson: skip'.

Signed-off-by: Adrian Reber <[email protected]>
@adrianreber
Copy link
Collaborator Author

@mheon @rhatdan Thanks for all your help. I was finally able to solve it. The problem was that make saw that container.go was newer than container_easyjson.go, therefore it tried to re-create container_easyjson.go which failed because the tool easyjson was missing. make was right as I actually changed container.go but the change was in a place marked as ffjson: skip. That is why the locally generated container_easyjson.go was unchanged and not updated in git.

To solve it I changed a comment in container_easyjson.go and was able to commit it to git. Now it is newer than container.go and make does not want to run easyjson: all tests finally green.

@rhatdan
Copy link
Member

rhatdan commented Jan 9, 2019

LGTM
@mheon @baude @vrothberg @umohnani8 @giuseppe PTAL

@mheon
Copy link
Member

mheon commented Jan 9, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2019
@openshift-merge-robot openshift-merge-robot merged commit 7b9d4f1 into containers:master Jan 9, 2019
@github-actions github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 27, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 27, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants