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

enable integration test on arm64 platform #1642

Merged
merged 1 commit into from
Dec 5, 2017
Merged

enable integration test on arm64 platform #1642

merged 1 commit into from
Dec 5, 2017

Conversation

lubinsz
Copy link

@lubinsz lubinsz commented Nov 7, 2017

Currently, integration test can't be done on arm64 platform due to several issues.
Fix points:
1, add busybox.tar with arm64 format
2, add hello-world.tar with arm64 format

Signed-off-by: Bin Lu bin.lu@arm.com

@lubinsz
Copy link
Author

lubinsz commented Nov 7, 2017

@tianon @cyphar @avagin @dqminh
Hi all, can you help to check it?
Thanks.

Dockerfile Outdated

RUN case $(uname -m) in \
Copy link
Member

Choose a reason for hiding this comment

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

I would always recommend against using uname for platform detection, both because it will often return the wrong result (building arm/v7 images on an arm64 box, for example) and because for many ARM 32-bit platforms it gets a little out of hand, and would recommend instead using something like dpkg --print-architecture or apk --print-arch to get a simple and consistent string that represents the architecture of the image/userspace instead.

Copy link
Author

Choose a reason for hiding this comment

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

@tianon
Can you check it?
Thanks.

@lubinsz
Copy link
Author

lubinsz commented Nov 7, 2017

Hi @tianon ,
I have updated it. Using 'dpkg --print-architecture' now.
Thanks.

@lubinsz
Copy link
Author

lubinsz commented Nov 10, 2017

@avagin
@caniszczyk
@crosbymichael
@cyphar
@dqminh
@hqhq
@mrunalp
@rjnagal
Hi all,
Could you take a look at it?
Thanks.

@@ -14,7 +14,15 @@ BUSYBOX_IMAGE="$BATS_TMPDIR/busybox.tar"
BUSYBOX_BUNDLE="$BATS_TMPDIR/busyboxtest"

# hello-world in tar format
HELLO_IMAGE="$TESTDATA/hello-world.tar"
case $(dpkg --print-architecture) in
Copy link
Contributor

Choose a reason for hiding this comment

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

If we use dpkg, non debian systems won't be able to run local integration tests, are there any other standard way to detect the osarch?

Copy link
Member

Choose a reason for hiding this comment

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

$(go env GOARCH) seems like it would be a much better idea.

Copy link
Author

Choose a reason for hiding this comment

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

OK. I will update it.

;;
*)
HELLO_IMAGE="$TESTDATA/hello-world.tar"
;;
Copy link
Member

Choose a reason for hiding this comment

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

It isn't necessary to have two different filenames for this.

Copy link
Author

Choose a reason for hiding this comment

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

@cyphar I uploaded the file of 'hello-world-aarch64.tar' in 'runc/tests/integration/testdata/'.

The file of 'hello-world.tar' can't be used on Arm64 platform, because the binary files with x86 format can't be used on Arm platform.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, I didn't notice that you included the .tar in testdata (I thought we always downloaded the rootfs archive).

Dockerfile Outdated
*) \
curl -o- -sSL 'https://github.com/docker-library/busybox/raw/a0558a9006ce0dd6f6ec5d56cfd3f32ebeeb815f/glibc/busybox.tar.xz' | tar xfJC - ${ROOTFS} \
;; \
esac
Copy link
Member

Choose a reason for hiding this comment

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

This should be a script that both this and helpers.bash call into. No point in replicating this logic in more than one place.

Copy link
Author

Choose a reason for hiding this comment

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

@cyphar
Hi, I have moved all the multi-arch related code into 'tests/integration/multi-arch.bash'.
Please check it.
Thanks.

@@ -270,7 +278,14 @@ function setup_busybox() {
BUSYBOX_IMAGE="/testdata/busybox.tar"
fi
if [ ! -e $BUSYBOX_IMAGE ]; then
curl -o $BUSYBOX_IMAGE -sSL 'https://github.com/docker-library/busybox/raw/a0558a9006ce0dd6f6ec5d56cfd3f32ebeeb815f/glibc/busybox.tar.xz'
case $(dpkg --print-architecture) in
Copy link
Member

Choose a reason for hiding this comment

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

Please use $(go env GOARCH).

Copy link
Author

Choose a reason for hiding this comment

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

ok, I will update it.

Currently, integration test can't be done on arm64 platform due to several issues.
 Fix points:
 1, add busybox.tar with arm64 format
 2, add hello-world.tar with arm64 format

Signed-off-by: Bin Lu <bin.lu@arm.com>
@lubinsz
Copy link
Author

lubinsz commented Nov 14, 2017

@cyphar
Hi, what's your opinion?

@lubinszARM
Copy link

@avagin
@caniszczyk
@crosbymichael
@cyphar
@dqminh
@hqhq
@mrunalp
@rjnagal
Hi all,
We need to enable integration test in arm servers. So could you help to check it?
Thanks.

@cyphar
Copy link
Member

cyphar commented Dec 5, 2017

LGTM.

Approved with PullApprove

Copy link

@TomSweeneyRedHat TomSweeneyRedHat left a comment

Choose a reason for hiding this comment

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

LGTM

@crosbymichael
Copy link
Member

crosbymichael commented Dec 5, 2017

LGTM

Approved with PullApprove

@crosbymichael crosbymichael merged commit 1d3ab6d into opencontainers:master Dec 5, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants