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

channel: Check for channel type in kernel cmdline options #508

Merged
merged 1 commit into from
Apr 2, 2019

Conversation

amshinde
Copy link
Member

@amshinde amshinde commented Apr 2, 2019

With a recent kernel change, the agent can no longer rely on
/dev/vsock and AF_VSOCK in socket(), to detect if vhost-vsock
channel has been passed by the runtime.
These are not created when the vhost-vsock driver is initialised.

The runtime should now pass this information explicilty.
Based on the channel type passed, the agent now checks for that
partcular channel type.
In case it is not passed, check for both serial and vsock channel.
This also introduces a change in the way vsock channel is detected
by checking for devices under the vhost-vsock driver path.

Fixes #506

Signed-off-by: Archana Shinde archana.m.shinde@intel.com

@amshinde amshinde requested review from sboeuf and egernst April 2, 2019 05:08
Copy link

@sboeuf sboeuf left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks @amshinde for implementing the proper solution with the fallback. And by having the fallback, you're solving the current issue without having to wait for the runtime PR to land 👍

serialCh commType = iota

// vsock channel
vsockCh
Copy link

Choose a reason for hiding this comment

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

Because there's a new line and comments between serialCh and vsockCh, and unknownCh, I think both vsockCh and unknownCh are going to be typed as int instead of commType.

Copy link
Member Author

Choose a reason for hiding this comment

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

@sboeuf I was able to assign between those values, I think compile would have complained in that case.

Copy link

Choose a reason for hiding this comment

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

You can still compile since it's a cast later in the code, but unless the Go compiler got smarter, this was a problem I had encountered before.

Copy link
Member Author

Choose a reason for hiding this comment

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

No golang will never cast automatically.
Looks like newline are allowed according to this wiki: https://github.com/golang/go/wiki/Iota

agent.go Show resolved Hide resolved
@sboeuf
Copy link

sboeuf commented Apr 2, 2019

/test

channel.go Outdated
defer span.Finish()

// check vsock path
var err error
Copy link

Choose a reason for hiding this comment

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

useless variable, please use

if _, err := os.Stat(vSockDevPath); err != nil {

Copy link
Member Author

Choose a reason for hiding this comment

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

@devimc Fixed.

Copy link
Contributor

@jodh-intel jodh-intel left a comment

Choose a reason for hiding this comment

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

Nice. Could you add a TestParseCmdlineOption*() test for the new kernel option though?

config.go Show resolved Hide resolved
@amshinde
Copy link
Member Author

amshinde commented Apr 2, 2019

/test

channel.go Outdated
@@ -228,23 +268,51 @@ func (c *serialChannel) teardown() error {
return c.serialConn.Close()
}

// isAFVSockSupported checks if vsock channel is used by the runtime
// by checking for devices under the vhost-vsock driver path.
// It returns true if more a device is found for the driver.
Copy link
Member

Choose a reason for hiding this comment

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

nit: can you clarify this last sentence?

Copy link
Member Author

Choose a reason for hiding this comment

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

done

Copy link
Member

@egernst egernst left a comment

Choose a reason for hiding this comment

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

aside from small nit LGTM

With a recent kernel change, the agent can no longer rely on
/dev/vsock and AF_VSOCK in socket(), to detect if vhost-vsock
channel has been passed by the runtime.
These are not created when the vhost-vsock driver is initialised.

The runtime should now pass this information explicilty.
Based on the channel type passed, the agent now checks for that
partcular channel type.
In case it is not passed, check for both serial and vsock channel.
This also introduces a change in the way vsock channel is detected
by checking for devices under the vhost-vsock driver path.

Fixes kata-containers#506

Signed-off-by: Archana Shinde <archana.m.shinde@intel.com>
@amshinde
Copy link
Member Author

amshinde commented Apr 2, 2019

/test

@egernst egernst merged commit 4f462c5 into kata-containers:master Apr 2, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants