Deprecate hyperstart, CC shim, and CC proxy #1428

gabibeyer · 2019-03-26T21:32:14Z

vc: as discussed, the hyperstart agent, clear containers shim, and clear containers proxy are no longer being supported, as of version 2.1. Therefore their components have been removed from the kata-runtime implementation and the kata agent, shim and proxy are used when necessary. The virtc API client is also removed, for it uses these deprecated features heavily, and is no longer in working condition.

Fixes issue #1419

sboeuf · 2019-03-26T23:37:55Z

pkg/katautils/config.go

-		case ccProxyTableType:
-			config.ProxyType = vc.CCProxyType
-		case kataProxyTableType:
+		if k == kataProxyTableType {


So two things after talking with @gabibeyer, we don't expect more than one proxy definition, so we should check the number of entries provided through the config. Also, we don't expect to support multiple types of proxy, shim or agent, so we should simply have a toml entry this way:

[proxy] path = "foo" [shim] path = "bar"

And to even go one step further, we can remove the agent entry itself since that was a simple placeholder to determine the type of agent.

/cc @jodh-intel @amshinde @egernst

I prefer we keep original ones and don't change it. Modify existing field names can break compatibility and make me anxious, proxy.kata doesn't harm :)

If we do this I think we should maintain backward compatibility here to allow runtime to be used with older toml possibly in /etc/kata-containers.

Sounds good! I kept the old formatting, then added a 'default' case statement that returns an error is the type passed is not supported.

sboeuf · 2019-03-26T23:40:39Z

Thanks for the PR @gabibeyer, it looks good! Just need to cleanup the whole configuration since we won't be carrying multiple shim/proxy entries.

WeiZhang555 · 2019-03-27T02:16:49Z

/test

allencloud · 2019-03-27T02:20:07Z

Actually alibaba is still using hyperstart for some legacy kernel support in GuestOS. Could we find another way to still support legacy kernels.

sboeuf · 2019-03-27T02:28:56Z

@allencloud but I thought hyperstart was not even working anymore with Kata. I might be wrong though, or is it because they maintain a downstream version of Kata?

teawater · 2019-03-27T02:30:11Z

Looks still have some people need the code.
Maybe we can do some discussions on this PR.

sboeuf · 2019-03-27T02:32:26Z

@teawater yes but I want to clarify if the hyperstart agent is used in the context of Kata or not. This PR removes the support from a Kata perspective, which I thought was not even working anymore with this agent.

bergwolf · 2019-03-27T02:33:01Z

@allencloud I believe you are referring hyperstart support via runv instead. hyperstart_agent.go in kata containers is missing a lot of interface implementations and I doubt it can even work.

WeiZhang555

Most parts LGTM except some comments.

WeiZhang555 · 2019-03-27T02:24:15Z

virtcontainers/kata_agent.go

@@ -70,6 +70,7 @@ var (
 	kataEphemeralDevType = "ephemeral"
 	ephemeralPath        = filepath.Join(kataGuestSandboxDir, kataEphemeralDevType)
 	grpcMaxDataSize      = int64(1024 * 1024)
+	maxHostnameLen       = 64


Why add a new var?

It was originally declared in the hyperstart_agent.go, but is needed here too, so I moved it :)

WeiZhang555 · 2019-03-27T02:25:04Z

virtcontainers/virtcontainers_test.go

-		os.RemoveAll(dir)
-		os.MkdirAll(dir, store.DirMode)
-	}
+	os.RemoveAll(testDir)


Why remove defaultSharedDir?

It was declared in the hyperstart_agent.go file, and seems to point to the hyper sandbox location, so I figured it may be irrelevant with the removal of hyperstart: var defaultSharedDir = /run/hyper/shared/sandboxes/", but I'm not entirely sure

The CI passed, so I think you should be right.

WeiZhang555 · 2019-03-27T02:34:44Z

pkg/katautils/config.go

-		case ccProxyTableType:
-			config.ProxyType = vc.CCProxyType
-		case kataProxyTableType:
+		if k == kataProxyTableType {


I prefer we keep original ones and don't change it. Modify existing field names can break compatibility and make me anxious, proxy.kata doesn't harm :)

WeiZhang555 · 2019-03-27T02:48:33Z

@allencloud I believe you are referring hyperstart support via runv instead. hyperstart_agent.go in kata containers is missing a lot of interface implementations and I doubt it can even work.

If it's runv+hyperstart, then I think it's safe to remove from kata. Or we need more discussion about our deprecated features.

gabibeyer · 2019-03-27T05:42:54Z

virtcontainers/api_test.go

-	bindUnmountAllRootfs(ctx, defaultSharedDir, pImpl)
+	// TODO: defaultSharedDir is a hyper var = /run/hyper/shared/sandboxes
+	// do we need to unmount sandboxes and containers?
+	bindUnmountAllRootfs(ctx, testDir, pImpl)


@WeiZhang555 your comment reminded me I needed ask about this, thank you!
Two test cases were calling bindUnmountAllRootfs with the defaultSharedDir as an argument, however neither test had anything to do with hyperstart (since defaultSharedDir = /run/hyper/shared/sandboxes/). I wasn't sure what to do about that situation; whether to bindUnmount the KataHostSharedDir, testDir, or some other variable instead.

I'm not sure either, I think it should be kataHostSharedDir, but I wonder if this is really necessary, maybe bindUnmountAllRootfs can be removed.

gabibeyer · 2019-04-02T19:02:56Z

/test

virtcontainers/api_test.go

amshinde · 2019-04-02T20:46:58Z

pkg/katautils/config.go

-		case ccProxyTableType:
-			config.ProxyType = vc.CCProxyType
-		case kataProxyTableType:
+		if k == kataProxyTableType {


If we do this I think we should maintain backward compatibility here to allow runtime to be used with older toml possibly in /etc/kata-containers.

virtcontainers/kata_shim_test.go

WeiZhang555 · 2019-04-03T08:15:41Z

/test

jcvenegas · 2019-04-10T13:54:24Z

@gabibeyer ping this PR require rebase

gabibeyer · 2019-04-10T22:07:21Z

/test

gabibeyer · 2019-04-12T17:23:45Z

/test

The hyperstart agent has not been supported in kata since 2.1, so remove it as a component to kata. Mentioned in issue kata-containers#1113. Fixes: kata-containers#1419 Signed-off-by: Gabi Beyer <gabrielle.n.beyer@intel.com>

previously used as a small api client for virtcontainers, virtc no longer needed. Fixes kata-containers#1419 Signed-off-by: Gabi Beyer <gabrielle.n.beyer@intel.com>

Clear Containers proxy and shim are no longer supported. This was mentioned in issue kata-containers#1113. Their functionalities are thus removed from the runtime. Fixes kata-containers#1419 Signed-off-by: Gabi Beyer <gabrielle.n.beyer@intel.com>

The kata shim tests make use of an ioctl function, so instead of having a custom one within that file, use the ioctl function in utils/utils_linux Fixes kata-containers#1419 Signed-off-by: Gabi Beyer <Gabrielle.n.beyer@intel.com>

gabibeyer · 2019-04-12T18:11:18Z

/test

WeiZhang555

LGTM.
Thanks! Only one small nit .

WeiZhang555 · 2019-04-13T03:01:59Z

virtcontainers/utils/utils_linux.go

+	if _, _, errno := unix.Syscall(unix.SYS_IOCTL, fd, request, data); errno != 0 {
+		//uintptr(request)
+		//uintptr(unsafe.Pointer(&arg1)),
+		//); errno != 0 {


need to clean unnecessary codes.

WeiZhang555 · 2019-04-13T03:05:06Z

I'll merge this. You can file another PR to clean unnecessary comments, it doesn't influence functions so I think it's OK

gabibeyer · 2019-04-14T03:33:49Z

Yay! Thank you @WeiZhang555

gabibeyer requested a review from a team as a code owner March 26, 2019 21:32

gabibeyer force-pushed the slashNburn branch 3 times, most recently from 2d6ac6b to 3d74dfa Compare March 26, 2019 22:28

sboeuf reviewed Mar 26, 2019

View reviewed changes

gabibeyer force-pushed the slashNburn branch from 3d74dfa to 36eaeae Compare March 27, 2019 00:00

teawater added the do-not-merge label Mar 27, 2019

teawater removed the do-not-merge label Mar 27, 2019

WeiZhang555 reviewed Mar 27, 2019

View reviewed changes

gabibeyer commented Mar 27, 2019

View reviewed changes

gabibeyer requested review from amshinde, egernst and jodh-intel March 27, 2019 18:31

gabibeyer force-pushed the slashNburn branch from 36eaeae to 8d06bba Compare April 2, 2019 18:45

amshinde reviewed Apr 2, 2019

View reviewed changes

gabibeyer force-pushed the slashNburn branch from 8d06bba to bf2899b Compare April 2, 2019 22:20

gabibeyer force-pushed the slashNburn branch 2 times, most recently from 1b9b2d2 to c611cab Compare April 10, 2019 20:36

gabibeyer requested review from WeiZhang555, amshinde and sboeuf and removed request for WeiZhang555 April 12, 2019 16:50

gabibeyer force-pushed the slashNburn branch from c611cab to 7d8ee3f Compare April 12, 2019 17:12

amshinde approved these changes Apr 12, 2019

View reviewed changes

gabibeyer and others added 4 commits April 12, 2019 10:48

vc: deprecate hyperstart agent

d4ef9c0

The hyperstart agent has not been supported in kata since 2.1, so remove it as a component to kata. Mentioned in issue kata-containers#1113. Fixes: kata-containers#1419 Signed-off-by: Gabi Beyer <gabrielle.n.beyer@intel.com>

vc: remove virtc api cli

c658770

previously used as a small api client for virtcontainers, virtc no longer needed. Fixes kata-containers#1419 Signed-off-by: Gabi Beyer <gabrielle.n.beyer@intel.com>

vc: Deprecate CC proxy and shim

c425079

Clear Containers proxy and shim are no longer supported. This was mentioned in issue kata-containers#1113. Their functionalities are thus removed from the runtime. Fixes kata-containers#1419 Signed-off-by: Gabi Beyer <gabrielle.n.beyer@intel.com>

vc: modify ioctl function to handle shim test

b08ab6a

The kata shim tests make use of an ioctl function, so instead of having a custom one within that file, use the ioctl function in utils/utils_linux Fixes kata-containers#1419 Signed-off-by: Gabi Beyer <Gabrielle.n.beyer@intel.com>

gabibeyer force-pushed the slashNburn branch from 7d8ee3f to b08ab6a Compare April 12, 2019 17:48

WeiZhang555 approved these changes Apr 13, 2019

View reviewed changes

WeiZhang555 merged commit fae022d into kata-containers:master Apr 13, 2019

gabibeyer deleted the slashNburn branch April 14, 2019 03:33

egernst mentioned this pull request Apr 16, 2019

Stable 1.5 backports #1546

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Deprecate hyperstart, CC shim, and CC proxy #1428

Deprecate hyperstart, CC shim, and CC proxy #1428

gabibeyer commented Mar 26, 2019

sboeuf Mar 26, 2019

WeiZhang555 Mar 27, 2019

amshinde Apr 2, 2019

gabibeyer Apr 10, 2019

sboeuf commented Mar 26, 2019

WeiZhang555 commented Mar 27, 2019

allencloud commented Mar 27, 2019

sboeuf commented Mar 27, 2019

teawater commented Mar 27, 2019

sboeuf commented Mar 27, 2019

bergwolf commented Mar 27, 2019

WeiZhang555 left a comment •

edited

Loading

WeiZhang555 Mar 27, 2019

gabibeyer Mar 27, 2019

WeiZhang555 Mar 27, 2019

WeiZhang555 Mar 27, 2019

gabibeyer Mar 27, 2019 •

edited

Loading

WeiZhang555 Mar 27, 2019

WeiZhang555 Mar 27, 2019

WeiZhang555 commented Mar 27, 2019 •

edited

Loading

gabibeyer Mar 27, 2019

WeiZhang555 Mar 27, 2019

gabibeyer commented Apr 2, 2019

amshinde Apr 2, 2019

WeiZhang555 commented Apr 3, 2019

jcvenegas commented Apr 10, 2019

gabibeyer commented Apr 10, 2019

gabibeyer commented Apr 12, 2019

gabibeyer commented Apr 12, 2019

WeiZhang555 left a comment

WeiZhang555 Apr 13, 2019

WeiZhang555 commented Apr 13, 2019

gabibeyer commented Apr 14, 2019

Deprecate hyperstart, CC shim, and CC proxy #1428

Deprecate hyperstart, CC shim, and CC proxy #1428

Conversation

gabibeyer commented Mar 26, 2019

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 Mar 26, 2019

WeiZhang555 commented Mar 27, 2019

allencloud commented Mar 27, 2019

sboeuf commented Mar 27, 2019

teawater commented Mar 27, 2019

sboeuf commented Mar 27, 2019

bergwolf commented Mar 27, 2019

WeiZhang555 left a comment • edited Loading

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

gabibeyer Mar 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WeiZhang555 commented Mar 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabibeyer commented Apr 2, 2019

Choose a reason for hiding this comment

WeiZhang555 commented Apr 3, 2019

jcvenegas commented Apr 10, 2019

gabibeyer commented Apr 10, 2019

gabibeyer commented Apr 12, 2019

gabibeyer commented Apr 12, 2019

WeiZhang555 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WeiZhang555 commented Apr 13, 2019

gabibeyer commented Apr 14, 2019

WeiZhang555 left a comment •

edited

Loading

gabibeyer Mar 27, 2019 •

edited

Loading

WeiZhang555 commented Mar 27, 2019 •

edited

Loading