config: support kernel parameters #369

wcwxyz · 2017-07-28T11:14:37Z

Add support to pass extra kernel parameters in configuration.

The story behind this feature is, I have trouble running centos:6
/bin/bash and it turns out guest kernel is using vsyscall=none which
breaks old libc.

Fixes: #368

Signed-off-by: WANG Chao chao.wang@ucloud.cn

coveralls · 2017-07-28T11:18:41Z

Coverage decreased (-0.3%) to 53.195% when pulling 36c9d44 on wcwxyz:config-kernel-params into df043b1 on clearcontainers:master.

jodh-intel

Hi @wcwxyz - thanks for raising this! The code looks good, but there are a few tweaks to do to update cc-env output and add some tests.

Also, note that the runtime is failing under SemaphoreCI with these changes, so that will need a bit of debugging.

I'm going to mark this PR as needing changes as a result.

Please let us know if you need any help with any of the above (and don't worry about the big red cross! 😄

jodh-intel · 2017-07-28T11:31:09Z

config.go

@@ -122,6 +124,31 @@ func (h hypervisor) image() string {
 	return h.Image
 }

+func (h hypervisor) kernelParams() []vc.Param {


Please could you add some tests to TestHypervisorDefaults() for this new function.

jodh-intel · 2017-07-28T11:31:20Z

config.go

+	return deserializeParams(h.KernelParams)
+}
+
+func deserializeParams(paramString string) []vc.Param {


This should have a test too.

jodh-intel · 2017-07-28T11:34:04Z

config.go

@@ -193,6 +221,7 @@ func newQemuHypervisorConfig(h hypervisor) (vc.HypervisorConfig, error) {
 		HypervisorPath:        hypervisor,
 		KernelPath:            kernel,
 		ImagePath:             image,
+		KernelParams:          kernelParams,


The cc-env command should display the kernel params too. See commit 257b46a for an example of adding extra details to the cc-env output (you'll need to update the formatVersion as that commit did).

jodh-intel · 2017-07-28T13:06:44Z

config/configuration.toml.in

@@ -4,6 +4,8 @@
 path = "@QEMUPATH@"
 kernel = "@KERNELPATH@"
 image = "@IMAGEPATH@"
+# Uncomment if having trouble running pre-2.15 glibc


I forgot to say, please can you add a comment stating the format of this variable. For example, it could become something like this:

# Optional space-separated list of options to pass to the guest kernel. # For example, uncomment the line below if you are having trouble running pre-2.15 glibc ## kernel_params = "vsyscall=emulate"

wcwxyz · 2017-07-28T16:45:00Z

I will update according to your comment. Thanks for review.

coveralls · 2017-07-31T11:21:25Z

Coverage increased (+0.9%) to 54.398% when pulling 4e24641 on wcwxyz:config-kernel-params into df043b1 on clearcontainers:master.

sboeuf

@wcwxyz The code looks very good to me. The only change I would like to see is having your changes rely as much as possible on the virtcontainers library. We already have a serializeParams in virtcontainers, and about deserializeParams, you should add it there.

sboeuf · 2017-07-31T15:43:39Z

config.go

+	return params
+}
+
+func serializeParams(params []vc.Param) string {


Please use the function that already exists in virtcontainers: https://github.com/containers/virtcontainers/blob/master/hypervisor.go#L183

I'd like to re-use such functions as much as you do.
Do you know how should I submit PRs in both virtcontainers and runtime, and have semaphoreci/travis-ci working?

Well the idea is that you submit the first PR to virtcontainers and after it got merged, we will revendor virtcontainers inside the runtime repository so that you can rely on the new function you have just created.

That's good to know. Thank you.

sboeuf · 2017-07-31T15:47:26Z

config.go

+	return h.KernelParams
+}
+
+func deserializeParams(paramString string) []vc.Param {


Please create this function in virtcontainers as it can be reused by any consumer of the virtcontainers library.

sboeuf · 2017-08-05T00:41:38Z

@wcwxyz I have updated the vendoring of virtcontainers for the runtime. You can now modify this PR so that it relies on the patch that you submitted to virtcontainers ;)

wcwxyz · 2017-08-07T02:53:45Z

Re-pushed(No code changes). Please take a look. Thanks.

coveralls · 2017-08-07T02:56:29Z

Coverage increased (+0.3%) to 55.441% when pulling 4f10446 on wcwxyz:config-kernel-params into 1ec2462 on clearcontainers:master.

jodh-intel · 2017-08-07T13:17:01Z

cc-env.go

+		Hypervisor:   hypervisor,
+		Image:        image,
+		Kernel:       kernel,
+		KernelParams: kernelParams,


Running cc-runtime cc-env gives the following:

[Kernel] Path = "/usr/share/clear-containers/vmlinux.container" Resolved = "/usr/share/clear-containers/vmlinux-4.9.35-62.container" [KernelParams] Parameters = ""

But what we really want is:

[Kernel] [Kernel.Location] Path = "/usr/share/clear-containers/vmlinux.container" Resolved = "/usr/share/clear-containers/vmlinux-4.9.35-62.container" Parameters = ""

That way, all the kernel details are kept under Kernel. This is also in line with what we already have for Hypervisor:

[Hypervisor] MachineType = "pc-lite" [Hypervisor.Location] Path = "/usr/bin/qemu-lite-system-x86_64" Resolved = "/usr/bin/qemu-lite-system-x86_64"

ok, will do.

Add support to pass extra kernel parameters in configuration or directly configured on build. This information can be retrieved from cc-env. The story behind this feature is, I have trouble running centos:6 /bin/bash and it turns out guest kernel is using vsyscall=none which breaks old libc. Fixes: #368 Signed-off-by: WANG Chao <chao.wang@ucloud.cn>

coveralls · 2017-08-08T03:24:27Z

Coverage increased (+0.3%) to 55.39% when pulling 528772f on wcwxyz:config-kernel-params into bcfc581 on clearcontainers:master.

jodh-intel · 2017-08-08T07:53:51Z

@wcwxyz - great! Thanks for making these changes.

@sboeuf - could you re-review please?

sboeuf · 2017-08-08T07:56:22Z

LGTM

jodh-intel · 2017-08-08T09:26:37Z

It looks like one of the cri-o tests is problematic under semaphore. I've raised clearcontainers/tests#326 to investigate that issue.

cpuinfo/arm64: Refine CPUInfo in Arm64

jodh-intel suggested changes Jul 28, 2017

View reviewed changes

jodh-intel reviewed Jul 28, 2017

View reviewed changes

sboeuf suggested changes Jul 31, 2017

View reviewed changes

jodh-intel suggested changes Aug 7, 2017

View reviewed changes

jodh-intel approved these changes Aug 8, 2017

View reviewed changes

sboeuf approved these changes Aug 8, 2017

View reviewed changes

jodh-intel mentioned this pull request Aug 8, 2017

crio.bats test "ctr device add" hangs under SemaphoreCI clearcontainers/tests#326

Closed

jodh-intel merged commit 939f7e3 into clearcontainers:master Aug 8, 2017

wcwxyz mentioned this pull request Aug 9, 2017

qemu: fix breakage of old binaries containers/virtcontainers#322

Closed

mcastelino pushed a commit to mcastelino/runtime that referenced this pull request Dec 6, 2018

Merge pull request clearcontainers#369 from Pennyzct/kata-check-arm64

dd2bf15

cpuinfo/arm64: Refine CPUInfo in Arm64

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

config: support kernel parameters #369

config: support kernel parameters #369

wcwxyz commented Jul 28, 2017

coveralls commented Jul 28, 2017 •

edited

jodh-intel left a comment

jodh-intel Jul 28, 2017

jodh-intel Jul 28, 2017

jodh-intel Jul 28, 2017

jodh-intel Jul 28, 2017

wcwxyz commented Jul 28, 2017

coveralls commented Jul 31, 2017 •

edited

sboeuf left a comment

sboeuf Jul 31, 2017

wcwxyz Aug 1, 2017

sboeuf Aug 1, 2017

wcwxyz Aug 1, 2017

sboeuf Jul 31, 2017

wcwxyz Aug 1, 2017

sboeuf commented Aug 5, 2017

wcwxyz commented Aug 7, 2017 •

edited

coveralls commented Aug 7, 2017 •

edited

jodh-intel Aug 7, 2017

wcwxyz Aug 7, 2017

coveralls commented Aug 8, 2017 •

edited

jodh-intel commented Aug 8, 2017

sboeuf commented Aug 8, 2017

jodh-intel commented Aug 8, 2017

config: support kernel parameters #369

config: support kernel parameters #369

Conversation

wcwxyz commented Jul 28, 2017

coveralls commented Jul 28, 2017 • edited

jodh-intel left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wcwxyz commented Jul 28, 2017

coveralls commented Jul 31, 2017 • edited

sboeuf left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sboeuf commented Aug 5, 2017

wcwxyz commented Aug 7, 2017 • edited

coveralls commented Aug 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Aug 8, 2017 • edited

jodh-intel commented Aug 8, 2017

sboeuf commented Aug 8, 2017

jodh-intel commented Aug 8, 2017

coveralls commented Jul 28, 2017 •

edited

coveralls commented Jul 31, 2017 •

edited

wcwxyz commented Aug 7, 2017 •

edited

coveralls commented Aug 7, 2017 •

edited

coveralls commented Aug 8, 2017 •

edited