Skip to content

Add critest to Makefile and Docker test image #563

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

Merged
merged 1 commit into from
Feb 18, 2022

Conversation

ginglis13
Copy link
Contributor

@ginglis13 ginglis13 commented Feb 16, 2022

Description of changes:

Add critest tool and dependencies to Docker test image.
Add rule to Makefile for running critest.

Preliminary step before adding CRI conformance testing to BuildKite pipeline.

Testing

Locally, modified entrypoint.sh to use local pool, in my case pool_name = fc-test-thinpool, -> make -C runtime critest. Errors reported by critest at the moment typically all connection refused when dialing firecracker.vsock . Checking logs within the container, looks like fc VMs are hitting a kernel panic:

/bin/sh: 0: cannot open nomodules: No such file [ 0.936147] Kernel panic - not syncing: Attempted to kill init! exitcode=0x00000200

Issue #, if available:

Related: #88

Signed-off-by: Gavin Inglis [email protected]

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@ginglis13 ginglis13 force-pushed the makefile-critest branch 2 times, most recently from 1e11ca7 to b142044 Compare February 17, 2022 23:40
@ginglis13 ginglis13 marked this pull request as ready for review February 17, 2022 23:43
@ginglis13 ginglis13 requested a review from a team as a code owner February 17, 2022 23:43
@ginglis13 ginglis13 changed the title [WIP] Add critest to Makefile and Docker test image Add critest to Makefile and Docker test image Feb 17, 2022
@@ -256,8 +256,12 @@ TEST_BRIDGED_TAP_BIN?=$(BINPATH)/test-bridged-tap
$(TEST_BRIDGED_TAP_BIN): $(shell find internal/cmd/test-bridged-tap -name *.go) $(GOMOD) $(GOSUM)
go build -o $@ $(CURDIR)/internal/cmd/test-bridged-tap

LOOPBACK_BIN?=$(BINPATH)/loopback
$(LOOPBACK_BIN):
GOBIN=$(dir $@) GO111MODULE=off go get -u github.com/containernetworking/plugins/plugins/main/loopback
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to pin the version of loopback? I believe go get is going to fetch the latest version.

Copy link
Contributor Author

@ginglis13 ginglis13 Feb 18, 2022

Choose a reason for hiding this comment

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

Yea, go get will get version @ latest . I was following how we currently get/install other container networking plugins:

BRIDGE_BIN?=$(BINPATH)/bridge
$(BRIDGE_BIN):
GOBIN=$(dir $@) GO111MODULE=off go get -u github.com/containernetworking/plugins/plugins/main/bridge
PTP_BIN?=$(BINPATH)/ptp
$(PTP_BIN):
GOBIN=$(dir $@) GO111MODULE=off go get -u github.com/containernetworking/plugins/plugins/main/ptp
HOSTLOCAL_BIN?=$(BINPATH)/host-local
$(HOSTLOCAL_BIN):
GOBIN=$(dir $@) GO111MODULE=off go get -u github.com/containernetworking/plugins/plugins/ipam/host-local
FIREWALL_BIN?=$(BINPATH)/firewall
$(FIREWALL_BIN):
GOBIN=$(dir $@) GO111MODULE=off go get -u github.com/containernetworking/plugins/plugins/meta/firewall

Do we want to be able to pin a version for each of these as well? Can take that up as a separate issue.

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh okay. I'm fine doing that in a separate PR in this case.

critest:
$(CURDIR)/../tools/thinpool.sh reset "$(FICD_DM_POOL)"
docker run --rm -it \
--privileged \
Copy link
Contributor

Choose a reason for hiding this comment

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

Does it work with --network=none?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as I have right now, it does not work with --network=none because critest need to pull an image for creating Pod sandboxes. Would it be best to pull this sandbox image into FIRECRACKER_CONTAINERD_TEST_IMAGE , then ?

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. It would be better to keep that as is. Technically we could pull some images beforehand, but that may make critest less useful, assuming we might break the pulling part someday.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok yes and just more on that, the sandbox image could be predownloading but networking would still have to be enable as there are tests for pulling images too (e.g. [It] public image with tag should be pulled and removed)

So for now I think we'll have to enable networking for this test.

Comment on lines 50 to 58
RUN wget https://github.com/kubernetes-sigs/cri-tools/releases/download/$VERSION/critest-$VERSION-linux-amd64.tar.gz
RUN tar zxvf critest-$VERSION-linux-amd64.tar.gz -C /tmp/
RUN install -D -o root -g root -m755 -t /usr/local/bin /tmp/critest
RUN rm -f critest-$VERSION-linux-amd64.tar.gz

RUN wget https://github.com/kubernetes-sigs/cri-tools/releases/download/$VERSION/crictl-$VERSION-linux-amd64.tar.gz
RUN tar zxvf crictl-$VERSION-linux-amd64.tar.gz -C /tmp/
RUN install -D -o root -g root -m755 -t /usr/local/bin /tmp/crictl
RUN rm -f crictl-$VERSION-linux-amd64.tar.gz
Copy link
Contributor

Choose a reason for hiding this comment

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

RUN wget https://github.com/kubernetes-sigs/cri-tools/releases/download/$VERSION/critest-$VERSION-linux-amd64.tar.gz && \
tar zxvf critest-$VERSION-linux-amd64.tar.gz -C /tmp/ && \
install -D -o root -g root -m755 -t /usr/local/bin /tmp/critest && \
rm -f critest-$VERSION-linux-amd64.tar.gz && \
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove the last && \

Add critest tool to test-images and invoke in Dockerfile.
Add rule to Makefile for running critest

Preliminary step before adding critest to BuildKite pipeline.

Signed-off-by: Gavin Inglis <[email protected]>
@ginglis13 ginglis13 merged commit 616849f into firecracker-microvm:main Feb 18, 2022
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.

2 participants