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

Mirage3 #262

Merged
merged 32 commits into from
Aug 3, 2017
Merged

Mirage3 #262

merged 32 commits into from
Aug 3, 2017

Conversation

samoht
Copy link
Member

@samoht samoht commented Jul 28, 2017

This is built on top of the major cleanup that I've pushed in #259

Everything compiles but it seems that there are 3 missing functions in the tcpip stack:

  • Stack_ipv4.set_ip_netmask
  • Stack_ipv4.set_ip_gateways
  • Stack_tcp_wire.src_of_id (e.g. there is no way to get the source IP)

Also the tests are compiling but not passing, either because of my very flaky connection or because I broke something with the dns ref that I modified to be None by default. I will investigate more next week.

@samoht samoht force-pushed the mirage3 branch 3 times, most recently from 188527e to 5487366 Compare July 31, 2017 17:25
@djs55
Copy link
Collaborator

djs55 commented Jul 31, 2017

Error: External library "tcpip.unix" not found.
-> required by "src/hostnet/jbuild (context default)"
Hint: try: jbuilder external-lib-deps --missing --dev src/bin/main.exe
make: *** [vpnkit.exe] Error 1

@samoht
Copy link
Member Author

samoht commented Jul 31, 2017

It needs the dev version of mirage-tcpip and charrua-core. I'll fix this.

@samoht samoht force-pushed the mirage3 branch 2 times, most recently from b002878 to 854b6d4 Compare August 1, 2017 08:22
@samoht
Copy link
Member Author

samoht commented Aug 1, 2017

Hum, it seems that some of the tests in mirage-tcpip do not work on windows:

--------------------------------------------------------------------------------
- ASSERT sender address
- --------------------------------------------------------------------------------
- Fatal error: exception (Failure "Error sender address: expecting fc00::23, got ::.")

And the vpnkit tests are failing on macOS (I can reproduce this locally, and @djs55 identified the error message to come from here):

main_uwt.exe: [WARNING] IP module couldn't send UDP packet to 8.8.8.8: no route to destination: no response for IP on local network
Fatal error: exception Dns__Protocol.Dns_resolve_error(_)
Raised at file "src/core/lwt.ml", line 2719, characters 24-25
Called from file "uwt.ml", line 1848, characters 10-23
Called from file "list.ml", line 77, characters 12-15
Called from file "list.ml", line 77, characters 12-15
Called from file "src/hostnet_test/main_uwt.ml", line 19, characters 2-282
make: *** [test] Error 2

@djs55
Copy link
Collaborator

djs55 commented Aug 1, 2017

I'll try to fix the routing problem by switching to DHCP in the test case. This requires: mirage/charrua#65

@samoht
Copy link
Member Author

samoht commented Aug 1, 2017

I can produce a different failure for ipv6 on Windows. Trying to understand what happens ...

@samoht
Copy link
Member Author

samoht commented Aug 1, 2017

There are 2 failing tests on windows:

  • ipv6 0:
test.exe: [INFO] Connected Ethernet interface 02:50:00:00:00:01
test.exe: [INFO] IP6: Starting
test.exe: [DEBUG] ND6: Sending RS
test.exe: [DEBUG] ND6: Sending NS src=:: dst=ff02::1:ff00:1 tgt=fe80::50:ff:fe00:1
test.exe: [DEBUG] no size limit, sending
test.exe: [DEBUG] no size limit, sending
test.exe: [DEBUG] ND6: Sending NS src=:: dst=ff02::1:ff00:23 tgt=fc00::23
test.exe: [DEBUG] no size limit, sending
test.exe: [INFO] UDP interface connected on
test.exe: [INFO] Connected Ethernet interface 02:50:00:00:00:02
test.exe: [INFO] IP6: Starting
test.exe: [DEBUG] ND6: Sending RS
test.exe: [DEBUG] ND6: Sending NS src=:: dst=ff02::1:ff00:2 tgt=fe80::50:ff:fe00:2
test.exe: [DEBUG] no size limit, sending
test.exe: [DEBUG] no size limit, sending
test.exe: [DEBUG] ND6: Sending NS src=:: dst=ff02::1:ff00:45 tgt=fc00::45
test.exe: [DEBUG] no size limit, sending
test.exe: [INFO] UDP interface connected on
test.exe: [DEBUG] IP6: Queueing packet: dst=fc00::45
test.exe: [DEBUG] ND6: Sending NS src=:: dst=ff02::1:ff00:45 tgt=fc00::45
test.exe: [DEBUG] no size limit, sending
test.exe: [DEBUG] ND: Received NS: src=:: dst=ff02::1:ff00:45 tgt=fc00::45
test.exe: [DEBUG] SLAAC: fe80::50:ff:fe00:2 --> PREFERRED
test.exe: [DEBUG] SLAAC: fc00::45 --> PREFERRED
test.exe: [DEBUG] SLAAC: fe80::50:ff:fe00:1 --> PREFERRED
test.exe: [DEBUG] SLAAC: fc00::23 --> PREFERRED
--------------------------------------------------------------------------------------------------------------------
ASSERT UDP packet should have been received
--------------------------------------------------------------------------------------------------------------------
Test error: Error UDP packet should have been received.
  • socket 1:
_build/_tests\socket.001.output:
test.exe: [INFO] Manager: connect
test.exe: [INFO] Manager: configuring
test.exe: [WARNING] Manager: sockets currently bind to all available IPs. IPs 127.0.0.1 were specified, but this will be ignored
test.exe: [INFO] Manager: connect
test.exe: [INFO] Manager: configuring
test.exe: [WARNING] Manager: sockets currently bind to all available IPs. IPs 127.0.0.1 were specified, but this will be ignored
[exception] Unix.Unix_error(Unix.EPROTONOSUPPORT, "socket", "")

I will follow up on mirage/mirage-tcpip

@djs55
Copy link
Collaborator

djs55 commented Aug 1, 2017

One of the tcpip tests just failed on CircleCI too:

[ERROR]             connect              1   connect two stacks, uniform packet loss of packets with no payload x 100.

(probably intermittent)

@samoht
Copy link
Member Author

samoht commented Aug 1, 2017

yes sorry I have changed the default test flags in my tcpip branch, I will create a more stable dev branch ...

@samoht
Copy link
Member Author

samoht commented Aug 1, 2017

I've pushed a new Git url for tcpip dev package, where only quick tests are running. Not sure why the slow test is failing, but this is probably due to a timeout/slow machine.

Lwt.return r >|= lift_error
Lwt.async
(fun () ->
Netif.listen netif @@ callback t >>= function
Copy link
Member Author

Choose a reason for hiding this comment

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

you can write this as:

 Netif.listen netif @@ callback t >|= function
| Ok ()   -> ()
| Error _ -> Log.err (fun f -> ... )

~gateway:None
~network:Ipaddr.V4.(Prefix.of_addr unspecified)
~network:Ipaddr.V4.Prefix.global
Copy link
Member Author

Choose a reason for hiding this comment

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

good catch! I wasn't really sure what to use there :-)

@samoht
Copy link
Member Author

samoht commented Aug 1, 2017

@djs55 it now seems that we have genuine test errors on both windows and macOS:

**** Starting test Host resolver: lookup 
Starting test case 
main_uwt.exe: [INFO] Made a loopback connection
main_uwt.exe: [INFO] PPP.negotiate: received { magic = VMN3T; version = 1; commit = 0123456789012345678901234567890123456789 }
main_uwt.exe: [INFO] Client.negotiate: received { magic = VMN3T; version = 1; commit = 0123456789012345678901234567890123456789 }
main_uwt.exe: [INFO] PPP.negotiate: received Ethernet d1d9cd61-d0dc-4715-9bb3-4c11da7ad7a5
main_uwt.exe: [INFO] PPP.negotiate: sending { mtu = 1500; max_packet_size = 1550; client_macaddr = c0:ff:ee:c0:ff:ee }
main_uwt.exe: [INFO] PPP.listen: rebinding the primary listen callback
main_uwt.exe: [INFO] Client mac: c0:ff:ee:c0:ff:ee server mac: f6:16:36:bc:f9:c6
main_uwt.exe: [INFO] TCP/IP ready
main_uwt.exe: [INFO] stack connected
main_uwt.exe: [INFO] Initialising client TCP/IP stack
main_uwt.exe: [INFO] PPP.listen: rebinding the primary listen callback
main_uwt.exe: [ERROR] Socket.TCPv4.read tcp:127.0.0.1:49510: caught Lwt.Completion_loop.Canceled returning Eof
Fatal error: exception (Failure timeout)
Raised at file "src/core/lwt.ml", line 2719, characters 24-25
Called from file "uwt.ml", line 1848, characters 10-23
Called from file "list.ml", line 77, characters 12-15
Called from file "list.ml", line 77, characters 12-15
Called from file "src/hostnet_test/main_uwt.ml", line 19, characters 2-282

@djs55
Copy link
Collaborator

djs55 commented Aug 2, 2017

Tests are still failing in CI but I've not managed to reproduce it locally yet. I've pushed some patches to improve the debug logging to see if that helps. I'll see if I can make myself a more exact replica of the CI environment.

@djs55
Copy link
Collaborator

djs55 commented Aug 2, 2017

OK it reproduced for me when I ran scripts/common.sh. A bit of environment-bisecting later and the culpit appears to be: mirage/charrua#66 -- the tests work for me when this is merged.

@samoht
Copy link
Member Author

samoht commented Aug 2, 2017

The new errors are related to missing license files, which means the tests passed, yay!!

I'll fix the licenses :-)

@samoht
Copy link
Member Author

samoht commented Aug 2, 2017

ALL GREEN!

The last worrying (or not) bit are the failing TCPIP tests on windows. Probably not really severe, but would be great to check that we understand what happens. I'll have a look tomorrow if I find some time.

@djs55
Copy link
Collaborator

djs55 commented Aug 2, 2017

Was it only the IPv6 tests? If so that's not too bad, as we don't use IPv6 (yet).

Since I'm paranoid, I'll put this vpnkit build through the Docker for Desktop end-to-end tests too and report back.

@samoht
Copy link
Member Author

samoht commented Aug 2, 2017

yea, the ipv6 test is failing but also using an unsuported protocol over a socket: mirage/mirage-tcpip#331

@djs55
Copy link
Collaborator

djs55 commented Aug 2, 2017

Docker Windows tests all passed but Mac tests all failed -- I'll investigate tomorrow. It's probably a startup issue somehow related to the Vmnet Unix domain socket.

Signed-off-by: Thomas Gazagnaire <thomas@gazagnaire.org>
Remove the ones which have been released and add new ones

Signed-off-by: Thomas Gazagnaire <thomas@gazagnaire.org>
Signed-off-by: Thomas Gazagnaire <thomas@gazagnaire.org>
djs55 and others added 16 commits August 3, 2017 11:55
Signed-off-by: David Scott <dave@recoil.org>
- use the `connect ~ip` argument rather than `set_ip` to set the stack's
  IP
- set the network to `global` (i.e. "all addresses are on my local network)
  instead of `unspecified` (i.e. "no addresses are on my local network).

Signed-off-by: David Scott <dave@recoil.org>
Previously the test cases were using an (unspecified?) static IP when
talking to the loopback vpnkit server. This patch makes the client use
DHCP which is

- more realistic: this is what we expect real clients to do
- more likely to work: previously the client's IP address appeared to be
  undefined

Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: Thomas Gazagnaire <thomas@gazagnaire.org>
because of mirage/mirage-tcpip#330 and mirage/mirage-tcpip#331

Signed-off-by: Thomas Gazagnaire <thomas@gazagnaire.org>
Signed-off-by: David Scott <dave.scott@docker.com>
Signed-off-by: Thomas Gazagnaire <thomas@gazagnaire.org>
Recall that `listen` can be called multiple times with a "last-callback-wins"
semantic. Previously we would start additional copies of the packet receive
loop() which can lead to I/O problems on the channel. This patch starts
at most one receive loop; other `listen` calls block until disconnect.

Signed-off-by: David Scott <dave.scott@docker.com>
Although this is a "Point to Point Protocol" it is not *the* "Point to
Point Protocol" so call it "Vmnet" instead (which at least matches the
module name, but it's still not ideal)

Signed-off-by: David Scott <dave.scott@docker.com>
Previously we would fail the thread with `Too_many_connections` but this
meant that the SYN was never responed to. With this change, we send explicit
RST packets to the caller.

This makes the `test_max_connections` test pass -- previously the TCP
`connect` call would resend SYNs for a long time.

Signed-off-by: David Scott <dave.scott@docker.com>
It's useful to know what the connection limit is supposed to be.

Signed-off-by: David Scott <dave.scott@docker.com>
When debugging traces from the test cases it's useful to know whether
a `Vmnet.listen` call is from the client end or the server. This patch
adds a logging prefix and sets this to either `Vmnet.Client` or
`Vmnet.Server`.

Signed-off-by: David Scott <dave.scott@docker.com>
When tests time out in CI it's useful to see where the time was spent.

Signed-off-by: David Scott <dave.scott@docker.com>
Signed-off-by: Thomas Gazagnaire <thomas@gazagnaire.org>
The mirage-http package depends on mirage-conduit which depends on
tls which depends on libgmp etc. We only need the definitions of `read_line`
`read` `write` and `flush` so include them directly.

Signed-off-by: David Scott <dave.scott@docker.com>
Signed-off-by: Thomas Gazagnaire <thomas@gazagnaire.org>
Signed-off-by: David Scott <dave.scott@docker.com>
…pnkit

Signed-off-by: Thomas Gazagnaire <thomas@gazagnaire.org>
@djs55
Copy link
Collaborator

djs55 commented Aug 3, 2017

@samoht sorry I force pushed this and lost 2 of your patches. I've re-applied them, hopefully it's ok now.

Signed-off-by: David Scott <dave@recoil.org>
Signed-off-by: David Scott <dave@recoil.org>
@djs55
Copy link
Collaborator

djs55 commented Aug 3, 2017

OK, the Docker Mac tests are passing now. I think we are safe to merge this. Any objections? (@samoht?)

@djs55 djs55 merged commit a06c975 into moby:master Aug 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants