From 0d7046e7d73bb3a4b34d9041c1ee9f55cb3e639a Mon Sep 17 00:00:00 2001 From: networkop Date: Mon, 10 May 2021 11:26:48 +0100 Subject: [PATCH 01/25] First crack at multinet implementation --- cmd/ignite-spawn/ignite-spawn.go | 12 +-- pkg/container/firecracker.go | 10 +-- pkg/container/network.go | 149 +++++++++++++++++++++++++++++-- 3 files changed, 152 insertions(+), 19 deletions(-) diff --git a/cmd/ignite-spawn/ignite-spawn.go b/cmd/ignite-spawn/ignite-spawn.go index 9702d0076..029ff7792 100644 --- a/cmd/ignite-spawn/ignite-spawn.go +++ b/cmd/ignite-spawn/ignite-spawn.go @@ -39,8 +39,12 @@ func decodeVM(vmID string) (*api.VM, error) { } func StartVM(vm *api.VM) (err error) { + // Serve metrics over an unix socket in the VM's own directory + metricsSocket := path.Join(vm.ObjectPath(), constants.PROMETHEUS_SOCKET) + serveMetrics(metricsSocket) + // Setup networking inside of the container, return the available interfaces - dhcpIfaces, err := container.SetupContainerNetworking() + fcIfaces, dhcpIfaces, err := container.SetupContainerNetworking() if err != nil { return fmt.Errorf("network setup failed: %v", err) } @@ -52,10 +56,6 @@ func StartVM(vm *api.VM) (err error) { return } - // Serve metrics over an unix socket in the VM's own directory - metricsSocket := path.Join(vm.ObjectPath(), constants.PROMETHEUS_SOCKET) - serveMetrics(metricsSocket) - // Patches the VM object to set state to stopped, and clear IP addresses defer util.DeferErr(&err, func() error { return patchStopped(vm) }) @@ -66,7 +66,7 @@ func StartVM(vm *api.VM) (err error) { defer util.DeferErr(&err, func() error { return os.Remove(metricsSocket) }) // Execute Firecracker - if err = container.ExecuteFirecracker(vm, dhcpIfaces); err != nil { + if err = container.ExecuteFirecracker(vm, fcIfaces); err != nil { return fmt.Errorf("runtime error for VM %q: %v", vm.GetUID(), err) } diff --git a/pkg/container/firecracker.go b/pkg/container/firecracker.go index 4f61ecd19..9893c021e 100644 --- a/pkg/container/firecracker.go +++ b/pkg/container/firecracker.go @@ -20,15 +20,15 @@ import ( ) // ExecuteFirecracker executes the firecracker process using the Go SDK -func ExecuteFirecracker(vm *api.VM, dhcpIfaces []DHCPInterface) (err error) { +func ExecuteFirecracker(vm *api.VM, fcIfaces []FCInterface) (err error) { drivePath := vm.SnapshotDev() - networkInterfaces := make([]firecracker.NetworkInterface, 0, len(dhcpIfaces)) - for _, dhcpIface := range dhcpIfaces { + networkInterfaces := make([]firecracker.NetworkInterface, 0, len(fcIfaces)) + for _, intf := range fcIfaces { networkInterfaces = append(networkInterfaces, firecracker.NetworkInterface{ StaticConfiguration: &firecracker.StaticNetworkConfiguration{ - MacAddress: dhcpIface.MACFilter, - HostDevName: dhcpIface.VMTAP, + MacAddress: intf.MACFilter, + HostDevName: intf.VMTAP, }, }) } diff --git a/pkg/container/network.go b/pkg/container/network.go index 52f9ac07a..c495a3cda 100644 --- a/pkg/container/network.go +++ b/pkg/container/network.go @@ -3,6 +3,8 @@ package container import ( "fmt" "net" + "os" + "syscall" "time" log "github.com/sirupsen/logrus" @@ -31,14 +33,25 @@ var ignoreInterfaces = map[string]struct{}{ "lo": {}, } -func SetupContainerNetworking() ([]DHCPInterface, error) { +type FCInterface struct { + VMTAP string + MACFilter string +} + +func SetupContainerNetworking() ([]FCInterface, []DHCPInterface, error) { var dhcpIfaces []DHCPInterface + var allIfaces []FCInterface + interval := 1 * time.Second timeout := 1 * time.Minute + // time.sleep to wait for some interface to be connected + log.Printf("Waiting for interfaces to be connected") + time.Sleep(time.Second * 10) + err := wait.PollImmediate(interval, timeout, func() (bool, error) { // This func returns true if it's done, and optionally an error - retry, err := networkSetup(&dhcpIfaces) + retry, err := networkSetup(&allIfaces, &dhcpIfaces) if err == nil { // We're done here return true, nil @@ -53,13 +66,28 @@ func SetupContainerNetworking() ([]DHCPInterface, error) { }) if err != nil { - return nil, err + return nil, nil, err } - return dhcpIfaces, nil + return allIfaces, dhcpIfaces, nil } -func networkSetup(dhcpIfaces *[]DHCPInterface) (bool, error) { +func isIgnored(link net.Interface) bool { + if _, ok := ignoreInterfaces[link.Name]; ok { + return true + } + + eth, err := netlink.LinkByIndex(link.Index) + if err != nil { + return true + } + + _, ok := eth.(*netlink.Veth) + + return !ok +} + +func networkSetup(allIfaces *[]FCInterface, dhcpIfaces *[]DHCPInterface) (bool, error) { ifaces, err := net.Interfaces() if err != nil || ifaces == nil || len(ifaces) == 0 { return true, fmt.Errorf("cannot get local network interfaces: %v", err) @@ -69,12 +97,22 @@ func networkSetup(dhcpIfaces *[]DHCPInterface) (bool, error) { interfacesCount := 0 for _, iface := range ifaces { // Skip the interface if it's ignored - if _, ok := ignoreInterfaces[iface.Name]; ok { + if isIgnored(iface) { continue } // Try to transfer the address from the container to the DHCP server - ipNet, gw, _, err := takeAddress(&iface) + ipNet, gw, noIPs, err := takeAddress(&iface) + // If interface has no IPs configured, setup tc redirect + if noIPs { + tcInterface, err := addTcRedirect(&iface) + if err != nil { + log.Errorf("Failed to setup tc redirect %v", err) + continue + } + *allIfaces = append(*allIfaces, *tcInterface) + continue + } if err != nil { // Log the problem, but don't quit the function here as there might be other good interfaces log.Errorf("Parsing interface %q failed: %v", iface.Name, err) @@ -98,6 +136,11 @@ func networkSetup(dhcpIfaces *[]DHCPInterface) (bool, error) { *dhcpIfaces = append(*dhcpIfaces, *dhcpIface) + *allIfaces = append(*allIfaces, FCInterface{ + VMTAP: dhcpIface.VMTAP, + MACFilter: dhcpIface.MACFilter, + }) + // This is an interface we care about interfacesCount++ } @@ -110,6 +153,96 @@ func networkSetup(dhcpIfaces *[]DHCPInterface) (bool, error) { return false, nil } +// addTcRedirect sets up tc redirect betweeb veth and tap https://github.com/awslabs/tc-redirect-tap/blob/master/internal/netlink.go +func addTcRedirect(iface *net.Interface) (*FCInterface, error) { + + eth, err := netlink.LinkByIndex(iface.Index) + if err != nil { + return nil, err + } + + tapName := constants.TAP_PREFIX + iface.Name + tuntap, err := createTAPAdapter(tapName) + if err != nil { + return nil, err + } + + log.Printf("Adding qdisc to veth %s", eth.Attrs().Name) + err = addIngressQdisc(eth) + if err != nil { + return nil, err + } + + log.Printf("Adding qdisc to veth %s", tuntap.Attrs().Name) + err = addIngressQdisc(tuntap) + if err != nil { + return nil, err + } + + log.Printf("Adding tc filter to veth %s", eth.Attrs().Name) + err = addRedirectFilter(eth, tuntap) + if err != nil { + return nil, err + } + + log.Printf("Adding tc filter to tuntap %s", tuntap.Attrs().Name) + err = addRedirectFilter(tuntap, eth) + if err != nil { + return nil, err + } + + return &FCInterface{ + VMTAP: tapName, + MACFilter: eth.Attrs().HardwareAddr.String(), + }, nil +} + +func addIngressQdisc(link netlink.Link) error { + qdisc := &netlink.Ingress{ + QdiscAttrs: netlink.QdiscAttrs{ + LinkIndex: link.Attrs().Index, + //Handle: netlink.MakeHandle(0xffff, 0), + Parent: netlink.HANDLE_INGRESS, + }, + } + + if err := netlink.QdiscAdd(qdisc); err != nil { + if !os.IsExist(err) { + return err + } + } + return nil +} + +func addRedirectFilter(linkSrc, linkDest netlink.Link) error { + filter := &netlink.U32{ + FilterAttrs: netlink.FilterAttrs{ + LinkIndex: linkSrc.Attrs().Index, + Parent: netlink.MakeHandle(0xffff, 0), + Protocol: syscall.ETH_P_ALL, + }, + //Sel: &netlink.TcU32Sel{ + // Keys: []netlink.TcU32Key{ + // { + // Mask: 0x0, + // Val: 0, + // }, + // }, + // Flags: netlink.TC_U32_TERMINAL, + //}, + Actions: []netlink.Action{ + &netlink.MirredAction{ + ActionAttrs: netlink.ActionAttrs{ + Action: netlink.TC_ACT_STOLEN, + }, + MirredAction: netlink.TCA_EGRESS_MIRROR, + Ifindex: linkDest.Attrs().Index, + }, + }, + } + return netlink.FilterAdd(filter) +} + // bridge creates the TAP device and performs the bridging, returning the base configuration for a DHCP server func bridge(iface *net.Interface) (*DHCPInterface, error) { tapName := constants.TAP_PREFIX + iface.Name @@ -207,7 +340,7 @@ func takeAddress(iface *net.Interface) (*net.IPNet, *net.IP, bool, error) { }, gw, false, nil } - return nil, nil, false, fmt.Errorf("interface %s has no valid addresses", iface.Name) + return nil, nil, true, fmt.Errorf("interface %s has no valid addresses", iface.Name) } // createTAPAdapter creates a new TAP device with the given name From 0b3dfdb3eddde7986f6fb091a81ceb0eb3153855 Mon Sep 17 00:00:00 2001 From: networkop Date: Mon, 10 May 2021 12:13:51 +0100 Subject: [PATCH 02/25] fix the dhcp server bug --- pkg/container/dhcp.go | 9 --------- pkg/container/network.go | 12 ++++++++++-- 2 files changed, 10 insertions(+), 11 deletions(-) diff --git a/pkg/container/dhcp.go b/pkg/container/dhcp.go index 5c011271e..58675edd3 100644 --- a/pkg/container/dhcp.go +++ b/pkg/container/dhcp.go @@ -11,7 +11,6 @@ import ( log "github.com/sirupsen/logrus" api "github.com/weaveworks/ignite/pkg/apis/ignite" "github.com/weaveworks/ignite/pkg/constants" - "github.com/weaveworks/ignite/pkg/util" ) var leaseDuration, _ = time.ParseDuration(constants.DHCP_INFINITE_LEASE) // Infinite lease time @@ -19,11 +18,6 @@ var leaseDuration, _ = time.ParseDuration(constants.DHCP_INFINITE_LEASE) // Infi // StartDHCPServers starts multiple DHCP servers for the VM, one per interface // It returns the IP addresses that the API object may post in .status, and a potential error func StartDHCPServers(vm *api.VM, dhcpIfaces []DHCPInterface) error { - // Generate the MAC addresses for the VM's adapters - macAddresses := make([]string, 0, len(dhcpIfaces)) - if err := util.NewMAC(&macAddresses); err != nil { - return fmt.Errorf("failed to generate MAC addresses: %v", err) - } // Fetch the DNS servers given to the container clientConfig, err := dns.ClientConfigFromFile("/etc/resolv.conf") @@ -36,9 +30,6 @@ func StartDHCPServers(vm *api.VM, dhcpIfaces []DHCPInterface) error { // Set the VM hostname to the VM ID dhcpIface.Hostname = vm.GetUID().String() - // Set the MAC address filter for the DHCP server - dhcpIface.MACFilter = macAddresses[i] - // Add the DNS servers from the container dhcpIface.SetDNSServers(clientConfig.Servers) diff --git a/pkg/container/network.go b/pkg/container/network.go index c495a3cda..2b92ef527 100644 --- a/pkg/container/network.go +++ b/pkg/container/network.go @@ -10,6 +10,7 @@ import ( log "github.com/sirupsen/logrus" "github.com/vishvananda/netlink" "github.com/weaveworks/ignite/pkg/constants" + "github.com/weaveworks/ignite/pkg/util" "k8s.io/apimachinery/pkg/util/wait" ) @@ -267,9 +268,16 @@ func bridge(iface *net.Interface) (*DHCPInterface, error) { return nil, err } + // Generate the MAC addresses for the VM's adapters + macAddress := make([]string, 0, 1) + if err := util.NewMAC(&macAddress); err != nil { + return nil, fmt.Errorf("failed to generate MAC addresses: %v", err) + } + return &DHCPInterface{ - VMTAP: tapName, - Bridge: bridgeName, + VMTAP: tapName, + Bridge: bridgeName, + MACFilter: macAddress[0], }, nil } From 56ca54692a874bb26bb176a69f3a5b53a3e3b8a3 Mon Sep 17 00:00:00 2001 From: networkop Date: Mon, 10 May 2021 22:16:00 +0100 Subject: [PATCH 03/25] Added wait for X number of interfaces to be connected --- cmd/ignite-spawn/ignite-spawn.go | 20 ++++++++++- pkg/container/network.go | 59 +++++++++++++++++--------------- 2 files changed, 51 insertions(+), 28 deletions(-) diff --git a/cmd/ignite-spawn/ignite-spawn.go b/cmd/ignite-spawn/ignite-spawn.go index 029ff7792..852f24ba0 100644 --- a/cmd/ignite-spawn/ignite-spawn.go +++ b/cmd/ignite-spawn/ignite-spawn.go @@ -4,6 +4,7 @@ import ( "fmt" "os" "path" + "strings" log "github.com/sirupsen/logrus" api "github.com/weaveworks/ignite/pkg/apis/ignite" @@ -43,8 +44,10 @@ func StartVM(vm *api.VM) (err error) { metricsSocket := path.Join(vm.ObjectPath(), constants.PROMETHEUS_SOCKET) serveMetrics(metricsSocket) + envVars := parseEnvVars(os.Environ()) + // Setup networking inside of the container, return the available interfaces - fcIfaces, dhcpIfaces, err := container.SetupContainerNetworking() + fcIfaces, dhcpIfaces, err := container.SetupContainerNetworking(envVars) if err != nil { return fmt.Errorf("network setup failed: %v", err) } @@ -96,3 +99,18 @@ func patchStopped(vm *api.VM) error { patch := []byte(`{"status":{"running":false,"network":null,"runtime":null,"startTime":null}}`) return patchutil.NewPatcher(scheme.Serializer).ApplyOnFile(constants.IGNITE_SPAWN_VM_FILE_PATH, patch, vm.GroupVersionKind()) } + +func parseEnvVars(vars []string) map[string]string { + result := make(map[string]string) + + for _, arg := range vars { + parts := strings.Split(arg, "=") + if len(parts) != 2 { + continue + } + + result[parts[0]] = parts[1] + } + + return result +} diff --git a/pkg/container/network.go b/pkg/container/network.go index 2b92ef527..2d51fa6fd 100644 --- a/pkg/container/network.go +++ b/pkg/container/network.go @@ -4,6 +4,7 @@ import ( "fmt" "net" "os" + "strconv" "syscall" "time" @@ -34,25 +35,30 @@ var ignoreInterfaces = map[string]struct{}{ "lo": {}, } +var ( + maxIntfsVar = "IGNITE_INTFS" + defaultMaxIntfs = 1 +) + type FCInterface struct { VMTAP string MACFilter string } -func SetupContainerNetworking() ([]FCInterface, []DHCPInterface, error) { +func SetupContainerNetworking(args map[string]string) ([]FCInterface, []DHCPInterface, error) { var dhcpIfaces []DHCPInterface var allIfaces []FCInterface interval := 1 * time.Second timeout := 1 * time.Minute + maxIntfs, err := strconv.Atoi(args[maxIntfsVar]) + if err != nil { + maxIntfs = defaultMaxIntfs + } - // time.sleep to wait for some interface to be connected - log.Printf("Waiting for interfaces to be connected") - time.Sleep(time.Second * 10) - - err := wait.PollImmediate(interval, timeout, func() (bool, error) { + err = wait.PollImmediate(interval, timeout, func() (bool, error) { // This func returns true if it's done, and optionally an error - retry, err := networkSetup(&allIfaces, &dhcpIfaces) + retry, err := networkSetup(&allIfaces, &dhcpIfaces, maxIntfs) if err == nil { // We're done here return true, nil @@ -73,11 +79,13 @@ func SetupContainerNetworking() ([]FCInterface, []DHCPInterface, error) { return allIfaces, dhcpIfaces, nil } -func isIgnored(link net.Interface) bool { +func isIgnored(link net.Interface, allIfaces *[]FCInterface) bool { + // ignore if in explicit ignore list if _, ok := ignoreInterfaces[link.Name]; ok { return true } + // ignore if _not_ a veth eth, err := netlink.LinkByIndex(link.Index) if err != nil { return true @@ -88,7 +96,7 @@ func isIgnored(link net.Interface) bool { return !ok } -func networkSetup(allIfaces *[]FCInterface, dhcpIfaces *[]DHCPInterface) (bool, error) { +func networkSetup(allIfaces *[]FCInterface, dhcpIfaces *[]DHCPInterface, intfNum int) (bool, error) { ifaces, err := net.Interfaces() if err != nil || ifaces == nil || len(ifaces) == 0 { return true, fmt.Errorf("cannot get local network interfaces: %v", err) @@ -98,12 +106,13 @@ func networkSetup(allIfaces *[]FCInterface, dhcpIfaces *[]DHCPInterface) (bool, interfacesCount := 0 for _, iface := range ifaces { // Skip the interface if it's ignored - if isIgnored(iface) { + if isIgnored(iface, allIfaces) { continue } // Try to transfer the address from the container to the DHCP server ipNet, gw, noIPs, err := takeAddress(&iface) + // If interface has no IPs configured, setup tc redirect if noIPs { tcInterface, err := addTcRedirect(&iface) @@ -111,7 +120,10 @@ func networkSetup(allIfaces *[]FCInterface, dhcpIfaces *[]DHCPInterface) (bool, log.Errorf("Failed to setup tc redirect %v", err) continue } + *allIfaces = append(*allIfaces, *tcInterface) + ignoreInterfaces[iface.Name] = struct{}{} + continue } if err != nil { @@ -141,20 +153,22 @@ func networkSetup(allIfaces *[]FCInterface, dhcpIfaces *[]DHCPInterface) (bool, VMTAP: dhcpIface.VMTAP, MACFilter: dhcpIface.MACFilter, }) + ignoreInterfaces[iface.Name] = struct{}{} // This is an interface we care about interfacesCount++ } // If there weren't any interfaces that were valid or active yet, retry the loop - if interfacesCount == 0 { - return true, fmt.Errorf("no active or valid interfaces available yet") + if interfacesCount < intfNum { + return true, fmt.Errorf("not enough active or valid interfaces available yet") } return false, nil } // addTcRedirect sets up tc redirect betweeb veth and tap https://github.com/awslabs/tc-redirect-tap/blob/master/internal/netlink.go +// on WSL2 this requires `CONFIG_NET_CLS_U32=y` func addTcRedirect(iface *net.Interface) (*FCInterface, error) { eth, err := netlink.LinkByIndex(iface.Index) @@ -168,19 +182,15 @@ func addTcRedirect(iface *net.Interface) (*FCInterface, error) { return nil, err } - log.Printf("Adding qdisc to veth %s", eth.Attrs().Name) err = addIngressQdisc(eth) if err != nil { return nil, err } - - log.Printf("Adding qdisc to veth %s", tuntap.Attrs().Name) err = addIngressQdisc(tuntap) if err != nil { return nil, err } - log.Printf("Adding tc filter to veth %s", eth.Attrs().Name) err = addRedirectFilter(eth, tuntap) if err != nil { return nil, err @@ -198,12 +208,12 @@ func addTcRedirect(iface *net.Interface) (*FCInterface, error) { }, nil } +// tc qdisc add dev $SRC_IFACE ingress func addIngressQdisc(link netlink.Link) error { qdisc := &netlink.Ingress{ QdiscAttrs: netlink.QdiscAttrs{ LinkIndex: link.Attrs().Index, - //Handle: netlink.MakeHandle(0xffff, 0), - Parent: netlink.HANDLE_INGRESS, + Parent: netlink.HANDLE_INGRESS, }, } @@ -215,6 +225,10 @@ func addIngressQdisc(link netlink.Link) error { return nil } +// tc filter add dev $SRC_IFACE parent ffff: +// protocol all +// u32 match u32 0 0 +// action mirred egress mirror dev $DST_IFACE func addRedirectFilter(linkSrc, linkDest netlink.Link) error { filter := &netlink.U32{ FilterAttrs: netlink.FilterAttrs{ @@ -222,15 +236,6 @@ func addRedirectFilter(linkSrc, linkDest netlink.Link) error { Parent: netlink.MakeHandle(0xffff, 0), Protocol: syscall.ETH_P_ALL, }, - //Sel: &netlink.TcU32Sel{ - // Keys: []netlink.TcU32Key{ - // { - // Mask: 0x0, - // Val: 0, - // }, - // }, - // Flags: netlink.TC_U32_TERMINAL, - //}, Actions: []netlink.Action{ &netlink.MirredAction{ ActionAttrs: netlink.ActionAttrs{ From 17765be0ee7f55f2b40a5ea29cf8b5e5f49e27f5 Mon Sep 17 00:00:00 2001 From: networkop Date: Tue, 11 May 2021 14:04:37 +0100 Subject: [PATCH 04/25] Fixed multiple bugs --- cmd/ignite/cmd/cmdutil/flags.go | 4 ++++ cmd/ignite/cmd/vmcmd/create.go | 1 + cmd/ignite/run/create.go | 5 +++++ cmd/ignite/run/start.go | 2 +- pkg/constants/vm.go | 3 +++ pkg/container/network.go | 13 +++++++++---- pkg/network/cni/cni.go | 1 - pkg/operations/reconcile/reconcile.go | 2 +- pkg/operations/remove.go | 2 +- pkg/operations/start.go | 19 ++++++++++++++++--- pkg/runtime/containerd/client.go | 1 + pkg/runtime/docker/client.go | 1 + pkg/runtime/types.go | 1 + 13 files changed, 44 insertions(+), 11 deletions(-) diff --git a/cmd/ignite/cmd/cmdutil/flags.go b/cmd/ignite/cmd/cmdutil/flags.go index d31712cc8..9ef17066c 100644 --- a/cmd/ignite/cmd/cmdutil/flags.go +++ b/cmd/ignite/cmd/cmdutil/flags.go @@ -35,3 +35,7 @@ func AddSSHFlags(fs *pflag.FlagSet, identityFile *string, timeout *uint32) { fs.StringVarP(identityFile, "identity", "i", "", "Override the vm's default identity file") fs.Uint32Var(timeout, "timeout", constants.SSH_DEFAULT_TIMEOUT_SECONDS, "Timeout waiting for connection in seconds") } + +func AddEnvVarsFlag(fs *pflag.FlagSet, vars *string) { + fs.StringVar(vars, "sandbox-env-vars", *vars, "A list of comma-separated key=value pairs to pass as sandbox env vars") +} diff --git a/cmd/ignite/cmd/vmcmd/create.go b/cmd/ignite/cmd/vmcmd/create.go index 18146424f..566a646e7 100644 --- a/cmd/ignite/cmd/vmcmd/create.go +++ b/cmd/ignite/cmd/vmcmd/create.go @@ -66,6 +66,7 @@ func addCreateFlags(fs *pflag.FlagSet, cf *run.CreateFlags) { // Register common flags cmdutil.AddNameFlag(fs, &cf.VM.ObjectMeta.Name) cmdutil.AddConfigFlag(fs, &cf.ConfigFile) + cmdutil.AddEnvVarsFlag(fs, &cf.EnvVars) // Register flags bound to temporary holder values fs.StringSliceVarP(&cf.PortMappings, "ports", "p", cf.PortMappings, "Map host ports to VM ports") diff --git a/cmd/ignite/run/create.go b/cmd/ignite/run/create.go index 9e450ed99..94491bc74 100644 --- a/cmd/ignite/run/create.go +++ b/cmd/ignite/run/create.go @@ -11,6 +11,7 @@ import ( "github.com/weaveworks/ignite/pkg/apis/ignite/validation" meta "github.com/weaveworks/ignite/pkg/apis/meta/v1alpha1" "github.com/weaveworks/ignite/pkg/config" + "github.com/weaveworks/ignite/pkg/constants" "github.com/weaveworks/ignite/pkg/dmlegacy" "github.com/weaveworks/ignite/pkg/metadata" "github.com/weaveworks/ignite/pkg/operations" @@ -39,6 +40,7 @@ type CreateFlags struct { ConfigFile string VM *api.VM Labels []string + EnvVars string RequireName bool } @@ -95,6 +97,9 @@ func (cf *CreateFlags) NewCreateOptions(args []string, fs *flag.FlagSet) (*Creat return nil, err } + // Save env variables + baseVM.SetAnnotation(constants.IGNITE_ENV_VAR_ANNOTATION, cf.EnvVars) + // If --require-name is true, VM name must be provided. if cf.RequireName && len(baseVM.Name) == 0 { return nil, fmt.Errorf("must set VM name, flag --require-name set") diff --git a/cmd/ignite/run/start.go b/cmd/ignite/run/start.go index 4219e83fb..f06b2f9cc 100644 --- a/cmd/ignite/run/start.go +++ b/cmd/ignite/run/start.go @@ -78,7 +78,7 @@ func Start(so *StartOptions, fs *flag.FlagSet) error { return err } - if err := operations.StartVM(so.vm, so.Debug); err != nil { + if err := operations.StartVM(so.vm, so.Debug, true); err != nil { return err } diff --git a/pkg/constants/vm.go b/pkg/constants/vm.go index 068b4e79f..4807652a2 100644 --- a/pkg/constants/vm.go +++ b/pkg/constants/vm.go @@ -38,4 +38,7 @@ const ( // DEFAULT_SANDBOX_IMAGE_NAME is the name of the default sandbox container // image to be used. DEFAULT_SANDBOX_IMAGE_TAG = "dev" + + // Env variable annotation key + IGNITE_ENV_VAR_ANNOTATION = "ignite.env.vars" ) diff --git a/pkg/container/network.go b/pkg/container/network.go index 2d51fa6fd..67c270550 100644 --- a/pkg/container/network.go +++ b/pkg/container/network.go @@ -57,8 +57,12 @@ func SetupContainerNetworking(args map[string]string) ([]FCInterface, []DHCPInte } err = wait.PollImmediate(interval, timeout, func() (bool, error) { + + // Expected number of intfs is total maximum minus what we've found already + expectedIntfs := maxIntfs - len(allIfaces) + // This func returns true if it's done, and optionally an error - retry, err := networkSetup(&allIfaces, &dhcpIfaces, maxIntfs) + retry, err := networkSetup(&allIfaces, &dhcpIfaces, expectedIntfs) if err == nil { // We're done here return true, nil @@ -114,7 +118,8 @@ func networkSetup(allIfaces *[]FCInterface, dhcpIfaces *[]DHCPInterface, intfNum ipNet, gw, noIPs, err := takeAddress(&iface) // If interface has no IPs configured, setup tc redirect - if noIPs { + if noIPs && iface.Name != "eth0" { + log.Print("Interface %s has no IP, setting up tc redirect", iface.Name) tcInterface, err := addTcRedirect(&iface) if err != nil { log.Errorf("Failed to setup tc redirect %v", err) @@ -124,6 +129,7 @@ func networkSetup(allIfaces *[]FCInterface, dhcpIfaces *[]DHCPInterface, intfNum *allIfaces = append(*allIfaces, *tcInterface) ignoreInterfaces[iface.Name] = struct{}{} + interfacesCount++ continue } if err != nil { @@ -132,7 +138,7 @@ func networkSetup(allIfaces *[]FCInterface, dhcpIfaces *[]DHCPInterface, intfNum // Try with the next interface continue } - + log.Print("IP detected, stealing ...") // Bridge the Firecracker TAP interface with the container veth interface dhcpIface, err := bridge(&iface) if err != nil { @@ -196,7 +202,6 @@ func addTcRedirect(iface *net.Interface) (*FCInterface, error) { return nil, err } - log.Printf("Adding tc filter to tuntap %s", tuntap.Attrs().Name) err = addRedirectFilter(tuntap, eth) if err != nil { return nil, err diff --git a/pkg/network/cni/cni.go b/pkg/network/cni/cni.go index 33e42a6c9..98d48c182 100644 --- a/pkg/network/cni/cni.go +++ b/pkg/network/cni/cni.go @@ -299,7 +299,6 @@ func getIPChains(containerID string) (result []*ipChain, err error) { if err != nil { return } - result = append(result, &ipChain{ ip: stat.Source, chain: stat.Target, diff --git a/pkg/operations/reconcile/reconcile.go b/pkg/operations/reconcile/reconcile.go index 57601bcae..7d70f4428 100644 --- a/pkg/operations/reconcile/reconcile.go +++ b/pkg/operations/reconcile/reconcile.go @@ -149,7 +149,7 @@ func start(vm *api.VM) error { log.Infof("Starting VM %q with name %q...", vm.GetUID(), vm.GetName()) vmStarted.Inc() - return operations.StartVM(vm, true) + return operations.StartVM(vm, true, true) } func stop(vm *api.VM) error { diff --git a/pkg/operations/remove.go b/pkg/operations/remove.go index 8c9ed5700..8aaf27d85 100644 --- a/pkg/operations/remove.go +++ b/pkg/operations/remove.go @@ -86,7 +86,7 @@ func StopVM(vm *api.VM, kill, silent bool) error { } // Remove VM networking - if err = removeNetworking(vm.PrefixedID(), vm.Spec.Network.Ports...); err != nil { + if err = removeNetworking(vm.Status.Runtime.ID, vm.Spec.Network.Ports...); err != nil { log.Warnf("Failed to cleanup networking for stopped container %s %q: %v", vm.GetKind(), vm.GetUID(), err) return err diff --git a/pkg/operations/start.go b/pkg/operations/start.go index 6a3755d98..814b774b9 100644 --- a/pkg/operations/start.go +++ b/pkg/operations/start.go @@ -4,6 +4,7 @@ import ( "fmt" "path" "path/filepath" + "strings" "time" log "github.com/sirupsen/logrus" @@ -19,7 +20,7 @@ import ( apiruntime "github.com/weaveworks/libgitops/pkg/runtime" ) -func StartVM(vm *api.VM, debug bool) error { +func StartVM(vm *api.VM, debug, wait bool) error { // Inspect the VM container and remove it if it exists inspectResult, _ := providers.Runtime.InspectContainer(vm.PrefixedID()) RemoveVMContainer(inspectResult) @@ -80,6 +81,16 @@ func StartVM(vm *api.VM, debug bool) error { PortBindings: vm.Spec.Network.Ports, // Add the port mappings to Docker } + // Only set envVars if at least one was configured + var envVars []string + parts := strings.Split(vm.GetAnnotation((constants.IGNITE_ENV_VAR_ANNOTATION)), ",") + for _, part := range parts { + if part == "" { + continue + } + envVars = append(envVars, part) + } + // Add the volumes to the container devices for _, volume := range vm.Spec.Storage.Volumes { if volume.BlockDevice == nil { @@ -122,8 +133,10 @@ func StartVM(vm *api.VM, debug bool) error { // TODO: Follow-up the container here with a defer, or dedicated goroutine. We should output // if it started successfully or not // TODO: This is temporary until we have proper communication to the container - if err := waitForSpawn(vm); err != nil { - return err + if wait { + if err := waitForSpawn(vm); err != nil { + return err + } } // Set the container ID for the VM diff --git a/pkg/runtime/containerd/client.go b/pkg/runtime/containerd/client.go index e573ac778..d369f0d0d 100644 --- a/pkg/runtime/containerd/client.go +++ b/pkg/runtime/containerd/client.go @@ -422,6 +422,7 @@ func (cc *ctdClient) RunContainer(image meta.OCIImageRef, config *runtime.Contai oci.WithDefaultUnixDevices, oci.WithTTY, oci.WithImageConfigArgs(img, config.Cmd), + oci.WithEnv(config.EnvVars), withAddedCaps(config.CapAdds), withHostname(config.Hostname), withMounts(config.Binds), diff --git a/pkg/runtime/docker/client.go b/pkg/runtime/docker/client.go index 05f530211..78f12933e 100644 --- a/pkg/runtime/docker/client.go +++ b/pkg/runtime/docker/client.go @@ -158,6 +158,7 @@ func (dc *dockerClient) RunContainer(image meta.OCIImageRef, config *runtime.Con Cmd: config.Cmd, Image: image.Normalized(), Labels: config.Labels, + Env: config.EnvVars, StopTimeout: &stopTimeout, }, &container.HostConfig{ Binds: binds, diff --git a/pkg/runtime/types.go b/pkg/runtime/types.go index 10737a3aa..d1240f136 100644 --- a/pkg/runtime/types.go +++ b/pkg/runtime/types.go @@ -40,6 +40,7 @@ type ContainerConfig struct { Cmd []string Hostname string Labels map[string]string + EnvVars []string Binds []*Bind CapAdds []string Devices []*Bind From 6e19436a48365135f1c7842bb9260f33e05b0a98 Mon Sep 17 00:00:00 2001 From: networkop Date: Tue, 11 May 2021 14:18:55 +0100 Subject: [PATCH 05/25] Ensure maxIntfs is at least 1 --- pkg/container/network.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/container/network.go b/pkg/container/network.go index 67c270550..8db2cb286 100644 --- a/pkg/container/network.go +++ b/pkg/container/network.go @@ -52,7 +52,7 @@ func SetupContainerNetworking(args map[string]string) ([]FCInterface, []DHCPInte interval := 1 * time.Second timeout := 1 * time.Minute maxIntfs, err := strconv.Atoi(args[maxIntfsVar]) - if err != nil { + if err != nil || maxIntfs < 1 { maxIntfs = defaultMaxIntfs } From 39d395e3b28af166fc3d1ce5739f0622071e879b Mon Sep 17 00:00:00 2001 From: networkop Date: Tue, 11 May 2021 14:28:24 +0100 Subject: [PATCH 06/25] Fixed formatting --- pkg/container/network.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/container/network.go b/pkg/container/network.go index 8db2cb286..3e634a140 100644 --- a/pkg/container/network.go +++ b/pkg/container/network.go @@ -119,7 +119,7 @@ func networkSetup(allIfaces *[]FCInterface, dhcpIfaces *[]DHCPInterface, intfNum // If interface has no IPs configured, setup tc redirect if noIPs && iface.Name != "eth0" { - log.Print("Interface %s has no IP, setting up tc redirect", iface.Name) + log.Printf("Interface %s has no IP, setting up tc redirect", iface.Name) tcInterface, err := addTcRedirect(&iface) if err != nil { log.Errorf("Failed to setup tc redirect %v", err) From 9a31fb926519fa9f34072ddcfb5d92e8f4287357 Mon Sep 17 00:00:00 2001 From: networkop Date: Tue, 11 May 2021 14:32:49 +0100 Subject: [PATCH 07/25] Forgot to assign envVars --- pkg/operations/start.go | 1 + 1 file changed, 1 insertion(+) diff --git a/pkg/operations/start.go b/pkg/operations/start.go index 814b774b9..89e76dbf9 100644 --- a/pkg/operations/start.go +++ b/pkg/operations/start.go @@ -90,6 +90,7 @@ func StartVM(vm *api.VM, debug, wait bool) error { } envVars = append(envVars, part) } + config.EnvVars = envVars // Add the volumes to the container devices for _, volume := range vm.Spec.Storage.Volumes { From 20b4aa8b5c140033d961bc3041c65a94685d44d5 Mon Sep 17 00:00:00 2001 From: networkop Date: Tue, 11 May 2021 14:39:49 +0100 Subject: [PATCH 08/25] Updated docs --- docs/cli/ignite/ignite_create.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/cli/ignite/ignite_create.md b/docs/cli/ignite/ignite_create.md index 9e5f75d2a..3c782decb 100644 --- a/docs/cli/ignite/ignite_create.md +++ b/docs/cli/ignite/ignite_create.md @@ -48,6 +48,7 @@ ignite create [flags] -p, --ports strings Map host ports to VM ports --require-name Require VM name to be passed, no name generation --runtime runtime Container runtime to use. Available options are: [docker containerd] (default containerd) + --sandbox-env-vars string A list of comma-separated key=value pairs to pass as sandbox env vars --sandbox-image oci-image Specify an OCI image for the VM sandbox (default weaveworks/ignite:dev) -s, --size size VM filesystem size, for example 5GB or 2048MB (default 4.0 GB) --ssh[=] Enable SSH for the VM. If is given, it will be imported as the public key. If just '--ssh' is specified, a new keypair will be generated. (default is unset, which disables SSH access to the VM) From 35dc891881971a03c111cf1290aa80940a424fe4 Mon Sep 17 00:00:00 2001 From: networkop Date: Tue, 11 May 2021 14:44:30 +0100 Subject: [PATCH 09/25] More updated docs --- docs/cli/ignite/ignite_vm_run.md | 1 + 1 file changed, 1 insertion(+) diff --git a/docs/cli/ignite/ignite_vm_run.md b/docs/cli/ignite/ignite_vm_run.md index a9bdc0d7e..de16acf80 100644 --- a/docs/cli/ignite/ignite_vm_run.md +++ b/docs/cli/ignite/ignite_vm_run.md @@ -44,6 +44,7 @@ ignite vm run [flags] -p, --ports strings Map host ports to VM ports --require-name Require VM name to be passed, no name generation --runtime runtime Container runtime to use. Available options are: [docker containerd] (default containerd) + --sandbox-env-vars string A list of comma-separated key=value pairs to pass as sandbox env vars --sandbox-image oci-image Specify an OCI image for the VM sandbox (default weaveworks/ignite:dev) -s, --size size VM filesystem size, for example 5GB or 2048MB (default 4.0 GB) --ssh[=] Enable SSH for the VM. If is given, it will be imported as the public key. If just '--ssh' is specified, a new keypair will be generated. (default is unset, which disables SSH access to the VM) From e8bd47c24deb1d68f4b29a3228ba62ec8673a2bd Mon Sep 17 00:00:00 2001 From: networkop Date: Tue, 11 May 2021 14:50:13 +0100 Subject: [PATCH 10/25] More docs updates --- docs/cli/ignite/ignite_run.md | 1 + docs/cli/ignite/ignite_vm_create.md | 1 + docs/cli/ignite/ignite_vm_run.md | 2 +- 3 files changed, 3 insertions(+), 1 deletion(-) diff --git a/docs/cli/ignite/ignite_run.md b/docs/cli/ignite/ignite_run.md index fd3f8edff..2601a5034 100644 --- a/docs/cli/ignite/ignite_run.md +++ b/docs/cli/ignite/ignite_run.md @@ -44,6 +44,7 @@ ignite run [flags] -p, --ports strings Map host ports to VM ports --require-name Require VM name to be passed, no name generation --runtime runtime Container runtime to use. Available options are: [docker containerd] (default containerd) + --sandbox-env-vars string A list of comma-separated key=value pairs to pass as sandbox env vars --sandbox-image oci-image Specify an OCI image for the VM sandbox (default weaveworks/ignite:dev) -s, --size size VM filesystem size, for example 5GB or 2048MB (default 4.0 GB) --ssh[=] Enable SSH for the VM. If is given, it will be imported as the public key. If just '--ssh' is specified, a new keypair will be generated. (default is unset, which disables SSH access to the VM) diff --git a/docs/cli/ignite/ignite_vm_create.md b/docs/cli/ignite/ignite_vm_create.md index ff1359d77..49577936f 100644 --- a/docs/cli/ignite/ignite_vm_create.md +++ b/docs/cli/ignite/ignite_vm_create.md @@ -48,6 +48,7 @@ ignite vm create [flags] -p, --ports strings Map host ports to VM ports --require-name Require VM name to be passed, no name generation --runtime runtime Container runtime to use. Available options are: [docker containerd] (default containerd) + --sandbox-env-vars string A list of comma-separated key=value pairs to pass as sandbox env vars --sandbox-image oci-image Specify an OCI image for the VM sandbox (default weaveworks/ignite:dev) -s, --size size VM filesystem size, for example 5GB or 2048MB (default 4.0 GB) --ssh[=] Enable SSH for the VM. If is given, it will be imported as the public key. If just '--ssh' is specified, a new keypair will be generated. (default is unset, which disables SSH access to the VM) diff --git a/docs/cli/ignite/ignite_vm_run.md b/docs/cli/ignite/ignite_vm_run.md index de16acf80..b838fa666 100644 --- a/docs/cli/ignite/ignite_vm_run.md +++ b/docs/cli/ignite/ignite_vm_run.md @@ -44,7 +44,7 @@ ignite vm run [flags] -p, --ports strings Map host ports to VM ports --require-name Require VM name to be passed, no name generation --runtime runtime Container runtime to use. Available options are: [docker containerd] (default containerd) - --sandbox-env-vars string A list of comma-separated key=value pairs to pass as sandbox env vars + --sandbox-env-vars string A list of comma-separated key=value pairs to pass as sandbox env vars --sandbox-image oci-image Specify an OCI image for the VM sandbox (default weaveworks/ignite:dev) -s, --size size VM filesystem size, for example 5GB or 2048MB (default 4.0 GB) --ssh[=] Enable SSH for the VM. If is given, it will be imported as the public key. If just '--ssh' is specified, a new keypair will be generated. (default is unset, which disables SSH access to the VM) From b8ef5d38f6c8672f08e7b3aca82f2bf444818d70 Mon Sep 17 00:00:00 2001 From: networkop Date: Sat, 22 May 2021 14:15:16 +0100 Subject: [PATCH 11/25] Not using env vars anymore --- cmd/ignite-spawn/ignite-spawn.go | 20 +--- cmd/ignite/cmd/cmdutil/flags.go | 4 - cmd/ignite/cmd/vmcmd/create.go | 1 - cmd/ignite/run/create.go | 5 - docs/cli/ignite/ignite_create.md | 1 - docs/cli/ignite/ignite_run.md | 1 - docs/cli/ignite/ignite_vm_create.md | 1 - docs/cli/ignite/ignite_vm_run.md | 1 - pkg/constants/vm.go | 4 +- pkg/container/firecracker.go | 14 +-- pkg/container/network.go | 152 ++++++++++++++++------------ pkg/operations/start.go | 12 --- pkg/runtime/containerd/client.go | 1 - pkg/runtime/docker/client.go | 1 - pkg/runtime/types.go | 1 - 15 files changed, 95 insertions(+), 124 deletions(-) diff --git a/cmd/ignite-spawn/ignite-spawn.go b/cmd/ignite-spawn/ignite-spawn.go index 62ebf45c8..c9349066e 100644 --- a/cmd/ignite-spawn/ignite-spawn.go +++ b/cmd/ignite-spawn/ignite-spawn.go @@ -4,7 +4,6 @@ import ( "fmt" "os" "path" - "strings" log "github.com/sirupsen/logrus" api "github.com/weaveworks/ignite/pkg/apis/ignite" @@ -41,10 +40,8 @@ func decodeVM(vmID string) (*api.VM, error) { func StartVM(vm *api.VM) (err error) { - envVars := parseEnvVars(os.Environ()) - // Setup networking inside of the container, return the available interfaces - fcIfaces, dhcpIfaces, err := container.SetupContainerNetworking(envVars) + fcIfaces, dhcpIfaces, err := container.SetupContainerNetworking(vm) if err != nil { return fmt.Errorf("network setup failed: %v", err) } @@ -100,18 +97,3 @@ func patchStopped(vm *api.VM) error { patch := []byte(`{"status":{"running":false,"network":null,"runtime":null,"startTime":null}}`) return patchutil.NewPatcher(scheme.Serializer).ApplyOnFile(constants.IGNITE_SPAWN_VM_FILE_PATH, patch, vm.GroupVersionKind()) } - -func parseEnvVars(vars []string) map[string]string { - result := make(map[string]string) - - for _, arg := range vars { - parts := strings.Split(arg, "=") - if len(parts) != 2 { - continue - } - - result[parts[0]] = parts[1] - } - - return result -} diff --git a/cmd/ignite/cmd/cmdutil/flags.go b/cmd/ignite/cmd/cmdutil/flags.go index 9ef17066c..d31712cc8 100644 --- a/cmd/ignite/cmd/cmdutil/flags.go +++ b/cmd/ignite/cmd/cmdutil/flags.go @@ -35,7 +35,3 @@ func AddSSHFlags(fs *pflag.FlagSet, identityFile *string, timeout *uint32) { fs.StringVarP(identityFile, "identity", "i", "", "Override the vm's default identity file") fs.Uint32Var(timeout, "timeout", constants.SSH_DEFAULT_TIMEOUT_SECONDS, "Timeout waiting for connection in seconds") } - -func AddEnvVarsFlag(fs *pflag.FlagSet, vars *string) { - fs.StringVar(vars, "sandbox-env-vars", *vars, "A list of comma-separated key=value pairs to pass as sandbox env vars") -} diff --git a/cmd/ignite/cmd/vmcmd/create.go b/cmd/ignite/cmd/vmcmd/create.go index 566a646e7..18146424f 100644 --- a/cmd/ignite/cmd/vmcmd/create.go +++ b/cmd/ignite/cmd/vmcmd/create.go @@ -66,7 +66,6 @@ func addCreateFlags(fs *pflag.FlagSet, cf *run.CreateFlags) { // Register common flags cmdutil.AddNameFlag(fs, &cf.VM.ObjectMeta.Name) cmdutil.AddConfigFlag(fs, &cf.ConfigFile) - cmdutil.AddEnvVarsFlag(fs, &cf.EnvVars) // Register flags bound to temporary holder values fs.StringSliceVarP(&cf.PortMappings, "ports", "p", cf.PortMappings, "Map host ports to VM ports") diff --git a/cmd/ignite/run/create.go b/cmd/ignite/run/create.go index 94491bc74..9e450ed99 100644 --- a/cmd/ignite/run/create.go +++ b/cmd/ignite/run/create.go @@ -11,7 +11,6 @@ import ( "github.com/weaveworks/ignite/pkg/apis/ignite/validation" meta "github.com/weaveworks/ignite/pkg/apis/meta/v1alpha1" "github.com/weaveworks/ignite/pkg/config" - "github.com/weaveworks/ignite/pkg/constants" "github.com/weaveworks/ignite/pkg/dmlegacy" "github.com/weaveworks/ignite/pkg/metadata" "github.com/weaveworks/ignite/pkg/operations" @@ -40,7 +39,6 @@ type CreateFlags struct { ConfigFile string VM *api.VM Labels []string - EnvVars string RequireName bool } @@ -97,9 +95,6 @@ func (cf *CreateFlags) NewCreateOptions(args []string, fs *flag.FlagSet) (*Creat return nil, err } - // Save env variables - baseVM.SetAnnotation(constants.IGNITE_ENV_VAR_ANNOTATION, cf.EnvVars) - // If --require-name is true, VM name must be provided. if cf.RequireName && len(baseVM.Name) == 0 { return nil, fmt.Errorf("must set VM name, flag --require-name set") diff --git a/docs/cli/ignite/ignite_create.md b/docs/cli/ignite/ignite_create.md index 3c782decb..9e5f75d2a 100644 --- a/docs/cli/ignite/ignite_create.md +++ b/docs/cli/ignite/ignite_create.md @@ -48,7 +48,6 @@ ignite create [flags] -p, --ports strings Map host ports to VM ports --require-name Require VM name to be passed, no name generation --runtime runtime Container runtime to use. Available options are: [docker containerd] (default containerd) - --sandbox-env-vars string A list of comma-separated key=value pairs to pass as sandbox env vars --sandbox-image oci-image Specify an OCI image for the VM sandbox (default weaveworks/ignite:dev) -s, --size size VM filesystem size, for example 5GB or 2048MB (default 4.0 GB) --ssh[=] Enable SSH for the VM. If is given, it will be imported as the public key. If just '--ssh' is specified, a new keypair will be generated. (default is unset, which disables SSH access to the VM) diff --git a/docs/cli/ignite/ignite_run.md b/docs/cli/ignite/ignite_run.md index 2601a5034..fd3f8edff 100644 --- a/docs/cli/ignite/ignite_run.md +++ b/docs/cli/ignite/ignite_run.md @@ -44,7 +44,6 @@ ignite run [flags] -p, --ports strings Map host ports to VM ports --require-name Require VM name to be passed, no name generation --runtime runtime Container runtime to use. Available options are: [docker containerd] (default containerd) - --sandbox-env-vars string A list of comma-separated key=value pairs to pass as sandbox env vars --sandbox-image oci-image Specify an OCI image for the VM sandbox (default weaveworks/ignite:dev) -s, --size size VM filesystem size, for example 5GB or 2048MB (default 4.0 GB) --ssh[=] Enable SSH for the VM. If is given, it will be imported as the public key. If just '--ssh' is specified, a new keypair will be generated. (default is unset, which disables SSH access to the VM) diff --git a/docs/cli/ignite/ignite_vm_create.md b/docs/cli/ignite/ignite_vm_create.md index 49577936f..ff1359d77 100644 --- a/docs/cli/ignite/ignite_vm_create.md +++ b/docs/cli/ignite/ignite_vm_create.md @@ -48,7 +48,6 @@ ignite vm create [flags] -p, --ports strings Map host ports to VM ports --require-name Require VM name to be passed, no name generation --runtime runtime Container runtime to use. Available options are: [docker containerd] (default containerd) - --sandbox-env-vars string A list of comma-separated key=value pairs to pass as sandbox env vars --sandbox-image oci-image Specify an OCI image for the VM sandbox (default weaveworks/ignite:dev) -s, --size size VM filesystem size, for example 5GB or 2048MB (default 4.0 GB) --ssh[=] Enable SSH for the VM. If is given, it will be imported as the public key. If just '--ssh' is specified, a new keypair will be generated. (default is unset, which disables SSH access to the VM) diff --git a/docs/cli/ignite/ignite_vm_run.md b/docs/cli/ignite/ignite_vm_run.md index b838fa666..a9bdc0d7e 100644 --- a/docs/cli/ignite/ignite_vm_run.md +++ b/docs/cli/ignite/ignite_vm_run.md @@ -44,7 +44,6 @@ ignite vm run [flags] -p, --ports strings Map host ports to VM ports --require-name Require VM name to be passed, no name generation --runtime runtime Container runtime to use. Available options are: [docker containerd] (default containerd) - --sandbox-env-vars string A list of comma-separated key=value pairs to pass as sandbox env vars --sandbox-image oci-image Specify an OCI image for the VM sandbox (default weaveworks/ignite:dev) -s, --size size VM filesystem size, for example 5GB or 2048MB (default 4.0 GB) --ssh[=] Enable SSH for the VM. If is given, it will be imported as the public key. If just '--ssh' is specified, a new keypair will be generated. (default is unset, which disables SSH access to the VM) diff --git a/pkg/constants/vm.go b/pkg/constants/vm.go index 4807652a2..b265384db 100644 --- a/pkg/constants/vm.go +++ b/pkg/constants/vm.go @@ -39,6 +39,6 @@ const ( // image to be used. DEFAULT_SANDBOX_IMAGE_TAG = "dev" - // Env variable annotation key - IGNITE_ENV_VAR_ANNOTATION = "ignite.env.vars" + // IGNITE_EXTRA_INTFS is the annotation key to store a list of extra interfaces + IGNITE_EXTRA_INTFS = "ignite.weave.works/extra-intfs" ) diff --git a/pkg/container/firecracker.go b/pkg/container/firecracker.go index 9893c021e..af4f839dd 100644 --- a/pkg/container/firecracker.go +++ b/pkg/container/firecracker.go @@ -20,19 +20,9 @@ import ( ) // ExecuteFirecracker executes the firecracker process using the Go SDK -func ExecuteFirecracker(vm *api.VM, fcIfaces []FCInterface) (err error) { +func ExecuteFirecracker(vm *api.VM, fcIfaces firecracker.NetworkInterfaces) (err error) { drivePath := vm.SnapshotDev() - networkInterfaces := make([]firecracker.NetworkInterface, 0, len(fcIfaces)) - for _, intf := range fcIfaces { - networkInterfaces = append(networkInterfaces, firecracker.NetworkInterface{ - StaticConfiguration: &firecracker.StaticNetworkConfiguration{ - MacAddress: intf.MACFilter, - HostDevName: intf.VMTAP, - }, - }) - } - vCPUCount := int64(vm.Spec.CPUs) memSizeMib := int64(vm.Spec.Memory.MBytes()) @@ -67,7 +57,7 @@ func ExecuteFirecracker(vm *api.VM, fcIfaces []FCInterface) (err error) { IsRootDevice: firecracker.Bool(true), PathOnHost: &drivePath, }}, - NetworkInterfaces: networkInterfaces, + NetworkInterfaces: fcIfaces, MachineCfg: models.MachineConfiguration{ VcpuCount: &vCPUCount, MemSizeMib: &memSizeMib, diff --git a/pkg/container/network.go b/pkg/container/network.go index 3e634a140..9e1a483cd 100644 --- a/pkg/container/network.go +++ b/pkg/container/network.go @@ -4,12 +4,15 @@ import ( "fmt" "net" "os" - "strconv" + "sort" + "strings" "syscall" "time" + "github.com/firecracker-microvm/firecracker-go-sdk" log "github.com/sirupsen/logrus" "github.com/vishvananda/netlink" + api "github.com/weaveworks/ignite/pkg/apis/ignite" "github.com/weaveworks/ignite/pkg/constants" "github.com/weaveworks/ignite/pkg/util" "k8s.io/apimachinery/pkg/util/wait" @@ -36,33 +39,25 @@ var ignoreInterfaces = map[string]struct{}{ } var ( - maxIntfsVar = "IGNITE_INTFS" - defaultMaxIntfs = 1 + mainInterface = "eth0" ) -type FCInterface struct { - VMTAP string - MACFilter string -} - -func SetupContainerNetworking(args map[string]string) ([]FCInterface, []DHCPInterface, error) { +func SetupContainerNetworking(vm *api.VM) (firecracker.NetworkInterfaces, []DHCPInterface, error) { var dhcpIfaces []DHCPInterface - var allIfaces []FCInterface + var fcIfaces firecracker.NetworkInterfaces - interval := 1 * time.Second - timeout := 1 * time.Minute - maxIntfs, err := strconv.Atoi(args[maxIntfsVar]) - if err != nil || maxIntfs < 1 { - maxIntfs = defaultMaxIntfs - } + extraIntfs := parseExtraIntfs(vm) + + // total number of interfaces is at least extraIntfs + eth0 + totalIntfNum := len(extraIntfs) + 1 - err = wait.PollImmediate(interval, timeout, func() (bool, error) { + interval := 1 * time.Second + timeout := 2 * time.Minute - // Expected number of intfs is total maximum minus what we've found already - expectedIntfs := maxIntfs - len(allIfaces) + err := wait.PollImmediate(interval, timeout, func() (bool, error) { // This func returns true if it's done, and optionally an error - retry, err := networkSetup(&allIfaces, &dhcpIfaces, expectedIntfs) + retry, err := networkSetup(&fcIfaces, &dhcpIfaces, extraIntfs, &totalIntfNum) if err == nil { // We're done here return true, nil @@ -80,45 +75,63 @@ func SetupContainerNetworking(args map[string]string) ([]FCInterface, []DHCPInte return nil, nil, err } - return allIfaces, dhcpIfaces, nil + return fcIfaces, dhcpIfaces, nil } -func isIgnored(link net.Interface, allIfaces *[]FCInterface) bool { - // ignore if in explicit ignore list - if _, ok := ignoreInterfaces[link.Name]; ok { - return true - } +func filterIgnored(allIfaces []net.Interface, extraIntfs map[string]struct{}) (result []net.Interface) { - // ignore if _not_ a veth - eth, err := netlink.LinkByIndex(link.Index) - if err != nil { - return true - } + for _, intf := range allIfaces { - _, ok := eth.(*netlink.Veth) + // first process explicitly ignored + if _, ok := ignoreInterfaces[intf.Name]; ok { + continue + } - return !ok + // next process extra intfs + if _, ok := extraIntfs[intf.Name]; ok { + result = append(result, intf) + } + + // add intfs with IPs + addrs, _ := intf.Addrs() + if len(addrs) > 0 { + result = append(result, intf) + } + + } + + return result } -func networkSetup(allIfaces *[]FCInterface, dhcpIfaces *[]DHCPInterface, intfNum int) (bool, error) { - ifaces, err := net.Interfaces() - if err != nil || ifaces == nil || len(ifaces) == 0 { +func networkSetup(fcIfaces *firecracker.NetworkInterfaces, dhcpIfaces *[]DHCPInterface, extraIntfs map[string]struct{}, expectedIntfNum *int) (bool, error) { + allIfaces, err := net.Interfaces() + if err != nil || allIfaces == nil || len(allIfaces) == 0 { return true, fmt.Errorf("cannot get local network interfaces: %v", err) } - // interfacesCount counts the interfaces that are relevant to Ignite (in other words, not ignored) - interfacesCount := 0 + ifaces := filterIgnored(allIfaces, extraIntfs) + if len(ifaces) < *expectedIntfNum { + return true, fmt.Errorf("not enough extra interfaces connected (%d/%d), waiting", len(ifaces), *expectedIntfNum) + } + + // Sorting interfaces to make sure eth0 is always first + sort.Slice(ifaces, func(i, j int) bool { + return ifaces[i].Name == mainInterface + }) + for _, iface := range ifaces { - // Skip the interface if it's ignored - if isIgnored(iface, allIfaces) { - continue - } // Try to transfer the address from the container to the DHCP server ipNet, gw, noIPs, err := takeAddress(&iface) + if err != nil { + // Log the problem, but don't quit the function here as there might be other good interfaces + log.Errorf("Parsing interface %q failed: %v", iface.Name, err) + // Try with the next interface + continue + } // If interface has no IPs configured, setup tc redirect - if noIPs && iface.Name != "eth0" { + if noIPs && iface.Name != mainInterface { log.Printf("Interface %s has no IP, setting up tc redirect", iface.Name) tcInterface, err := addTcRedirect(&iface) if err != nil { @@ -126,16 +139,10 @@ func networkSetup(allIfaces *[]FCInterface, dhcpIfaces *[]DHCPInterface, intfNum continue } - *allIfaces = append(*allIfaces, *tcInterface) + *fcIfaces = append(*fcIfaces, *tcInterface) ignoreInterfaces[iface.Name] = struct{}{} - interfacesCount++ - continue - } - if err != nil { - // Log the problem, but don't quit the function here as there might be other good interfaces - log.Errorf("Parsing interface %q failed: %v", iface.Name, err) - // Try with the next interface + *expectedIntfNum-- continue } log.Print("IP detected, stealing ...") @@ -155,19 +162,20 @@ func networkSetup(allIfaces *[]FCInterface, dhcpIfaces *[]DHCPInterface, intfNum *dhcpIfaces = append(*dhcpIfaces, *dhcpIface) - *allIfaces = append(*allIfaces, FCInterface{ - VMTAP: dhcpIface.VMTAP, - MACFilter: dhcpIface.MACFilter, + *fcIfaces = append(*fcIfaces, firecracker.NetworkInterface{ + StaticConfiguration: &firecracker.StaticNetworkConfiguration{ + MacAddress: dhcpIface.MACFilter, + HostDevName: dhcpIface.VMTAP, + }, }) ignoreInterfaces[iface.Name] = struct{}{} - // This is an interface we care about - interfacesCount++ + *expectedIntfNum-- } // If there weren't any interfaces that were valid or active yet, retry the loop - if interfacesCount < intfNum { - return true, fmt.Errorf("not enough active or valid interfaces available yet") + if *expectedIntfNum > 0 { + return true, fmt.Errorf("still expecting %d interface(s) to be connected", *expectedIntfNum) } return false, nil @@ -175,7 +183,7 @@ func networkSetup(allIfaces *[]FCInterface, dhcpIfaces *[]DHCPInterface, intfNum // addTcRedirect sets up tc redirect betweeb veth and tap https://github.com/awslabs/tc-redirect-tap/blob/master/internal/netlink.go // on WSL2 this requires `CONFIG_NET_CLS_U32=y` -func addTcRedirect(iface *net.Interface) (*FCInterface, error) { +func addTcRedirect(iface *net.Interface) (*firecracker.NetworkInterface, error) { eth, err := netlink.LinkByIndex(iface.Index) if err != nil { @@ -207,9 +215,11 @@ func addTcRedirect(iface *net.Interface) (*FCInterface, error) { return nil, err } - return &FCInterface{ - VMTAP: tapName, - MACFilter: eth.Attrs().HardwareAddr.String(), + return &firecracker.NetworkInterface{ + StaticConfiguration: &firecracker.StaticNetworkConfiguration{ + MacAddress: iface.HardwareAddr.String(), + HostDevName: tapName, + }, }, nil } @@ -444,3 +454,21 @@ func maskString(mask net.IPMask) string { return fmt.Sprintf("%d.%d.%d.%d", mask[0], mask[1], mask[2], mask[3]) } + +// this function extracts a list of extra interfaces from VM's API definition +// currently it's a comma-separated string stored in annotations +func parseExtraIntfs(vm *api.VM) map[string]struct{} { + result := make(map[string]struct{}) + + parts := strings.Split(vm.GetAnnotation(constants.IGNITE_EXTRA_INTFS), ",") + if len(parts) < 1 || parts[0] == "" { + return result + } + + for _, part := range parts { + result[part] = struct{}{} + } + + return result + +} diff --git a/pkg/operations/start.go b/pkg/operations/start.go index 89e76dbf9..a3dbd73db 100644 --- a/pkg/operations/start.go +++ b/pkg/operations/start.go @@ -4,7 +4,6 @@ import ( "fmt" "path" "path/filepath" - "strings" "time" log "github.com/sirupsen/logrus" @@ -81,17 +80,6 @@ func StartVM(vm *api.VM, debug, wait bool) error { PortBindings: vm.Spec.Network.Ports, // Add the port mappings to Docker } - // Only set envVars if at least one was configured - var envVars []string - parts := strings.Split(vm.GetAnnotation((constants.IGNITE_ENV_VAR_ANNOTATION)), ",") - for _, part := range parts { - if part == "" { - continue - } - envVars = append(envVars, part) - } - config.EnvVars = envVars - // Add the volumes to the container devices for _, volume := range vm.Spec.Storage.Volumes { if volume.BlockDevice == nil { diff --git a/pkg/runtime/containerd/client.go b/pkg/runtime/containerd/client.go index d369f0d0d..e573ac778 100644 --- a/pkg/runtime/containerd/client.go +++ b/pkg/runtime/containerd/client.go @@ -422,7 +422,6 @@ func (cc *ctdClient) RunContainer(image meta.OCIImageRef, config *runtime.Contai oci.WithDefaultUnixDevices, oci.WithTTY, oci.WithImageConfigArgs(img, config.Cmd), - oci.WithEnv(config.EnvVars), withAddedCaps(config.CapAdds), withHostname(config.Hostname), withMounts(config.Binds), diff --git a/pkg/runtime/docker/client.go b/pkg/runtime/docker/client.go index 78f12933e..05f530211 100644 --- a/pkg/runtime/docker/client.go +++ b/pkg/runtime/docker/client.go @@ -158,7 +158,6 @@ func (dc *dockerClient) RunContainer(image meta.OCIImageRef, config *runtime.Con Cmd: config.Cmd, Image: image.Normalized(), Labels: config.Labels, - Env: config.EnvVars, StopTimeout: &stopTimeout, }, &container.HostConfig{ Binds: binds, diff --git a/pkg/runtime/types.go b/pkg/runtime/types.go index d1240f136..10737a3aa 100644 --- a/pkg/runtime/types.go +++ b/pkg/runtime/types.go @@ -40,7 +40,6 @@ type ContainerConfig struct { Cmd []string Hostname string Labels map[string]string - EnvVars []string Binds []*Bind CapAdds []string Devices []*Bind From 47c9140c4e1abd5b7db4c084fbf2d58750718b54 Mon Sep 17 00:00:00 2001 From: networkop Date: Sun, 23 May 2021 12:07:43 +0100 Subject: [PATCH 12/25] Re-introducing env var support This is needed in order to be able to control default firecracker timeouts. --- pkg/constants/vm.go | 3 +++ pkg/container/network.go | 4 +++- pkg/operations/start.go | 12 ++++++++++++ pkg/runtime/containerd/client.go | 1 + pkg/runtime/docker/client.go | 1 + pkg/runtime/types.go | 1 + 6 files changed, 21 insertions(+), 1 deletion(-) diff --git a/pkg/constants/vm.go b/pkg/constants/vm.go index b265384db..fd45b7f9a 100644 --- a/pkg/constants/vm.go +++ b/pkg/constants/vm.go @@ -41,4 +41,7 @@ const ( // IGNITE_EXTRA_INTFS is the annotation key to store a list of extra interfaces IGNITE_EXTRA_INTFS = "ignite.weave.works/extra-intfs" + + // IGNITE_SANDBOX_ENV_VAR is the annotation key to store a list of env variables + IGNITE_SANDBOX_ENV_VAR = "ignite.weave.works/sanbox-env" ) diff --git a/pkg/container/network.go b/pkg/container/network.go index 9e1a483cd..ca0879aa2 100644 --- a/pkg/container/network.go +++ b/pkg/container/network.go @@ -35,7 +35,9 @@ ip link set vm0 master br0 // Array of container interfaces to ignore (not forward to vm) var ignoreInterfaces = map[string]struct{}{ - "lo": {}, + "lo": {}, + "sit0": {}, + "tunl0": {}, } var ( diff --git a/pkg/operations/start.go b/pkg/operations/start.go index a3dbd73db..08033c740 100644 --- a/pkg/operations/start.go +++ b/pkg/operations/start.go @@ -4,6 +4,7 @@ import ( "fmt" "path" "path/filepath" + "strings" "time" log "github.com/sirupsen/logrus" @@ -80,6 +81,17 @@ func StartVM(vm *api.VM, debug, wait bool) error { PortBindings: vm.Spec.Network.Ports, // Add the port mappings to Docker } + // Only set envVars if at least one was configured + var envVars []string + parts := strings.Split(vm.GetAnnotation((constants.IGNITE_SANDBOX_ENV_VAR)), ",") + for _, part := range parts { + if part == "" { + continue + } + envVars = append(envVars, part) + } + config.EnvVars = envVars + // Add the volumes to the container devices for _, volume := range vm.Spec.Storage.Volumes { if volume.BlockDevice == nil { diff --git a/pkg/runtime/containerd/client.go b/pkg/runtime/containerd/client.go index e573ac778..d369f0d0d 100644 --- a/pkg/runtime/containerd/client.go +++ b/pkg/runtime/containerd/client.go @@ -422,6 +422,7 @@ func (cc *ctdClient) RunContainer(image meta.OCIImageRef, config *runtime.Contai oci.WithDefaultUnixDevices, oci.WithTTY, oci.WithImageConfigArgs(img, config.Cmd), + oci.WithEnv(config.EnvVars), withAddedCaps(config.CapAdds), withHostname(config.Hostname), withMounts(config.Binds), diff --git a/pkg/runtime/docker/client.go b/pkg/runtime/docker/client.go index 05f530211..78f12933e 100644 --- a/pkg/runtime/docker/client.go +++ b/pkg/runtime/docker/client.go @@ -158,6 +158,7 @@ func (dc *dockerClient) RunContainer(image meta.OCIImageRef, config *runtime.Con Cmd: config.Cmd, Image: image.Normalized(), Labels: config.Labels, + Env: config.EnvVars, StopTimeout: &stopTimeout, }, &container.HostConfig{ Binds: binds, diff --git a/pkg/runtime/types.go b/pkg/runtime/types.go index 10737a3aa..d1240f136 100644 --- a/pkg/runtime/types.go +++ b/pkg/runtime/types.go @@ -40,6 +40,7 @@ type ContainerConfig struct { Cmd []string Hostname string Labels map[string]string + EnvVars []string Binds []*Bind CapAdds []string Devices []*Bind From 6f1f14a1862c0ac84b3c0538ada56ea011079df8 Mon Sep 17 00:00:00 2001 From: networkop Date: Mon, 24 May 2021 21:06:02 +0100 Subject: [PATCH 13/25] Reverting the order to error checks --- pkg/container/network.go | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/pkg/container/network.go b/pkg/container/network.go index ca0879aa2..56bd166e3 100644 --- a/pkg/container/network.go +++ b/pkg/container/network.go @@ -126,12 +126,6 @@ func networkSetup(fcIfaces *firecracker.NetworkInterfaces, dhcpIfaces *[]DHCPInt // Try to transfer the address from the container to the DHCP server ipNet, gw, noIPs, err := takeAddress(&iface) - if err != nil { - // Log the problem, but don't quit the function here as there might be other good interfaces - log.Errorf("Parsing interface %q failed: %v", iface.Name, err) - // Try with the next interface - continue - } // If interface has no IPs configured, setup tc redirect if noIPs && iface.Name != mainInterface { log.Printf("Interface %s has no IP, setting up tc redirect", iface.Name) @@ -147,6 +141,14 @@ func networkSetup(fcIfaces *firecracker.NetworkInterfaces, dhcpIfaces *[]DHCPInt *expectedIntfNum-- continue } + + if err != nil { + // Log the problem, but don't quit the function here as there might be other good interfaces + log.Errorf("Parsing interface %q failed: %v", iface.Name, err) + // Try with the next interface + continue + } + log.Print("IP detected, stealing ...") // Bridge the Firecracker TAP interface with the container veth interface dhcpIface, err := bridge(&iface) From 90eaa589271b8bb390beada2ef760b02b35ecf3a Mon Sep 17 00:00:00 2001 From: networkop Date: Wed, 9 Jun 2021 21:54:29 +0100 Subject: [PATCH 14/25] Added multinet tests --- cmd/ignite/cmd/cmdutil/flags.go | 4 + cmd/ignite/cmd/vmcmd/start.go | 1 + cmd/ignite/run/exec.go | 4 + cmd/ignite/run/start.go | 5 +- e2e/multinet_test.go | 320 ++++++++++++++++++++++++++++++++ pkg/container/network.go | 4 +- pkg/container/network_test.go | 74 ++++++++ 7 files changed, 409 insertions(+), 3 deletions(-) create mode 100644 e2e/multinet_test.go create mode 100644 pkg/container/network_test.go diff --git a/cmd/ignite/cmd/cmdutil/flags.go b/cmd/ignite/cmd/cmdutil/flags.go index d31712cc8..1f6276a13 100644 --- a/cmd/ignite/cmd/cmdutil/flags.go +++ b/cmd/ignite/cmd/cmdutil/flags.go @@ -27,6 +27,10 @@ func AddInteractiveFlag(fs *pflag.FlagSet, interactive *bool) { fs.BoolVarP(interactive, "interactive", "i", *interactive, "Attach to the VM after starting") } +func AddWaitFlag(fs *pflag.FlagSet, wait *bool) { + fs.BoolVarP(wait, "wait", "w", true, "wait for VM to start") +} + func AddForceFlag(fs *pflag.FlagSet, force *bool) { fs.BoolVarP(force, "force", "f", *force, "Force this operation. Warning, use of this mode may have unintended consequences.") } diff --git a/cmd/ignite/cmd/vmcmd/start.go b/cmd/ignite/cmd/vmcmd/start.go index 7d3bc3af1..33c566f1f 100644 --- a/cmd/ignite/cmd/vmcmd/start.go +++ b/cmd/ignite/cmd/vmcmd/start.go @@ -51,6 +51,7 @@ func NewCmdStart(out io.Writer) *cobra.Command { func addStartFlags(fs *pflag.FlagSet, sf *run.StartFlags) { cmdutil.AddInteractiveFlag(fs, &sf.Interactive) + cmdutil.AddWaitFlag(fs, &sf.Wait) fs.BoolVarP(&sf.Debug, "debug", "d", false, "Debug mode, keep container after VM shutdown") fs.StringSliceVar(&sf.IgnoredPreflightErrors, "ignore-preflight-checks", []string{}, "A list of checks whose errors will be shown as warnings. Example: 'BinaryInPath,Port,ExistingFile'. Value 'all' ignores errors from all checks.") } diff --git a/cmd/ignite/run/exec.go b/cmd/ignite/run/exec.go index 5e6882b04..531f3b634 100644 --- a/cmd/ignite/run/exec.go +++ b/cmd/ignite/run/exec.go @@ -2,6 +2,7 @@ package run import ( api "github.com/weaveworks/ignite/pkg/apis/ignite" + "github.com/weaveworks/ignite/pkg/constants" ) // ExecFlags contains the flags supported by the exec command. @@ -30,5 +31,8 @@ func (ef *ExecFlags) NewExecOptions(vmMatch string, command ...string) (eo *Exec // Exec executes command in a VM based on the provided ExecOptions. func Exec(eo *ExecOptions) error { + if err := waitForSSH(eo.vm, constants.SSH_DEFAULT_TIMEOUT_SECONDS, 5); err != nil { + return err + } return runSSH(eo.vm, eo.IdentityFile, eo.command, eo.Tty, eo.Timeout) } diff --git a/cmd/ignite/run/start.go b/cmd/ignite/run/start.go index f06b2f9cc..b435d4265 100644 --- a/cmd/ignite/run/start.go +++ b/cmd/ignite/run/start.go @@ -22,6 +22,7 @@ import ( type StartFlags struct { Interactive bool + Wait bool Debug bool IgnoredPreflightErrors []string } @@ -78,12 +79,12 @@ func Start(so *StartOptions, fs *flag.FlagSet) error { return err } - if err := operations.StartVM(so.vm, so.Debug, true); err != nil { + if err := operations.StartVM(so.vm, so.Debug, so.Wait); err != nil { return err } // When --ssh is enabled, wait until SSH service started on port 22 at most N seconds - if ssh := so.vm.Spec.SSH; ssh != nil && ssh.Generate && len(so.vm.Status.Network.IPAddresses) > 0 { + if ssh := so.vm.Spec.SSH; ssh != nil && ssh.Generate && len(so.vm.Status.Network.IPAddresses) > 0 && so.Wait { if err := waitForSSH(so.vm, constants.SSH_DEFAULT_TIMEOUT_SECONDS, 5); err != nil { return err } diff --git a/e2e/multinet_test.go b/e2e/multinet_test.go new file mode 100644 index 000000000..b380cb8bd --- /dev/null +++ b/e2e/multinet_test.go @@ -0,0 +1,320 @@ +package e2e + +import ( + "fmt" + "io/ioutil" + "os" + "path/filepath" + "strings" + "testing" + + "github.com/weaveworks/ignite/e2e/util" + "gotest.tools/assert" +) + +// TestMultipleInterface tests that a VM's can be configured with more than 1 interface +func TestOneExtraInterface(t *testing.T) { + assert.Assert(t, e2eHome != "", "IGNITE_E2E_HOME should be set") + + vmName := "e2e-test-vm-multinet" + + igniteCmd := util.NewCommand(t, igniteBin) + dockerCmd := util.NewCommand(t, "docker") + + // Clone this repo in a new dir. + tempDir, err := ioutil.TempDir("", "ignite-multinet") + if err != nil { + t.Fatalf("failed to create temp dir: %v", err) + } + defer os.RemoveAll(tempDir) + + // Write a VM config with annotations + + vmConfig := []byte(`--- +apiVersion: ignite.weave.works/v1alpha4 +kind: VM +metadata: + name: e2e-test-vm-multinet + annotations: + "ignite.weave.works/extra-intfs": "foo" +spec: + image: + oci: weaveworks/ignite-ubuntu + cpus: 1 + diskSize: 3GB + memory: 800MB + ssh: true +`) + + vmConfigPath := filepath.Join(tempDir, "my-vm.yaml") + assert.Check(t, ioutil.WriteFile(vmConfigPath, vmConfig, 0644), "failed to write VM config") + + // Clean-up the following VM. + defer igniteCmd.New(). + With("rm", "-f", vmName). + Run() + + // Run VM. + igniteCmd.New(). + WithRuntime("docker"). + WithNetwork("docker-bridge"). + With("run"). + With("--ssh"). + With("--wait=false"). + With("--config=" + vmConfigPath). + Run() + + // Get the VM ID + idCmd := igniteCmd.New(). + With("ps"). + With("--filter"). + With(fmt.Sprintf("{{.ObjectMeta.Name}}=%s", vmName)). + With("--quiet") + + idOut, idErr := idCmd.Cmd.CombinedOutput() + assert.Check(t, idErr, fmt.Sprintf("vm id not found: \n%q\n%s", idCmd.Cmd, idOut)) + vmID := string(idOut[:len(idOut)-2]) + + fooAddr := "aa:ca:e9:12:34:56" + dockerCmd.New(). + With("exec", fmt.Sprintf("ignite-%s", vmID)). + With("ip", "link", "add", "foo", "type", "veth"). + Run() + + dockerCmd.New(). + With("exec", fmt.Sprintf("ignite-%s", vmID)). + With("ip", "link", "set", "foo", "address", fooAddr). + Run() + + eth1Addr := igniteCmd.New(). + With("exec", vmName). + With("cat", "/sys/class/net/eth1/address") + + foundEth1Addr, _ := eth1Addr.Cmd.CombinedOutput() + gotEth1Addr := strings.TrimSuffix(string(foundEth1Addr), "\n") + assert.Equal(t, gotEth1Addr, fooAddr, fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", fooAddr, gotEth1Addr)) + +} + +func TestMultipleInterface(t *testing.T) { + assert.Assert(t, e2eHome != "", "IGNITE_E2E_HOME should be set") + + vmName := "e2e-test-vm-multinet" + + igniteCmd := util.NewCommand(t, igniteBin) + dockerCmd := util.NewCommand(t, "docker") + + // Clone this repo in a new dir. + tempDir, err := ioutil.TempDir("", "ignite-multinet") + if err != nil { + t.Fatalf("failed to create temp dir: %v", err) + } + defer os.RemoveAll(tempDir) + + // Write a VM config with annotations + + vmConfig := []byte(`--- +apiVersion: ignite.weave.works/v1alpha4 +kind: VM +metadata: + name: e2e-test-vm-multinet + annotations: + "ignite.weave.works/extra-intfs": "bar,foo" +spec: + image: + oci: weaveworks/ignite-ubuntu + cpus: 1 + diskSize: 3GB + memory: 800MB + ssh: true +`) + + vmConfigPath := filepath.Join(tempDir, "my-vm.yaml") + assert.Check(t, ioutil.WriteFile(vmConfigPath, vmConfig, 0644), "failed to write VM config") + + // Clean-up the following VM. + defer igniteCmd.New(). + With("rm", "-f", vmName). + Run() + + // Run VM. + igniteCmd.New(). + WithRuntime("docker"). + WithNetwork("docker-bridge"). + With("run"). + With("--ssh"). + With("--wait=false"). + With("--config=" + vmConfigPath). + Run() + + // Get the VM ID + idCmd := igniteCmd.New(). + With("ps"). + With("--filter"). + With(fmt.Sprintf("{{.ObjectMeta.Name}}=%s", vmName)). + With("--quiet") + + idOut, idErr := idCmd.Cmd.CombinedOutput() + assert.Check(t, idErr, fmt.Sprintf("vm id not found: \n%q\n%s", idCmd.Cmd, idOut)) + vmID := string(idOut[:len(idOut)-2]) + + fooAddr := "aa:ca:e9:12:34:56" + dockerCmd.New(). + With("exec", fmt.Sprintf("ignite-%s", vmID)). + With("ip", "link", "add", "foo", "type", "veth"). + Run() + + dockerCmd.New(). + With("exec", fmt.Sprintf("ignite-%s", vmID)). + With("ip", "link", "set", "foo", "address", fooAddr). + Run() + + barAddr := "aa:ca:e9:12:34:78" + dockerCmd.New(). + With("exec", fmt.Sprintf("ignite-%s", vmID)). + With("ip", "link", "add", "bar", "type", "veth"). + Run() + + dockerCmd.New(). + With("exec", fmt.Sprintf("ignite-%s", vmID)). + With("ip", "link", "set", "bar", "address", barAddr). + Run() + + eth1Addr := igniteCmd.New(). + With("exec", vmName). + With("cat", "/sys/class/net/eth1/address") + + foundEth1Addr, _ := eth1Addr.Cmd.CombinedOutput() + gotEth1Addr := strings.TrimSuffix(string(foundEth1Addr), "\n") + assert.Equal(t, gotEth1Addr, fooAddr, fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", fooAddr, gotEth1Addr)) + + eth2Addr := igniteCmd.New(). + With("exec", vmName). + With("cat", "/sys/class/net/eth2/address") + + foundEth2Addr, _ := eth2Addr.Cmd.CombinedOutput() + gotEth2Addr := strings.TrimSuffix(string(foundEth2Addr), "\n") + assert.Equal(t, gotEth2Addr, barAddr, fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", barAddr, gotEth2Addr)) + +} + +func TestMultipleInterfaceImplicit(t *testing.T) { + assert.Assert(t, e2eHome != "", "IGNITE_E2E_HOME should be set") + + vmName := "e2e-test-vm-multinet" + + igniteCmd := util.NewCommand(t, igniteBin) + dockerCmd := util.NewCommand(t, "docker") + + // Clone this repo in a new dir. + tempDir, err := ioutil.TempDir("", "ignite-multinet") + if err != nil { + t.Fatalf("failed to create temp dir: %v", err) + } + defer os.RemoveAll(tempDir) + + // Write a VM config with annotations + + vmConfig := []byte(`--- +apiVersion: ignite.weave.works/v1alpha4 +kind: VM +metadata: + name: e2e-test-vm-multinet + annotations: + "ignite.weave.works/extra-intfs": "bar,foo" +spec: + image: + oci: weaveworks/ignite-ubuntu + cpus: 1 + diskSize: 3GB + memory: 800MB + ssh: true +`) + + vmConfigPath := filepath.Join(tempDir, "my-vm.yaml") + assert.Check(t, ioutil.WriteFile(vmConfigPath, vmConfig, 0644), "failed to write VM config") + + // Clean-up the following VM. + defer igniteCmd.New(). + With("rm", "-f", vmName). + Run() + + // Run VM. + igniteCmd.New(). + WithRuntime("docker"). + WithNetwork("docker-bridge"). + With("run"). + With("--ssh"). + With("--wait=false"). + With("--config=" + vmConfigPath). + Run() + + // Get the VM ID + idCmd := igniteCmd.New(). + With("ps"). + With("--filter"). + With(fmt.Sprintf("{{.ObjectMeta.Name}}=%s", vmName)). + With("--quiet") + + idOut, idErr := idCmd.Cmd.CombinedOutput() + assert.Check(t, idErr, fmt.Sprintf("vm id not found: \n%q\n%s", idCmd.Cmd, idOut)) + vmID := string(idOut[:len(idOut)-2]) + + fooAddr := "aa:ca:e9:12:34:56" + dockerCmd.New(). + With("exec", fmt.Sprintf("ignite-%s", vmID)). + With("ip", "link", "add", "foo", "type", "veth"). + Run() + + dockerCmd.New(). + With("exec", fmt.Sprintf("ignite-%s", vmID)). + With("ip", "link", "set", "foo", "address", fooAddr). + Run() + + barAddr := "aa:ca:e9:12:34:78" + dockerCmd.New(). + With("exec", fmt.Sprintf("ignite-%s", vmID)). + With("ip", "link", "add", "bar", "type", "veth"). + Run() + + dockerCmd.New(). + With("exec", fmt.Sprintf("ignite-%s", vmID)). + With("ip", "link", "set", "bar", "address", barAddr). + Run() + + // this interface should never be found inside a VM + bazAddr := "aa:ca:e9:12:34:90" + dockerCmd.New(). + With("exec", fmt.Sprintf("ignite-%s", vmID)). + With("ip", "link", "add", "baz", "type", "veth"). + Run() + + dockerCmd.New(). + With("exec", fmt.Sprintf("ignite-%s", vmID)). + With("ip", "link", "set", "baz", "address", bazAddr). + Run() + + eth1Addr := igniteCmd.New(). + With("exec", vmName). + With("cat", "/sys/class/net/eth1/address") + + foundEth1Addr, _ := eth1Addr.Cmd.CombinedOutput() + gotEth1Addr := strings.TrimSuffix(string(foundEth1Addr), "\n") + assert.Equal(t, gotEth1Addr, fooAddr, fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", fooAddr, gotEth1Addr)) + + eth2Addr := igniteCmd.New(). + With("exec", vmName). + With("cat", "/sys/class/net/eth2/address") + + foundEth2Addr, _ := eth2Addr.Cmd.CombinedOutput() + gotEth2Addr := strings.TrimSuffix(string(foundEth2Addr), "\n") + assert.Equal(t, gotEth2Addr, barAddr, fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", barAddr, gotEth2Addr)) + + eth3Addr := igniteCmd.New(). + With("exec", vmName). + With("cat", "/sys/class/net/eth3/address") + + _, foundEth3Err := eth3Addr.Cmd.CombinedOutput() + assert.Error(t, foundEth3Err, "exit status 1", fmt.Sprintf("unexpected output when looking for eth3 : \n%s", foundEth3Err)) + +} diff --git a/pkg/container/network.go b/pkg/container/network.go index 56bd166e3..799a741d4 100644 --- a/pkg/container/network.go +++ b/pkg/container/network.go @@ -470,7 +470,9 @@ func parseExtraIntfs(vm *api.VM) map[string]struct{} { } for _, part := range parts { - result[part] = struct{}{} + if part != "" && part != mainInterface { + result[part] = struct{}{} + } } return result diff --git a/pkg/container/network_test.go b/pkg/container/network_test.go new file mode 100644 index 000000000..1da3d2fd6 --- /dev/null +++ b/pkg/container/network_test.go @@ -0,0 +1,74 @@ +package container + +import ( + "testing" + + api "github.com/weaveworks/ignite/pkg/apis/ignite" + "github.com/weaveworks/ignite/pkg/constants" +) + +func TestParseExtraIntfs(t *testing.T) { + cases := []struct { + name string + annotations string + wantIntfs map[string]struct{} + }{ + { + name: "empty object", + wantIntfs: make(map[string]struct{}), + }, + { + name: "wrong annotations", + annotations: ",", + wantIntfs: make(map[string]struct{}), + }, + { + name: "one interface", + annotations: "eth1", + wantIntfs: map[string]struct{}{ + "eth1": {}, + }, + }, + { + name: "many interfaces", + annotations: "eth1,eth2,,eth5,", + wantIntfs: map[string]struct{}{ + "eth1": {}, + "eth2": {}, + "eth5": {}, + }, + }, + { + name: "many interfaces with mainInterface (eth0)", + annotations: "eth1,eth2,,eth0,", + wantIntfs: map[string]struct{}{ + "eth1": {}, + "eth2": {}, + }, + }, + } + + for _, rt := range cases { + t.Run(rt.name, func(t *testing.T) { + vm := &api.VM{} + vm.SetAnnotation(constants.IGNITE_EXTRA_INTFS, rt.annotations) + + parsedIntfs := parseExtraIntfs(vm) + + // Check if we're not missing an interface + for k := range rt.wantIntfs { + if _, ok := parsedIntfs[k]; !ok { + t.Errorf("expected interface %q not found in parsed Interfaces: %q", k, parsedIntfs) + } + } + + // Check if we don't have extra interfaces + for k := range parsedIntfs { + if _, ok := rt.wantIntfs[k]; !ok { + t.Errorf("found interface %q which was not expected", k) + } + } + + }) + } +} From 1020f90838806ca9c3a8253dd9f48af758dc7ff5 Mon Sep 17 00:00:00 2001 From: networkop Date: Wed, 9 Jun 2021 22:01:12 +0100 Subject: [PATCH 15/25] Fixing dox --- docs/cli/ignite/ignite_run.md | 1 + docs/cli/ignite/ignite_start.md | 1 + docs/cli/ignite/ignite_vm_run.md | 1 + docs/cli/ignite/ignite_vm_start.md | 1 + 4 files changed, 4 insertions(+) diff --git a/docs/cli/ignite/ignite_run.md b/docs/cli/ignite/ignite_run.md index fd3f8edff..a5e2f8eef 100644 --- a/docs/cli/ignite/ignite_run.md +++ b/docs/cli/ignite/ignite_run.md @@ -48,6 +48,7 @@ ignite run [flags] -s, --size size VM filesystem size, for example 5GB or 2048MB (default 4.0 GB) --ssh[=] Enable SSH for the VM. If is given, it will be imported as the public key. If just '--ssh' is specified, a new keypair will be generated. (default is unset, which disables SSH access to the VM) -v, --volumes volume Expose block devices from the host inside the VM + -w, --wait wait for VM to start (default true) ``` ### Options inherited from parent commands diff --git a/docs/cli/ignite/ignite_start.md b/docs/cli/ignite/ignite_start.md index 00eef2ebb..07adc828f 100644 --- a/docs/cli/ignite/ignite_start.md +++ b/docs/cli/ignite/ignite_start.md @@ -23,6 +23,7 @@ ignite start [flags] -i, --interactive Attach to the VM after starting --network-plugin plugin Network plugin to use. Available options are: [cni docker-bridge] (default cni) --runtime runtime Container runtime to use. Available options are: [docker containerd] (default containerd) + -w, --wait wait for VM to start (default true) ``` ### Options inherited from parent commands diff --git a/docs/cli/ignite/ignite_vm_run.md b/docs/cli/ignite/ignite_vm_run.md index a9bdc0d7e..afdcb22d4 100644 --- a/docs/cli/ignite/ignite_vm_run.md +++ b/docs/cli/ignite/ignite_vm_run.md @@ -48,6 +48,7 @@ ignite vm run [flags] -s, --size size VM filesystem size, for example 5GB or 2048MB (default 4.0 GB) --ssh[=] Enable SSH for the VM. If is given, it will be imported as the public key. If just '--ssh' is specified, a new keypair will be generated. (default is unset, which disables SSH access to the VM) -v, --volumes volume Expose block devices from the host inside the VM + -w, --wait wait for VM to start (default true) ``` ### Options inherited from parent commands diff --git a/docs/cli/ignite/ignite_vm_start.md b/docs/cli/ignite/ignite_vm_start.md index b6fe63271..c02615bb7 100644 --- a/docs/cli/ignite/ignite_vm_start.md +++ b/docs/cli/ignite/ignite_vm_start.md @@ -23,6 +23,7 @@ ignite vm start [flags] -i, --interactive Attach to the VM after starting --network-plugin plugin Network plugin to use. Available options are: [cni docker-bridge] (default cni) --runtime runtime Container runtime to use. Available options are: [docker containerd] (default containerd) + -w, --wait wait for VM to start (default true) ``` ### Options inherited from parent commands From 053def520ffcfc0356f6b622c56b9067dff68439 Mon Sep 17 00:00:00 2001 From: networkop Date: Thu, 10 Jun 2021 21:07:06 +0100 Subject: [PATCH 16/25] Refactored StartVM with async spawn channel --- cmd/ignite/run/start.go | 10 ++++- e2e/multinet_test.go | 10 ++--- pkg/operations/reconcile/reconcile.go | 2 +- pkg/operations/start.go | 55 ++++++++++++++++++--------- 4 files changed, 52 insertions(+), 25 deletions(-) diff --git a/cmd/ignite/run/start.go b/cmd/ignite/run/start.go index b435d4265..8c191d5c3 100644 --- a/cmd/ignite/run/start.go +++ b/cmd/ignite/run/start.go @@ -79,7 +79,15 @@ func Start(so *StartOptions, fs *flag.FlagSet) error { return err } - if err := operations.StartVM(so.vm, so.Debug, so.Wait); err != nil { + var err error + if so.Wait { + err = operations.StartVM(so.vm, so.Debug) + } else { + // Can't do much with vmChannels here, we need to return ASAP + _, err = operations.StartVMNonBlocking(so.vm, so.Debug) + } + + if err != nil { return err } diff --git a/e2e/multinet_test.go b/e2e/multinet_test.go index b380cb8bd..9da1fa247 100644 --- a/e2e/multinet_test.go +++ b/e2e/multinet_test.go @@ -92,7 +92,7 @@ spec: foundEth1Addr, _ := eth1Addr.Cmd.CombinedOutput() gotEth1Addr := strings.TrimSuffix(string(foundEth1Addr), "\n") - assert.Equal(t, gotEth1Addr, fooAddr, fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", fooAddr, gotEth1Addr)) + assert.Check(t, strings.Contains(gotEth1Addr, fooAddr), fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", fooAddr, gotEth1Addr)) } @@ -186,7 +186,7 @@ spec: foundEth1Addr, _ := eth1Addr.Cmd.CombinedOutput() gotEth1Addr := strings.TrimSuffix(string(foundEth1Addr), "\n") - assert.Equal(t, gotEth1Addr, fooAddr, fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", fooAddr, gotEth1Addr)) + assert.Check(t, strings.Contains(gotEth1Addr, fooAddr), fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", fooAddr, gotEth1Addr)) eth2Addr := igniteCmd.New(). With("exec", vmName). @@ -194,7 +194,7 @@ spec: foundEth2Addr, _ := eth2Addr.Cmd.CombinedOutput() gotEth2Addr := strings.TrimSuffix(string(foundEth2Addr), "\n") - assert.Equal(t, gotEth2Addr, barAddr, fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", barAddr, gotEth2Addr)) + assert.Check(t, strings.Contains(gotEth2Addr, barAddr), fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", barAddr, gotEth2Addr)) } @@ -300,7 +300,7 @@ spec: foundEth1Addr, _ := eth1Addr.Cmd.CombinedOutput() gotEth1Addr := strings.TrimSuffix(string(foundEth1Addr), "\n") - assert.Equal(t, gotEth1Addr, fooAddr, fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", fooAddr, gotEth1Addr)) + assert.Check(t, strings.Contains(gotEth1Addr, fooAddr), fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", fooAddr, gotEth1Addr)) eth2Addr := igniteCmd.New(). With("exec", vmName). @@ -308,7 +308,7 @@ spec: foundEth2Addr, _ := eth2Addr.Cmd.CombinedOutput() gotEth2Addr := strings.TrimSuffix(string(foundEth2Addr), "\n") - assert.Equal(t, gotEth2Addr, barAddr, fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", barAddr, gotEth2Addr)) + assert.Check(t, strings.Contains(gotEth2Addr, barAddr), fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", barAddr, gotEth2Addr)) eth3Addr := igniteCmd.New(). With("exec", vmName). diff --git a/pkg/operations/reconcile/reconcile.go b/pkg/operations/reconcile/reconcile.go index 7d70f4428..57601bcae 100644 --- a/pkg/operations/reconcile/reconcile.go +++ b/pkg/operations/reconcile/reconcile.go @@ -149,7 +149,7 @@ func start(vm *api.VM) error { log.Infof("Starting VM %q with name %q...", vm.GetUID(), vm.GetName()) vmStarted.Inc() - return operations.StartVM(vm, true, true) + return operations.StartVM(vm, true) } func stop(vm *api.VM) error { diff --git a/pkg/operations/start.go b/pkg/operations/start.go index 08033c740..b67b21fb1 100644 --- a/pkg/operations/start.go +++ b/pkg/operations/start.go @@ -20,20 +20,44 @@ import ( apiruntime "github.com/weaveworks/libgitops/pkg/runtime" ) -func StartVM(vm *api.VM, debug, wait bool) error { +// VMChannels can be used to get signals for different stages of VM lifecycle +type VMChannels struct { + SpawnFinished chan error +} + +func StartVM(vm *api.VM, debug bool) error { + + vmChans, err := StartVMNonBlocking(vm, debug) + if err != nil { + return err + } + + if err := <-vmChans.SpawnFinished; err != nil { + return err + } + + return nil +} + +func StartVMNonBlocking(vm *api.VM, debug bool) (*VMChannels, error) { // Inspect the VM container and remove it if it exists inspectResult, _ := providers.Runtime.InspectContainer(vm.PrefixedID()) RemoveVMContainer(inspectResult) + // Make sure we always initialize all channels + vmChans := &VMChannels{ + SpawnFinished: make(chan error), + } + // Setup the snapshot overlay filesystem snapshotDevPath, err := dmlegacy.ActivateSnapshot(vm) if err != nil { - return err + return vmChans, err } kernelUID, err := lookup.KernelUIDForVM(vm, providers.Client) if err != nil { - return err + return vmChans, err } vmDir := filepath.Join(constants.VM_DIR, vm.GetUID().String()) @@ -42,7 +66,7 @@ func StartVM(vm *api.VM, debug, wait bool) error { // Verify that the image containing ignite-spawn is pulled // TODO: Integrate automatic pulling into pkg/runtime if err := verifyPulled(vm.Spec.Sandbox.OCI); err != nil { - return err + return vmChans, err } config := &runtime.ContainerConfig{ @@ -106,7 +130,7 @@ func StartVM(vm *api.VM, debug, wait bool) error { // Prepare the networking for the container, for the given network plugin if err := providers.NetworkPlugin.PrepareContainerSpec(config); err != nil { - return err + return vmChans, err } // If we're not debugging, remove the container post-run @@ -117,13 +141,13 @@ func StartVM(vm *api.VM, debug, wait bool) error { // Run the VM container in Docker containerID, err := providers.Runtime.RunContainer(vm.Spec.Sandbox.OCI, config, vm.PrefixedID(), vm.GetUID().String()) if err != nil { - return fmt.Errorf("failed to start container for VM %q: %v", vm.GetUID(), err) + return vmChans, fmt.Errorf("failed to start container for VM %q: %v", vm.GetUID(), err) } // Set up the networking result, err := providers.NetworkPlugin.SetupContainerNetwork(containerID, vm.Spec.Network.Ports...) if err != nil { - return err + return vmChans, err } if !logs.Quiet { @@ -131,14 +155,8 @@ func StartVM(vm *api.VM, debug, wait bool) error { log.Infof("Started Firecracker VM %q in a container with ID %q", vm.GetUID(), containerID) } - // TODO: Follow-up the container here with a defer, or dedicated goroutine. We should output - // if it started successfully or not // TODO: This is temporary until we have proper communication to the container - if wait { - if err := waitForSpawn(vm); err != nil { - return err - } - } + go waitForSpawn(vm, vmChans) // Set the container ID for the VM vm.Status.Runtime.ID = containerID @@ -160,7 +178,7 @@ func StartVM(vm *api.VM, debug, wait bool) error { vm.Status.Running = true // Write the state changes - return providers.Client.VMs().Set(vm) + return vmChans, providers.Client.VMs().Set(vm) } // verifyPulled pulls the ignite-spawn image if it's not present @@ -182,7 +200,7 @@ func verifyPulled(image meta.OCIImageRef) error { // TODO: This check for the Prometheus socket file is temporary // until we get a proper ignite <-> ignite-spawn communication channel -func waitForSpawn(vm *api.VM) error { +func waitForSpawn(vm *api.VM, vmChans *VMChannels) { const timeout = 10 * time.Second const checkInterval = 100 * time.Millisecond @@ -191,9 +209,10 @@ func waitForSpawn(vm *api.VM) error { time.Sleep(checkInterval) if util.FileExists(path.Join(vm.ObjectPath(), constants.PROMETHEUS_SOCKET)) { - return nil + vmChans.SpawnFinished <- nil + return } } - return fmt.Errorf("timeout waiting for ignite-spawn startup") + vmChans.SpawnFinished <- fmt.Errorf("timeout waiting for ignite-spawn startup") } From eb8997f387e281aab1da0d25a350f54c1afd815d Mon Sep 17 00:00:00 2001 From: networkop Date: Thu, 10 Jun 2021 23:50:19 +0100 Subject: [PATCH 17/25] Refactored ignite-spawn's network setup --- e2e/multinet_test.go | 17 +-- pkg/constants/vm.go | 4 +- pkg/container/network.go | 200 ++++++++++++++++++---------------- pkg/container/network_test.go | 69 ++++++------ 4 files changed, 148 insertions(+), 142 deletions(-) diff --git a/e2e/multinet_test.go b/e2e/multinet_test.go index 9da1fa247..93d93130d 100644 --- a/e2e/multinet_test.go +++ b/e2e/multinet_test.go @@ -36,7 +36,7 @@ kind: VM metadata: name: e2e-test-vm-multinet annotations: - "ignite.weave.works/extra-intfs": "foo" + "ignite.weave.works/interface/foo": "tc-redirect" spec: image: oci: weaveworks/ignite-ubuntu @@ -60,6 +60,7 @@ spec: WithNetwork("docker-bridge"). With("run"). With("--ssh"). + With("--debug"). With("--wait=false"). With("--config=" + vmConfigPath). Run() @@ -119,7 +120,8 @@ kind: VM metadata: name: e2e-test-vm-multinet annotations: - "ignite.weave.works/extra-intfs": "bar,foo" + "ignite.weave.works/interface/foo": "tc-redirect" + "ignite.weave.works/interface/bar": "tc-redirect" spec: image: oci: weaveworks/ignite-ubuntu @@ -186,7 +188,7 @@ spec: foundEth1Addr, _ := eth1Addr.Cmd.CombinedOutput() gotEth1Addr := strings.TrimSuffix(string(foundEth1Addr), "\n") - assert.Check(t, strings.Contains(gotEth1Addr, fooAddr), fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", fooAddr, gotEth1Addr)) + assert.Check(t, strings.Contains(gotEth1Addr, barAddr), fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", barAddr, gotEth1Addr)) eth2Addr := igniteCmd.New(). With("exec", vmName). @@ -194,7 +196,7 @@ spec: foundEth2Addr, _ := eth2Addr.Cmd.CombinedOutput() gotEth2Addr := strings.TrimSuffix(string(foundEth2Addr), "\n") - assert.Check(t, strings.Contains(gotEth2Addr, barAddr), fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", barAddr, gotEth2Addr)) + assert.Check(t, strings.Contains(gotEth2Addr, fooAddr), fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", fooAddr, gotEth2Addr)) } @@ -221,7 +223,8 @@ kind: VM metadata: name: e2e-test-vm-multinet annotations: - "ignite.weave.works/extra-intfs": "bar,foo" + "ignite.weave.works/interface/foo": "tc-redirect" + "ignite.weave.works/interface/bar": "tc-redirect" spec: image: oci: weaveworks/ignite-ubuntu @@ -300,7 +303,7 @@ spec: foundEth1Addr, _ := eth1Addr.Cmd.CombinedOutput() gotEth1Addr := strings.TrimSuffix(string(foundEth1Addr), "\n") - assert.Check(t, strings.Contains(gotEth1Addr, fooAddr), fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", fooAddr, gotEth1Addr)) + assert.Check(t, strings.Contains(gotEth1Addr, barAddr), fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", barAddr, gotEth1Addr)) eth2Addr := igniteCmd.New(). With("exec", vmName). @@ -308,7 +311,7 @@ spec: foundEth2Addr, _ := eth2Addr.Cmd.CombinedOutput() gotEth2Addr := strings.TrimSuffix(string(foundEth2Addr), "\n") - assert.Check(t, strings.Contains(gotEth2Addr, barAddr), fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", barAddr, gotEth2Addr)) + assert.Check(t, strings.Contains(gotEth2Addr, fooAddr), fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", fooAddr, gotEth2Addr)) eth3Addr := igniteCmd.New(). With("exec", vmName). diff --git a/pkg/constants/vm.go b/pkg/constants/vm.go index fd45b7f9a..f93babb57 100644 --- a/pkg/constants/vm.go +++ b/pkg/constants/vm.go @@ -39,8 +39,8 @@ const ( // image to be used. DEFAULT_SANDBOX_IMAGE_TAG = "dev" - // IGNITE_EXTRA_INTFS is the annotation key to store a list of extra interfaces - IGNITE_EXTRA_INTFS = "ignite.weave.works/extra-intfs" + // IGNITE_INTERFACE_ANNOTATION is the annotation key to store a list of extra interfaces + IGNITE_INTERFACE_ANNOTATION = "ignite.weave.works/interface/" // IGNITE_SANDBOX_ENV_VAR is the annotation key to store a list of env variables IGNITE_SANDBOX_ENV_VAR = "ignite.weave.works/sanbox-env" diff --git a/pkg/container/network.go b/pkg/container/network.go index 799a741d4..7024805f2 100644 --- a/pkg/container/network.go +++ b/pkg/container/network.go @@ -40,18 +40,25 @@ var ignoreInterfaces = map[string]struct{}{ "tunl0": {}, } -var ( - mainInterface = "eth0" -) +var supportedModes = struct { + dhcp, tc string +}{ + dhcp: "dhcp-bridge", + tc: "tc-redirect", +} + +var mainInterface = "eth0" func SetupContainerNetworking(vm *api.VM) (firecracker.NetworkInterfaces, []DHCPInterface, error) { - var dhcpIfaces []DHCPInterface - var fcIfaces firecracker.NetworkInterfaces + var dhcpIntfs []DHCPInterface + var fcIntfs firecracker.NetworkInterfaces - extraIntfs := parseExtraIntfs(vm) + vmIntfs := parseExtraIntfs(vm) - // total number of interfaces is at least extraIntfs + eth0 - totalIntfNum := len(extraIntfs) + 1 + // Setting up mainInterface if not defined + if _, ok := vmIntfs[mainInterface]; !ok { + vmIntfs[mainInterface] = supportedModes.dhcp + } interval := 1 * time.Second timeout := 2 * time.Minute @@ -59,7 +66,9 @@ func SetupContainerNetworking(vm *api.VM) (firecracker.NetworkInterfaces, []DHCP err := wait.PollImmediate(interval, timeout, func() (bool, error) { // This func returns true if it's done, and optionally an error - retry, err := networkSetup(&fcIfaces, &dhcpIfaces, extraIntfs, &totalIntfNum) + retry, err := collectInterfaces(vmIntfs) + + // if err == nil { // We're done here return true, nil @@ -77,118 +86,109 @@ func SetupContainerNetworking(vm *api.VM) (firecracker.NetworkInterfaces, []DHCP return nil, nil, err } - return fcIfaces, dhcpIfaces, nil -} + if err := networkSetup(&fcIntfs, &dhcpIntfs, vmIntfs); err != nil { + return nil, nil, err + } -func filterIgnored(allIfaces []net.Interface, extraIntfs map[string]struct{}) (result []net.Interface) { + return fcIntfs, dhcpIntfs, nil +} - for _, intf := range allIfaces { +func collectInterfaces(vmIntfs map[string]string) (bool, error) { + allIntfs, err := net.Interfaces() + if err != nil || allIntfs == nil || len(allIntfs) == 0 { + return false, fmt.Errorf("cannot get local network interfaces: %v", err) + } - // first process explicitly ignored + // create a map of candidate interfaces + foundIntfs := make(map[string]struct{}) + for _, intf := range allIntfs { if _, ok := ignoreInterfaces[intf.Name]; ok { continue } + foundIntfs[intf.Name] = struct{}{} - // next process extra intfs - if _, ok := extraIntfs[intf.Name]; ok { - result = append(result, intf) - } - - // add intfs with IPs + // default fallback behaviour to always consider intfs with an address addrs, _ := intf.Addrs() if len(addrs) > 0 { - result = append(result, intf) + vmIntfs[intf.Name] = supportedModes.dhcp } - } - return result + // make sure we've found all expected interfaces + for intfName, mode := range vmIntfs { + if _, ok := foundIntfs[intfName]; !ok { + return true, fmt.Errorf("interface %q (mode %q) is still not found", intfName, mode) + } + } + return false, nil } -func networkSetup(fcIfaces *firecracker.NetworkInterfaces, dhcpIfaces *[]DHCPInterface, extraIntfs map[string]struct{}, expectedIntfNum *int) (bool, error) { - allIfaces, err := net.Interfaces() - if err != nil || allIfaces == nil || len(allIfaces) == 0 { - return true, fmt.Errorf("cannot get local network interfaces: %v", err) - } +func networkSetup(fcIntfs *firecracker.NetworkInterfaces, dhcpIntfs *[]DHCPInterface, vmIntfs map[string]string) error { - ifaces := filterIgnored(allIfaces, extraIntfs) - if len(ifaces) < *expectedIntfNum { - return true, fmt.Errorf("not enough extra interfaces connected (%d/%d), waiting", len(ifaces), *expectedIntfNum) + var keys []string + for k := range vmIntfs { + keys = append(keys, k) } + sort.Strings(keys) - // Sorting interfaces to make sure eth0 is always first - sort.Slice(ifaces, func(i, j int) bool { - return ifaces[i].Name == mainInterface + // making sure eth0 is always first + sort.Slice(keys, func(i, j int) bool { + return keys[i] == mainInterface }) - for _, iface := range ifaces { - - // Try to transfer the address from the container to the DHCP server - ipNet, gw, noIPs, err := takeAddress(&iface) - - // If interface has no IPs configured, setup tc redirect - if noIPs && iface.Name != mainInterface { - log.Printf("Interface %s has no IP, setting up tc redirect", iface.Name) - tcInterface, err := addTcRedirect(&iface) - if err != nil { - log.Errorf("Failed to setup tc redirect %v", err) - continue - } - - *fcIfaces = append(*fcIfaces, *tcInterface) - ignoreInterfaces[iface.Name] = struct{}{} - - *expectedIntfNum-- - continue - } + for _, intfName := range keys { + intf, err := net.InterfaceByName(intfName) if err != nil { - // Log the problem, but don't quit the function here as there might be other good interfaces - log.Errorf("Parsing interface %q failed: %v", iface.Name, err) - // Try with the next interface - continue + return fmt.Errorf("cannot find interface %q: %s", intfName, err) } - log.Print("IP detected, stealing ...") - // Bridge the Firecracker TAP interface with the container veth interface - dhcpIface, err := bridge(&iface) - if err != nil { - // Log the problem, but don't quit the function here as there might be other good interfaces - // Don't set shouldRetry here as there is no point really with retrying with this interface - // that seems broken/unsupported in some way. - log.Errorf("Bridging interface %q failed: %v", iface.Name, err) - // Try with the next interface - continue - } + switch vmIntfs[intfName] { + case supportedModes.dhcp: + ipNet, gw, noIPs, err := takeAddress(intf) + if err != nil { + return fmt.Errorf("error parsing interface %q: %s", intfName, err) + } + if noIPs { + return fmt.Errorf("interface %q expected to have an IP but none found", intfName) + } - dhcpIface.VMIPNet = ipNet - dhcpIface.GatewayIP = gw + dhcpIface, err := bridge(intf) + if err != nil { + return fmt.Errorf("bridging interface %q failed: %v", intfName, err) + } - *dhcpIfaces = append(*dhcpIfaces, *dhcpIface) + dhcpIface.VMIPNet = ipNet + dhcpIface.GatewayIP = gw - *fcIfaces = append(*fcIfaces, firecracker.NetworkInterface{ - StaticConfiguration: &firecracker.StaticNetworkConfiguration{ - MacAddress: dhcpIface.MACFilter, - HostDevName: dhcpIface.VMTAP, - }, - }) - ignoreInterfaces[iface.Name] = struct{}{} + *dhcpIntfs = append(*dhcpIntfs, *dhcpIface) - *expectedIntfNum-- - } + *fcIntfs = append(*fcIntfs, firecracker.NetworkInterface{ + StaticConfiguration: &firecracker.StaticNetworkConfiguration{ + MacAddress: dhcpIface.MACFilter, + HostDevName: dhcpIface.VMTAP, + }, + }) + case supportedModes.tc: + tcInterface, err := addTcRedirect(intf) + if err != nil { + log.Errorf("Failed to setup tc redirect %v", err) + continue + } - // If there weren't any interfaces that were valid or active yet, retry the loop - if *expectedIntfNum > 0 { - return true, fmt.Errorf("still expecting %d interface(s) to be connected", *expectedIntfNum) + *fcIntfs = append(*fcIntfs, *tcInterface) + } } - return false, nil + return nil } // addTcRedirect sets up tc redirect betweeb veth and tap https://github.com/awslabs/tc-redirect-tap/blob/master/internal/netlink.go // on WSL2 this requires `CONFIG_NET_CLS_U32=y` func addTcRedirect(iface *net.Interface) (*firecracker.NetworkInterface, error) { + log.Infof("Adding tc-redirect for %q", iface.Name) + eth, err := netlink.LinkByIndex(iface.Index) if err != nil { return nil, err @@ -459,20 +459,30 @@ func maskString(mask net.IPMask) string { return fmt.Sprintf("%d.%d.%d.%d", mask[0], mask[1], mask[2], mask[3]) } -// this function extracts a list of extra interfaces from VM's API definition +// this function extracts a list of interfaces from VM's API definition // currently it's a comma-separated string stored in annotations -func parseExtraIntfs(vm *api.VM) map[string]struct{} { - result := make(map[string]struct{}) +func parseExtraIntfs(vm *api.VM) map[string]string { + result := make(map[string]string) - parts := strings.Split(vm.GetAnnotation(constants.IGNITE_EXTRA_INTFS), ",") - if len(parts) < 1 || parts[0] == "" { - return result - } + for k, v := range vm.GetObjectMeta().Annotations { + if !strings.HasPrefix(k, constants.IGNITE_INTERFACE_ANNOTATION) { + continue + } - for _, part := range parts { - if part != "" && part != mainInterface { - result[part] = struct{}{} + k = strings.TrimPrefix(k, constants.IGNITE_INTERFACE_ANNOTATION) + if k == "" { + continue } + + switch v { + case supportedModes.dhcp: + result[k] = v + case supportedModes.tc: + result[k] = v + default: + continue + } + } return result diff --git a/pkg/container/network_test.go b/pkg/container/network_test.go index 1da3d2fd6..5b63e78e2 100644 --- a/pkg/container/network_test.go +++ b/pkg/container/network_test.go @@ -4,46 +4,49 @@ import ( "testing" api "github.com/weaveworks/ignite/pkg/apis/ignite" - "github.com/weaveworks/ignite/pkg/constants" + "gotest.tools/assert" ) func TestParseExtraIntfs(t *testing.T) { cases := []struct { name string - annotations string - wantIntfs map[string]struct{} + annotations map[string]string + wantIntfs map[string]string }{ { name: "empty object", - wantIntfs: make(map[string]struct{}), + wantIntfs: make(map[string]string), }, { - name: "wrong annotations", - annotations: ",", - wantIntfs: make(map[string]struct{}), - }, - { - name: "one interface", - annotations: "eth1", - wantIntfs: map[string]struct{}{ - "eth1": {}, + name: "wrong annotations", + annotations: map[string]string{ + "foo": "bar", + "ignite.weave.works/interface/": "dhcp-bridge", + "ignite.weave.works/interface/eth123": "foo", }, + wantIntfs: make(map[string]string), }, { - name: "many interfaces", - annotations: "eth1,eth2,,eth5,", - wantIntfs: map[string]struct{}{ - "eth1": {}, - "eth2": {}, - "eth5": {}, + name: "one interface", + annotations: map[string]string{ + "foo": "bar", + "ignite.weave.works/interface/": "dhcp-bridge", + "ignite.weave.works/interface/eth123": "tc-redirect", + }, + wantIntfs: map[string]string{ + "eth123": "tc-redirect", }, }, { - name: "many interfaces with mainInterface (eth0)", - annotations: "eth1,eth2,,eth0,", - wantIntfs: map[string]struct{}{ - "eth1": {}, - "eth2": {}, + name: "many interfaces", + annotations: map[string]string{ + "foo": "bar", + "ignite.weave.works/interface/eth0": "dhcp-bridge", + "ignite.weave.works/interface/eth123": "tc-redirect", + }, + wantIntfs: map[string]string{ + "eth0": "dhcp-bridge", + "eth123": "tc-redirect", }, }, } @@ -51,23 +54,13 @@ func TestParseExtraIntfs(t *testing.T) { for _, rt := range cases { t.Run(rt.name, func(t *testing.T) { vm := &api.VM{} - vm.SetAnnotation(constants.IGNITE_EXTRA_INTFS, rt.annotations) + for k, v := range rt.annotations { + vm.SetAnnotation(k, v) + } parsedIntfs := parseExtraIntfs(vm) - // Check if we're not missing an interface - for k := range rt.wantIntfs { - if _, ok := parsedIntfs[k]; !ok { - t.Errorf("expected interface %q not found in parsed Interfaces: %q", k, parsedIntfs) - } - } - - // Check if we don't have extra interfaces - for k := range parsedIntfs { - if _, ok := rt.wantIntfs[k]; !ok { - t.Errorf("found interface %q which was not expected", k) - } - } + assert.DeepEqual(t, parsedIntfs, rt.wantIntfs) }) } From 051ad2bf5b0c259a30653ee4566439c500892db3 Mon Sep 17 00:00:00 2001 From: networkop Date: Fri, 11 Jun 2021 11:25:07 +0100 Subject: [PATCH 18/25] Fixed the CNI plugin bug --- pkg/constants/vm.go | 6 ++-- pkg/container/network.go | 77 ++++++++++++++++++++++++++-------------- pkg/operations/start.go | 10 +++--- 3 files changed, 57 insertions(+), 36 deletions(-) diff --git a/pkg/constants/vm.go b/pkg/constants/vm.go index f93babb57..0b96f42ee 100644 --- a/pkg/constants/vm.go +++ b/pkg/constants/vm.go @@ -39,9 +39,9 @@ const ( // image to be used. DEFAULT_SANDBOX_IMAGE_TAG = "dev" - // IGNITE_INTERFACE_ANNOTATION is the annotation key to store a list of extra interfaces + // IGNITE_INTERFACE_ANNOTATION is the annotation prefix to store a list of extra interfaces IGNITE_INTERFACE_ANNOTATION = "ignite.weave.works/interface/" - // IGNITE_SANDBOX_ENV_VAR is the annotation key to store a list of env variables - IGNITE_SANDBOX_ENV_VAR = "ignite.weave.works/sanbox-env" + // IGNITE_SANDBOX_ENV_VAR is the annotation prefix to store a list of env variables + IGNITE_SANDBOX_ENV_VAR = "ignite.weave.works/sanbox-env/" ) diff --git a/pkg/container/network.go b/pkg/container/network.go index 7024805f2..b20fdaf36 100644 --- a/pkg/container/network.go +++ b/pkg/container/network.go @@ -100,12 +100,12 @@ func collectInterfaces(vmIntfs map[string]string) (bool, error) { } // create a map of candidate interfaces - foundIntfs := make(map[string]struct{}) + foundIntfs := make(map[string]*net.Interface) for _, intf := range allIntfs { if _, ok := ignoreInterfaces[intf.Name]; ok { continue } - foundIntfs[intf.Name] = struct{}{} + foundIntfs[intf.Name] = &intf // default fallback behaviour to always consider intfs with an address addrs, _ := intf.Addrs() @@ -119,6 +119,18 @@ func collectInterfaces(vmIntfs map[string]string) (bool, error) { if _, ok := foundIntfs[intfName]; !ok { return true, fmt.Errorf("interface %q (mode %q) is still not found", intfName, mode) } + + // for DHCP interface, we need to make sure IP and route exist + if mode == supportedModes.dhcp { + _, _, _, noIPs, err := getAddress(foundIntfs[intfName]) + if err != nil { + return true, err + } + + if noIPs { + return true, fmt.Errorf("IP is still not found on %q", intfName) + } + } } return false, nil } @@ -145,13 +157,10 @@ func networkSetup(fcIntfs *firecracker.NetworkInterfaces, dhcpIntfs *[]DHCPInter switch vmIntfs[intfName] { case supportedModes.dhcp: - ipNet, gw, noIPs, err := takeAddress(intf) + ipNet, gw, err := takeAddress(intf) if err != nil { return fmt.Errorf("error parsing interface %q: %s", intfName, err) } - if noIPs { - return fmt.Errorf("interface %q expected to have an IP but none found", intfName) - } dhcpIface, err := bridge(intf) if err != nil { @@ -305,12 +314,13 @@ func bridge(iface *net.Interface) (*DHCPInterface, error) { }, nil } -// takeAddress removes the first address of an interface and returns it and the appropriate gateway -func takeAddress(iface *net.Interface) (*net.IPNet, *net.IP, bool, error) { +// getAddress collects the first IP and gateway information from an interface +// in case of multiple routes over an interface, only the first one is considered +func getAddress(iface *net.Interface) (*net.IPNet, *net.IP, netlink.Link, bool, error) { addrs, err := iface.Addrs() if err != nil || addrs == nil || len(addrs) == 0 { // set the bool to true so the caller knows to retry - return nil, nil, true, fmt.Errorf("interface %q has no address", iface.Name) + return nil, nil, nil, true, fmt.Errorf("interface %q has no address", iface.Name) } for _, addr := range addrs { @@ -337,42 +347,55 @@ func takeAddress(iface *net.Interface) (*net.IPNet, *net.IP, bool, error) { link, err := netlink.LinkByName(iface.Name) if err != nil { - return nil, nil, false, fmt.Errorf("failed to get interface %q by name: %v", iface.Name, err) + return nil, nil, nil, false, fmt.Errorf("failed to get interface %q by name: %v", iface.Name, err) } var gw *net.IP - gwString := "" routes, err := netlink.RouteList(link, netlink.FAMILY_ALL) if err != nil { - return nil, nil, false, fmt.Errorf("failed to get default gateway for interface %q: %v", iface.Name, err) + return nil, nil, nil, false, fmt.Errorf("failed to get default gateway for interface %q: %v", iface.Name, err) } for _, rt := range routes { if rt.Gw != nil { gw = &rt.Gw - gwString = gw.String() break } } - delAddr := &netlink.Addr{ - IPNet: &net.IPNet{ - IP: ip, - Mask: mask, - }, - } - if err = netlink.AddrDel(link, delAddr); err != nil { - return nil, nil, false, fmt.Errorf("failed to remove address %q from interface %q: %v", delAddr, iface.Name, err) - } - - log.Infof("Moving IP address %s (%s) with gateway %s from container to VM", ip.String(), maskString(mask), gwString) - return &net.IPNet{ IP: ip, Mask: mask, - }, gw, false, nil + }, gw, link, false, nil } - return nil, nil, true, fmt.Errorf("interface %s has no valid addresses", iface.Name) + return nil, nil, nil, true, fmt.Errorf("interface %q has no valid addresses", iface.Name) +} + +// takeAddress removes the first address of an interface and returns it and the appropriate gateway +func takeAddress(iface *net.Interface) (*net.IPNet, *net.IP, error) { + + ip, gw, link, noIPs, err := getAddress(iface) + if err != nil { + return nil, nil, err + } + if noIPs { + return nil, nil, fmt.Errorf("interface %q expected to have an IP but none found", iface.Name) + } + + delAddr := &netlink.Addr{ + IPNet: &net.IPNet{ + IP: ip.IP, + Mask: ip.Mask, + }, + } + + if err = netlink.AddrDel(link, delAddr); err != nil { + return nil, nil, fmt.Errorf("failed to remove address %q from interface %q: %v", delAddr, iface.Name, err) + } + + log.Infof("Moving IP address %s (%s) with gateway %s from container to VM", ip.String(), maskString(ip.Mask), gw.String()) + + return ip, gw, nil } // createTAPAdapter creates a new TAP device with the given name diff --git a/pkg/operations/start.go b/pkg/operations/start.go index b67b21fb1..b553fc3c2 100644 --- a/pkg/operations/start.go +++ b/pkg/operations/start.go @@ -105,14 +105,12 @@ func StartVMNonBlocking(vm *api.VM, debug bool) (*VMChannels, error) { PortBindings: vm.Spec.Network.Ports, // Add the port mappings to Docker } - // Only set envVars if at least one was configured var envVars []string - parts := strings.Split(vm.GetAnnotation((constants.IGNITE_SANDBOX_ENV_VAR)), ",") - for _, part := range parts { - if part == "" { - continue + for k, v := range vm.GetObjectMeta().Annotations { + if strings.HasPrefix(k, constants.IGNITE_SANDBOX_ENV_VAR) { + k := strings.TrimPrefix(k, constants.IGNITE_SANDBOX_ENV_VAR) + envVars = append(envVars, fmt.Sprintf("%s=%s", k, v)) } - envVars = append(envVars, part) } config.EnvVars = envVars From 1c70e788f3209af1a65e9b378e33405f4bb989b3 Mon Sep 17 00:00:00 2001 From: networkop Date: Sat, 12 Jun 2021 17:50:46 +0100 Subject: [PATCH 19/25] Tested with containerlab --- pkg/container/network.go | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/pkg/container/network.go b/pkg/container/network.go index b20fdaf36..2c28e99c7 100644 --- a/pkg/container/network.go +++ b/pkg/container/network.go @@ -100,12 +100,18 @@ func collectInterfaces(vmIntfs map[string]string) (bool, error) { } // create a map of candidate interfaces - foundIntfs := make(map[string]*net.Interface) + foundIntfs := make(map[string]net.Interface) for _, intf := range allIntfs { if _, ok := ignoreInterfaces[intf.Name]; ok { continue } - foundIntfs[intf.Name] = &intf + + foundIntfs[intf.Name] = intf + + // If the interface is explictly defined, no changes are needed + if _, ok := vmIntfs[intf.Name]; ok { + continue + } // default fallback behaviour to always consider intfs with an address addrs, _ := intf.Addrs() @@ -122,7 +128,8 @@ func collectInterfaces(vmIntfs map[string]string) (bool, error) { // for DHCP interface, we need to make sure IP and route exist if mode == supportedModes.dhcp { - _, _, _, noIPs, err := getAddress(foundIntfs[intfName]) + intf := foundIntfs[intfName] + _, _, _, noIPs, err := getAddress(&intf) if err != nil { return true, err } @@ -137,13 +144,13 @@ func collectInterfaces(vmIntfs map[string]string) (bool, error) { func networkSetup(fcIntfs *firecracker.NetworkInterfaces, dhcpIntfs *[]DHCPInterface, vmIntfs map[string]string) error { + // The order in which interfaces are plugged in is intentionally deterministic + // All interfaces are sorted alphabetically and 'eth0' is always first var keys []string for k := range vmIntfs { keys = append(keys, k) } sort.Strings(keys) - - // making sure eth0 is always first sort.Slice(keys, func(i, j int) bool { return keys[i] == mainInterface }) From 9957b7099fa3958fb6c503a4d7ca466ad9494b4a Mon Sep 17 00:00:00 2001 From: networkop Date: Mon, 21 Jun 2021 21:56:25 +0100 Subject: [PATCH 20/25] Merging suggested changes --- e2e/multinet_test.go | 12 ++++++------ pkg/container/network.go | 42 ++++++++++++++++++++-------------------- pkg/operations/start.go | 38 +++++++++++++++++++++++------------- 3 files changed, 52 insertions(+), 40 deletions(-) diff --git a/e2e/multinet_test.go b/e2e/multinet_test.go index 93d93130d..dde5e4a1e 100644 --- a/e2e/multinet_test.go +++ b/e2e/multinet_test.go @@ -70,11 +70,11 @@ spec: With("ps"). With("--filter"). With(fmt.Sprintf("{{.ObjectMeta.Name}}=%s", vmName)). - With("--quiet") + With("--template={{.ObjectMeta.UID}}") idOut, idErr := idCmd.Cmd.CombinedOutput() assert.Check(t, idErr, fmt.Sprintf("vm id not found: \n%q\n%s", idCmd.Cmd, idOut)) - vmID := string(idOut[:len(idOut)-2]) + vmID := strings.TrimSuffix(string(idOut), "\n") fooAddr := "aa:ca:e9:12:34:56" dockerCmd.New(). @@ -154,11 +154,11 @@ spec: With("ps"). With("--filter"). With(fmt.Sprintf("{{.ObjectMeta.Name}}=%s", vmName)). - With("--quiet") + With("--template={{.ObjectMeta.UID}}") idOut, idErr := idCmd.Cmd.CombinedOutput() assert.Check(t, idErr, fmt.Sprintf("vm id not found: \n%q\n%s", idCmd.Cmd, idOut)) - vmID := string(idOut[:len(idOut)-2]) + vmID := strings.TrimSuffix(string(idOut), "\n") fooAddr := "aa:ca:e9:12:34:56" dockerCmd.New(). @@ -257,11 +257,11 @@ spec: With("ps"). With("--filter"). With(fmt.Sprintf("{{.ObjectMeta.Name}}=%s", vmName)). - With("--quiet") + With("--template={{.ObjectMeta.UID}}") idOut, idErr := idCmd.Cmd.CombinedOutput() assert.Check(t, idErr, fmt.Sprintf("vm id not found: \n%q\n%s", idCmd.Cmd, idOut)) - vmID := string(idOut[:len(idOut)-2]) + vmID := strings.TrimSuffix(string(idOut), "\n") fooAddr := "aa:ca:e9:12:34:56" dockerCmd.New(). diff --git a/pkg/container/network.go b/pkg/container/network.go index 2c28e99c7..af00e47fd 100644 --- a/pkg/container/network.go +++ b/pkg/container/network.go @@ -33,18 +33,20 @@ ip link set eth0 master br0 ip link set vm0 master br0 */ -// Array of container interfaces to ignore (not forward to vm) +// ignoreInterfaces is a Set of container interface names to ignore (not forward to vm) var ignoreInterfaces = map[string]struct{}{ "lo": {}, "sit0": {}, "tunl0": {}, } -var supportedModes = struct { - dhcp, tc string -}{ - dhcp: "dhcp-bridge", - tc: "tc-redirect", +const MODE_DHCP = "dhcp-bridge" +const MODE_TC = "tc-redirect" + +// supportedModes is a Set of Mode strings +var supportedModes = map[string]struct{}{ + MODE_DHCP: {}, + MODE_TC: {}, } var mainInterface = "eth0" @@ -57,7 +59,7 @@ func SetupContainerNetworking(vm *api.VM) (firecracker.NetworkInterfaces, []DHCP // Setting up mainInterface if not defined if _, ok := vmIntfs[mainInterface]; !ok { - vmIntfs[mainInterface] = supportedModes.dhcp + vmIntfs[mainInterface] = MODE_DHCP } interval := 1 * time.Second @@ -116,7 +118,7 @@ func collectInterfaces(vmIntfs map[string]string) (bool, error) { // default fallback behaviour to always consider intfs with an address addrs, _ := intf.Addrs() if len(addrs) > 0 { - vmIntfs[intf.Name] = supportedModes.dhcp + vmIntfs[intf.Name] = MODE_DHCP } } @@ -127,7 +129,7 @@ func collectInterfaces(vmIntfs map[string]string) (bool, error) { } // for DHCP interface, we need to make sure IP and route exist - if mode == supportedModes.dhcp { + if mode == MODE_DHCP { intf := foundIntfs[intfName] _, _, _, noIPs, err := getAddress(&intf) if err != nil { @@ -163,7 +165,7 @@ func networkSetup(fcIntfs *firecracker.NetworkInterfaces, dhcpIntfs *[]DHCPInter } switch vmIntfs[intfName] { - case supportedModes.dhcp: + case MODE_DHCP: ipNet, gw, err := takeAddress(intf) if err != nil { return fmt.Errorf("error parsing interface %q: %s", intfName, err) @@ -185,7 +187,7 @@ func networkSetup(fcIntfs *firecracker.NetworkInterfaces, dhcpIntfs *[]DHCPInter HostDevName: dhcpIface.VMTAP, }, }) - case supportedModes.tc: + case MODE_TC: tcInterface, err := addTcRedirect(intf) if err != nil { log.Errorf("Failed to setup tc redirect %v", err) @@ -494,22 +496,20 @@ func maskString(mask net.IPMask) string { func parseExtraIntfs(vm *api.VM) map[string]string { result := make(map[string]string) - for k, v := range vm.GetObjectMeta().Annotations { - if !strings.HasPrefix(k, constants.IGNITE_INTERFACE_ANNOTATION) { + for intf, mode := range vm.GetObjectMeta().Annotations { + if !strings.HasPrefix(intf, constants.IGNITE_INTERFACE_ANNOTATION) { continue } - k = strings.TrimPrefix(k, constants.IGNITE_INTERFACE_ANNOTATION) - if k == "" { + intf = strings.TrimPrefix(intf, constants.IGNITE_INTERFACE_ANNOTATION) + if intf == "" { continue } - switch v { - case supportedModes.dhcp: - result[k] = v - case supportedModes.tc: - result[k] = v - default: + if _, ok := supportedModes[mode]; ok { + result[intf] = mode + } else { + log.Warnf("VM specifies unrecognized mode %q for interface %q", mode, intf) continue } diff --git a/pkg/operations/start.go b/pkg/operations/start.go index b553fc3c2..fa33abddb 100644 --- a/pkg/operations/start.go +++ b/pkg/operations/start.go @@ -153,30 +153,31 @@ func StartVMNonBlocking(vm *api.VM, debug bool) (*VMChannels, error) { log.Infof("Started Firecracker VM %q in a container with ID %q", vm.GetUID(), containerID) } - // TODO: This is temporary until we have proper communication to the container - go waitForSpawn(vm, vmChans) - // Set the container ID for the VM vm.Status.Runtime.ID = containerID vm.Status.Runtime.Name = providers.RuntimeName - // Set the start time for the VM - startTime := apiruntime.Timestamp() - vm.Status.StartTime = &startTime - vm.Status.Network.Plugin = providers.NetworkPluginName - // Append non-loopback runtime IP addresses of the VM to its state for _, addr := range result.Addresses { if !addr.IP.IsLoopback() { vm.Status.Network.IPAddresses = append(vm.Status.Network.IPAddresses, addr.IP) } } + vm.Status.Network.Plugin = providers.NetworkPluginName // Set the VM's status to running vm.Status.Running = true - // Write the state changes - return vmChans, providers.Client.VMs().Set(vm) + // write the API object in a non-running state before we wait for spawn's network logic and firecracker + if err := providers.Client.VMs().Set(vm); err != nil { + return vmChans, err + } + + // TODO: This is temporary until we have proper communication to the container + // It's best to perform any imperative changes to the VM object pointer before this go-routine starts + go waitForSpawn(vm, vmChans) + + return vmChans, nil } // verifyPulled pulls the ignite-spawn image if it's not present @@ -202,12 +203,23 @@ func waitForSpawn(vm *api.VM, vmChans *VMChannels) { const timeout = 10 * time.Second const checkInterval = 100 * time.Millisecond - startTime := time.Now() - for time.Since(startTime) < timeout { + timer := time.Now() + for time.Since(timer) < timeout { time.Sleep(checkInterval) if util.FileExists(path.Join(vm.ObjectPath(), constants.PROMETHEUS_SOCKET)) { - vmChans.SpawnFinished <- nil + // Before we write the VM, we should REALLY REALLY re-fetch the API object from storage + vm, err := providers.Client.VMs().Get(vm.GetUID()) + if err != nil { + vmChans.SpawnFinished <- err + } + + // Set the start time for the VM + startTime := apiruntime.Timestamp() + vm.Status.StartTime = &startTime + + // Write the state changes, send any errors through the channel + vmChans.SpawnFinished <- providers.Client.VMs().Set(vm) return } } From 3161ef4e1bdb7e5aa730ff7b09106b59b6f964db Mon Sep 17 00:00:00 2001 From: networkop Date: Tue, 22 Jun 2021 17:29:10 +0100 Subject: [PATCH 21/25] Alternative e2e test --- e2e/multinet_test.go | 140 +++++++++++++++++++++++++++---------------- 1 file changed, 89 insertions(+), 51 deletions(-) diff --git a/e2e/multinet_test.go b/e2e/multinet_test.go index dde5e4a1e..157343adc 100644 --- a/e2e/multinet_test.go +++ b/e2e/multinet_test.go @@ -9,72 +9,105 @@ import ( "testing" "github.com/weaveworks/ignite/e2e/util" + api "github.com/weaveworks/ignite/pkg/apis/ignite" + meta "github.com/weaveworks/ignite/pkg/apis/meta/v1alpha1" + igniteConstants "github.com/weaveworks/ignite/pkg/constants" + "github.com/weaveworks/ignite/pkg/dmlegacy" + "github.com/weaveworks/ignite/pkg/metadata" + "github.com/weaveworks/ignite/pkg/network" + "github.com/weaveworks/ignite/pkg/operations" + "github.com/weaveworks/ignite/pkg/providers" + igniteDocker "github.com/weaveworks/ignite/pkg/providers/docker" + "github.com/weaveworks/ignite/pkg/providers/ignite" + "github.com/weaveworks/ignite/pkg/runtime" + igniteUtil "github.com/weaveworks/ignite/pkg/util" "gotest.tools/assert" ) -// TestMultipleInterface tests that a VM's can be configured with more than 1 interface -func TestOneExtraInterface(t *testing.T) { +var ( + multinetVM = "e2e-test-vm-multinet" + sanboxImage = "weaveworks/ignite:dev" + kernelImage = "weaveworks/ignite-kernel:5.4.108" + vmImage = "weaveworks/ignite-ubuntu" +) + +func startAsyncVM(t *testing.T, intfs []string) (*operations.VMChannels, string) { + assert.Assert(t, e2eHome != "", "IGNITE_E2E_HOME should be set") + igniteUtil.GenericCheckErr(providers.Populate(ignite.Preload)) - vmName := "e2e-test-vm-multinet" + _ = igniteDocker.SetDockerRuntime() + _ = igniteDocker.SetDockerNetwork() - igniteCmd := util.NewCommand(t, igniteBin) - dockerCmd := util.NewCommand(t, "docker") + providers.RuntimeName = runtime.RuntimeDocker + providers.NetworkPluginName = network.PluginDockerBridge + _ = providers.Populate(ignite.Providers) - // Clone this repo in a new dir. - tempDir, err := ioutil.TempDir("", "ignite-multinet") + vm := providers.Client.VMs().New() + vm.Status.Runtime.Name = runtime.RuntimeDocker + vm.Status.Network.Plugin = network.PluginDockerBridge + + ociRef, err := meta.NewOCIImageRef(sanboxImage) if err != nil { - t.Fatalf("failed to create temp dir: %v", err) + t.Fatalf("Failed to parse OCI image ref %s: %s", sanboxImage, err) } - defer os.RemoveAll(tempDir) + vm.Spec.Sandbox.OCI = ociRef - // Write a VM config with annotations + ociRef, err = meta.NewOCIImageRef(kernelImage) + if err != nil { + t.Fatalf("Failed to parse OCI image ref %s: %s", kernelImage, err) + } + vm.Spec.Kernel.OCI = ociRef + k, _ := operations.FindOrImportKernel(providers.Client, ociRef) + vm.SetKernel(k) - vmConfig := []byte(`--- -apiVersion: ignite.weave.works/v1alpha4 -kind: VM -metadata: - name: e2e-test-vm-multinet - annotations: - "ignite.weave.works/interface/foo": "tc-redirect" -spec: - image: - oci: weaveworks/ignite-ubuntu - cpus: 1 - diskSize: 3GB - memory: 800MB - ssh: true -`) + ociRef, err = meta.NewOCIImageRef(vmImage) + if err != nil { + t.Fatalf("Failed to parse OCI image ref %s: %s", vmImage, err) + } + img, err := operations.FindOrImportImage(providers.Client, ociRef) + if err != nil { + t.Fatalf("Failed to find OCI image ref %s: %s", ociRef, err) + } + vm.SetImage(img) - vmConfigPath := filepath.Join(tempDir, "my-vm.yaml") - assert.Check(t, ioutil.WriteFile(vmConfigPath, vmConfig, 0644), "failed to write VM config") + vm.Name = multinetVM + vm.Spec.SSH = &api.SSH{Generate: true} - // Clean-up the following VM. - defer igniteCmd.New(). - With("rm", "-f", vmName). - Run() + _ = metadata.SetNameAndUID(vm, providers.Client) - // Run VM. - igniteCmd.New(). - WithRuntime("docker"). - WithNetwork("docker-bridge"). - With("run"). - With("--ssh"). - With("--debug"). - With("--wait=false"). - With("--config=" + vmConfigPath). - Run() + for _, intf := range intfs { + vm.SetAnnotation(igniteConstants.IGNITE_INTERFACE_ANNOTATION+intf, "tc-redirect") + } - // Get the VM ID - idCmd := igniteCmd.New(). - With("ps"). - With("--filter"). - With(fmt.Sprintf("{{.ObjectMeta.Name}}=%s", vmName)). - With("--template={{.ObjectMeta.UID}}") + _ = providers.Client.VMs().Set(vm) - idOut, idErr := idCmd.Cmd.CombinedOutput() - assert.Check(t, idErr, fmt.Sprintf("vm id not found: \n%q\n%s", idCmd.Cmd, idOut)) - vmID := strings.TrimSuffix(string(idOut), "\n") + err = dmlegacy.AllocateAndPopulateOverlay(vm) + if err != nil { + t.Fatalf("Error AllocateAndPopulateOverlay: %s", err) + } + + vmChans, err := operations.StartVMNonBlocking(vm, false) + if err != nil { + t.Fatalf("failed to start a VM: \n%q\n", err) + } + + return vmChans, vm.GetUID().String() +} + +// TestMultipleInterface tests that a VM's can be configured with more than 1 interface +func TestOneExtraInterface(t *testing.T) { + assert.Assert(t, e2eHome != "", "IGNITE_E2E_HOME should be set") + + igniteCmd := util.NewCommand(t, igniteBin) + dockerCmd := util.NewCommand(t, runtime.RuntimeDocker.String()) + + vmChans, vmID := startAsyncVM(t, []string{"foo"}) + + // Clean-up the following VM. + defer igniteCmd.New(). + With("rm", "-f", multinetVM). + Run() fooAddr := "aa:ca:e9:12:34:56" dockerCmd.New(). @@ -87,8 +120,13 @@ spec: With("ip", "link", "set", "foo", "address", fooAddr). Run() + // check that the VM has started before trying exec + if err := <-vmChans.SpawnFinished; err != nil { + t.Fatalf("failed to start a VM: \n%q\n", err) + } + eth1Addr := igniteCmd.New(). - With("exec", vmName). + With("exec", multinetVM). With("cat", "/sys/class/net/eth1/address") foundEth1Addr, _ := eth1Addr.Cmd.CombinedOutput() From f8d361d3baa697f0a1aeddbe69c7556910cd37c5 Mon Sep 17 00:00:00 2001 From: networkop Date: Tue, 22 Jun 2021 20:12:16 +0100 Subject: [PATCH 22/25] Removed --wait flag and fixed up multinet tests --- cmd/ignite/cmd/cmdutil/flags.go | 4 - cmd/ignite/cmd/vmcmd/start.go | 1 - cmd/ignite/run/start.go | 13 +-- docs/cli/ignite/ignite_run.md | 1 - docs/cli/ignite/ignite_start.md | 1 - docs/cli/ignite/ignite_vm_run.md | 1 - docs/cli/ignite/ignite_vm_start.md | 1 - e2e/multinet_test.go | 135 +++++------------------------ pkg/operations/start.go | 6 +- 9 files changed, 26 insertions(+), 137 deletions(-) diff --git a/cmd/ignite/cmd/cmdutil/flags.go b/cmd/ignite/cmd/cmdutil/flags.go index 1f6276a13..d31712cc8 100644 --- a/cmd/ignite/cmd/cmdutil/flags.go +++ b/cmd/ignite/cmd/cmdutil/flags.go @@ -27,10 +27,6 @@ func AddInteractiveFlag(fs *pflag.FlagSet, interactive *bool) { fs.BoolVarP(interactive, "interactive", "i", *interactive, "Attach to the VM after starting") } -func AddWaitFlag(fs *pflag.FlagSet, wait *bool) { - fs.BoolVarP(wait, "wait", "w", true, "wait for VM to start") -} - func AddForceFlag(fs *pflag.FlagSet, force *bool) { fs.BoolVarP(force, "force", "f", *force, "Force this operation. Warning, use of this mode may have unintended consequences.") } diff --git a/cmd/ignite/cmd/vmcmd/start.go b/cmd/ignite/cmd/vmcmd/start.go index 33c566f1f..7d3bc3af1 100644 --- a/cmd/ignite/cmd/vmcmd/start.go +++ b/cmd/ignite/cmd/vmcmd/start.go @@ -51,7 +51,6 @@ func NewCmdStart(out io.Writer) *cobra.Command { func addStartFlags(fs *pflag.FlagSet, sf *run.StartFlags) { cmdutil.AddInteractiveFlag(fs, &sf.Interactive) - cmdutil.AddWaitFlag(fs, &sf.Wait) fs.BoolVarP(&sf.Debug, "debug", "d", false, "Debug mode, keep container after VM shutdown") fs.StringSliceVar(&sf.IgnoredPreflightErrors, "ignore-preflight-checks", []string{}, "A list of checks whose errors will be shown as warnings. Example: 'BinaryInPath,Port,ExistingFile'. Value 'all' ignores errors from all checks.") } diff --git a/cmd/ignite/run/start.go b/cmd/ignite/run/start.go index 8c191d5c3..4219e83fb 100644 --- a/cmd/ignite/run/start.go +++ b/cmd/ignite/run/start.go @@ -22,7 +22,6 @@ import ( type StartFlags struct { Interactive bool - Wait bool Debug bool IgnoredPreflightErrors []string } @@ -79,20 +78,12 @@ func Start(so *StartOptions, fs *flag.FlagSet) error { return err } - var err error - if so.Wait { - err = operations.StartVM(so.vm, so.Debug) - } else { - // Can't do much with vmChannels here, we need to return ASAP - _, err = operations.StartVMNonBlocking(so.vm, so.Debug) - } - - if err != nil { + if err := operations.StartVM(so.vm, so.Debug); err != nil { return err } // When --ssh is enabled, wait until SSH service started on port 22 at most N seconds - if ssh := so.vm.Spec.SSH; ssh != nil && ssh.Generate && len(so.vm.Status.Network.IPAddresses) > 0 && so.Wait { + if ssh := so.vm.Spec.SSH; ssh != nil && ssh.Generate && len(so.vm.Status.Network.IPAddresses) > 0 { if err := waitForSSH(so.vm, constants.SSH_DEFAULT_TIMEOUT_SECONDS, 5); err != nil { return err } diff --git a/docs/cli/ignite/ignite_run.md b/docs/cli/ignite/ignite_run.md index a5e2f8eef..fd3f8edff 100644 --- a/docs/cli/ignite/ignite_run.md +++ b/docs/cli/ignite/ignite_run.md @@ -48,7 +48,6 @@ ignite run [flags] -s, --size size VM filesystem size, for example 5GB or 2048MB (default 4.0 GB) --ssh[=] Enable SSH for the VM. If is given, it will be imported as the public key. If just '--ssh' is specified, a new keypair will be generated. (default is unset, which disables SSH access to the VM) -v, --volumes volume Expose block devices from the host inside the VM - -w, --wait wait for VM to start (default true) ``` ### Options inherited from parent commands diff --git a/docs/cli/ignite/ignite_start.md b/docs/cli/ignite/ignite_start.md index 07adc828f..00eef2ebb 100644 --- a/docs/cli/ignite/ignite_start.md +++ b/docs/cli/ignite/ignite_start.md @@ -23,7 +23,6 @@ ignite start [flags] -i, --interactive Attach to the VM after starting --network-plugin plugin Network plugin to use. Available options are: [cni docker-bridge] (default cni) --runtime runtime Container runtime to use. Available options are: [docker containerd] (default containerd) - -w, --wait wait for VM to start (default true) ``` ### Options inherited from parent commands diff --git a/docs/cli/ignite/ignite_vm_run.md b/docs/cli/ignite/ignite_vm_run.md index afdcb22d4..a9bdc0d7e 100644 --- a/docs/cli/ignite/ignite_vm_run.md +++ b/docs/cli/ignite/ignite_vm_run.md @@ -48,7 +48,6 @@ ignite vm run [flags] -s, --size size VM filesystem size, for example 5GB or 2048MB (default 4.0 GB) --ssh[=] Enable SSH for the VM. If is given, it will be imported as the public key. If just '--ssh' is specified, a new keypair will be generated. (default is unset, which disables SSH access to the VM) -v, --volumes volume Expose block devices from the host inside the VM - -w, --wait wait for VM to start (default true) ``` ### Options inherited from parent commands diff --git a/docs/cli/ignite/ignite_vm_start.md b/docs/cli/ignite/ignite_vm_start.md index c02615bb7..b6fe63271 100644 --- a/docs/cli/ignite/ignite_vm_start.md +++ b/docs/cli/ignite/ignite_vm_start.md @@ -23,7 +23,6 @@ ignite vm start [flags] -i, --interactive Attach to the VM after starting --network-plugin plugin Network plugin to use. Available options are: [cni docker-bridge] (default cni) --runtime runtime Container runtime to use. Available options are: [docker containerd] (default containerd) - -w, --wait wait for VM to start (default true) ``` ### Options inherited from parent commands diff --git a/e2e/multinet_test.go b/e2e/multinet_test.go index 157343adc..14334f4d0 100644 --- a/e2e/multinet_test.go +++ b/e2e/multinet_test.go @@ -2,9 +2,6 @@ package e2e import ( "fmt" - "io/ioutil" - "os" - "path/filepath" "strings" "testing" @@ -138,66 +135,16 @@ func TestOneExtraInterface(t *testing.T) { func TestMultipleInterface(t *testing.T) { assert.Assert(t, e2eHome != "", "IGNITE_E2E_HOME should be set") - vmName := "e2e-test-vm-multinet" - igniteCmd := util.NewCommand(t, igniteBin) - dockerCmd := util.NewCommand(t, "docker") + dockerCmd := util.NewCommand(t, runtime.RuntimeDocker.String()) - // Clone this repo in a new dir. - tempDir, err := ioutil.TempDir("", "ignite-multinet") - if err != nil { - t.Fatalf("failed to create temp dir: %v", err) - } - defer os.RemoveAll(tempDir) - - // Write a VM config with annotations - - vmConfig := []byte(`--- -apiVersion: ignite.weave.works/v1alpha4 -kind: VM -metadata: - name: e2e-test-vm-multinet - annotations: - "ignite.weave.works/interface/foo": "tc-redirect" - "ignite.weave.works/interface/bar": "tc-redirect" -spec: - image: - oci: weaveworks/ignite-ubuntu - cpus: 1 - diskSize: 3GB - memory: 800MB - ssh: true -`) - - vmConfigPath := filepath.Join(tempDir, "my-vm.yaml") - assert.Check(t, ioutil.WriteFile(vmConfigPath, vmConfig, 0644), "failed to write VM config") + vmChans, vmID := startAsyncVM(t, []string{"foo", "bar"}) // Clean-up the following VM. defer igniteCmd.New(). - With("rm", "-f", vmName). - Run() - - // Run VM. - igniteCmd.New(). - WithRuntime("docker"). - WithNetwork("docker-bridge"). - With("run"). - With("--ssh"). - With("--wait=false"). - With("--config=" + vmConfigPath). + With("rm", "-f", multinetVM). Run() - // Get the VM ID - idCmd := igniteCmd.New(). - With("ps"). - With("--filter"). - With(fmt.Sprintf("{{.ObjectMeta.Name}}=%s", vmName)). - With("--template={{.ObjectMeta.UID}}") - - idOut, idErr := idCmd.Cmd.CombinedOutput() - assert.Check(t, idErr, fmt.Sprintf("vm id not found: \n%q\n%s", idCmd.Cmd, idOut)) - vmID := strings.TrimSuffix(string(idOut), "\n") - fooAddr := "aa:ca:e9:12:34:56" dockerCmd.New(). With("exec", fmt.Sprintf("ignite-%s", vmID)). @@ -220,8 +167,13 @@ spec: With("ip", "link", "set", "bar", "address", barAddr). Run() + // check that the VM has started before trying exec + if err := <-vmChans.SpawnFinished; err != nil { + t.Fatalf("failed to start a VM: \n%q\n", err) + } + eth1Addr := igniteCmd.New(). - With("exec", vmName). + With("exec", multinetVM). With("cat", "/sys/class/net/eth1/address") foundEth1Addr, _ := eth1Addr.Cmd.CombinedOutput() @@ -229,7 +181,7 @@ spec: assert.Check(t, strings.Contains(gotEth1Addr, barAddr), fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", barAddr, gotEth1Addr)) eth2Addr := igniteCmd.New(). - With("exec", vmName). + With("exec", multinetVM). With("cat", "/sys/class/net/eth2/address") foundEth2Addr, _ := eth2Addr.Cmd.CombinedOutput() @@ -241,66 +193,16 @@ spec: func TestMultipleInterfaceImplicit(t *testing.T) { assert.Assert(t, e2eHome != "", "IGNITE_E2E_HOME should be set") - vmName := "e2e-test-vm-multinet" - igniteCmd := util.NewCommand(t, igniteBin) - dockerCmd := util.NewCommand(t, "docker") + dockerCmd := util.NewCommand(t, runtime.RuntimeDocker.String()) - // Clone this repo in a new dir. - tempDir, err := ioutil.TempDir("", "ignite-multinet") - if err != nil { - t.Fatalf("failed to create temp dir: %v", err) - } - defer os.RemoveAll(tempDir) - - // Write a VM config with annotations - - vmConfig := []byte(`--- -apiVersion: ignite.weave.works/v1alpha4 -kind: VM -metadata: - name: e2e-test-vm-multinet - annotations: - "ignite.weave.works/interface/foo": "tc-redirect" - "ignite.weave.works/interface/bar": "tc-redirect" -spec: - image: - oci: weaveworks/ignite-ubuntu - cpus: 1 - diskSize: 3GB - memory: 800MB - ssh: true -`) - - vmConfigPath := filepath.Join(tempDir, "my-vm.yaml") - assert.Check(t, ioutil.WriteFile(vmConfigPath, vmConfig, 0644), "failed to write VM config") + vmChans, vmID := startAsyncVM(t, []string{"foo", "bar"}) // Clean-up the following VM. defer igniteCmd.New(). - With("rm", "-f", vmName). - Run() - - // Run VM. - igniteCmd.New(). - WithRuntime("docker"). - WithNetwork("docker-bridge"). - With("run"). - With("--ssh"). - With("--wait=false"). - With("--config=" + vmConfigPath). + With("rm", "-f", multinetVM). Run() - // Get the VM ID - idCmd := igniteCmd.New(). - With("ps"). - With("--filter"). - With(fmt.Sprintf("{{.ObjectMeta.Name}}=%s", vmName)). - With("--template={{.ObjectMeta.UID}}") - - idOut, idErr := idCmd.Cmd.CombinedOutput() - assert.Check(t, idErr, fmt.Sprintf("vm id not found: \n%q\n%s", idCmd.Cmd, idOut)) - vmID := strings.TrimSuffix(string(idOut), "\n") - fooAddr := "aa:ca:e9:12:34:56" dockerCmd.New(). With("exec", fmt.Sprintf("ignite-%s", vmID)). @@ -335,8 +237,13 @@ spec: With("ip", "link", "set", "baz", "address", bazAddr). Run() + // check that the VM has started before trying exec + if err := <-vmChans.SpawnFinished; err != nil { + t.Fatalf("failed to start a VM: \n%q\n", err) + } + eth1Addr := igniteCmd.New(). - With("exec", vmName). + With("exec", multinetVM). With("cat", "/sys/class/net/eth1/address") foundEth1Addr, _ := eth1Addr.Cmd.CombinedOutput() @@ -344,7 +251,7 @@ spec: assert.Check(t, strings.Contains(gotEth1Addr, barAddr), fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", barAddr, gotEth1Addr)) eth2Addr := igniteCmd.New(). - With("exec", vmName). + With("exec", multinetVM). With("cat", "/sys/class/net/eth2/address") foundEth2Addr, _ := eth2Addr.Cmd.CombinedOutput() @@ -352,7 +259,7 @@ spec: assert.Check(t, strings.Contains(gotEth2Addr, fooAddr), fmt.Sprintf("unexpected address found:\n\t(WNT): %q\n\t(GOT): %q", fooAddr, gotEth2Addr)) eth3Addr := igniteCmd.New(). - With("exec", vmName). + With("exec", multinetVM). With("cat", "/sys/class/net/eth3/address") _, foundEth3Err := eth3Addr.Cmd.CombinedOutput() diff --git a/pkg/operations/start.go b/pkg/operations/start.go index fa33abddb..39a51e590 100644 --- a/pkg/operations/start.go +++ b/pkg/operations/start.go @@ -165,9 +165,6 @@ func StartVMNonBlocking(vm *api.VM, debug bool) (*VMChannels, error) { } vm.Status.Network.Plugin = providers.NetworkPluginName - // Set the VM's status to running - vm.Status.Running = true - // write the API object in a non-running state before we wait for spawn's network logic and firecracker if err := providers.Client.VMs().Set(vm); err != nil { return vmChans, err @@ -214,6 +211,9 @@ func waitForSpawn(vm *api.VM, vmChans *VMChannels) { vmChans.SpawnFinished <- err } + // Set the VM's status to running + vm.Status.Running = true + // Set the start time for the VM startTime := apiruntime.Timestamp() vm.Status.StartTime = &startTime From 668b5fe42a4dfd11ab3908540dd6349f9376b7c6 Mon Sep 17 00:00:00 2001 From: networkop Date: Thu, 24 Jun 2021 15:07:25 +0100 Subject: [PATCH 23/25] Removing waitForSSH from exec --- cmd/ignite/run/exec.go | 4 ---- 1 file changed, 4 deletions(-) diff --git a/cmd/ignite/run/exec.go b/cmd/ignite/run/exec.go index 531f3b634..5e6882b04 100644 --- a/cmd/ignite/run/exec.go +++ b/cmd/ignite/run/exec.go @@ -2,7 +2,6 @@ package run import ( api "github.com/weaveworks/ignite/pkg/apis/ignite" - "github.com/weaveworks/ignite/pkg/constants" ) // ExecFlags contains the flags supported by the exec command. @@ -31,8 +30,5 @@ func (ef *ExecFlags) NewExecOptions(vmMatch string, command ...string) (eo *Exec // Exec executes command in a VM based on the provided ExecOptions. func Exec(eo *ExecOptions) error { - if err := waitForSSH(eo.vm, constants.SSH_DEFAULT_TIMEOUT_SECONDS, 5); err != nil { - return err - } return runSSH(eo.vm, eo.IdentityFile, eo.command, eo.Tty, eo.Timeout) } From 71c801c62ea437da1b29c1063947158c48d9b7e6 Mon Sep 17 00:00:00 2001 From: networkop Date: Thu, 24 Jun 2021 15:39:01 +0100 Subject: [PATCH 24/25] Removing leftover comment --- pkg/container/network.go | 1 - 1 file changed, 1 deletion(-) diff --git a/pkg/container/network.go b/pkg/container/network.go index af00e47fd..0641bee3b 100644 --- a/pkg/container/network.go +++ b/pkg/container/network.go @@ -70,7 +70,6 @@ func SetupContainerNetworking(vm *api.VM) (firecracker.NetworkInterfaces, []DHCP // This func returns true if it's done, and optionally an error retry, err := collectInterfaces(vmIntfs) - // if err == nil { // We're done here return true, nil From abddda2b2b98922b8addff92510b33a679dc17b1 Mon Sep 17 00:00:00 2001 From: networkop Date: Thu, 24 Jun 2021 16:41:54 +0100 Subject: [PATCH 25/25] Adding waitforSSH back --- cmd/ignite/run/exec.go | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/cmd/ignite/run/exec.go b/cmd/ignite/run/exec.go index 5e6882b04..046cb9504 100644 --- a/cmd/ignite/run/exec.go +++ b/cmd/ignite/run/exec.go @@ -2,6 +2,7 @@ package run import ( api "github.com/weaveworks/ignite/pkg/apis/ignite" + "github.com/weaveworks/ignite/pkg/constants" ) // ExecFlags contains the flags supported by the exec command. @@ -30,5 +31,8 @@ func (ef *ExecFlags) NewExecOptions(vmMatch string, command ...string) (eo *Exec // Exec executes command in a VM based on the provided ExecOptions. func Exec(eo *ExecOptions) error { + if err := waitForSSH(eo.vm, constants.SSH_DEFAULT_TIMEOUT_SECONDS, int(eo.Timeout)); err != nil { + return err + } return runSSH(eo.vm, eo.IdentityFile, eo.command, eo.Tty, eo.Timeout) }