diff --git a/cmd/metal-api/internal/metal/network.go b/cmd/metal-api/internal/metal/network.go index 8241aa464..5aee1c403 100644 --- a/cmd/metal-api/internal/metal/network.go +++ b/cmd/metal-api/internal/metal/network.go @@ -7,8 +7,6 @@ import ( "slices" "strconv" "strings" - - "github.com/samber/lo" ) // SwitchPortStatus is a type alias for a string that represents the status of a switch port. @@ -56,8 +54,7 @@ type Nic struct { // NicState represents the desired and actual state of a network interface // controller (NIC). The Desired field indicates the intended state of the -// NIC, while Actual indicates its current operational state. The Desired -// state will be removed when the actual state is equal to the desired state. +// NIC, while Actual indicates its current operational state. type NicState struct { Desired *SwitchPortStatus `rethinkdb:"desired" json:"desired"` Actual SwitchPortStatus `rethinkdb:"actual" json:"actual"` @@ -75,68 +72,22 @@ type SwitchBGPPortState struct { // SetState updates the NicState with the given SwitchPortStatus. It returns // a new NicState and a bool indicating if the state was changed. -// -// If the given status matches the current Actual state, it checks if Desired -// is set and matches too. If so, Desired is set to nil since the desired -// state has been reached. -// -// If the given status differs from the current Actual state, Desired is left -// unchanged if it differs from the new state so the desired state is still tracked. -// The Actual state is updated to the given status. -// -// This allows tracking both the desired and actual states, while clearing -// Desired once the desired state is achieved. -func (ns *NicState) SetState(s SwitchPortStatus) (NicState, bool) { +func (ns *NicState) SetState(status SwitchPortStatus) (NicState, bool) { if ns == nil { return NicState{ - Actual: s, + Actual: status, Desired: nil, }, true } - if ns.Actual == s { - if ns.Desired != nil { - if *ns.Desired == s { - // we now have the desired state, so set the desired state to nil - return NicState{ - Actual: s, - Desired: nil, - }, true - } else { - // we already have the reported state, but the desired one is different - // so nothing changed - return *ns, false - } - } - // nothing changed - return *ns, false - } - // we got another state as we had before - if ns.Desired != nil { - if *ns.Desired == s { - // we now have the desired state, so set the desired state to nil - return NicState{ - Actual: s, - Desired: nil, - }, true - } else { - // a new state was reported, but the desired one is different - // so we have to update the state but keep the desired state - return NicState{ - Actual: s, - Desired: ns.Desired, - }, true - } - } + changed := ns.Actual != status return NicState{ - Actual: s, - Desired: nil, - }, true + Actual: status, + Desired: ns.Desired, + }, changed } // WantState sets the desired state for the NIC. It returns a new NicState // struct with the desired state set and a bool indicating if the state changed. -// If the current state already matches the desired state, it returns a state -// with a cleared desired field. func (ns *NicState) WantState(s SwitchPortStatus) (NicState, bool) { if ns == nil { return NicState{ @@ -144,22 +95,17 @@ func (ns *NicState) WantState(s SwitchPortStatus) (NicState, bool) { Desired: &s, }, true } - if ns.Actual == s { - // we want a state we already have - if ns.Desired != nil { - return NicState{ - Actual: s, - Desired: nil, - }, true - } - return *ns, false + if ns.Desired == nil { + return NicState{ + Desired: &s, + Actual: ns.Actual, + }, true } - // return a new state with the desired state set and a bool indicating a state change - // only if the desired state is different from the current one + changed := s != *ns.Desired return NicState{ - Actual: ns.Actual, Desired: &s, - }, lo.FromPtr(ns.Desired) != s + Actual: ns.Actual, + }, changed } // GetIdentifier returns the identifier of a nic. diff --git a/cmd/metal-api/internal/metal/network_test.go b/cmd/metal-api/internal/metal/network_test.go index ca88aaf6a..3406a0977 100644 --- a/cmd/metal-api/internal/metal/network_test.go +++ b/cmd/metal-api/internal/metal/network_test.go @@ -136,103 +136,90 @@ func TestNicState_WantState(t *testing.T) { changed bool }{ { - name: "up to desired down", + name: "current is nil", + nic: nil, + arg: up, + want: NicState{ + Desired: &up, + Actual: unknown, + }, + changed: true, + }, + { + name: "current desired is nil, new desired matches current actual", nic: &NicState{ Desired: nil, Actual: down, }, - arg: up, + arg: down, want: NicState{ - Desired: &up, + Desired: &down, Actual: down, }, changed: true, }, { - name: "up to up with empty desired", + name: "current desired is nil, new desired differs from current actual", nic: &NicState{ Desired: nil, - Actual: up, + Actual: down, }, arg: up, want: NicState{ - Desired: nil, - Actual: up, + Desired: &up, + Actual: down, }, - changed: false, + changed: true, }, { - name: "up to up with other desired", + name: "new desired differs from current desired and actual", nic: &NicState{ Desired: &down, - Actual: up, + Actual: down, }, arg: up, - want: NicState{ - Desired: nil, - Actual: up, - }, - changed: true, - }, - { - name: "nil to up", - nic: nil, - arg: up, want: NicState{ Desired: &up, - Actual: unknown, + Actual: down, }, changed: true, }, { - name: "different actual with same desired", + name: "new desired differs from current desired", nic: &NicState{ Desired: &down, Actual: up, }, - arg: down, + arg: up, want: NicState{ - Desired: &down, - Actual: up, - }, - changed: false, - }, - { - name: "different actual with other desired", - nic: &NicState{ Desired: &up, Actual: up, }, - arg: down, - want: NicState{ - Desired: &down, - Actual: up, - }, changed: true, }, { - name: "different actual with empty desired", + name: "new desired matches current desired but differs from current actual", nic: &NicState{ - Desired: nil, - Actual: up, + Desired: &up, + Actual: down, }, - arg: down, + arg: up, want: NicState{ - Desired: &down, - Actual: up, + Desired: &up, + Actual: down, }, - changed: true, + changed: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, got1 := tt.nic.WantState(tt.arg) - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("NicState.WantState() got = %+v, want %+v", got, tt.want) + got, changed := tt.nic.WantState(tt.arg) + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("NicState.WantState() diff = %s", diff) } - if got1 != tt.changed { - t.Errorf("NicState.WantState() got1 = %v, want %v", got1, tt.changed) + if changed != tt.changed { + t.Errorf("NicState.WantState() changed = %v, want %v", changed, tt.changed) } }) } @@ -244,96 +231,109 @@ func TestNicState_SetState(t *testing.T) { unknown := SwitchPortStatusUnknown tests := []struct { - name string - nic *NicState - arg SwitchPortStatus - want NicState - changed bool + name string + ns *NicState + status SwitchPortStatus + want NicState + wantChanged bool }{ { - name: "different actual with empty desired", - nic: &NicState{ + name: "state is nil", + ns: nil, + status: down, + want: NicState{ + Desired: nil, + Actual: down, + }, + wantChanged: true, + }, + { + name: "desired is nil and actual unchanged", + ns: &NicState{ Desired: nil, Actual: up, }, - arg: down, + status: up, want: NicState{ Desired: nil, - Actual: down, + Actual: up, }, - changed: true, + wantChanged: false, }, { - name: "different actual with same state in desired", - nic: &NicState{ - Desired: &down, + name: "desired is nil and actual changes", + ns: &NicState{ + Desired: nil, Actual: up, }, - arg: down, + status: unknown, want: NicState{ Desired: nil, - Actual: down, + Actual: unknown, }, - changed: true, + wantChanged: true, }, { - name: "different actual with other state in desired", - nic: &NicState{ - Desired: &unknown, - Actual: up, + name: "desired is set, new state changed and does not match desired", + ns: &NicState{ + Desired: &up, + Actual: unknown, }, - arg: down, + status: down, want: NicState{ - Desired: &unknown, + Desired: &up, Actual: down, }, - changed: true, + wantChanged: true, }, { - name: "nil nic", - nic: nil, - arg: down, + name: "desired is set, new state unchanged and does not match desired", + ns: &NicState{ + Desired: &up, + Actual: unknown, + }, + status: unknown, want: NicState{ - Desired: nil, - Actual: down, + Desired: &up, + Actual: unknown, }, - changed: true, + wantChanged: false, }, { - name: "same state with same desired", - nic: &NicState{ - Desired: &down, + name: "desired is set, new state changed and matches desired", + ns: &NicState{ + Desired: &up, Actual: down, }, - arg: down, + status: up, want: NicState{ - Desired: nil, - Actual: down, + Desired: &up, + Actual: up, }, - changed: true, + wantChanged: true, }, { - name: "same state with other desired", - nic: &NicState{ + name: "desired is set, new state unchanged and matches desired", + ns: &NicState{ Desired: &up, - Actual: down, + Actual: up, }, - arg: down, + status: up, want: NicState{ Desired: &up, - Actual: down, + Actual: up, }, - changed: false, + wantChanged: false, }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - got, got1 := tt.nic.SetState(tt.arg) - if !reflect.DeepEqual(got, tt.want) { - t.Errorf("NicState.SetState() got = %+v, want %+v", got, tt.want) + got, changed := tt.ns.SetState(tt.status) + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("NicState.SetState() diff = %s", diff) } - if got1 != tt.changed { - t.Errorf("NicState.SetState() got1 = %v, want %v", got1, tt.changed) + if changed != tt.wantChanged { + t.Errorf("NicState.SetState() got1 = %v, want %v", changed, tt.wantChanged) } }) } diff --git a/cmd/metal-api/internal/service/machine-service_integration_test.go b/cmd/metal-api/internal/service/machine-service_integration_test.go index 51cf4a1f9..1bc2fda94 100644 --- a/cmd/metal-api/internal/service/machine-service_integration_test.go +++ b/cmd/metal-api/internal/service/machine-service_integration_test.go @@ -300,7 +300,7 @@ func BenchmarkMachineList(b *testing.B) { b.ResetTimer() - for range b.N { + for b.Loop() { var machines []v1.MachineResponse code := webRequestGet(b, machineService, &testUserDirectory.admin, nil, "/v1/machine", &machines) diff --git a/cmd/metal-api/internal/service/switch-service.go b/cmd/metal-api/internal/service/switch-service.go index 1fa62779c..c8f2b0ba1 100644 --- a/cmd/metal-api/internal/service/switch-service.go +++ b/cmd/metal-api/internal/service/switch-service.go @@ -373,7 +373,7 @@ func (r *switchResource) notifySwitch(request *restful.Request, response *restfu } // toggleSwitchPort handles a request to toggle the state of a port on a switch. It reads the request body, finds the switch, updates its NIC state if needed, and returns the updated switch on success. -// If the given port is not found or the given status is not concrete, a 400 error is returned. Another requirement is that there must be a machine connected to the port. +// If the given port is not found or the given status is not concrete, a 400 error is returned. func (r *switchResource) toggleSwitchPort(request *restful.Request, response *restful.Response) { var requestPayload v1.SwitchPortToggleRequest err := request.ReadEntity(&requestPayload) @@ -381,6 +381,7 @@ func (r *switchResource) toggleSwitchPort(request *restful.Request, response *re r.sendError(request, response, httperrors.BadRequest(err)) return } + r.log.Debug("toggle switch port", "request", requestPayload) desired := metal.SwitchPortStatus(requestPayload.Status) @@ -423,22 +424,6 @@ func (r *switchResource) toggleSwitchPort(request *restful.Request, response *re return } - // now check if there is something connected at the given nic. - machineConnection := false - - for _, mcs := range newSwitch.MachineConnections { - for _, mc := range mcs { - if strings.EqualFold(mc.Nic.Name, requestPayload.NicName) { - machineConnection = true - break - } - } - } - if !machineConnection { - r.sendError(request, response, httperrors.BadRequest(fmt.Errorf("switch %q does not have a connected machine at port %q", id, requestPayload.NicName))) - return - } - if updated { if err := r.ds.UpdateSwitch(oldSwitch, &newSwitch); err != nil { r.sendError(request, response, defaultError(err)) @@ -1032,7 +1017,10 @@ func (r *switchResource) makeSwitchResponse(s *metal.Switch) (*v1.SwitchResponse if err != nil { return nil, err } - cons := r.makeSwitchCons(s) + cons, err := r.makeSwitchCons(s) + if err != nil { + return nil, err + } return v1.NewSwitchResponse(s, ss, p, nics, cons), nil } @@ -1205,10 +1193,9 @@ func (r *switchResource) makeSwitchNics(s *metal.Switch, nws metal.NetworkMap, i BGPPortState: n.BGPPortState, } if n.State != nil { + nic.Actual = v1.SwitchPortStatus(n.State.Actual) if n.State.Desired != nil { - nic.Actual = v1.SwitchPortStatus(*n.State.Desired) - } else { - nic.Actual = v1.SwitchPortStatus(n.State.Actual) + nic.AdminStatus = new(v1.SwitchPortStatus(*n.State.Desired)) } } nics = append(nics, nic) @@ -1221,38 +1208,36 @@ func (r *switchResource) makeSwitchNics(s *metal.Switch, nws metal.NetworkMap, i return nics, nil } -func (r *switchResource) makeSwitchCons(s *metal.Switch) []v1.SwitchConnection { +func (r *switchResource) makeSwitchCons(s *metal.Switch) ([]v1.SwitchConnection, error) { cons := []v1.SwitchConnection{} nicMap := s.Nics.ByName() for _, metalConnections := range s.MachineConnections { for _, mc := range metalConnections { - // The connection state is set to the state of the NIC in the database. - // This state is not necessarily the actual state of the port on the switch. - // When the port is toggled, the connection state in the DB is updated after - // the real switch port changed state. - // So if a client queries the current switch state, it will see the desired - // state in the global NIC state, but the actual state of the port in the - // connection map. - n := nicMap[mc.Nic.Name] - state := metal.SwitchPortStatusUnknown - var bps *metal.SwitchBGPPortState - if n != nil && n.State != nil { - state = n.State.Actual - } - if n != nil && n.BGPPortState != nil { - bps = n.BGPPortState + n, ok := nicMap[mc.Nic.Name] + if !ok || n == nil { + return nil, fmt.Errorf("nic %s is connected to machine %s but could not be found on the switch %s", mc.Nic.Name, mc.MachineID, s.ID) } nic := v1.SwitchNic{ - MacAddress: string(mc.Nic.MacAddress), - Name: mc.Nic.Name, - Identifier: mc.Nic.Identifier, - Vrf: mc.Nic.Vrf, - Actual: v1.SwitchPortStatus(state), - BGPPortState: bps, + MacAddress: string(mc.Nic.MacAddress), + Name: mc.Nic.Name, + Identifier: mc.Nic.Identifier, + Vrf: mc.Nic.Vrf, + Actual: v1.SwitchPortStatusUnknown, + } + + if n.BGPPortState != nil { + nic.BGPPortState = n.BGPPortState } + if n.State != nil { + nic.Actual = v1.SwitchPortStatus(n.State.Actual) + if n.State.Desired != nil { + nic.AdminStatus = new(v1.SwitchPortStatus(*n.State.Desired)) + } + } + con := v1.SwitchConnection{ Nic: nic, MachineID: mc.MachineID, @@ -1265,7 +1250,7 @@ func (r *switchResource) makeSwitchCons(s *metal.Switch) []v1.SwitchConnection { return cons[i].MachineID < cons[j].MachineID }) - return cons + return cons, nil } func (r *switchResource) findSwitchReferencedEntities(s *metal.Switch) (*metal.Partition, metal.NetworkMap, metal.IPsMap, metal.Machines, *metal.SwitchStatus, error) { @@ -1329,7 +1314,10 @@ func (r *switchResource) makeSwitchResponseList(ss metal.Switches) ([]*v1.Switch if err != nil { return nil, err } - cons := r.makeSwitchCons(&sw) + cons, err := r.makeSwitchCons(&sw) + if err != nil { + return nil, err + } ss, err := r.ds.GetSwitchStatus(sw.ID) if err != nil && !metal.IsNotFound(err) { return nil, err diff --git a/cmd/metal-api/internal/service/switch-service_test.go b/cmd/metal-api/internal/service/switch-service_test.go index 623fa6e72..02ee0e625 100644 --- a/cmd/metal-api/internal/service/switch-service_test.go +++ b/cmd/metal-api/internal/service/switch-service_test.go @@ -24,6 +24,7 @@ import ( v1 "github.com/metal-stack/metal-api/cmd/metal-api/internal/service/v1" "github.com/metal-stack/metal-api/cmd/metal-api/internal/testdata" "github.com/metal-stack/metal-lib/httperrors" + "github.com/metal-stack/metal-lib/pkg/pointer" ) func TestRegisterSwitch(t *testing.T) { @@ -147,11 +148,22 @@ func TestRegisterExistingSwitchWithRoomChange(t *testing.T) { RackID: "1", RoomID: oldRoomID, OS: &metal.SwitchOS{Vendor: metal.SwitchOSVendorCumulus}, + Nics: metal.Nics{ + { + Name: "swp1", + MacAddress: "aa:aa:aa:aa:aa:01", + }, + { + Name: "swp2", + MacAddress: "aa:aa:aa:aa:aa:02", + }, + }, MachineConnections: metal.ConnectionMap{ "machine-1": metal.Connections{ { Nic: metal.Nic{ - Name: "swp1", + Name: "swp1", + MacAddress: "aa:aa:aa:aa:aa:01", }, MachineID: "machine-1", }, @@ -159,7 +171,8 @@ func TestRegisterExistingSwitchWithRoomChange(t *testing.T) { "machine-2": metal.Connections{ { Nic: metal.Nic{ - Name: "swp2", + Name: "swp2", + MacAddress: "aa:aa:aa:aa:aa:02", }, MachineID: "machine-2", }, @@ -197,6 +210,16 @@ func TestRegisterExistingSwitchWithRoomChange(t *testing.T) { ID: switchID, }, }, + Nics: v1.SwitchNics{ + { + Name: "swp1", + MacAddress: "aa:aa:aa:aa:aa:01", + }, + { + Name: "swp2", + MacAddress: "aa:aa:aa:aa:aa:02", + }, + }, PartitionID: "1", SwitchBase: v1.SwitchBase{ RackID: "1", @@ -1868,7 +1891,7 @@ func TestToggleSwitch(t *testing.T) { require.NoError(t, err) require.Equal(t, testdata.Switch1.ID, result.ID) require.Equal(t, testdata.Switch1.Name, *result.Name) - require.Equal(t, v1.SwitchPortStatusDown, result.Nics[0].Actual) + require.Equal(t, v1.SwitchPortStatusDown, pointer.SafeDeref(result.Nics[0].AdminStatus)) require.Equal(t, v1.SwitchPortStatusUnknown, result.Connections[0].Nic.Actual) } @@ -1896,12 +1919,14 @@ func TestToggleSwitchNicWithoutMachine(t *testing.T) { resp := w.Result() defer resp.Body.Close() - require.Equal(t, http.StatusBadRequest, resp.StatusCode, w.Body.String()) - var result httperrors.HTTPErrorResponse + require.Equal(t, http.StatusOK, resp.StatusCode, w.Body.String()) + var result v1.SwitchResponse err = json.NewDecoder(resp.Body).Decode(&result) require.NoError(t, err) - require.Equal(t, result.Message, fmt.Sprintf("switch %q does not have a connected machine at port %q", testdata.Switch1.ID, testdata.Switch1.Nics[1].Name)) + require.Equal(t, testdata.Switch1.ID, result.ID) + require.Equal(t, testdata.Switch1.Name, *result.Name) + require.Equal(t, v1.SwitchPortStatusDown, pointer.SafeDeref(result.Nics[1].AdminStatus)) } func Test_adjustMachineNics(t *testing.T) { diff --git a/cmd/metal-api/internal/service/v1/switch.go b/cmd/metal-api/internal/service/v1/switch.go index 150ddb0e1..5e76c51e4 100644 --- a/cmd/metal-api/internal/service/v1/switch.go +++ b/cmd/metal-api/internal/service/v1/switch.go @@ -47,6 +47,7 @@ type SwitchNic struct { Vrf string `json:"vrf" description:"the vrf this network interface is part of" optional:"true"` BGPFilter *BGPFilter `json:"filter" description:"configures the bgp filter applied at the switch port" optional:"true"` Actual SwitchPortStatus `json:"actual" description:"the current state of the nic" enum:"UP|DOWN|UNKNOWN"` + AdminStatus *SwitchPortStatus `json:"admin_status" description:"the desired state of the nic" enum:"UP|DOWN" optional:"true"` BGPPortState *metal.SwitchBGPPortState `json:"bgp_port_state" description:"the current bgp port state" optional:"true"` } diff --git a/cmd/metal-api/internal/testdata/testdata.go b/cmd/metal-api/internal/testdata/testdata.go index e6974d651..9f3c43e24 100644 --- a/cmd/metal-api/internal/testdata/testdata.go +++ b/cmd/metal-api/internal/testdata/testdata.go @@ -486,7 +486,7 @@ var ( Name: "IPAM Network", Description: "description IPAM", }, - Prefixes: prefixesIPAM, + Prefixes: prefixesIPAM, } // IPs @@ -595,12 +595,6 @@ var ( }, MachineID: "1", }, - metal.Connection{ - Nic: metal.Nic{ - MacAddress: metal.MacAddress("11:11:11:11:11:22"), - }, - MachineID: "1", - }, }, }, } diff --git a/spec/metal-api.json b/spec/metal-api.json index 78016ce80..7dfde82d0 100644 --- a/spec/metal-api.json +++ b/spec/metal-api.json @@ -5468,6 +5468,14 @@ ], "type": "string" }, + "admin_status": { + "description": "the desired state of the nic", + "enum": [ + "DOWN", + "UP" + ], + "type": "string" + }, "bgp_port_state": { "$ref": "#/definitions/metal.SwitchBGPPortState", "description": "the current bgp port state"