diff --git a/spinifex/daemon/daemon_handlers_eni_test.go b/spinifex/daemon/daemon_handlers_eni_test.go new file mode 100644 index 000000000..a055db7f1 --- /dev/null +++ b/spinifex/daemon/daemon_handlers_eni_test.go @@ -0,0 +1,441 @@ +package daemon + +import ( + "encoding/json" + "errors" + "fmt" + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + handlers_ec2_vpc "github.com/mulgadc/spinifex/spinifex/handlers/ec2/vpc" + "github.com/mulgadc/spinifex/spinifex/qmp" + spxtypes "github.com/mulgadc/spinifex/spinifex/types" + "github.com/mulgadc/spinifex/spinifex/utils" + "github.com/mulgadc/spinifex/spinifex/vm" + "github.com/nats-io/nats.go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// --- eniHotplugErrorCode mapping --- + +// TestENIHotplugErrorCode locks down the vm.Manager error → AWS API code +// mapping. Wrong mapping silently breaks AWS-SDK retry semantics: clients +// expect 4xx codes for caller-fixable problems and 5xx for server faults. +func TestENIHotplugErrorCode(t *testing.T) { + tests := []struct { + name string + err error + want string + }{ + {"ErrInstanceNotFound", vm.ErrInstanceNotFound, awserrors.ErrorInvalidInstanceIDNotFound}, + {"wrapped ErrInstanceNotFound", fmt.Errorf("ctx: %w", vm.ErrInstanceNotFound), awserrors.ErrorInvalidInstanceIDNotFound}, + {"ErrInvalidTransition", vm.ErrInvalidTransition, awserrors.ErrorIncorrectInstanceState}, + {"ErrAttachmentLimitExceeded", vm.ErrAttachmentLimitExceeded, awserrors.ErrorAttachmentLimitExceeded}, + {"ErrENINotAttached", vm.ErrENINotAttached, awserrors.ErrorInvalidAttachmentIDNotFound}, + {"wrapped ErrENINotAttached", fmt.Errorf("%w: eni-x", vm.ErrENINotAttached), awserrors.ErrorInvalidAttachmentIDNotFound}, + {"ErrQMPUnavailable", vm.ErrQMPUnavailable, awserrors.ErrorServerInternal}, + {"ErrENIPipelineTimeout", vm.ErrENIPipelineTimeout, awserrors.ErrorServerInternal}, + {"unknown error", errors.New("network unreachable"), awserrors.ErrorServerInternal}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + assert.Equal(t, tt.want, eniHotplugErrorCode(tt.err)) + }) + } +} + +// --- publishENIHotplugEvent --- + +func TestPublishENIHotplugEvent_NilConnNoop(t *testing.T) { + publishENIHotplugEvent(nil, "vpc.eni-hotplug.attached", "i-x", map[string]any{"k": "v"}) +} + +func TestPublishENIHotplugEvent_Publishes(t *testing.T) { + d := createVPCTestDaemon(t) + + got := make(chan *nats.Msg, 1) + sub, err := d.natsConn.SubscribeSync("vpc.eni-hotplug.attached.i-evt") + require.NoError(t, err) + t.Cleanup(func() { _ = sub.Unsubscribe() }) + go func() { + msg, _ := sub.NextMsg(2 * time.Second) + got <- msg + }() + + publishENIHotplugEvent(d.natsConn, "vpc.eni-hotplug.attached", "i-evt", map[string]any{ + "eniId": "eni-evt", + "slot": 1.0, + }) + + msg := <-got + require.NotNil(t, msg) + var payload map[string]any + require.NoError(t, json.Unmarshal(msg.Data, &payload)) + assert.Equal(t, "eni-evt", payload["eniId"]) + assert.Equal(t, 1.0, payload["slot"]) +} + +// --- shared fixture --- + +// eniHotPlugFixture builds a daemon with vpcService + a running VM +// registered in vmMgr, plus a stub DeviceController bound via +// vm.SetHotPlugTestSeams. The returned ENI record is already in the KV +// in "available" state and the VM has 4 free hot-plug slots. +type eniHotPlugFixture struct { + daemon *Daemon + vmInst *vm.VM + stub *vm.StubDeviceController + subnetID string + vpcID string + eniID string + mac string +} + +func newENIHotPlugFixture(t *testing.T) *eniHotPlugFixture { + t.Helper() + d := createVPCTestDaemon(t) + + vpcOut, err := d.vpcService.CreateVpc(&ec2.CreateVpcInput{ + CidrBlock: aws.String("10.0.0.0/16"), + }, testAccountID) + require.NoError(t, err) + subnetOut, err := d.vpcService.CreateSubnet(&ec2.CreateSubnetInput{ + VpcId: vpcOut.Vpc.VpcId, + CidrBlock: aws.String("10.0.1.0/24"), + }, testAccountID) + require.NoError(t, err) + eniOut, err := d.vpcService.CreateNetworkInterface(&ec2.CreateNetworkInterfaceInput{ + SubnetId: subnetOut.Subnet.SubnetId, + }, testAccountID) + require.NoError(t, err) + + rec, err := d.vpcService.GetENIRecord(testAccountID, *eniOut.NetworkInterface.NetworkInterfaceId) + require.NoError(t, err) + + v := &vm.VM{ + ID: "i-hp-test", + Status: vm.StateRunning, + QMPClient: &qmp.QMPClient{}, + ENIRequests: spxtypes.ENIRequests{ + AvailableSlots: []int{1, 2, 3, 4}, + AttachedByENIID: map[string]int{}, + }, + } + d.vmMgr.Insert(v) + + stub := vm.NewStubDeviceController() + restore := vm.SetHotPlugTestSeams( + func(*vm.VM) vm.DeviceController { return stub }, + func(time.Duration) {}, + ) + t.Cleanup(restore) + + return &eniHotPlugFixture{ + daemon: d, + vmInst: v, + stub: stub, + subnetID: *subnetOut.Subnet.SubnetId, + vpcID: *vpcOut.Vpc.VpcId, + eniID: rec.NetworkInterfaceId, + mac: rec.MacAddress, + } +} + +// driveHandler subscribes to subject, dispatches the supplied handler +// from the subscriber, and returns the reply payload. Mirrors how the +// daemon dispatches AttachENI / DetachENI off the ec2.cmd channel in +// production. +func driveHandler(t *testing.T, nc *nats.Conn, subject string, handler func(*nats.Msg)) []byte { + t.Helper() + sub, err := nc.Subscribe(subject, handler) + require.NoError(t, err) + defer func() { _ = sub.Unsubscribe() }() + + reqMsg := nats.NewMsg(subject) + reqMsg.Header.Set(utils.AccountIDHeader, testAccountID) + reply, err := nc.RequestMsg(reqMsg, 5*time.Second) + require.NoError(t, err) + return reply.Data +} + +// assertErrorCode is shared with daemon_handlers_eks_test.go (same package). + +// --- handleAttachNetworkInterface --- + +func TestHandleAttachNetworkInterface_NilData(t *testing.T) { + f := newENIHotPlugFixture(t) + cmd := spxtypes.EC2InstanceCommand{ID: f.vmInst.ID} + payload := driveHandler(t, f.daemon.natsConn, "test.attach.nildata", func(msg *nats.Msg) { + f.daemon.handleAttachNetworkInterface(msg, cmd, f.vmInst) + }) + assertErrorCode(t, payload, awserrors.ErrorInvalidParameterValue) +} + +func TestHandleAttachNetworkInterface_EmptyEniID(t *testing.T) { + f := newENIHotPlugFixture(t) + cmd := spxtypes.EC2InstanceCommand{ + ID: f.vmInst.ID, + AttachENIData: &spxtypes.AttachENIData{NetworkInterfaceID: "", DeviceIndex: 0}, + } + payload := driveHandler(t, f.daemon.natsConn, "test.attach.emptyid", func(msg *nats.Msg) { + f.daemon.handleAttachNetworkInterface(msg, cmd, f.vmInst) + }) + assertErrorCode(t, payload, awserrors.ErrorInvalidParameterValue) +} + +func TestHandleAttachNetworkInterface_NotRunning(t *testing.T) { + f := newENIHotPlugFixture(t) + f.vmInst.Status = vm.StateStopped + cmd := spxtypes.EC2InstanceCommand{ + ID: f.vmInst.ID, + AttachENIData: &spxtypes.AttachENIData{NetworkInterfaceID: f.eniID, DeviceIndex: 1}, + } + payload := driveHandler(t, f.daemon.natsConn, "test.attach.notrunning", func(msg *nats.Msg) { + f.daemon.handleAttachNetworkInterface(msg, cmd, f.vmInst) + }) + assertErrorCode(t, payload, awserrors.ErrorIncorrectInstanceState) +} + +func TestHandleAttachNetworkInterface_ENINotFound(t *testing.T) { + f := newENIHotPlugFixture(t) + cmd := spxtypes.EC2InstanceCommand{ + ID: f.vmInst.ID, + AttachENIData: &spxtypes.AttachENIData{NetworkInterfaceID: "eni-missing", DeviceIndex: 1}, + } + payload := driveHandler(t, f.daemon.natsConn, "test.attach.eninotfound", func(msg *nats.Msg) { + f.daemon.handleAttachNetworkInterface(msg, cmd, f.vmInst) + }) + assertErrorCode(t, payload, awserrors.ErrorInvalidNetworkInterfaceIDNotFound) +} + +func TestHandleAttachNetworkInterface_WrongOwner(t *testing.T) { + f := newENIHotPlugFixture(t) + _, err := f.daemon.vpcService.AttachENI(testAccountID, f.eniID, "i-other-instance", 0) + require.NoError(t, err) + + cmd := spxtypes.EC2InstanceCommand{ + ID: f.vmInst.ID, + AttachENIData: &spxtypes.AttachENIData{NetworkInterfaceID: f.eniID, DeviceIndex: 1}, + } + payload := driveHandler(t, f.daemon.natsConn, "test.attach.wrongowner", func(msg *nats.Msg) { + f.daemon.handleAttachNetworkInterface(msg, cmd, f.vmInst) + }) + assertErrorCode(t, payload, awserrors.ErrorInvalidNetworkInterfaceInUse) +} + +func TestHandleAttachNetworkInterface_Success(t *testing.T) { + f := newENIHotPlugFixture(t) + + gotEvent := make(chan []byte, 1) + evtSub, err := f.daemon.natsConn.Subscribe("vpc.eni-hotplug.attached."+f.vmInst.ID, func(msg *nats.Msg) { + gotEvent <- msg.Data + }) + require.NoError(t, err) + t.Cleanup(func() { _ = evtSub.Unsubscribe() }) + + cmd := spxtypes.EC2InstanceCommand{ + ID: f.vmInst.ID, + AttachENIData: &spxtypes.AttachENIData{NetworkInterfaceID: f.eniID, DeviceIndex: 1}, + } + payload := driveHandler(t, f.daemon.natsConn, "test.attach.success", func(msg *nats.Msg) { + f.daemon.handleAttachNetworkInterface(msg, cmd, f.vmInst) + }) + + var out ec2.AttachNetworkInterfaceOutput + require.NoError(t, json.Unmarshal(payload, &out)) + require.NotNil(t, out.AttachmentId) + assert.Contains(t, *out.AttachmentId, "eni-attach-") + + rec, err := f.daemon.vpcService.GetENIRecord(testAccountID, f.eniID) + require.NoError(t, err) + assert.Equal(t, "attached", rec.AttachmentStatus) + assert.Equal(t, 1, rec.HotPlugSlot) + assert.Equal(t, f.vmInst.ID, rec.InstanceId) + + assert.True(t, f.stub.HasDevice("net-eni-1")) + assert.True(t, f.stub.HasNetdev("hostnet-eni-1")) + + select { + case evtData := <-gotEvent: + var evt map[string]any + require.NoError(t, json.Unmarshal(evtData, &evt)) + assert.Equal(t, f.eniID, evt["eniId"]) + assert.Equal(t, float64(1), evt["hotPlugSlot"]) + case <-time.After(1 * time.Second): + t.Fatal("expected vpc.eni-hotplug.attached event, none received") + } +} + +func TestHandleAttachNetworkInterface_HotPlugFails_RollsBackKV(t *testing.T) { + f := newENIHotPlugFixture(t) + f.stub.SetFailNext("device_add", errors.New("simulated QMP failure")) + + cmd := spxtypes.EC2InstanceCommand{ + ID: f.vmInst.ID, + AttachENIData: &spxtypes.AttachENIData{NetworkInterfaceID: f.eniID, DeviceIndex: 1}, + } + payload := driveHandler(t, f.daemon.natsConn, "test.attach.hotplugfails", func(msg *nats.Msg) { + f.daemon.handleAttachNetworkInterface(msg, cmd, f.vmInst) + }) + assertErrorCode(t, payload, awserrors.ErrorServerInternal) + + rec, err := f.daemon.vpcService.GetENIRecord(testAccountID, f.eniID) + require.NoError(t, err) + assert.Equal(t, "available", rec.Status, "KV should have rolled back to available") + assert.Empty(t, rec.AttachmentStatus, "AttachmentStatus should be cleared on rollback") + assert.Empty(t, rec.InstanceId) + assert.NotEmpty(t, rec.LastAttachError, "LastAttachError should record the rollback reason") +} + +// --- handleDetachNetworkInterface --- + +// attachedFixture extends the base fixture by pre-attaching the ENI to +// the running VM via the full handler, so detach tests start from the +// realistic post-attach state. +func attachedFixture(t *testing.T) (*eniHotPlugFixture, string) { + t.Helper() + f := newENIHotPlugFixture(t) + attachID, err := f.daemon.vpcService.AttachENI(testAccountID, f.eniID, f.vmInst.ID, 1) + require.NoError(t, err) + require.NoError(t, f.daemon.vpcService.UpdateENI(testAccountID, f.eniID, func(r *handlers_ec2_vpc.ENIRecord) { + r.AttachmentStatus = "attached" + r.HotPlugSlot = 1 + })) + f.vmInst.ENIRequests.Mu.Lock() + f.vmInst.ENIRequests.AvailableSlots = []int{2, 3, 4} + f.vmInst.ENIRequests.AttachedByENIID[f.eniID] = 1 + f.vmInst.ENIRequests.Mu.Unlock() + // Pre-seed the stub so HotUnplugENI's device_del + query-pci absence + // poll has something to remove. + require.NoError(t, f.stub.NetdevAdd(map[string]any{"id": "hostnet-eni-1"})) + require.NoError(t, f.stub.DeviceAdd(map[string]any{"id": "net-eni-1", "bus": "hotplug-eni1"})) + return f, attachID +} + +func TestHandleDetachNetworkInterface_NilData(t *testing.T) { + f, _ := attachedFixture(t) + cmd := spxtypes.EC2InstanceCommand{ID: f.vmInst.ID} + payload := driveHandler(t, f.daemon.natsConn, "test.detach.nildata", func(msg *nats.Msg) { + f.daemon.handleDetachNetworkInterface(msg, cmd, f.vmInst) + }) + assertErrorCode(t, payload, awserrors.ErrorInvalidParameterValue) +} + +func TestHandleDetachNetworkInterface_EmptyAttachID(t *testing.T) { + f, _ := attachedFixture(t) + cmd := spxtypes.EC2InstanceCommand{ + ID: f.vmInst.ID, + DetachENIData: &spxtypes.DetachENIData{AttachmentID: ""}, + } + payload := driveHandler(t, f.daemon.natsConn, "test.detach.emptyid", func(msg *nats.Msg) { + f.daemon.handleDetachNetworkInterface(msg, cmd, f.vmInst) + }) + assertErrorCode(t, payload, awserrors.ErrorInvalidParameterValue) +} + +func TestHandleDetachNetworkInterface_UnknownAttachment(t *testing.T) { + f, _ := attachedFixture(t) + cmd := spxtypes.EC2InstanceCommand{ + ID: f.vmInst.ID, + DetachENIData: &spxtypes.DetachENIData{AttachmentID: "eni-attach-missing"}, + } + payload := driveHandler(t, f.daemon.natsConn, "test.detach.unknown", func(msg *nats.Msg) { + f.daemon.handleDetachNetworkInterface(msg, cmd, f.vmInst) + }) + assertErrorCode(t, payload, awserrors.ErrorInvalidAttachmentIDNotFound) +} + +func TestHandleDetachNetworkInterface_WrongOwner(t *testing.T) { + f, attachID := attachedFixture(t) + otherVM := &vm.VM{ + ID: "i-other", + Status: vm.StateRunning, + ENIRequests: spxtypes.ENIRequests{ + AvailableSlots: []int{1}, + AttachedByENIID: map[string]int{}, + }, + } + f.daemon.vmMgr.Insert(otherVM) + cmd := spxtypes.EC2InstanceCommand{ + ID: otherVM.ID, + DetachENIData: &spxtypes.DetachENIData{AttachmentID: attachID}, + } + payload := driveHandler(t, f.daemon.natsConn, "test.detach.wrongowner", func(msg *nats.Msg) { + f.daemon.handleDetachNetworkInterface(msg, cmd, otherVM) + }) + assertErrorCode(t, payload, awserrors.ErrorInvalidAttachmentIDNotFound) +} + +func TestHandleDetachNetworkInterface_NotRunning(t *testing.T) { + f, attachID := attachedFixture(t) + f.vmInst.Status = vm.StateStopped + cmd := spxtypes.EC2InstanceCommand{ + ID: f.vmInst.ID, + DetachENIData: &spxtypes.DetachENIData{AttachmentID: attachID}, + } + payload := driveHandler(t, f.daemon.natsConn, "test.detach.notrunning", func(msg *nats.Msg) { + f.daemon.handleDetachNetworkInterface(msg, cmd, f.vmInst) + }) + assertErrorCode(t, payload, awserrors.ErrorIncorrectInstanceState) +} + +func TestHandleDetachNetworkInterface_Success(t *testing.T) { + f, attachID := attachedFixture(t) + + gotEvent := make(chan []byte, 1) + evtSub, err := f.daemon.natsConn.Subscribe("vpc.eni-hotplug.detached."+f.vmInst.ID, func(msg *nats.Msg) { + gotEvent <- msg.Data + }) + require.NoError(t, err) + t.Cleanup(func() { _ = evtSub.Unsubscribe() }) + + cmd := spxtypes.EC2InstanceCommand{ + ID: f.vmInst.ID, + DetachENIData: &spxtypes.DetachENIData{AttachmentID: attachID, Force: false}, + } + payload := driveHandler(t, f.daemon.natsConn, "test.detach.success", func(msg *nats.Msg) { + f.daemon.handleDetachNetworkInterface(msg, cmd, f.vmInst) + }) + var out ec2.DetachNetworkInterfaceOutput + require.NoError(t, json.Unmarshal(payload, &out)) + + rec, err := f.daemon.vpcService.GetENIRecord(testAccountID, f.eniID) + require.NoError(t, err) + assert.Equal(t, "available", rec.Status) + assert.Empty(t, rec.AttachmentStatus) + assert.False(t, rec.DetachInFlight) + assert.False(t, rec.DetachForce) + assert.Equal(t, 0, rec.HotPlugSlot) + + assert.False(t, f.stub.HasDevice("net-eni-1")) + + select { + case <-gotEvent: + case <-time.After(1 * time.Second): + t.Fatal("expected vpc.eni-hotplug.detached event, none received") + } +} + +func TestHandleDetachNetworkInterface_HotUnplugFails(t *testing.T) { + f, attachID := attachedFixture(t) + f.stub.SetFailNext("device_del", errors.New("simulated QMP failure")) + + cmd := spxtypes.EC2InstanceCommand{ + ID: f.vmInst.ID, + DetachENIData: &spxtypes.DetachENIData{AttachmentID: attachID, Force: false}, + } + payload := driveHandler(t, f.daemon.natsConn, "test.detach.hotunplugfails", func(msg *nats.Msg) { + f.daemon.handleDetachNetworkInterface(msg, cmd, f.vmInst) + }) + assertErrorCode(t, payload, awserrors.ErrorServerInternal) + + rec, err := f.daemon.vpcService.GetENIRecord(testAccountID, f.eniID) + require.NoError(t, err) + assert.Equal(t, "in-use", rec.Status, "KV should remain in-use after detach failure") + assert.Equal(t, "detaching", rec.AttachmentStatus, "AttachmentStatus stays detaching for reconciler pickup") + assert.False(t, rec.DetachInFlight, "DetachInFlight cleared so reconciler can re-claim") +} diff --git a/spinifex/gateway/ec2/vpc/network_interface_hotplug_test.go b/spinifex/gateway/ec2/vpc/network_interface_hotplug_test.go new file mode 100644 index 000000000..e1cef6798 --- /dev/null +++ b/spinifex/gateway/ec2/vpc/network_interface_hotplug_test.go @@ -0,0 +1,324 @@ +package gateway_ec2_vpc + +import ( + "encoding/json" + "testing" + "time" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/mulgadc/spinifex/spinifex/awserrors" + "github.com/mulgadc/spinifex/spinifex/types" + "github.com/mulgadc/spinifex/spinifex/utils" + "github.com/nats-io/nats-server/v2/server" + "github.com/nats-io/nats.go" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func newTestNATS(t *testing.T) *nats.Conn { + t.Helper() + opts := &server.Options{ + Host: "127.0.0.1", + Port: -1, + NoLog: true, + NoSigs: true, + } + ns, err := server.NewServer(opts) + require.NoError(t, err) + go ns.Start() + require.True(t, ns.ReadyForConnections(5*time.Second)) + t.Cleanup(func() { ns.Shutdown() }) + + nc, err := nats.Connect(ns.ClientURL()) + require.NoError(t, err) + t.Cleanup(func() { nc.Close() }) + return nc +} + +// describeNetworkInterfacesResponder seeds a DescribeNetworkInterfaces NATS +// responder that returns the supplied attachment-id → instance-id mapping. +func describeNetworkInterfacesResponder(t *testing.T, nc *nats.Conn, attachID, instanceID, eniID string) { + t.Helper() + sub, err := nc.Subscribe("ec2.DescribeNetworkInterfaces", func(msg *nats.Msg) { + resp := ec2.DescribeNetworkInterfacesOutput{ + NetworkInterfaces: []*ec2.NetworkInterface{ + { + NetworkInterfaceId: aws.String(eniID), + Attachment: &ec2.NetworkInterfaceAttachment{ + AttachmentId: aws.String(attachID), + InstanceId: aws.String(instanceID), + }, + }, + }, + } + data, _ := json.Marshal(resp) + _ = msg.Respond(data) + }) + require.NoError(t, err) + t.Cleanup(func() { _ = sub.Unsubscribe() }) +} + +// --- AttachNetworkInterface --- + +func TestAttachNetworkInterface_NilInput(t *testing.T) { + _, err := AttachNetworkInterface(nil, nil, testAccountID) + assert.EqualError(t, err, awserrors.ErrorInvalidParameterValue) +} + +func TestAttachNetworkInterface_MissingNetworkInterfaceID(t *testing.T) { + _, err := AttachNetworkInterface(&ec2.AttachNetworkInterfaceInput{ + InstanceId: aws.String("i-abc"), + DeviceIndex: aws.Int64(0), + }, nil, testAccountID) + assert.EqualError(t, err, awserrors.ErrorMissingParameter) +} + +func TestAttachNetworkInterface_EmptyNetworkInterfaceID(t *testing.T) { + _, err := AttachNetworkInterface(&ec2.AttachNetworkInterfaceInput{ + NetworkInterfaceId: aws.String(""), + InstanceId: aws.String("i-abc"), + DeviceIndex: aws.Int64(0), + }, nil, testAccountID) + assert.EqualError(t, err, awserrors.ErrorMissingParameter) +} + +func TestAttachNetworkInterface_MissingInstanceID(t *testing.T) { + _, err := AttachNetworkInterface(&ec2.AttachNetworkInterfaceInput{ + NetworkInterfaceId: aws.String("eni-1"), + DeviceIndex: aws.Int64(0), + }, nil, testAccountID) + assert.EqualError(t, err, awserrors.ErrorMissingParameter) +} + +func TestAttachNetworkInterface_MissingDeviceIndex(t *testing.T) { + _, err := AttachNetworkInterface(&ec2.AttachNetworkInterfaceInput{ + NetworkInterfaceId: aws.String("eni-1"), + InstanceId: aws.String("i-abc"), + }, nil, testAccountID) + assert.EqualError(t, err, awserrors.ErrorMissingParameter) +} + +func TestAttachNetworkInterface_NoResponders(t *testing.T) { + nc := newTestNATS(t) + _, err := AttachNetworkInterface(&ec2.AttachNetworkInterfaceInput{ + NetworkInterfaceId: aws.String("eni-1"), + InstanceId: aws.String("i-no-responder"), + DeviceIndex: aws.Int64(0), + }, nc, testAccountID) + assert.EqualError(t, err, awserrors.ErrorInvalidInstanceIDNotFound) +} + +func TestAttachNetworkInterface_Success(t *testing.T) { + nc := newTestNATS(t) + + var received types.EC2InstanceCommand + var receivedAccount string + sub, err := nc.Subscribe("ec2.cmd.i-success", func(msg *nats.Msg) { + receivedAccount = msg.Header.Get(utils.AccountIDHeader) + _ = json.Unmarshal(msg.Data, &received) + data, _ := json.Marshal(ec2.AttachNetworkInterfaceOutput{ + AttachmentId: aws.String("eni-attach-xyz"), + }) + _ = msg.Respond(data) + }) + require.NoError(t, err) + t.Cleanup(func() { _ = sub.Unsubscribe() }) + + out, err := AttachNetworkInterface(&ec2.AttachNetworkInterfaceInput{ + NetworkInterfaceId: aws.String("eni-feedface"), + InstanceId: aws.String("i-success"), + DeviceIndex: aws.Int64(2), + }, nc, testAccountID) + + require.NoError(t, err) + require.NotNil(t, out.AttachmentId) + assert.Equal(t, "eni-attach-xyz", *out.AttachmentId) + assert.Equal(t, testAccountID, receivedAccount) + assert.True(t, received.Attributes.AttachENI) + require.NotNil(t, received.AttachENIData) + assert.Equal(t, "eni-feedface", received.AttachENIData.NetworkInterfaceID) + assert.Equal(t, int64(2), received.AttachENIData.DeviceIndex) + assert.Equal(t, "i-success", received.ID) +} + +func TestAttachNetworkInterface_DaemonErrorResponse(t *testing.T) { + nc := newTestNATS(t) + + sub, err := nc.Subscribe("ec2.cmd.i-err", func(msg *nats.Msg) { + _ = msg.Respond(utils.GenerateErrorPayload(awserrors.ErrorIncorrectInstanceState)) + }) + require.NoError(t, err) + t.Cleanup(func() { _ = sub.Unsubscribe() }) + + _, err = AttachNetworkInterface(&ec2.AttachNetworkInterfaceInput{ + NetworkInterfaceId: aws.String("eni-bad"), + InstanceId: aws.String("i-err"), + DeviceIndex: aws.Int64(0), + }, nc, testAccountID) + assert.EqualError(t, err, awserrors.ErrorIncorrectInstanceState) +} + +func TestAttachNetworkInterface_MalformedDaemonResponse(t *testing.T) { + nc := newTestNATS(t) + + sub, err := nc.Subscribe("ec2.cmd.i-malformed", func(msg *nats.Msg) { + _ = msg.Respond([]byte("not-json")) + }) + require.NoError(t, err) + t.Cleanup(func() { _ = sub.Unsubscribe() }) + + _, err = AttachNetworkInterface(&ec2.AttachNetworkInterfaceInput{ + NetworkInterfaceId: aws.String("eni-bad"), + InstanceId: aws.String("i-malformed"), + DeviceIndex: aws.Int64(0), + }, nc, testAccountID) + assert.EqualError(t, err, awserrors.ErrorServerInternal) +} + +// --- DetachNetworkInterface --- + +func TestDetachNetworkInterface_NilInput(t *testing.T) { + _, err := DetachNetworkInterface(nil, nil, testAccountID) + assert.EqualError(t, err, awserrors.ErrorInvalidParameterValue) +} + +func TestDetachNetworkInterface_MissingAttachmentID(t *testing.T) { + _, err := DetachNetworkInterface(&ec2.DetachNetworkInterfaceInput{}, nil, testAccountID) + assert.EqualError(t, err, awserrors.ErrorMissingParameter) +} + +func TestDetachNetworkInterface_EmptyAttachmentID(t *testing.T) { + _, err := DetachNetworkInterface(&ec2.DetachNetworkInterfaceInput{ + AttachmentId: aws.String(""), + }, nil, testAccountID) + assert.EqualError(t, err, awserrors.ErrorMissingParameter) +} + +func TestDetachNetworkInterface_UnknownAttachment(t *testing.T) { + nc := newTestNATS(t) + + sub, err := nc.Subscribe("ec2.DescribeNetworkInterfaces", func(msg *nats.Msg) { + data, _ := json.Marshal(ec2.DescribeNetworkInterfacesOutput{}) + _ = msg.Respond(data) + }) + require.NoError(t, err) + t.Cleanup(func() { _ = sub.Unsubscribe() }) + + _, err = DetachNetworkInterface(&ec2.DetachNetworkInterfaceInput{ + AttachmentId: aws.String("eni-attach-missing"), + }, nc, testAccountID) + assert.EqualError(t, err, awserrors.ErrorInvalidAttachmentIDNotFound) +} + +func TestDetachNetworkInterface_DescribeErrorResponse(t *testing.T) { + nc := newTestNATS(t) + + sub, err := nc.Subscribe("ec2.DescribeNetworkInterfaces", func(msg *nats.Msg) { + _ = msg.Respond(utils.GenerateErrorPayload(awserrors.ErrorServerInternal)) + }) + require.NoError(t, err) + t.Cleanup(func() { _ = sub.Unsubscribe() }) + + _, err = DetachNetworkInterface(&ec2.DetachNetworkInterfaceInput{ + AttachmentId: aws.String("eni-attach-1"), + }, nc, testAccountID) + assert.EqualError(t, err, awserrors.ErrorInvalidAttachmentIDNotFound) +} + +func TestDetachNetworkInterface_AttachmentWithoutInstance(t *testing.T) { + nc := newTestNATS(t) + + sub, err := nc.Subscribe("ec2.DescribeNetworkInterfaces", func(msg *nats.Msg) { + resp := ec2.DescribeNetworkInterfacesOutput{ + NetworkInterfaces: []*ec2.NetworkInterface{ + { + NetworkInterfaceId: aws.String("eni-orphan"), + Attachment: &ec2.NetworkInterfaceAttachment{ + AttachmentId: aws.String("eni-attach-orphan"), + }, + }, + }, + } + data, _ := json.Marshal(resp) + _ = msg.Respond(data) + }) + require.NoError(t, err) + t.Cleanup(func() { _ = sub.Unsubscribe() }) + + _, err = DetachNetworkInterface(&ec2.DetachNetworkInterfaceInput{ + AttachmentId: aws.String("eni-attach-orphan"), + }, nc, testAccountID) + assert.EqualError(t, err, awserrors.ErrorInvalidAttachmentIDNotFound) +} + +func TestDetachNetworkInterface_NoResponders(t *testing.T) { + nc := newTestNATS(t) + describeNetworkInterfacesResponder(t, nc, "eni-attach-1", "i-no-daemon", "eni-1") + + _, err := DetachNetworkInterface(&ec2.DetachNetworkInterfaceInput{ + AttachmentId: aws.String("eni-attach-1"), + }, nc, testAccountID) + assert.EqualError(t, err, awserrors.ErrorInvalidInstanceIDNotFound) +} + +func TestDetachNetworkInterface_Success(t *testing.T) { + nc := newTestNATS(t) + describeNetworkInterfacesResponder(t, nc, "eni-attach-ok", "i-detach", "eni-detachable") + + var received types.EC2InstanceCommand + var receivedAccount string + sub, err := nc.Subscribe("ec2.cmd.i-detach", func(msg *nats.Msg) { + receivedAccount = msg.Header.Get(utils.AccountIDHeader) + _ = json.Unmarshal(msg.Data, &received) + data, _ := json.Marshal(ec2.DetachNetworkInterfaceOutput{}) + _ = msg.Respond(data) + }) + require.NoError(t, err) + t.Cleanup(func() { _ = sub.Unsubscribe() }) + + _, err = DetachNetworkInterface(&ec2.DetachNetworkInterfaceInput{ + AttachmentId: aws.String("eni-attach-ok"), + Force: aws.Bool(true), + }, nc, testAccountID) + require.NoError(t, err) + + assert.Equal(t, testAccountID, receivedAccount) + assert.True(t, received.Attributes.DetachENI) + require.NotNil(t, received.DetachENIData) + assert.Equal(t, "eni-attach-ok", received.DetachENIData.AttachmentID) + assert.True(t, received.DetachENIData.Force) + assert.Equal(t, "i-detach", received.ID) +} + +func TestDetachNetworkInterface_DaemonErrorResponse(t *testing.T) { + nc := newTestNATS(t) + describeNetworkInterfacesResponder(t, nc, "eni-attach-err", "i-err", "eni-x") + + sub, err := nc.Subscribe("ec2.cmd.i-err", func(msg *nats.Msg) { + _ = msg.Respond(utils.GenerateErrorPayload(awserrors.ErrorIncorrectInstanceState)) + }) + require.NoError(t, err) + t.Cleanup(func() { _ = sub.Unsubscribe() }) + + _, err = DetachNetworkInterface(&ec2.DetachNetworkInterfaceInput{ + AttachmentId: aws.String("eni-attach-err"), + }, nc, testAccountID) + assert.EqualError(t, err, awserrors.ErrorIncorrectInstanceState) +} + +func TestDetachNetworkInterface_MalformedDaemonResponse(t *testing.T) { + nc := newTestNATS(t) + describeNetworkInterfacesResponder(t, nc, "eni-attach-bad", "i-bad", "eni-bad") + + sub, err := nc.Subscribe("ec2.cmd.i-bad", func(msg *nats.Msg) { + _ = msg.Respond([]byte("not-json")) + }) + require.NoError(t, err) + t.Cleanup(func() { _ = sub.Unsubscribe() }) + + _, err = DetachNetworkInterface(&ec2.DetachNetworkInterfaceInput{ + AttachmentId: aws.String("eni-attach-bad"), + }, nc, testAccountID) + assert.EqualError(t, err, awserrors.ErrorServerInternal) +} diff --git a/spinifex/handlers/ec2/vpc/eni_hotplug_helpers_test.go b/spinifex/handlers/ec2/vpc/eni_hotplug_helpers_test.go new file mode 100644 index 000000000..8099151e4 --- /dev/null +++ b/spinifex/handlers/ec2/vpc/eni_hotplug_helpers_test.go @@ -0,0 +1,163 @@ +package handlers_ec2_vpc + +import ( + "testing" + + "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/service/ec2" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestGetENIRecord(t *testing.T) { + svc := setupTestVPCService(t) + vpcId := createTestVPC(t, svc, "10.0.0.0/16") + subnetId := createTestSubnet(t, svc, vpcId, "10.0.1.0/24") + eniId := createTestENI(t, svc, subnetId) + + rec, err := svc.GetENIRecord(testAccountID, eniId) + require.NoError(t, err) + assert.Equal(t, eniId, rec.NetworkInterfaceId) + assert.Equal(t, subnetId, rec.SubnetId) + assert.Equal(t, "available", rec.Status) + assert.NotEmpty(t, rec.MacAddress) +} + +func TestGetENIRecord_NotFound(t *testing.T) { + svc := setupTestVPCService(t) + _, err := svc.GetENIRecord(testAccountID, "eni-nonexistent") + assert.ErrorContains(t, err, "InvalidNetworkInterfaceID.NotFound") +} + +func TestUpdateENI_PatchesFields(t *testing.T) { + svc := setupTestVPCService(t) + vpcId := createTestVPC(t, svc, "10.0.0.0/16") + subnetId := createTestSubnet(t, svc, vpcId, "10.0.1.0/24") + eniId := createTestENI(t, svc, subnetId) + + err := svc.UpdateENI(testAccountID, eniId, func(r *ENIRecord) { + r.AttachmentStatus = "attaching" + r.HotPlugSlot = 3 + r.LastAttachError = "prior failure" + }) + require.NoError(t, err) + + rec, err := svc.GetENIRecord(testAccountID, eniId) + require.NoError(t, err) + assert.Equal(t, "attaching", rec.AttachmentStatus) + assert.Equal(t, 3, rec.HotPlugSlot) + assert.Equal(t, "prior failure", rec.LastAttachError) +} + +func TestUpdateENI_NotFound(t *testing.T) { + svc := setupTestVPCService(t) + err := svc.UpdateENI(testAccountID, "eni-nonexistent", func(r *ENIRecord) { + r.AttachmentStatus = "attached" + }) + assert.ErrorContains(t, err, "InvalidNetworkInterfaceID.NotFound") +} + +func TestUpdateENI_NoopPatchPersists(t *testing.T) { + svc := setupTestVPCService(t) + vpcId := createTestVPC(t, svc, "10.0.0.0/16") + subnetId := createTestSubnet(t, svc, vpcId, "10.0.1.0/24") + eniId := createTestENI(t, svc, subnetId) + + err := svc.UpdateENI(testAccountID, eniId, func(*ENIRecord) {}) + require.NoError(t, err) + + rec, err := svc.GetENIRecord(testAccountID, eniId) + require.NoError(t, err) + assert.Equal(t, eniId, rec.NetworkInterfaceId) +} + +func TestFindENIByAttachment(t *testing.T) { + svc := setupTestVPCService(t) + vpcId := createTestVPC(t, svc, "10.0.0.0/16") + subnetId := createTestSubnet(t, svc, vpcId, "10.0.1.0/24") + eniId := createTestENI(t, svc, subnetId) + + attachId, err := svc.AttachENI(testAccountID, eniId, "i-find-test", 2) + require.NoError(t, err) + + rec, err := svc.FindENIByAttachment(testAccountID, attachId) + require.NoError(t, err) + assert.Equal(t, eniId, rec.NetworkInterfaceId) + assert.Equal(t, "i-find-test", rec.InstanceId) + assert.Equal(t, int64(2), rec.DeviceIndex) +} + +func TestFindENIByAttachment_NotFound(t *testing.T) { + svc := setupTestVPCService(t) + _, err := svc.FindENIByAttachment(testAccountID, "eni-attach-missing") + assert.ErrorContains(t, err, "InvalidAttachmentID.NotFound") +} + +func TestFindENIByAttachment_EmptyBucket(t *testing.T) { + svc := setupTestVPCService(t) + _, err := svc.FindENIByAttachment(testAccountID, "eni-attach-anything") + assert.ErrorContains(t, err, "InvalidAttachmentID.NotFound") +} + +func TestFindENIByAttachment_AccountScoped(t *testing.T) { + svc := setupTestVPCService(t) + vpcId := createTestVPC(t, svc, "10.0.0.0/16") + subnetId := createTestSubnet(t, svc, vpcId, "10.0.1.0/24") + eniId := createTestENI(t, svc, subnetId) + + attachId, err := svc.AttachENI(testAccountID, eniId, "i-scoped", 0) + require.NoError(t, err) + + _, err = svc.FindENIByAttachment("999988887777", attachId) + assert.ErrorContains(t, err, "InvalidAttachmentID.NotFound") +} + +// TestUpdateENI_RoundTripsAllNewFields exercises every Sprint 3b field on +// ENIRecord so a struct-field rename would surface as a test failure rather +// than a silent JSON-tag drift. +func TestUpdateENI_RoundTripsAllNewFields(t *testing.T) { + svc := setupTestVPCService(t) + vpcId := createTestVPC(t, svc, "10.0.0.0/16") + subnetId := createTestSubnet(t, svc, vpcId, "10.0.1.0/24") + eniId := createTestENI(t, svc, subnetId) + + require.NoError(t, svc.UpdateENI(testAccountID, eniId, func(r *ENIRecord) { + r.AttachmentStatus = "detaching" + r.HotPlugSlot = 7 + r.LastAttachError = "boom" + r.DetachInFlight = true + r.DetachForce = true + })) + + rec, err := svc.GetENIRecord(testAccountID, eniId) + require.NoError(t, err) + assert.Equal(t, "detaching", rec.AttachmentStatus) + assert.Equal(t, 7, rec.HotPlugSlot) + assert.Equal(t, "boom", rec.LastAttachError) + assert.True(t, rec.DetachInFlight) + assert.True(t, rec.DetachForce) +} + +// TestUpdateENI_PreservesUnrelatedFields ensures a partial patch does not +// clobber pre-existing record state. +func TestUpdateENI_PreservesUnrelatedFields(t *testing.T) { + svc := setupTestVPCService(t) + vpcId := createTestVPC(t, svc, "10.0.0.0/16") + subnetId := createTestSubnet(t, svc, vpcId, "10.0.1.0/24") + out, err := svc.CreateNetworkInterface(&ec2.CreateNetworkInterfaceInput{ + SubnetId: aws.String(subnetId), + Description: aws.String("preserved-desc"), + }, testAccountID) + require.NoError(t, err) + eniId := *out.NetworkInterface.NetworkInterfaceId + + require.NoError(t, svc.UpdateENI(testAccountID, eniId, func(r *ENIRecord) { + r.AttachmentStatus = "attaching" + })) + + rec, err := svc.GetENIRecord(testAccountID, eniId) + require.NoError(t, err) + assert.Equal(t, "preserved-desc", rec.Description) + assert.Equal(t, subnetId, rec.SubnetId) + assert.NotEmpty(t, rec.MacAddress) +} diff --git a/spinifex/vm/eni_hotplug.go b/spinifex/vm/eni_hotplug.go index c46909c2a..0b15c626a 100644 --- a/spinifex/vm/eni_hotplug.go +++ b/spinifex/vm/eni_hotplug.go @@ -41,10 +41,10 @@ type HotPlugENIResult struct { // HotPlugENI runs the 4-step attach pipeline against the live VM: // -// 1. tap device creation (placeholder — wired in Sprint 3c) -// 2. ovs-vsctl add-port (placeholder — wired in Sprint 3c) -// 3. QMP netdev_add (real) -// 4. QMP device_add + poll (real) +// 1. tap device + OVS port (NetworkPlumber.SetupTap — atomic tap+br-int) +// 2. QMP netdev_add +// 3. QMP device_add +// 4. poll query-pci until materialized // // On step-N failure, steps 1..N-1 are rolled back in reverse order and // the slot is returned to ENIRequests.AvailableSlots. The instance must @@ -92,22 +92,13 @@ func (m *Manager) HotPlugENI(instance *VM, eniID, mac string) (HotPlugENIResult, tapName := TapDeviceName(eniID) busID := fmt.Sprintf("hotplug-eni%d", slot) - // Step 1: tap device. Sprint 3c replaces this stub with a real call - // to NetworkPlumber (ip tuntap add). - if err := stubTapAdd(instance.ID, eniID, tapName); err != nil { + // Step 1: tap device + OVS port on br-int (carries OVN iface-id binding). + if err := m.setupENITap(instance.ID, eniID, mac); err != nil { m.releaseSlotLocked(instance, eniID, slot) - return HotPlugENIResult{}, fmt.Errorf("tap add (stub): %w", err) + return HotPlugENIResult{}, fmt.Errorf("tap/ovs setup: %w", err) } - // Step 2: OVS port. Sprint 3c replaces this stub with the real - // ovs-vsctl add-port invocation against br-int. - if err := stubOVSAddPort(instance.ID, eniID, tapName, mac); err != nil { - _ = stubTapDel(instance.ID, eniID, tapName) - m.releaseSlotLocked(instance, eniID, slot) - return HotPlugENIResult{}, fmt.Errorf("ovs add-port (stub): %w", err) - } - - // Step 3: QMP netdev_add. + // Step 2: QMP netdev_add. if err := dc.NetdevAdd(map[string]any{ "type": "tap", "id": netdevID, @@ -115,13 +106,12 @@ func (m *Manager) HotPlugENI(instance *VM, eniID, mac string) (HotPlugENIResult, "script": "no", "downscript": "no", }); err != nil { - _ = stubOVSDelPort(instance.ID, eniID, tapName) - _ = stubTapDel(instance.ID, eniID, tapName) + m.cleanupENITap(instance.ID, eniID, tapName) m.releaseSlotLocked(instance, eniID, slot) return HotPlugENIResult{}, fmt.Errorf("QMP netdev_add: %w", err) } - // Step 4: QMP device_add then poll query-pci until materialized. + // Step 3: QMP device_add. if err := dc.DeviceAdd(map[string]any{ "driver": "virtio-net-pci", "id": deviceID, @@ -130,17 +120,16 @@ func (m *Manager) HotPlugENI(instance *VM, eniID, mac string) (HotPlugENIResult, "mac": mac, }); err != nil { _ = dc.NetdevDel(netdevID) - _ = stubOVSDelPort(instance.ID, eniID, tapName) - _ = stubTapDel(instance.ID, eniID, tapName) + m.cleanupENITap(instance.ID, eniID, tapName) m.releaseSlotLocked(instance, eniID, slot) return HotPlugENIResult{}, fmt.Errorf("QMP device_add: %w", err) } + // Step 4: poll query-pci until the guest materializes the device. if err := waitForPCIDevice(dc, deviceID, true); err != nil { _ = dc.DeviceDel(deviceID) _ = dc.NetdevDel(netdevID) - _ = stubOVSDelPort(instance.ID, eniID, tapName) - _ = stubTapDel(instance.ID, eniID, tapName) + m.cleanupENITap(instance.ID, eniID, tapName) m.releaseSlotLocked(instance, eniID, slot) return HotPlugENIResult{}, fmt.Errorf("query-pci wait (attach): %w", err) } @@ -204,9 +193,8 @@ func (m *Manager) HotUnplugENI(instance *VM, eniID string, force bool) error { "instanceId", instance.ID, "eniId", eniID, "err", err) } - // Step 4: OVS port + tap (stubs, real wiring in Sprint 3c). - _ = stubOVSDelPort(instance.ID, eniID, tapName) - _ = stubTapDel(instance.ID, eniID, tapName) + // Step 4: OVS port + tap teardown (idempotent). + m.cleanupENITap(instance.ID, eniID, tapName) delete(instance.ENIRequests.AttachedByENIID, eniID) instance.ENIRequests.AvailableSlots = append(instance.ENIRequests.AvailableSlots, slot) @@ -259,32 +247,51 @@ func waitForPCIDevice(dc DeviceController, deviceID string, wantPresent bool) er // override it to drive poll cadence without burning wall-clock time. var eniPipelineSleep = time.Sleep -// stubTapAdd / stubTapDel / stubOVSAddPort / stubOVSDelPort emit a debug -// log and return nil. Sprint 3c replaces these with real -// NetworkPlumber.SetupTap / TeardownTap + ovs-vsctl invocations. The -// pipeline structure and rollback semantics are stable in 3b so 3c is a -// drop-in replacement. +// SetHotPlugTestSeams swaps the device-controller factory and the +// query-pci poll sleep for the duration of a test, returning a restore +// func that callers pass to t.Cleanup. Mirrors utils.SetSudoCommandForTest: +// the indirection lets tests in other packages drive the hot-plug +// pipeline against a StubDeviceController without reassigning +// unexported vars (which the reassign linter forbids). Pass nil for +// either argument to leave the corresponding seam untouched. +func SetHotPlugTestSeams(factory func(*VM) DeviceController, sleep func(time.Duration)) (restore func()) { + origFactory := newDeviceController + origSleep := eniPipelineSleep + if factory != nil { + newDeviceController = factory + } + if sleep != nil { + eniPipelineSleep = sleep + } + return func() { + newDeviceController = origFactory + eniPipelineSleep = origSleep + } +} -func stubTapAdd(instanceID, eniID, tapName string) error { - slog.Debug("ENI hot-plug step 1 (tap add) — stubbed in Sprint 3b", - "instanceId", instanceID, "eniId", eniID, "tap", tapName) +// setupENITap creates the tap on br-int and wires the OVS port carrying the +// OVN binding (iface-id) and attached MAC, via the shared NetworkPlumber. A +// nil plumber (unit-test managers) is a noop, mirroring setupExtraENINICs. +func (m *Manager) setupENITap(instanceID, eniID, mac string) error { + if m.deps.NetworkPlumber == nil { + return nil + } + spec := VPCTapSpec(eniID, mac) + if err := m.deps.NetworkPlumber.SetupTap(spec); err != nil { + return fmt.Errorf("setup tap %s for eni %s on instance %s: %w", spec.Name, eniID, instanceID, err) + } return nil } -func stubTapDel(instanceID, eniID, tapName string) error { - slog.Debug("ENI hot-plug rollback (tap del) — stubbed in Sprint 3b", - "instanceId", instanceID, "eniId", eniID, "tap", tapName) - return nil -} - -func stubOVSAddPort(instanceID, eniID, tapName, mac string) error { - slog.Debug("ENI hot-plug step 2 (ovs add-port) — stubbed in Sprint 3b", - "instanceId", instanceID, "eniId", eniID, "tap", tapName, "mac", mac) - return nil -} - -func stubOVSDelPort(instanceID, eniID, tapName string) error { - slog.Debug("ENI hot-plug rollback (ovs del-port) — stubbed in Sprint 3b", - "instanceId", instanceID, "eniId", eniID, "tap", tapName) - return nil +// cleanupENITap removes the tap and its OVS port. Idempotent (CleanupTap +// tolerates already-absent state); a nil plumber is a noop. Failures are +// logged, not returned — rollback/detach must make best-effort progress. +func (m *Manager) cleanupENITap(instanceID, eniID, tapName string) { + if m.deps.NetworkPlumber == nil { + return + } + if err := m.deps.NetworkPlumber.CleanupTap(tapName); err != nil { + slog.Warn("ENI hot-plug tap cleanup failed", + "instanceId", instanceID, "eniId", eniID, "tap", tapName, "err", err) + } } diff --git a/spinifex/vm/eni_hotplug_test.go b/spinifex/vm/eni_hotplug_test.go index 981d35937..6bbb913bc 100644 --- a/spinifex/vm/eni_hotplug_test.go +++ b/spinifex/vm/eni_hotplug_test.go @@ -3,6 +3,7 @@ package vm import ( "errors" "fmt" + "reflect" "slices" "strings" "sync" @@ -19,7 +20,16 @@ import ( // test (cleanup restores the production factory). func newHotPlugTestVM(t *testing.T, slots int) (*Manager, *VM, *StubDeviceController) { t.Helper() - mgr := NewManager() + mgr, v, stub, _ := newHotPlugTestVMWithPlumber(t, slots) + return mgr, v, stub +} + +// newHotPlugTestVMWithPlumber is the variant that exposes the fake +// NetworkPlumber so 3c tests can assert tap/OVS SetupTap / CleanupTap calls. +func newHotPlugTestVMWithPlumber(t *testing.T, slots int) (*Manager, *VM, *StubDeviceController, *fakeNetworkPlumber) { + t.Helper() + plumber := &fakeNetworkPlumber{} + mgr := NewManagerWithDeps(Deps{NetworkPlumber: plumber}) stub := NewStubDeviceController() available := make([]int, 0, slots) for i := 1; i <= slots; i++ { @@ -44,7 +54,7 @@ func newHotPlugTestVM(t *testing.T, slots int) (*Manager, *VM, *StubDeviceContro newDeviceController = origFactory eniPipelineSleep = origSleep }) - return mgr, v, stub + return mgr, v, stub, plumber } func TestHotPlugENI_SuccessPath(t *testing.T) { @@ -296,6 +306,68 @@ func TestHotPlugENI_ConcurrentAttachesSerialize(t *testing.T) { } } +func TestHotPlugENI_WiresTapOnBrInt(t *testing.T) { + mgr, v, _, plumber := newHotPlugTestVMWithPlumber(t, 2) + + if _, err := mgr.HotPlugENI(v, "eni-abc123", "02:00:00:aa:bb:cc"); err != nil { + t.Fatalf("HotPlugENI: %v", err) + } + if len(plumber.setupCalls) != 1 { + t.Fatalf("SetupTap calls = %d, want 1", len(plumber.setupCalls)) + } + got := plumber.setupCalls[0] + want := VPCTapSpec("eni-abc123", "02:00:00:aa:bb:cc") + if !reflect.DeepEqual(got, want) { + t.Errorf("SetupTap spec = %+v, want %+v", got, want) + } + if len(plumber.cleanupCalls) != 0 { + t.Errorf("CleanupTap called %d times on success, want 0", len(plumber.cleanupCalls)) + } +} + +func TestHotPlugENI_TapSetupFailureAbortsBeforeQMP(t *testing.T) { + mgr, v, stub, plumber := newHotPlugTestVMWithPlumber(t, 1) + plumber.setupErr = errors.New("ovs add-port boom") + + _, err := mgr.HotPlugENI(v, "eni-abc123", "02:00:00:aa:bb:cc") + if err == nil || !strings.Contains(err.Error(), "tap/ovs setup") { + t.Fatalf("err = %v, want tap/ovs setup failure", err) + } + if len(stub.Calls()) != 0 { + t.Errorf("QMP calls issued after tap setup failure: %v", stub.Calls()) + } + if !containsSlot(v.ENIRequests.AvailableSlots, 1) { + t.Errorf("slot 1 not returned to free-list: %v", v.ENIRequests.AvailableSlots) + } +} + +func TestHotPlugENI_QMPFailureCleansUpTap(t *testing.T) { + mgr, v, stub, plumber := newHotPlugTestVMWithPlumber(t, 1) + stub.SetFailNext("netdev_add", errors.New("netdev boom")) + + if _, err := mgr.HotPlugENI(v, "eni-abc123", "02:00:00:aa:bb:cc"); err == nil { + t.Fatalf("HotPlugENI: want error, got nil") + } + wantTap := TapDeviceName("eni-abc123") + if len(plumber.cleanupCalls) != 1 || plumber.cleanupCalls[0] != wantTap { + t.Errorf("CleanupTap calls = %v, want [%s]", plumber.cleanupCalls, wantTap) + } +} + +func TestHotUnplugENI_CleansUpTap(t *testing.T) { + mgr, v, _, plumber := newHotPlugTestVMWithPlumber(t, 1) + if _, err := mgr.HotPlugENI(v, "eni-abc123", "02:00:00:aa:bb:cc"); err != nil { + t.Fatalf("attach: %v", err) + } + if err := mgr.HotUnplugENI(v, "eni-abc123", false); err != nil { + t.Fatalf("detach: %v", err) + } + wantTap := TapDeviceName("eni-abc123") + if len(plumber.cleanupCalls) != 1 || plumber.cleanupCalls[0] != wantTap { + t.Errorf("CleanupTap calls = %v, want [%s]", plumber.cleanupCalls, wantTap) + } +} + func containsSlot(slots []int, s int) bool { return slices.Contains(slots, s) }