-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
remove multi-arch.bash but use skopeo to get images directly #2508
remove multi-arch.bash but use skopeo to get images directly #2508
Conversation
#2486 is trying to clean up this issue (and should also allow us to remove multi-arch.sh). I don't really like this approach since using and requiring Docker just to get images is overkill. |
3678286
to
0ab2dce
Compare
Sorry, I ignored the "overkill" regulation and only simplify the process.
Yes, I need MIPS support indeed. |
Makefile
Outdated
$(CONTAINER_ENGINE) build $(CONTAINER_ENGINE_BUILD_FLAGS) -t $(RUNC_IMAGE) . | ||
|
||
.hello-world: | ||
docker export $(shell docker create hello-world) -o $(CURDIR)/tests/integration/testdata/hello-world.tar |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
docker -> $(CONTAINER_ENGINE)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I will fix it.
@cyphar I might have no "overkill" problem, I used docker in Makefile before starting container, it is same as executing integration test with docker. Thanks! |
Please clean this PR up (you probably did you do |
56cc22d
to
67aa77e
Compare
Needs a rebase |
2882345
to
cda7423
Compare
This PR can make the test cases run on more CPU architecture using the ability from docker. |
Makefile
Outdated
.debian: | ||
$(CONTAINER_ENGINE) export $(shell $(CONTAINER_ENGINE) create debian) -o $(CURDIR)/tests/integration/testdata/debian.tar | ||
chmod a+r $(CURDIR)/tests/integration/testdata/debian.tar | ||
touch "$@" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
-
You need to add something like
rm -f .debian .busybox .hello-world
toclean
target. The downloaded images, too, I guess. -
Also, it might be helpful to do
ln -sf $(CURDIR)/tests/integration/testdata/debian.tar $@
instead of just touch
, as in the case a tarball will be removed, make
will redo the target thanks to a broken symlink.
-
Using $(CURDIR) is redundant I think.
-
These three rules look similar, and it should be easy to generalize and merge those into one single rule.
-
Maybe move this code out to a separate Makefile in tests/integration/testdata, and just call it from the main Makefile.
Makefile
Outdated
test: unittest integration rootlessintegration | ||
|
||
localtest: localunittest localintegration localrootlessintegration | ||
localtest: | all .busybox .hello-world .debian |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This repetition can be avoided by having something like
TEST_IMAGES ?= debian busybox hello-world
GET_IMAGE_TARGETS := $(addprefix ., $(TEST_IMAGES))
...
localtest: | $(GET_IMAGE_TARGETS)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, specifying these prerequisites here is redundant, as every one of (localunittest, localintegration, localrootlessintegration) already have those.
Makefile
Outdated
test: unittest integration rootlessintegration | ||
|
||
localtest: localunittest localintegration localrootlessintegration | ||
localtest: | all .busybox .hello-world .debian | ||
make localunittest localintegration localrootlessintegration |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to call make here.
Makefile
Outdated
@@ -72,7 +88,7 @@ unittest: runcimage | |||
-v $(CURDIR):/go/src/$(PROJECT) \ | |||
$(RUNC_IMAGE) make localunittest TESTFLAGS=$(TESTFLAGS) | |||
|
|||
localunittest: all | |||
localunittest: | all .busybox .hello-world .debian |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be localunittest: all | .busybox .hello-world .debian
?
Vagrantfile.centos7
Outdated
systemctl start docker | ||
mkdir /busybox /debian | ||
docker export $(docker create busybox) | tar -C /busybox/ -xvf - | ||
docker export $(docker create debian) | tar -C /debian/ -xvf - |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a way to maybe use something lighter than docker to just download/extract images from the repo? I frankly don't know
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's just I don't like much the idea of needing to run dockerd inside vagrant for the sole purpose of obtaining busybox (or the like) image.
What needs to be done instead is to download the images before running vagrant. Something like (in .travis.yml):
- ln -sf Vagrantfile.fedora32 Vagrantfile
+ - make download-test-images
- sudo vagrant up && sudo mkdir -p /root/.ssh && sudo sh -c "vagrant ssh-config >> /ro
(and then vagrant up
will copy those as it copies everything from the current directory into /vagrant
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skopeo + umoci is what #2486 uses and (personal biases aside) I would much prefer that over using Docker just for the purposes of downloading like 3 images.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also want us to remove the need for three different images for our tests. Since it looks like folks want super esoteric architectures, we can just use Debian in #2486 and work around the missing packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@cyphar
I don't know umoci can or can not download images for the CPU architecture supporting debian images. But umoci only support the ARM64 and AMD64 in the view of runc master branch.
At first, I only want to make the test cases run on more CPU architecture.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
skopeo does the downloading and it does support multi-arch. umoci is a Go program and doesn't have any real architecture restrictions (aside from "you must be able to build a Go program on it"). Heck, you can even run umoci on MacOS.
All I'm suggesting is that this:
% docker export $(docker create busybox) | tar -C /busybox/ -xvf -
Becomes this:
% skopeo copy docker://busybox:latest oci:/tmp/busybox-image:latest
% umoci raw unpack --image /tmp/busybox-image:latest /busybox
That's it. And the latter will be more performant because docker export $(docker create) | tar xvf -
requires extracting and generating tar archives multiple times (not to mention transferring the archive and all the rest of it)!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I got it.
I like the idea in general, the implementation can be improved though |
70025dc
to
cbf0a2d
Compare
@cyphar @kolyshkin Thanks for your advice. I have pushed a new commit for this PR. |
Signed-off-by: liuxiaodong <liuxiaodong@loongson.cn>
tests/integration/hooks.bats
Outdated
@@ -14,6 +14,7 @@ function setup() { | |||
|
|||
teardown_debian | |||
setup_debian | |||
update_config '.root.readonly |= false' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
as I noted earlier, it should be done
- in a very test case that requires it
- as a separate patch describing why it is done (since the linker in hooks test likes to create files)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If this PR don't have this patch, CI verification will fail. Can I add some notes ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I skiped the hook test case, but add some TODO notes for an new PR to fix this issue.
7a661dc
to
922e507
Compare
@kolyshkin
|
Please see the .travis.yml file a few lines before your changes -- it calls apt, too, so you don't have to call it again. Also, as you can see, it uses
|
2455aae
to
db6d0bf
Compare
2aa26a4
to
9a9e527
Compare
Signed-off-by: Xiaodong Liu <liuxiaodong@loongson.cn>
9a9e527
to
fdc6311
Compare
updated , @kolyshkin PTAL |
@cyphar do you think we should proceed with this one? Overall I like it. |
When I test runc on mips64le, I found that the integration and rootlessintegration test only support arm64 and amd64 cpu architecture.
I developed a new way to execute the integration test which can support more cpu arch.
Signed-off-by: Xiaodong Liu liuxiaodong@loongson.cn