[WIP][DO_NOT_MERGE] KVM implementation of tasks by cshari-zededa · Pull Request #978 · lf-edge/eve

cshari-zededa · 2020-05-07T17:24:14Z

Signed-off-by: Hariharasubramanian C S cshari@zededa.com

rvs · 2020-05-08T03:26:12Z

 	_ = CtrStop(containerID, true)

+	stdinDir := path.Join("/run", "containers-stdin", containerID)
+	if err := os.RemoveAll(stdinDir); err != nil {


Why are you doing this?

Just cleaning it up, since the next CtrCreate expects these directories to be not present. This got exposed when LKTaskLaunch() deletes existing container (if any) before creating new container with the same domainID

I think we need to dig into this more @cshari-zededa -- since it may expose an issue with how we're using containerd and I'd rather fix the root cause (since containerd becomes a center piece of our design). Is there an easy way to reproduce this?

It is not reproducible, I think I added this change unnecessarily. Removed it!

Hi @rvs,
With the #containerd issue fixed,
we no longer need this stdin workaround, so removed that piece of code, and this cleanup
code as well. Please let me know if you think we should retain it.

rvs · 2020-05-08T03:27:18Z

-	config := "/containers/services" + linuxkit + "/config.json"
-	rootfs := "/containers/services" + linuxkit + "/lower"
+	config := "/containers/services/" + linuxkit + "/config.json"
+	rootfs := "/containers/services/" + linuxkit + "/rootfs"


If you need a R/W filesystem you need to prepare a volume for it. A better question is: why DO you need a R/W root filesystem? All the R/W bits are given to you via /run and /persist anyway -- so what's missing?

I had to make rootfs as R/W because of /dev/, /sys, /proc systems, since qemu/kernel is creating /dev/pts/ and /proc/ files during the VM launch.

If those are the only ones -- we need to dig deeper into this as well. Those are supposed to be R/W mounted from the outside when you ask runc to run the container. Lets start with seeing what gets mounted on those endpoint when you call LKTaskLaunch() and we can proceed from there.

Hi @rvs, you are right, I unnecessarily marked this as R/W, whereas Readonly rootfs works just fine, as I verified it today.

Hey @cshari-zededa -- do you plan to update your PR to reflect that going back to a R/O rootfs?

Hi @rvs, I already reverted the Root.Readonly = false part.

The only issue with read-only root partition is that, we can't use "screen" with default settings,
since it tries to create /tmp/.screens directory, and it fails. But we can always override that by
explicitly mentioning "SCREENDIR" env variable. So we are all good with the Readonly root fs

rvs · 2020-05-08T03:29:46Z


 	spec.Root.Path = rootfs
-	spec.Root.Readonly = true
 	if domSettings != nil {


Same question here

I had to make rootfs as R/W because of /dev/, /sys, /proc systems, since qemu/kernel is creating /dev/pts/ and /proc/ files during the VM launch.

reverted the Readonly change to true.

The only issue with read-only root partition is that, we can't use "screen" with default settings,
since it tries to create /tmp/.screens directory, and it fails. But we can always override that by
explicitly mentioning "SCREENDIR" env variable. So we are all good with the Readonly root fs.

rvs

Hey @cshari-zededa -- sorry for the delay -- lets make sure we commit it this week. A few things left to do here:

rebase on the latest EVE
take care of the feedback I left just now
lets see if we can reduce overhead to less than 800M (800M really seems excessive)

For #3 I suggest we do the memory pinning and start passing -overcommit mem-lock=on and -overcommit cpu-pm=on to the qemu we're running.

I think that will help us arrive at a more reasonable overhead figure

rvs · 2020-05-26T00:50:43Z

-	config := "/containers/services" + linuxkit + "/config.json"
-	rootfs := "/containers/services" + linuxkit + "/lower"
+	config := "/containers/services/" + linuxkit + "/config.json"
+	rootfs := "/containers/services/" + linuxkit + "/rootfs"


Hey @cshari-zededa -- do you plan to update your PR to reflect that going back to a R/O rootfs?

rvs · 2020-05-26T00:57:52Z

 		}

-		m := int64(dom.Memory * 1024)
+		m := int64(dom.Memory*1024) + int64(qemuOverHead)


This is not quite right @cshari-zededa -- since this function is going to be used for much more than just launching qemu tasks -- we need a more generic way to accommodate the overhead. Please move it one level up to at least LKTaskLaunch(). IOW, make LKTaskLaunch do a generic UpdateFromDomain() and later manually tweak the memory setting at that level based on the overhead argument passed to LKTaskLaunch

cshari-zededa · 2020-05-29T15:10:31Z

Hey @cshari-zededa -- sorry for the delay -- lets make sure we commit it this week. A few things left to do here:

rebase on the latest EVE

Done

take care of the feedback I left just now

Done, please let me know if you think I have missed anything.

lets see if we can reduce overhead to less than 800M (800M really seems excessive)

Arrived at 500MB with some trail-and-error experiments with PCI passthrough settings.

For #3 I suggest we do the memory pinning and start passing -overcommit mem-lock=on and -overcommit cpu-pm=on to the qemu we're running.

Done

I think that will help us arrive at a more reasonable overhead figure

rvs

Hey @cshari-zededa -- this is great, but there's a tiny, tiny nit left (see below). I would've pulled as-is but it pushes the size of rootfs over the 250Mb limit on ARM -- hence it breaks the ARM build.

rvs · 2020-06-02T05:05:27Z

    pixman=0.34.0-r5   \
    yajl=2.1.0-r0      \
    keyutils=1.5.10-r0 \
+    screen=4.6.2-r1    \


Lets not pull screen in for now -- anyone debugging the system can always apk add screen (and in fact, we should probably move towards using a socket for stdio anyway -- but that shouldn't be part of this PR).

Note that you don't have to be in the same R/O rootfs/container for using screen

Hi @rvs, excluded this change to pull in screen. I've updated the PR. Please take a look.

Signed-off-by: Hariharasubramanian C S <cshari@zededa.com>

rvs · 2020-06-02T19:56:18Z

Thanks @cshari-zededa -- merged!

cshari-zededa requested review from eriknordmark and rvs as code owners May 7, 2020 17:24

rvs requested changes May 8, 2020

View reviewed changes

rvs requested changes May 26, 2020

View reviewed changes

cshari-zededa changed the title ~~[WIP] Initial review of KVM implementation of tasks~~ [WIP][DO_NOT_MERGE] Initial review of KVM implementation of tasks May 29, 2020

cshari-zededa changed the title ~~[WIP][DO_NOT_MERGE] Initial review of KVM implementation of tasks~~ [WIP][DO_NOT_MERGE] KVM implementation of tasks May 29, 2020

rvs approved these changes Jun 2, 2020

View reviewed changes

[WIP] Initial review of KVM implementation of tasks

e03046e

Signed-off-by: Hariharasubramanian C S <cshari@zededa.com>

rvs merged commit 50c4122 into lf-edge:master Jun 2, 2020

Conversation

cshari-zededa commented May 7, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cshari-zededa May 26, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cshari-zededa May 29, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rvs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cshari-zededa commented May 29, 2020

Uh oh!

rvs left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rvs commented Jun 2, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

cshari-zededa May 26, 2020 •

edited

Loading

cshari-zededa May 29, 2020 •

edited

Loading