Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New addition of vSockets to the python socket module #71771

Closed
caavery mannequin opened this issue Jul 21, 2016 · 32 comments
Closed

New addition of vSockets to the python socket module #71771

caavery mannequin opened this issue Jul 21, 2016 · 32 comments
Labels
3.7 stdlib type-feature

Comments

@caavery
Copy link
Mannequin

@caavery caavery mannequin commented Jul 21, 2016

BPO 27584
Nosy @gpshead, @ncoghlan, @tiran, @bitdancer, @berkerpeksag, @kushaldas, @caavery
PRs
  • #2489
  • Files
  • vsocket.patch: vsockets for linux support
  • vsock_rev2.patch: vsockets for linux support revision 2
  • nc-vsock
  • 0001-bpo-27584-New-addition-of-vSockets-to-the-python-soc.patch
  • REAME.txt
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2017-09-06.22:18:41.338>
    created_at = <Date 2016-07-21.15:36:30.666>
    labels = ['3.7', 'type-feature', 'library']
    title = 'New addition of vSockets to the python socket module'
    updated_at = <Date 2017-09-06.22:18:41.338>
    user = 'https://github.com/caavery'

    bugs.python.org fields:

    activity = <Date 2017-09-06.22:18:41.338>
    actor = 'christian.heimes'
    assignee = 'none'
    closed = True
    closed_date = <Date 2017-09-06.22:18:41.338>
    closer = 'christian.heimes'
    components = ['Library (Lib)']
    creation = <Date 2016-07-21.15:36:30.666>
    creator = 'Cathy Avery'
    dependencies = []
    files = ['43816', '45411', '45964', '46977', '46978']
    hgrepos = ['349']
    issue_num = 27584
    keywords = ['patch']
    message_count = 32.0
    messages = ['270935', '272358', '272363', '272366', '272403', '280423', '282798', '283534', '283535', '283607', '283608', '283612', '283620', '296410', '296413', '297064', '297065', '297276', '297285', '297516', '297568', '297835', '298165', '298166', '298167', '298169', '298171', '298173', '298174', '298805', '298953', '301527']
    nosy_count = 7.0
    nosy_names = ['gregory.p.smith', 'ncoghlan', 'christian.heimes', 'r.david.murray', 'berker.peksag', 'kushal.das', 'Cathy Avery']
    pr_nums = ['2489']
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'enhancement'
    url = 'https://bugs.python.org/issue27584'
    versions = ['Python 3.7']

    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Jul 21, 2016

    I have added AF_VSOCK support to python's 3.6 socket module ( socketmodule.c socketmodule.h cloned from https://hg.python.org/cpython ). The implementation is very similar to AF_NETLINK. AF_VSOCK requires the VMware-specific VMCI transport which is currently upstream or the virtio-vsock drivers developed by Stefan Hajnoczi at Red Hat. The virtio-vsock drivers are not upstream yet but more information with source and build instructions can be found at http://qemu-project.org/Features/VirtioVsock.

    More information on vSocket programming can be found at https://pubs.vmware.com/vsphere-60/topic/com.vmware.ICbase/PDF/ws9_esx60_vmci_sockets.pdf

    The VMCI transport supports SOCK_DGRAM and SOCK_STREAM on both Linux and Windows. Virtio-vsock currently supports SOCK_STREAM only on Linux.

    My python addition supports SOCK_STREAM and SOCK_DGRAM calls on Linux only.

    I have tested my implementation on both driver sets on Linux.

    Attached is a diff file so you can see which files I've modified. These include a new configure.ac. I have already tested the new file generation by running autoreconf.

    Also included in the patch is an updated socket.rst file however I could not get the final html page to be double spaced.

    @caavery caavery mannequin added stdlib type-feature labels Jul 21, 2016
    @bitdancer
    Copy link
    Member

    @bitdancer bitdancer commented Aug 10, 2016

    Looks like there's a missing versionadded directive in the doc patch.

    Is it possible/sensible to add tests for the new feature?

    (I haven't reviewed the patch in detail, hopefully someone with more experience with C socket programming than I have will do that.)

    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Aug 10, 2016

    Sure I can add tests. I would like to base them on the existing socket tests. Where are those?

    I did add a version

    + .. versionadded:: 3.4

    It just not may not be the right one.

    @bitdancer
    Copy link
    Member

    @bitdancer bitdancer commented Aug 10, 2016

    Ah, I see. No, the versionadded will be 3.6, and should go *after* the documentation of the new socket type.

    The existing socket tests are in Lib/test/test_socket.py.

    @kushaldas
    Copy link
    Member

    @kushaldas kushaldas commented Aug 11, 2016

    The patch can be applied, and build successfully. I have ran the current test suite[1]. The two failed tests do not seem to be have anything to do with this patch (read the end of the consoleText output). I think the thing remaining is the new test cases, and NEWS file update.

    [1] https://ci.centos.org/job/cPython-build-patch/24/consoleText

    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Nov 9, 2016

    Please forgive the long delay in providing this update. I got a little sidetracked. Attached is the patch for Python 3.7. It includes fixes suggested in rev 1 plus VSOCK tests in test_socket.py.

    Thanks,

    Cathy

    @caavery caavery mannequin added the 3.7 label Nov 9, 2016
    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Dec 9, 2016

    Is there anything else that is needed for this patch?

    Thanks!

    @bitdancer
    Copy link
    Member

    @bitdancer bitdancer commented Dec 18, 2016

    The second patch seems to be missing the configure changes. Also, the tests have some over-long lines (we limit line lengths to 79 characters). I realize there are other long lines in that file, but no need to add more :)

    There is trailing whitespace on a number of lines in your patch.

    Since this is new, we may not want to accept it until the support hits upstream. Specifically, it will be difficult to get a review if the reviewer has to build a custom kernel to test the code :) You do say that the VMCI is upstream, but I don't know what that means. Which upstream?

    Note: I'm not familiar with the socket C code, so I haven't reviewed the C code changes. The tests look fine to me.

    For the docs, the proposal doesn't seem to follow the format of the existing docs. I would expect only the first paragraph located where you have it. The remaining constants should be in the 'module contents'/'constants' section, I think. Yes, that means each one gets a '.. versionadded' label. Presumably also an 'availablility' label with whatever the minimum kernel version is...another reason we may need to wait.

    @bitdancer
    Copy link
    Member

    @bitdancer bitdancer commented Dec 18, 2016

    Oh, I see, the ac changes are there, I was looking at the patch delta instead of the complete patch.

    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Dec 19, 2016

    Is there a format checker I could use on the patch?

    VMCI and the vmw_vsock_vmci_transport kernel modules are located in the upstream linux tree at

    git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git

    They have been there for about years. These drivers are part of various downstream kernels such as RHEL. You will need a Vmware virtual machine in order to test it.

    Only the virtio-vsock driver is a new vsock application that needs to be custom built.

    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Dec 19, 2016

    Sorry about the typo the drivers have been there for about 4 years.

    @bitdancer
    Copy link
    Member

    @bitdancer bitdancer commented Dec 19, 2016

    'make patchcheck' will do whitespace checking because that's hard to eyeball (although many editors/IDEs do support making it visible nowadays). We don't use any other checking tools other than eyeballs, since not all of the existing code conforms to PEP7/8 and for various reasons we aren't going to update most of the old code to conform.

    So, if I'm running an ubuntu virtual machine under VMWare Fusion (which I already have set up) I should be able to get the tests to run? Or does it need to be RedHat (or presumably CentOS)?

    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Dec 19, 2016

    First make sure the driver is in your kernel. It will be with RHEL. Look in /lib/modeles/"your kernel name"/kernel/net/vmw_vsock/vmw_vsock_vmci_transport. I have never tried it on vmware fusion. I have tested it on ESX. See if there is a VMCI option to enable on your VM's settings. Start the vm and do an lsmod to see if vmw_vsock_vmci_transport is loaded.

    I've attached a little C program thats netcat for vsock. Its a quick confirmation that your transport is loaded correctly. It will show you your CID.

    run ./nc-vsock

    CID = 973033371
    CID = 0x39ff4f9b
    usage: ./nc-vsock [-l <port> [-t <dst> <dstport>] | <cid> <port>]

    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Jun 20, 2017

    The vsock code is now in the linux upstream kernel and qemu. I will be resubmitting my vsock patches for python. So from what I can tell the tip of the devel tree is for 3.7 and that the source control has switched to git. My question is do I need a github account or can I push git patches directly to this issues page?

    Thanks,

    Cathy

    @berkerpeksag
    Copy link
    Member

    @berkerpeksag berkerpeksag commented Jun 20, 2017

    You don't need a GitHub account for contributing to CPython, but the pull request workflow is the preferred way. You can still attach your patches to this issue.

    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Jun 27, 2017

    I've attached the third version of VSOCK patch addressing the concerns of the last rev. I've also included a README file that lists instructions on how to setup a test environment.

    Thanks

    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Jun 27, 2017

    Help file.

    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Jun 29, 2017

    I also issued a pull request

    #2489

    Let me know if I screwed it up.

    Thanks,

    Cathy

    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Jun 29, 2017

    The build and test bots failed but I don't understand why. The build could not find module _socket but that is not new and other modules failed.

    The test could not import fcntl which I did add but should not fail as I have run this test many time and other tests import fcntl.

    #2489

    @ncoghlan
    Copy link
    Contributor

    @ncoghlan ncoghlan commented Jul 2, 2017

    I'm attempting to figure out whether or not we have a buildbot in the Buildbot fleet that will cover this test case.

    Based on the pre-merge CI run, it seems Ubuntu 14.04 is too old to include the required kernel headers.

    However, it looks like RHEL/CentOS are also currently still missing the userspace changes to fully enable AF_VSOCK support (as the Red Hat backport flow appears to have gone through the dedicated hypervisor variant first): https://bugzilla.redhat.com/show_bug.cgi?id=1315822

    So it's looking to me like we're going to need either a recent Fedora, a non-LTS Ubuntu, or a Debian 9 system to be confident we have the right headers available.

    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Jul 3, 2017

    Fedora 25 has the proper headers.

    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Jul 6, 2017

    So I revised my code based on the reviews and I passed all the checks ... now what?

    Thanks,

    Cathy

    @bitdancer
    Copy link
    Member

    @bitdancer bitdancer commented Jul 11, 2017

    I think we are waiting on confirmation that we have a buildbot that has the necessary headers.

    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Jul 11, 2017

    OK thanks!

    @gpshead
    Copy link
    Member

    @gpshead gpshead commented Jul 11, 2017

    I updated my PGO buildbot to Debian 9 which should have them.

    http://buildbot.python.org/all/builders/AMD64%20Debian%20PGO%203.x

    ~$ grep AF_VSOCK /usr/include///*
    /usr/include/x86_64-linux-gnu/bits/socket.h:#define AF_VSOCK PF_VSOCK

    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Jul 11, 2017

    You will also need linux/vm_sockets.h in order to build.

    @gpshead
    Copy link
    Member

    @gpshead gpshead commented Jul 11, 2017

    yep, linux/vm_sockets.h exists. I believe it's kernel headers are from 4.9.

    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Jul 11, 2017

    That should do it.

    @gpshead
    Copy link
    Member

    @gpshead gpshead commented Jul 11, 2017

    Everything compiles successfully on this host - configure detects the header has HAVE_LINUX_VM_SOCKETS_H set to 1.

    test_socket passes on this host. However since i'm not running with a /dev/vsock, the unittests (correctly) skip the new tests.

    testCreateSocket (test.test_socket.BasicVSOCKTest) ... skipped 'VSOCK sockets required for this test.'
    testCrucialConstants (test.test_socket.BasicVSOCKTest) ... skipped 'VSOCK sockets required for this test.'
    testSocketBufferSize (test.test_socket.BasicVSOCKTest) ... skipped 'VSOCK sockets required for this test.'
    testVSOCKConstants (test.test_socket.BasicVSOCKTest) ... skipped 'VSOCK sockets required for this test.'
    testStream (test.test_socket.ThreadedVSOCKSocketStreamTest) ... skipped 'VSOCK sockets required for this test.'

    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Jul 21, 2017

    Hi,

    I believe I am waiting for a final review. Is there anything else I need to be doing at this point.

    Thanks,

    Cathy

    @caavery
    Copy link
    Mannequin Author

    @caavery caavery mannequin commented Jul 24, 2017

    There is an outstanding review on my pull request at #2489 as there is an red X at changes requested by kushaldas and I believe I have made the necessary changes.

    Again please let me know if there is anything that I need to do as I am new to this process.

    Thanks,

    Cathy

    @tiran
    Copy link
    Member

    @tiran tiran commented Sep 6, 2017

    New changeset effc12f by Christian Heimes (caavery) in branch 'master':
    bpo-27584: New addition of vSockets to the python socket module (bpo-2489)
    effc12f

    @tiran tiran closed this as completed Sep 6, 2017
    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    3.7 stdlib type-feature
    Projects
    None yet
    Development

    No branches or pull requests

    6 participants