Skip to content
This repository has been archived by the owner on May 12, 2021. It is now read-only.

cli: Error out if initrd/rootfs not define in config file #588

Merged
merged 1 commit into from
Sep 13, 2018

Conversation

nitkon
Copy link
Contributor

@nitkon nitkon commented Aug 16, 2018

If neither initrd nor rootfs path is mentioned in
the configuration.toml file, then error out stating
the same

Fixes: #587

Signed-off-by: Nitesh Konkar [email protected]

Copy link
Contributor

@grahamwhaley grahamwhaley left a comment

Choose a reason for hiding this comment

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

lgtm
nice find :-)
I look forward to a healthy English debate on the use of 'nor' vs 'or' here ;-)

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 169222 KB
Proxy: 4099 KB
Shim: 8882 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2003456 KB

Copy link

@devimc devimc left a comment

Choose a reason for hiding this comment

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

lgtm

@jodh-intel
Copy link
Contributor

jodh-intel commented Aug 16, 2018

lgtm

Approved with PullApprove

@opendev-zuul
Copy link

opendev-zuul bot commented Aug 16, 2018

Build succeeded (third-party-check pipeline).

@amshinde
Copy link
Member

@nitkon TestMinimalRuntimeConfig test is failing. Can you take a look.

@egernst
Copy link
Member

egernst commented Aug 17, 2018

I agree with Graham. I think you have a double negative; s/nor/or?

@egernst
Copy link
Member

egernst commented Aug 17, 2018

I agree with Garham. I think you have a double negative; s/nor/or?

@WeiZhang555
Copy link
Member

WeiZhang555 commented Aug 20, 2018

Oh my god, why not make life easier?

"either image or initrd must be specified in configuration file"

I can't understand your error message as a non-native English speaker 😢

@jodh-intel
Copy link
Contributor

Agreed - could you update please @nitkon?

@nitkon
Copy link
Contributor Author

nitkon commented Aug 20, 2018

@jodh-intel : Sure.

@nitkon
Copy link
Contributor Author

nitkon commented Aug 21, 2018

@jodh-intel : Done.

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 169179 KB
Proxy: 4103 KB
Shim: 9048 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2003564 KB

@opendev-zuul
Copy link

opendev-zuul bot commented Aug 21, 2018

Build succeeded (third-party-check pipeline).

@devimc
Copy link

devimc commented Aug 21, 2018

@nitkon please fix the unit tests

--- FAIL: TestMinimalRuntimeConfig (0.00s)
	config_test.go:587: /tmp/kata-runtime-831458654/minimal-runtime-config-003076135/runtime.toml: either image or initrd must be defined in the configuration file

@opendev-zuul
Copy link

opendev-zuul bot commented Aug 24, 2018

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

@jodh-intel
Copy link
Contributor

Tests still failing...

@nitkon
Copy link
Contributor Author

nitkon commented Aug 24, 2018

@jodh-intel : Yes realized that. I am unsure about how do I overcome it. If I return initrd other test cases fail and If I do not set initrd or rootfs, it errors out as expected but test case fails as config and expectedConfig don't match.

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 169477 KB
Proxy: 4210 KB
Shim: 8806 KB

Memory inside container:
Total Memory: 2043464 KB
Free Memory: 2003448 KB

@opendev-zuul
Copy link

opendev-zuul bot commented Aug 24, 2018

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

@jodh-intel
Copy link
Contributor

Hi @nitkon - I've got to look at that part of the tests to fix the cyclomatic complexity CI failure on #639 if you want to wait on that landing first?

@jodh-intel
Copy link
Contributor

Hi @nitkon - could you re-push? The CI logs seems to have expired :)

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 169991 KB
Proxy: 4016 KB
Shim: 8706 KB

Memory inside container:
Total Memory: 2043460 KB
Free Memory: 2003432 KB

@opendev-zuul
Copy link

opendev-zuul bot commented Sep 11, 2018

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

@sboeuf
Copy link

sboeuf commented Sep 11, 2018

@nitkon please rebase and repush, the CI should pass, and I'll merge once this happens!

@amshinde
Copy link
Member

@nitkon Looks like you need to fix this failing test as well:

--- FAIL: TestMinimalRuntimeConfigWithVsock (0.00s)
	config_test.go:667: /tmp/kata-runtime-963320441/minimal-runtime-config-600452101/runtime.toml: either image or initrd must be defined in the configuration file

@opendev-zuul
Copy link

opendev-zuul bot commented Sep 11, 2018

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

@sboeuf sboeuf added the enhancement Improvement to an existing feature label Sep 12, 2018
@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 169967 KB
Proxy: 4165 KB
Shim: 8757 KB

Memory inside container:
Total Memory: 2043460 KB
Free Memory: 2003068 KB

@jodh-intel
Copy link
Contributor

Ah - good catch @nitkon! I think you can merge the commits though as otherwise the previous commit will fail the tests.

@opendev-zuul
Copy link

opendev-zuul bot commented Sep 13, 2018

Build failed (third-party-check pipeline) integration testing with
OpenStack. For information on how to proceed, see
http://docs.openstack.org/infra/manual/developers.html#automated-testing

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 172287 KB
Proxy: 4232 KB
Shim: 9044 KB

Memory inside container:
Total Memory: 2043460 KB
Free Memory: 2003076 KB

@nitkon
Copy link
Contributor Author

nitkon commented Sep 13, 2018

@jodh-intel : Thanks for the input! 😃 Updated my PR. Couple of failures I guess not related to my PR. Re-push?

@jodh-intel
Copy link
Contributor

Hi @nitkon - yep - this error seems unrelated.

@grahamwhaley, @chavafg - the error though is similar to #727 (comment):

Openshift started successfully
1..1
not ok 1 Hello Openshift using Kata Containers
# (in test file hello_world.bats, line 28)
#   `sudo -E oc create -f "${BATS_TEST_DIRNAME}/data/hello-pod-kata.json"' failed
# fedora-27-1019.vexxhost.net   Ready     <none>    14s       v1.10.0+b81c8f8
# W0913 11:28:46.267162   29782 util_unix.go:75] Using "/var/run/crio/crio.sock" as endpoint is deprecated, please consider using full url format "unix:///var/run/crio/crio.sock".
# Image is update to date for docker.io/openshift/hello-openshift@sha256:aaea76ff622d2f8bcb32e538e7b3cd0ef6d291953f3e7c9f556c1ba5baf47e2e
# Error from server (Forbidden): error when creating "/tmp/jenkins/workspace/kata-containers-runtime-fedora-27-PR/go/src/github.com/kata-containers/tests/integration/openshift/data/hello-pod-kata.json": pods "hello-openshift" is forbidden: error looking up service account default/default: serviceaccount "default" not found
# Error from server (NotFound): pods "hello-openshift" not found
# rm: cannot remove '': No such file or directory
# Error from server (NotFound): pods "hello-openshift" not found

@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 166192 KB
Proxy: 4090 KB
Shim: 8823 KB

Memory inside container:
Total Memory: 2043460 KB
Free Memory: 2003564 KB

@opendev-zuul
Copy link

opendev-zuul bot commented Sep 13, 2018

Build succeeded (third-party-check pipeline).

If neither initrd nor rootfs path is mentioned in
the configuration.toml file, then error out stating
the same

Fixes: kata-containers#587

Signed-off-by: Nitesh Konkar [email protected]
@katacontainersbot
Copy link
Contributor

PSS Measurement:
Qemu: 167904 KB
Proxy: 4165 KB
Shim: 8753 KB

Memory inside container:
Total Memory: 2043460 KB
Free Memory: 2006852 KB

@codecov
Copy link

codecov bot commented Sep 13, 2018

Codecov Report

Merging #588 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #588      +/-   ##
==========================================
- Coverage   65.25%   65.23%   -0.02%     
==========================================
  Files          85       85              
  Lines        9960     9959       -1     
==========================================
- Hits         6499     6497       -2     
  Misses       2809     2809              
- Partials      652      653       +1

@opendev-zuul
Copy link

opendev-zuul bot commented Sep 13, 2018

Build succeeded (third-party-check pipeline).

@caoruidong
Copy link
Member

Since we have lots of approvals and All CI pass, merge this.

@caoruidong caoruidong merged commit 22aedc4 into kata-containers:master Sep 13, 2018
egernst pushed a commit to egernst/runtime that referenced this pull request Feb 9, 2021
vendor: update dependency opencontainers/runc
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement Improvement to an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants