Skip to content
This repository has been archived by the owner on Jun 20, 2024. It is now read-only.

weave script misses many errors #2053

Closed
rade opened this issue Mar 16, 2016 · 2 comments
Closed

weave script misses many errors #2053

rade opened this issue Mar 16, 2016 · 2 comments
Assignees
Labels
Milestone

Comments

@rade
Copy link
Member

rade commented Mar 16, 2016

set -e gets disabled in certain contexts

I've just combed the weave script for places where that has an impact...

  • configure_veth_bridged_fastdp (via create_veth)
    • if adding the fastdp iface fails but adding the bridge iface
      succeeds then we end up with the datapath and bridge not being
      connected, and hence presumably mysterious overlay connection
      failures
  • configure_veth_attached_bridge (via create_veth)
    • if adding the interface fails but the ip link set succeeds, then
      weave attach-bridge succeeds even though the bridge hasn't been
      attached.
  • create_veth (via with_container_netns ... setup_router_iface_...)
    • if setting the MTUs fails then this still succeeds, so routers started with WEAVE_NO_FASTDP=1 can end up exhibiting lower performance or connectivity issues

And then there are some places where the impact is quite light...

  • args_match (via start_if_necessary)
    • if the existing container gets removed just before this is
      invoked, we still end up comparing the inspect result. This is
      fine though, since it won't match.
  • with_container_netns (via with_container_netns_or_die)
    • if the container gets removed (not just stopped) just before this
      is invoked, this will print an error about the missing container
      but continue executing, usually resulting in some obscure
      follow-on error like "cannot open /hostproc//ns/net"
  • ipam_cidrs (via ipam_cidrs_or_die)
  • http_call_unix (via proxy_addrs)
    • if the docker exec ... curl ... fails but also outputs something on stdout, then proxy_addrs will return that. This really shouldn't happen.

The last three are also present in 1.4.x.

Strategic placement of || return 1 or || exit 1 will cure these.

There are another ~20 functions which are suffering from the same problem but which are currently safe because they are not invoked in a context that disables set -e.

@rade rade added the bug label Mar 16, 2016
@rade rade added this to the 1.5.0 milestone Mar 16, 2016
@bboreham
Copy link
Contributor

@bboreham bboreham self-assigned this Mar 17, 2016
@bboreham
Copy link
Contributor

  • create_veth (via with_container_netns ... setup_router_iface_...)

This line is only in attach-router, which is only called when the proxy sees the router restart. The with_container_netns is unnecessary after #1930.

if setting the MTUs fails then this still succeeds

In practice it doesn't, since the next line ip link set $VETHL up will fail since the link doesn't exist.

To repro:

# weave reset
# WEAVE_NO_FASTDP=1 weave launch-router
# ip link del weave
# ip link del vethwe-bridge
# WEAVE_NO_FASTDP=1 WEAVE_MTU=99999 weave attach-router

Failure during network configuration for container weave:
RTNETLINK answers: Invalid argument
Cannot find device "vethwe-bridge"

rade added a commit that referenced this issue Mar 17, 2016
Fix missed errors in weave script

Fixes #2053.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

No branches or pull requests

2 participants