From 1649e704462c5ab071b8561dce9779baa637e5cc Mon Sep 17 00:00:00 2001 From: Mykola Solianko Date: Thu, 30 Jul 2026 16:16:10 +0300 Subject: [PATCH 1/6] sm: networkmanager: add link and netns existence queries InterfaceManagerItf exposed only mutating operations, so there was no way to tell an interface that must be created from one that is already on the system and can be adopted as is. Same for network namespaces. Add InterfaceManagerItf::GetLink returning link kind, master, vlan ID and admin state (eNotFound when the link is absent) and NamespaceManagerItf::IsNetworkNamespaceExist. Signed-off-by: Mykola Solianko Reviewed-by: Mykola Kobets Reviewed-by: Oleksandr Grytsov Reviewed-by: Mykhailo Lohvynenko --- .../networkmanager/itf/interfacemanager.hpp | 40 +++++++++++++++++++ .../networkmanager/itf/namespacemanager.hpp | 8 ++++ .../tests/mocks/interfacemanagermock.hpp | 1 + .../tests/mocks/namespacemanagermock.hpp | 1 + 4 files changed, 50 insertions(+) diff --git a/src/core/sm/networkmanager/itf/interfacemanager.hpp b/src/core/sm/networkmanager/itf/interfacemanager.hpp index e552eb3f8..2f1ee9418 100644 --- a/src/core/sm/networkmanager/itf/interfacemanager.hpp +++ b/src/core/sm/networkmanager/itf/interfacemanager.hpp @@ -7,7 +7,9 @@ #ifndef AOS_CORE_SM_NETWORKMANAGER_ITF_INTERFACEMANAGER_HPP_ #define AOS_CORE_SM_NETWORKMANAGER_ITF_INTERFACEMANAGER_HPP_ +#include #include +#include namespace aos::sm::networkmanager { @@ -15,6 +17,35 @@ namespace aos::sm::networkmanager { * @{ */ +/** + * Link kind type. + */ +class LinkKindType { +public: + enum class Enum { eUnknown, eBridge, eVlan, eVeth }; + + static const Array GetStrings() + { + static const char* const sLinkKindStrings[] = {"unknown", "bridge", "vlan", "veth"}; + + return Array(sLinkKindStrings, ArraySize(sLinkKindStrings)); + }; +}; + +using LinkKindEnum = LinkKindType::Enum; +using LinkKind = EnumStringer; + +/** + * Network link attributes as seen on the system. + */ +struct LinkInfo { + StaticString mName; + LinkKind mKind; + StaticString mMaster; + uint64_t mVlanID {}; + bool mUp {}; +}; + /** * Network interface manager interface. */ @@ -25,6 +56,15 @@ class InterfaceManagerItf { */ virtual ~InterfaceManagerItf() = default; + /** + * Returns link attributes as they are on the system. + * + * @param ifname interface name. + * @param[out] info link attributes. + * @return Error, eNotFound if the link doesn't exist. + */ + virtual Error GetLink(const String& ifname, LinkInfo& info) const = 0; + /** * Removes interface. * diff --git a/src/core/sm/networkmanager/itf/namespacemanager.hpp b/src/core/sm/networkmanager/itf/namespacemanager.hpp index d14744afa..e5bf90b26 100644 --- a/src/core/sm/networkmanager/itf/namespacemanager.hpp +++ b/src/core/sm/networkmanager/itf/namespacemanager.hpp @@ -33,6 +33,14 @@ class NamespaceManagerItf { */ virtual Error CreateNetworkNamespace(const String& ns) = 0; + /** + * Checks whether network namespace exists on the system. + * + * @param ns network namespace name. + * @return RetWithError. + */ + virtual RetWithError IsNetworkNamespaceExist(const String& ns) const = 0; + /** * Returns network namespace path. * diff --git a/src/core/sm/networkmanager/tests/mocks/interfacemanagermock.hpp b/src/core/sm/networkmanager/tests/mocks/interfacemanagermock.hpp index 9084ea755..36296083c 100644 --- a/src/core/sm/networkmanager/tests/mocks/interfacemanagermock.hpp +++ b/src/core/sm/networkmanager/tests/mocks/interfacemanagermock.hpp @@ -15,6 +15,7 @@ namespace aos::sm::networkmanager { class InterfaceManagerMock : public InterfaceManagerItf { public: + MOCK_METHOD(Error, GetLink, (const String&, LinkInfo&), (const, override)); MOCK_METHOD(Error, DeleteLink, (const String&), (override)); MOCK_METHOD(Error, SetupLink, (const String&, const String&), (override)); MOCK_METHOD(Error, SetMasterLink, (const String&, const String&), (override)); diff --git a/src/core/sm/networkmanager/tests/mocks/namespacemanagermock.hpp b/src/core/sm/networkmanager/tests/mocks/namespacemanagermock.hpp index 30be8bf71..4565a4523 100644 --- a/src/core/sm/networkmanager/tests/mocks/namespacemanagermock.hpp +++ b/src/core/sm/networkmanager/tests/mocks/namespacemanagermock.hpp @@ -16,6 +16,7 @@ namespace aos::sm::networkmanager { class NamespaceManagerMock : public NamespaceManagerItf { public: MOCK_METHOD(Error, CreateNetworkNamespace, (const String&), (override)); + MOCK_METHOD(RetWithError, IsNetworkNamespaceExist, (const String&), (const, override)); MOCK_METHOD(RetWithError>, GetNetworkNamespacePath, (const String&), (const, override)); MOCK_METHOD(Error, DeleteNetworkNamespace, (const String&), (override)); }; From a59781c18d2fe48046ee1d2f18b6e636641f8122 Mon Sep 17 00:00:00 2001 From: Mykola Solianko Date: Thu, 30 Jul 2026 16:41:15 +0300 Subject: [PATCH 2/6] sm: networkmanager: adopt existing bridge and vlan links After a crash SM restarts with an empty mPhysicalNetworks, so the first StartInstanceNetwork ran CreateNetwork over links that are still up. Recreating them is not a no-op: rtnl_link_add is issued without NLM_F_EXCL, so the kernel treats it as a modify request and CreateVlan pushes a freshly generated MAC onto the live vlan, breaking traffic of the instances still running on it. Probe each link with the new InterfaceManagerItf::GetLink and create only what is missing. Rollback deletes a link only when this call created it, so a failure midway no longer tears down an adopted one. Signed-off-by: Mykola Solianko Reviewed-by: Mykola Kobets Reviewed-by: Oleksandr Grytsov Reviewed-by: Mykhailo Lohvynenko --- src/core/sm/networkmanager/networkmanager.cpp | 65 +++++++-- src/core/sm/networkmanager/networkmanager.hpp | 7 +- .../networkmanager/tests/networkmanager.cpp | 126 ++++++++++++++++++ 3 files changed, 185 insertions(+), 13 deletions(-) diff --git a/src/core/sm/networkmanager/networkmanager.cpp b/src/core/sm/networkmanager/networkmanager.cpp index 11e293632..e8c9993a1 100644 --- a/src/core/sm/networkmanager/networkmanager.cpp +++ b/src/core/sm/networkmanager/networkmanager.cpp @@ -1367,6 +1367,21 @@ Error NetworkManager::PrepareDNSServerParams(const NetworkInfo& network, DNSServ return ErrorEnum::eNone; } +RetWithError NetworkManager::IsLinkExist(const String& ifName) const +{ + LinkInfo link; + + if (auto err = mNetIf->GetLink(ifName, link); !err.IsNone()) { + if (err.Is(ErrorEnum::eNotFound)) { + return {false, ErrorEnum::eNone}; + } + + return {false, AOS_ERROR_WRAP(err)}; + } + + return {true, ErrorEnum::eNone}; +} + Error NetworkManager::CreateNetwork(const NetworkInfo& network) { LOG_DBG() << "Create network" << Log::Field("networkID", network.mNetworkID) @@ -1376,24 +1391,54 @@ Error NetworkManager::CreateNetwork(const NetworkInfo& network) Error err; - if (err = mNetIfFactory->CreateBridge(network.mBridgeIfName, network.mIP, network.mSubnet); !err.IsNone()) { - return AOS_ERROR_WRAP(err); + // A link may already be there when SM crashed without running its teardown. + // Recreating it is not a no-op: the kernel takes RTM_NEWLINK without + // NLM_F_EXCL as a modify request, so CreateVlan would push a freshly + // generated MAC onto the live vlan and break the traffic of the instances + // still running on it. Adopt what exists and create only what is missing. + bool bridgeExists = false; + + if (Tie(bridgeExists, err) = IsLinkExist(network.mBridgeIfName); !err.IsNone()) { + return err; } - auto cleanupBridge = DeferRelease(&network, [this, &err](const NetworkInfo* network) { - if (!err.IsNone()) { + bool bridgeCreated = false; + + if (!bridgeExists) { + if (err = mNetIfFactory->CreateBridge(network.mBridgeIfName, network.mIP, network.mSubnet); !err.IsNone()) { + return AOS_ERROR_WRAP(err); + } + + bridgeCreated = true; + } + + auto cleanupBridge = DeferRelease(&network, [this, &err, bridgeCreated](const NetworkInfo* network) { + if (!err.IsNone() && bridgeCreated) { mNetIf->DeleteLink(network->mBridgeIfName); } }); - // Create the vlan already enslaved to the bridge (master) in one operation, - // avoiding a separate SetMasterLink round-trip. - if (err = mNetIfFactory->CreateVlan(network.mVlanIfName, network.mVlanID, network.mBridgeIfName); !err.IsNone()) { - return AOS_ERROR_WRAP(err); + bool vlanExists = false; + + if (Tie(vlanExists, err) = IsLinkExist(network.mVlanIfName); !err.IsNone()) { + return err; } - auto cleanupVlan = DeferRelease(&network, [this, &err](const NetworkInfo* network) { - if (!err.IsNone()) { + bool vlanCreated = false; + + if (!vlanExists) { + // Create the vlan already enslaved to the bridge (master) in one operation, + // avoiding a separate SetMasterLink round-trip. + if (err = mNetIfFactory->CreateVlan(network.mVlanIfName, network.mVlanID, network.mBridgeIfName); + !err.IsNone()) { + return AOS_ERROR_WRAP(err); + } + + vlanCreated = true; + } + + auto cleanupVlan = DeferRelease(&network, [this, &err, vlanCreated](const NetworkInfo* network) { + if (!err.IsNone() && vlanCreated) { mNetIf->DeleteLink(network->mVlanIfName); } }); diff --git a/src/core/sm/networkmanager/networkmanager.hpp b/src/core/sm/networkmanager/networkmanager.hpp index 49b6229cc..665dcac6c 100644 --- a/src/core/sm/networkmanager/networkmanager.hpp +++ b/src/core/sm/networkmanager/networkmanager.hpp @@ -250,9 +250,10 @@ class NetworkManager : public NetworkManagerItf { Error IsHostnameExist(const InstanceCache& instanceCache, const Array>& hosts) const; Error PushHostWithDomain( const String& host, const String& networkID, Array>& hosts) const; - Error CreateNetwork(const NetworkInfo& network); - Error DeleteInstanceNetworkConfig(const String& instanceID, const String& networkID); - Error GenerateIfName(String& ifName, const String& ifPrefix); + RetWithError IsLinkExist(const String& ifName) const; + Error CreateNetwork(const NetworkInfo& network); + Error DeleteInstanceNetworkConfig(const String& instanceID, const String& networkID); + Error GenerateIfName(String& ifName, const String& ifPrefix); template Error GenerateUniqueIfName(String& ifName, const String& ifPrefix, P&& isUnique) diff --git a/src/core/sm/networkmanager/tests/networkmanager.cpp b/src/core/sm/networkmanager/tests/networkmanager.cpp index 55859a460..984ce5fd1 100644 --- a/src/core/sm/networkmanager/tests/networkmanager.cpp +++ b/src/core/sm/networkmanager/tests/networkmanager.cpp @@ -56,6 +56,8 @@ class NetworkManagerTest : public Test { EXPECT_CALL(mDNSName, RemoveServer(_)).Times(AnyNumber()).WillRepeatedly(Return(aos::ErrorEnum::eNone)); EXPECT_CALL(mDNSServer, RemoveHost(_)).Times(AnyNumber()).WillRepeatedly(Return(aos::ErrorEnum::eNone)); + EXPECT_CALL(mNetIf, GetLink(_, _)).Times(AnyNumber()).WillRepeatedly(Return(aos::ErrorEnum::eNotFound)); + // Masquerade is a per-network rule installed/removed by CreateNetwork / // ClearNetwork; leave it lenient so per-test sequences need not assert it. EXPECT_CALL(mFirewall, AddMasquerade(_, _)).Times(AnyNumber()).WillRepeatedly(Return(aos::ErrorEnum::eNone)); @@ -196,6 +198,55 @@ class NetworkManagerTest : public Test { EXPECT_CALL(mStorage, UpdateInstanceNetworkInfo(_)).Times(times).WillRepeatedly(Return(aos::ErrorEnum::eNone)); } + NetworkInfo CreateTestNetworkInfo() + { + NetworkInfo network; + network.mNetworkID = "network1"; + network.mIP = "192.168.1.1"; + network.mSubnet = "192.168.1.0/24"; + network.mVlanID = 100ULL; + network.mVlanIfName = "vlan-1234abcd"; + network.mBridgeIfName = "br-ef567890"; + + return network; + } + + void InitWithStoredNetwork(const NetworkInfo& network) + { + mNetworkInfos.PushBack(network); + + EXPECT_CALL(mStorage, GetNetworksInfo(_)) + .WillOnce(DoAll(SetArgReferee<0>(mNetworkInfos), Return(aos::ErrorEnum::eNone))); + EXPECT_CALL(mStorage, GetInstanceNetworksInfo(_)) + .WillOnce(DoAll(SetArgReferee<0>(mInstanceNetworkInfos), Return(aos::ErrorEnum::eNone))); + + mNetManager = std::make_unique(); + + ASSERT_EQ(mNetManager->Init(mStorage, mBridgeNetwork, mFirewall, mBandwidth, mDNSName, mTrafficMonitor, mNetns, + mNetIf, mRandom, mNetIfFactory, mNetworkProvider, "test-node"), + aos::ErrorEnum::eNone); + } + + void ExpectLinkExists(const aos::String& ifName, LinkKind kind) + { + LinkInfo link; + link.mName = ifName; + link.mKind = kind; + + EXPECT_CALL(mNetIf, GetLink(ifName, _)) + .WillRepeatedly(DoAll(SetArgReferee<1>(link), Return(aos::ErrorEnum::eNone))); + } + + void ExpectStartInstanceOnStoredNetwork(const aos::String& instanceID, const aos::String& networkID) + { + EXPECT_CALL(mNetworkProvider, AllocateInstanceNetwork(_, networkID, aos::String("test-node"), _, _)) + .WillOnce(DoAll(SetArgReferee<4>(CreateTestAllocatedParams()), Return(aos::ErrorEnum::eNone))); + EXPECT_CALL(mStorage, AddInstanceNetworkInfo(_)).WillOnce(Return(aos::ErrorEnum::eNone)); + + ASSERT_EQ(mNetManager->CreateInstanceNetwork(instanceID, networkID, CreateTestInstanceNetworkConfig()), + aos::ErrorEnum::eNone); + } + void ExpectDeleteInstanceCalls(int times = 1) { EXPECT_CALL(mDNSServer, RemoveHost(_)).Times(times).WillRepeatedly(Return(aos::ErrorEnum::eNone)); @@ -1029,6 +1080,81 @@ TEST_F(NetworkManagerTest, InitWithExistingNetworks) ASSERT_EQ(mNetManager->StartInstanceNetwork(instanceID, "network1"), aos::ErrorEnum::eNone); } +TEST_F(NetworkManagerTest, CreateNetwork_AdoptsExistingBridgeAndVlan) +{ + const auto network = CreateTestNetworkInfo(); + const aos::String instanceID = "test-instance"; + + InitWithStoredNetwork(network); + + ExpectLinkExists(network.mBridgeIfName, LinkKindEnum::eBridge); + ExpectLinkExists(network.mVlanIfName, LinkKindEnum::eVlan); + + EXPECT_CALL(mNetIfFactory, CreateBridge(_, _, _)).Times(0); + EXPECT_CALL(mNetIfFactory, CreateVlan(_, _, _)).Times(0); + EXPECT_CALL(mDNSName, CreateServer(_, _)) + .WillOnce(Return(aos::RetWithError {&mDNSServer, aos::ErrorEnum::eNone})); + + ExpectStartInstanceOnStoredNetwork(instanceID, network.mNetworkID); + + ExpectAddInstanceCalls(); + ExpectPersistInstanceCalls(); + EXPECT_CALL(mNetns, CreateNetworkNamespace(_)).WillOnce(Return(aos::ErrorEnum::eNone)); + EXPECT_CALL(mNetns, GetNetworkNamespacePath(_)) + .WillOnce(Return(aos::RetWithError> {{}, aos::ErrorEnum::eNone})); + EXPECT_CALL(mTrafficMonitor, StartInstanceMonitoring(_, _, _, _)).WillOnce(Return(aos::ErrorEnum::eNone)); + + ASSERT_EQ(mNetManager->StartInstanceNetwork(instanceID, network.mNetworkID), aos::ErrorEnum::eNone); +} + +TEST_F(NetworkManagerTest, CreateNetwork_CreatesOnlyMissingVlan) +{ + const auto network = CreateTestNetworkInfo(); + const aos::String instanceID = "test-instance"; + + InitWithStoredNetwork(network); + + ExpectLinkExists(network.mBridgeIfName, LinkKindEnum::eBridge); + + EXPECT_CALL(mNetIfFactory, CreateBridge(_, _, _)).Times(0); + EXPECT_CALL(mNetIfFactory, CreateVlan(network.mVlanIfName, network.mVlanID, network.mBridgeIfName)) + .WillOnce(Return(aos::ErrorEnum::eNone)); + EXPECT_CALL(mDNSName, CreateServer(_, _)) + .WillOnce(Return(aos::RetWithError {&mDNSServer, aos::ErrorEnum::eNone})); + + ExpectStartInstanceOnStoredNetwork(instanceID, network.mNetworkID); + + ExpectAddInstanceCalls(); + ExpectPersistInstanceCalls(); + EXPECT_CALL(mNetns, CreateNetworkNamespace(_)).WillOnce(Return(aos::ErrorEnum::eNone)); + EXPECT_CALL(mNetns, GetNetworkNamespacePath(_)) + .WillOnce(Return(aos::RetWithError> {{}, aos::ErrorEnum::eNone})); + EXPECT_CALL(mTrafficMonitor, StartInstanceMonitoring(_, _, _, _)).WillOnce(Return(aos::ErrorEnum::eNone)); + + ASSERT_EQ(mNetManager->StartInstanceNetwork(instanceID, network.mNetworkID), aos::ErrorEnum::eNone); +} + +TEST_F(NetworkManagerTest, CreateNetwork_KeepsAdoptedBridgeWhenVlanCreationFails) +{ + const auto network = CreateTestNetworkInfo(); + const aos::String instanceID = "test-instance"; + + InitWithStoredNetwork(network); + + ExpectLinkExists(network.mBridgeIfName, LinkKindEnum::eBridge); + + EXPECT_CALL(mNetIfFactory, CreateBridge(_, _, _)).Times(0); + EXPECT_CALL(mNetIfFactory, CreateVlan(_, _, _)).WillOnce(Return(aos::ErrorEnum::eFailed)); + EXPECT_CALL(mNetIf, DeleteLink(_)).Times(0); + + ExpectStartInstanceOnStoredNetwork(instanceID, network.mNetworkID); + + EXPECT_FALSE(mNetManager->StartInstanceNetwork(instanceID, network.mNetworkID).IsNone()); + + Mock::VerifyAndClearExpectations(&mNetIf); + Mock::VerifyAndClearExpectations(&mNetIfFactory); +} + TEST_F(NetworkManagerTest, CreateInstanceNetwork_VerifyUpdateItemNetworkParams) { const aos::String networkID = "test-network"; From e7f6460e529cf5277c31b896ae7c17a3f4b02e60 Mon Sep 17 00:00:00 2001 From: Mykola Solianko Date: Thu, 30 Jul 2026 19:41:19 +0300 Subject: [PATCH 3/6] sm: networkmanager: keep running instances across SM restart CleanupLeftoverInstances tore down every instance recorded in storage on Start: it detached the host veth and deleted the network namespace even when the instance was alive and correctly wired, so an SM crash cut the network of containers that kept running. Replace it with ReconcileInstances, which checks the system before acting. An instance whose host veth is up, is a veth and is enslaved to its own bridge, and whose network namespace still exists, is adopted: nothing on the system is touched, only the runtime cache is restored. Everything else keeps the previous teardown path. Adopting into the runtime cache also makes the launcher restart flow work: StartInstanceNetwork now short-circuits with eAlreadyExist, which the launcher already tolerates. Signed-off-by: Mykola Solianko Reviewed-by: Mykola Kobets Reviewed-by: Oleksandr Grytsov Reviewed-by: Mykhailo Lohvynenko --- src/core/sm/networkmanager/networkmanager.cpp | 131 +++++++++-- src/core/sm/networkmanager/networkmanager.hpp | 20 +- .../networkmanager/tests/networkmanager.cpp | 212 ++++++++++++------ 3 files changed, 275 insertions(+), 88 deletions(-) diff --git a/src/core/sm/networkmanager/networkmanager.cpp b/src/core/sm/networkmanager/networkmanager.cpp index e8c9993a1..dc834c3d2 100644 --- a/src/core/sm/networkmanager/networkmanager.cpp +++ b/src/core/sm/networkmanager/networkmanager.cpp @@ -109,7 +109,7 @@ Error NetworkManager::Start() return err; } - if (err = CleanupLeftoverInstances(); !err.IsNone()) { + if (err = ReconcileInstances(); !err.IsNone()) { return err; } @@ -977,13 +977,93 @@ Error NetworkManager::UpdateInstanceNetworkCache( return ErrorEnum::eNone; } -Error NetworkManager::CleanupLeftoverInstances() +RetWithError NetworkManager::IsInstanceInterfaceAlive( + const String& instanceID, const String& hostIfName, const String& bridgeIfName) const { - LOG_DBG() << "Cleanup leftover instances"; + if (bridgeIfName.IsEmpty()) { + return {false, ErrorEnum::eNone}; + } + + LinkInfo link; + + if (auto err = mNetIf->GetLink(hostIfName, link); !err.IsNone()) { + if (err.Is(ErrorEnum::eNotFound)) { + return {false, ErrorEnum::eNone}; + } + + return {false, AOS_ERROR_WRAP(err)}; + } + + if (link.mKind != LinkKindEnum::eVeth || link.mMaster != bridgeIfName) { + return {false, ErrorEnum::eNone}; + } + + bool nsExists = false; + Error err; + + if (Tie(nsExists, err) = mNetns->IsNetworkNamespaceExist(instanceID); !err.IsNone()) { + return {false, AOS_ERROR_WRAP(err)}; + } + + return {nsExists, ErrorEnum::eNone}; +} + +Error NetworkManager::InitInstance(const String& instanceID, const String& networkID) +{ + LOG_DBG() << "Adopt running instance" << Log::Field("instanceID", instanceID) << Log::Field("networkID", networkID); + + if (auto errCache = AddInstanceToCache(instanceID, networkID); !errCache.IsNone()) { + return errCache; + } + + Error err; + + auto cleanupCache = DeferRelease(&instanceID, [this, &networkID, &err](const String* id) { + if (!err.IsNone()) { + if (auto errRemove = RemoveInstanceFromCache(*id, networkID); !errRemove.IsNone()) { + LOG_ERR() << "Failed to remove instance from cache" << Log::Field("instanceID", *id) + << Log::Field("networkID", networkID) << Log::Field(errRemove); + } + } + }); + + auto config = MakeUnique(&mAllocator); + + { + LockGuard lock {mMutex}; + + auto it = mInstanceNetworkInfos.Find(instanceID); + if (it == mInstanceNetworkInfos.end()) { + err = AOS_ERROR_WRAP(Error(ErrorEnum::eNotFound, "instance network info not found")); + + return err; + } + + *config = it->mSecond.mNetworkConfig; + } + + auto hosts = MakeUnique(&mAllocator); + + if (err = PrepareHosts(instanceID, networkID, *config, *hosts); !err.IsNone()) { + return err; + } + + if (err = UpdateInstanceNetworkCache(instanceID, networkID, *hosts); !err.IsNone()) { + return err; + } + + return ErrorEnum::eNone; +} + +Error NetworkManager::ReconcileInstances() +{ + LOG_DBG() << "Reconcile instances"; struct Entry { - StaticString mInstanceID; - StaticString mNetworkID; + StaticString mInstanceID; + StaticString mNetworkID; + StaticString mHostIfName; + StaticString mBridgeIfName; }; auto entries = MakeUnique>(&mAllocator); @@ -992,25 +1072,44 @@ Error NetworkManager::CleanupLeftoverInstances() LockGuard lock {mMutex}; for (const auto& item : mInstanceNetworkInfos) { - if (auto err = entries->PushBack({item.mFirst, item.mSecond.mNetworkID}); !err.IsNone()) { + StaticString bridgeIfName; + + if (auto it = mNetworkProviders.Find(item.mSecond.mNetworkID); it != mNetworkProviders.end()) { + bridgeIfName = it->mSecond.mBridgeIfName; + } + + if (auto err + = entries->PushBack({item.mFirst, item.mSecond.mNetworkID, item.mSecond.mHostIfName, bridgeIfName}); + !err.IsNone()) { return AOS_ERROR_WRAP(err); } } } - // Adopt a DNS handle for each network with leftover instances, so the - // RemoveHost call inside DeleteInstanceNetworkConfig has a backend to - // talk to (CreateInstance is idempotent — it adopts a surviving dnsmasq - // for this networkID or respawns a fresh one). for (const auto& entry : *entries) { - if (auto err = AdoptDNSServer(entry.mNetworkID); !err.IsNone()) { - LOG_WRN() << "Failed to adopt DNS server for leftover cleanup" << Log::Field("networkID", entry.mNetworkID) - << Log::Field(err); + if (entry.mHostIfName.IsEmpty()) { + continue; } - } - for (const auto& entry : *entries) { - if (auto err = DeleteInstanceNetworkConfig(entry.mInstanceID, entry.mNetworkID); !err.IsNone()) { + bool alive = false; + Error err; + + if (Tie(alive, err) = IsInstanceInterfaceAlive(entry.mInstanceID, entry.mHostIfName, entry.mBridgeIfName); + !err.IsNone()) { + LOG_WRN() << "Failed to check leftover instance interface" << Log::Field("instanceID", entry.mInstanceID) + << Log::Field("hostIfName", entry.mHostIfName) << Log::Field(err); + } + + if (alive) { + if (err = InitInstance(entry.mInstanceID, entry.mNetworkID); err.IsNone()) { + continue; + } else { + LOG_WRN() << "Failed to adopt leftover instance, falling back to cleanup" + << Log::Field("instanceID", entry.mInstanceID) << Log::Field(err); + } + } + + if (err = DeleteInstanceNetworkConfig(entry.mInstanceID, entry.mNetworkID); !err.IsNone()) { LOG_WRN() << "Failed to delete leftover instance network config" << Log::Field("instanceID", entry.mInstanceID) << Log::Field("networkID", entry.mNetworkID) << Log::Field(err); diff --git a/src/core/sm/networkmanager/networkmanager.hpp b/src/core/sm/networkmanager/networkmanager.hpp index 665dcac6c..e364ded38 100644 --- a/src/core/sm/networkmanager/networkmanager.hpp +++ b/src/core/sm/networkmanager/networkmanager.hpp @@ -202,12 +202,15 @@ class NetworkManager : public NetworkManagerItf { // Start()/OnConnect() run once, outside the concurrent instance-operation hot path, so their // allocations are added rather than multiplied by cMaxNumConcurrentItems: RemoveDNSOrphans' known - // networks list, CleanupLeftoverInstances' leftover instance/network ID pairs plus the - // InstanceNetworkInfo DeleteInstanceNetworkConfig allocates while clearing a host interface, and - // OnConnect's state sync snapshot. + // networks list, ReconcileInstances' leftover entries (instance/network IDs plus host/bridge + // interface names, so four ID-sized strings per instance) plus, alive at the same time, either the + // InstanceNetworkInfo DeleteInstanceNetworkConfig allocates while clearing a host interface or the + // config and hosts AdoptInstance allocates while restoring the runtime cache, and OnConnect's state + // sync snapshot. static constexpr auto cAllocatorSize = cMaxOperationAllocatorSize * cMaxNumConcurrentItems + sizeof(StaticArray, cMaxNumOwners>) - + sizeof(StaticArray, cMaxNumInstances>) * 2 + sizeof(InstanceNetworkInfo) + + sizeof(StaticArray, cMaxNumInstances>) * 4 + sizeof(InstanceNetworkInfo) + + sizeof(InstanceNetworkConfig) + sizeof(InstanceHosts) + sizeof(StaticArray); static constexpr auto cNumAllocations = 8 * cMaxNumConcurrentItems; @@ -226,9 +229,12 @@ class NetworkManager : public NetworkManagerItf { static constexpr auto cVlanIfPrefix = "vlan-"; static constexpr auto cResolvConfLineLen = AOS_CONFIG_NETWORKMANAGER_RESOLV_CONF_LINE_LEN; - Error IsInstanceInNetwork(const String& instanceID, const String& networkID) const; - Error AddInstanceToCache(const String& instanceID, const String& networkID); - Error CleanupLeftoverInstances(); + Error IsInstanceInNetwork(const String& instanceID, const String& networkID) const; + Error AddInstanceToCache(const String& instanceID, const String& networkID); + RetWithError IsInstanceInterfaceAlive( + const String& instanceID, const String& hostIfName, const String& bridgeIfName) const; + Error InitInstance(const String& instanceID, const String& networkID); + Error ReconcileInstances(); Error RemoveDNSOrphans(); Error AdoptDNSServer(const String& networkID); Error PrepareBridgeParams( diff --git a/src/core/sm/networkmanager/tests/networkmanager.cpp b/src/core/sm/networkmanager/tests/networkmanager.cpp index 984ce5fd1..d3a124ac4 100644 --- a/src/core/sm/networkmanager/tests/networkmanager.cpp +++ b/src/core/sm/networkmanager/tests/networkmanager.cpp @@ -227,16 +227,84 @@ class NetworkManagerTest : public Test { aos::ErrorEnum::eNone); } - void ExpectLinkExists(const aos::String& ifName, LinkKind kind) + void ExpectLinkExists(const aos::String& ifName, LinkKind kind, const aos::String& master = "") { LinkInfo link; - link.mName = ifName; - link.mKind = kind; + link.mName = ifName; + link.mKind = kind; + link.mMaster = master; EXPECT_CALL(mNetIf, GetLink(ifName, _)) .WillRepeatedly(DoAll(SetArgReferee<1>(link), Return(aos::ErrorEnum::eNone))); } + void RestartWithStoredState(const aos::Array& networks, + const aos::Array& instances) + { + EXPECT_CALL(mTrafficMonitor, Stop()).WillOnce(Return(aos::ErrorEnum::eNone)); + EXPECT_CALL(mFirewall, Stop()).WillOnce(Return(aos::ErrorEnum::eNone)); + ASSERT_EQ(mNetManager->Stop(), aos::ErrorEnum::eNone); + + EXPECT_CALL(mNetIf, DeleteLink(_)).Times(AnyNumber()).WillRepeatedly(Return(aos::ErrorEnum::eNone)); + mNetManager.reset(); + + EXPECT_CALL(mFirewall, Start()).WillOnce(Return(aos::ErrorEnum::eNone)); + EXPECT_CALL(mTrafficMonitor, Start()).WillOnce(Return(aos::ErrorEnum::eNone)); + + EXPECT_CALL(mStorage, GetNetworksInfo(_)) + .WillOnce(Invoke([&networks](aos::Array& out) { + out = networks; + return aos::ErrorEnum::eNone; + })); + EXPECT_CALL(mStorage, GetInstanceNetworksInfo(_)) + .WillOnce(Invoke([&instances](aos::Array& out) { + out = instances; + return aos::ErrorEnum::eNone; + })); + + mNetManager = std::make_unique(); + + ASSERT_EQ(mNetManager->Init(mStorage, mBridgeNetwork, mFirewall, mBandwidth, mDNSName, mTrafficMonitor, mNetns, + mNetIf, mRandom, mNetIfFactory, mNetworkProvider, "test-node"), + aos::ErrorEnum::eNone); + } + + aos::sm::networkmanager::InstanceNetworkInfo CreateLeftoverInstance( + const aos::sm::networkmanager::NetworkInfo& network) + { + aos::sm::networkmanager::InstanceNetworkInfo leftover; + leftover.mInstanceID = "leftover-instance"; + leftover.mNetworkID = network.mNetworkID; + leftover.mNetworkConfig.mHostname = "leftover-host"; + leftover.mAllocatedParams.mIP = "192.168.1.5"; + leftover.mAllocatedParams.mSubnet = network.mSubnet; + leftover.mHostIfName = "veth-leftover"; + + return leftover; + } + + void ExpectLeftoverInstanceCleaned() + { + EXPECT_CALL(mDNSName, CreateServer(_, _)).Times(0); + EXPECT_CALL(mDNSServer, RemoveHost(_)).Times(0); + EXPECT_CALL(mBandwidth, Clear(_)).WillOnce(Return(aos::ErrorEnum::eNone)); + EXPECT_CALL(mFirewall, RemoveInstance(_)).WillOnce(Return(aos::ErrorEnum::eNone)); + EXPECT_CALL(mBridgeNetwork, Detach(_, _)).WillOnce(Return(aos::ErrorEnum::eNone)); + EXPECT_CALL(mNetns, DeleteNetworkNamespace(_)).WillOnce(Return(aos::ErrorEnum::eNone)); + EXPECT_CALL(mStorage, UpdateInstanceNetworkInfo(_)).WillOnce(Return(aos::ErrorEnum::eNone)); + } + + void ExpectLeftoverInstanceUntouched() + { + EXPECT_CALL(mDNSName, CreateServer(_, _)).Times(0); + EXPECT_CALL(mDNSServer, RemoveHost(_)).Times(0); + EXPECT_CALL(mBandwidth, Clear(_)).Times(0); + EXPECT_CALL(mFirewall, RemoveInstance(_)).Times(0); + EXPECT_CALL(mBridgeNetwork, Detach(_, _)).Times(0); + EXPECT_CALL(mNetns, DeleteNetworkNamespace(_)).Times(0); + EXPECT_CALL(mStorage, UpdateInstanceNetworkInfo(_)).Times(0); + } + void ExpectStartInstanceOnStoredNetwork(const aos::String& instanceID, const aos::String& networkID) { EXPECT_CALL(mNetworkProvider, AllocateInstanceNetwork(_, networkID, aos::String("test-node"), _, _)) @@ -1318,84 +1386,98 @@ TEST_F(NetworkManagerTest, OnConnect_SyncsNetworkStateWithCM) mNetManager->OnConnect(); } -TEST_F(NetworkManagerTest, Start_AdoptsDNSForLeftoverInstancesAndCleansHosts) +TEST_F(NetworkManagerTest, Start_CleansLeftoverInstanceWithMissingInterface) { - // A leftover instance from a previous SM lifetime: its network is still - // in storage. On Start, NetworkManager should reap DNS orphans (with the - // known network in the list), then adopt the DNS handle for the leftover - // network and call RemoveHost while cleaning up the leftover instance. - aos::sm::networkmanager::NetworkInfo existingNetwork; - existingNetwork.mNetworkID = "leftover-net"; - existingNetwork.mIP = "192.168.7.1"; - existingNetwork.mSubnet = "192.168.7.0/24"; - existingNetwork.mVlanID = 700ULL; - existingNetwork.mVlanIfName = "vlan-leftover"; - existingNetwork.mBridgeIfName = "br-leftover"; - - aos::sm::networkmanager::InstanceNetworkInfo leftover; - leftover.mInstanceID = "leftover-instance"; - leftover.mNetworkID = existingNetwork.mNetworkID; - leftover.mAllocatedParams.mIP = "192.168.7.5"; - leftover.mAllocatedParams.mSubnet = existingNetwork.mSubnet; - leftover.mHostIfName = "veth-leftover"; + const auto network = CreateTestNetworkInfo(); + const auto leftover = CreateLeftoverInstance(network); aos::StaticArray networks; aos::StaticArray instances; - networks.PushBack(existingNetwork); + networks.PushBack(network); instances.PushBack(leftover); - EXPECT_CALL(mFirewall, Start()).WillOnce(Return(aos::ErrorEnum::eNone)); - EXPECT_CALL(mTrafficMonitor, Start()).WillOnce(Return(aos::ErrorEnum::eNone)); - - EXPECT_CALL(mStorage, GetNetworksInfo(_)) - .WillOnce(Invoke([&](aos::Array& out) { - out = networks; - return aos::ErrorEnum::eNone; - })); - EXPECT_CALL(mStorage, GetInstanceNetworksInfo(_)) - .WillOnce(Invoke([&](aos::Array& out) { - out = instances; - return aos::ErrorEnum::eNone; - })); - - // Stop the fixture instance — we drive Init/Start manually below. - EXPECT_CALL(mTrafficMonitor, Stop()).WillOnce(Return(aos::ErrorEnum::eNone)); - EXPECT_CALL(mFirewall, Stop()).WillOnce(Return(aos::ErrorEnum::eNone)); - ASSERT_EQ(mNetManager->Stop(), aos::ErrorEnum::eNone); - EXPECT_CALL(mNetIf, DeleteLink(_)).Times(AnyNumber()).WillRepeatedly(Return(aos::ErrorEnum::eNone)); - mNetManager.reset(); - - mNetManager = std::make_unique(); - - ASSERT_EQ(mNetManager->Init(mStorage, mBridgeNetwork, mFirewall, mBandwidth, mDNSName, mTrafficMonitor, mNetns, - mNetIf, mRandom, mNetIfFactory, mNetworkProvider, "test-node"), - aos::ErrorEnum::eNone); + RestartWithStoredState(networks, instances); - // RemoveOrphans must receive the known networkID from storage. EXPECT_CALL(mDNSName, RemoveOrphans(_)) .WillOnce(Invoke([&](const aos::Array>& known) { EXPECT_EQ(known.Size(), 1U); if (known.Size() == 1) { - EXPECT_EQ(known[0], existingNetwork.mNetworkID); + EXPECT_EQ(known[0], network.mNetworkID); } return aos::ErrorEnum::eNone; })); - // Pre-adopt: CreateInstance with the leftover network's bridge IP / ifname. - EXPECT_CALL(mDNSName, CreateServer(existingNetwork.mNetworkID, _)) - .WillOnce(Invoke([&](const aos::String&, const DNSServerParams& params) { - EXPECT_EQ(params.mBridgeIP, existingNetwork.mIP); - EXPECT_EQ(params.mBridgeIfName, existingNetwork.mBridgeIfName); - return aos::RetWithError {&mDNSServer, aos::ErrorEnum::eNone}; - })); + ExpectLeftoverInstanceCleaned(); - // Leftover instance cleanup goes through the adopted handle. - EXPECT_CALL(mDNSServer, RemoveHost(aos::String("leftover-instance"))).WillOnce(Return(aos::ErrorEnum::eNone)); - EXPECT_CALL(mBandwidth, Clear(_)).WillOnce(Return(aos::ErrorEnum::eNone)); - EXPECT_CALL(mFirewall, RemoveInstance(_)).WillOnce(Return(aos::ErrorEnum::eNone)); - EXPECT_CALL(mBridgeNetwork, Detach(_, _)).WillOnce(Return(aos::ErrorEnum::eNone)); - EXPECT_CALL(mNetns, DeleteNetworkNamespace(_)).WillOnce(Return(aos::ErrorEnum::eNone)); - EXPECT_CALL(mStorage, UpdateInstanceNetworkInfo(_)).WillOnce(Return(aos::ErrorEnum::eNone)); + ASSERT_EQ(mNetManager->Start(), aos::ErrorEnum::eNone); +} + +TEST_F(NetworkManagerTest, Start_KeepsLeftoverInstanceWithLiveInterface) +{ + const auto network = CreateTestNetworkInfo(); + const auto leftover = CreateLeftoverInstance(network); + + aos::StaticArray networks; + aos::StaticArray instances; + networks.PushBack(network); + instances.PushBack(leftover); + + RestartWithStoredState(networks, instances); + + ExpectLinkExists(leftover.mHostIfName, LinkKindEnum::eVeth, network.mBridgeIfName); + EXPECT_CALL(mNetns, IsNetworkNamespaceExist(leftover.mInstanceID)) + .WillRepeatedly(Return(aos::RetWithError {true, aos::ErrorEnum::eNone})); + + EXPECT_CALL(mDNSName, RemoveOrphans(_)).WillOnce(Return(aos::ErrorEnum::eNone)); + + ExpectLeftoverInstanceUntouched(); + + ASSERT_EQ(mNetManager->Start(), aos::ErrorEnum::eNone); + + EXPECT_TRUE( + mNetManager->StartInstanceNetwork(leftover.mInstanceID, network.mNetworkID).Is(aos::ErrorEnum::eAlreadyExist)); +} + +TEST_F(NetworkManagerTest, Start_CleansLeftoverInstanceWhenNamespaceMissing) +{ + const auto network = CreateTestNetworkInfo(); + const auto leftover = CreateLeftoverInstance(network); + + aos::StaticArray networks; + aos::StaticArray instances; + networks.PushBack(network); + instances.PushBack(leftover); + + RestartWithStoredState(networks, instances); + + ExpectLinkExists(leftover.mHostIfName, LinkKindEnum::eVeth, network.mBridgeIfName); + EXPECT_CALL(mNetns, IsNetworkNamespaceExist(leftover.mInstanceID)) + .WillRepeatedly(Return(aos::RetWithError {false, aos::ErrorEnum::eNone})); + + EXPECT_CALL(mDNSName, RemoveOrphans(_)).WillOnce(Return(aos::ErrorEnum::eNone)); + + ExpectLeftoverInstanceCleaned(); + + ASSERT_EQ(mNetManager->Start(), aos::ErrorEnum::eNone); +} + +TEST_F(NetworkManagerTest, Start_CleansLeftoverInstanceAttachedToForeignBridge) +{ + const auto network = CreateTestNetworkInfo(); + const auto leftover = CreateLeftoverInstance(network); + + aos::StaticArray networks; + aos::StaticArray instances; + networks.PushBack(network); + instances.PushBack(leftover); + + RestartWithStoredState(networks, instances); + + ExpectLinkExists(leftover.mHostIfName, LinkKindEnum::eVeth, "br-someoneelse"); + + EXPECT_CALL(mDNSName, RemoveOrphans(_)).WillOnce(Return(aos::ErrorEnum::eNone)); + + ExpectLeftoverInstanceCleaned(); ASSERT_EQ(mNetManager->Start(), aos::ErrorEnum::eNone); } From 59023050187c27f7a8f6f48c4088055a38765988 Mon Sep 17 00:00:00 2001 From: Mykola Solianko Date: Thu, 30 Jul 2026 21:02:24 +0300 Subject: [PATCH 4/6] sm: networkmanager: restore DNS handle before cleaning leftovers DeleteInstanceNetworkConfig removes the DNS record through the handle in mDNSServers, which is runtime state and is empty after a restart. Without a handle a dead leftover instance kept its addnhosts record. Adopt the handle before the cleanup again, now that DNSServer::Init loads the existing records instead of truncating them and so no longer drops the records of instances adopted on the same network. Signed-off-by: Mykola Solianko Reviewed-by: Mykola Kobets Reviewed-by: Oleksandr Grytsov Reviewed-by: Mykhailo Lohvynenko --- src/core/sm/networkmanager/networkmanager.cpp | 5 +++++ src/core/sm/networkmanager/tests/networkmanager.cpp | 5 +++-- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/src/core/sm/networkmanager/networkmanager.cpp b/src/core/sm/networkmanager/networkmanager.cpp index dc834c3d2..cfe992d48 100644 --- a/src/core/sm/networkmanager/networkmanager.cpp +++ b/src/core/sm/networkmanager/networkmanager.cpp @@ -1109,6 +1109,11 @@ Error NetworkManager::ReconcileInstances() } } + if (err = AdoptDNSServer(entry.mNetworkID); !err.IsNone()) { + LOG_WRN() << "Failed to adopt DNS server for leftover cleanup" << Log::Field("networkID", entry.mNetworkID) + << Log::Field(err); + } + if (err = DeleteInstanceNetworkConfig(entry.mInstanceID, entry.mNetworkID); !err.IsNone()) { LOG_WRN() << "Failed to delete leftover instance network config" << Log::Field("instanceID", entry.mInstanceID) << Log::Field("networkID", entry.mNetworkID) diff --git a/src/core/sm/networkmanager/tests/networkmanager.cpp b/src/core/sm/networkmanager/tests/networkmanager.cpp index d3a124ac4..c1588ca7f 100644 --- a/src/core/sm/networkmanager/tests/networkmanager.cpp +++ b/src/core/sm/networkmanager/tests/networkmanager.cpp @@ -285,8 +285,9 @@ class NetworkManagerTest : public Test { void ExpectLeftoverInstanceCleaned() { - EXPECT_CALL(mDNSName, CreateServer(_, _)).Times(0); - EXPECT_CALL(mDNSServer, RemoveHost(_)).Times(0); + EXPECT_CALL(mDNSName, CreateServer(_, _)) + .WillOnce(Return(aos::RetWithError {&mDNSServer, aos::ErrorEnum::eNone})); + EXPECT_CALL(mDNSServer, RemoveHost(aos::String("leftover-instance"))).WillOnce(Return(aos::ErrorEnum::eNone)); EXPECT_CALL(mBandwidth, Clear(_)).WillOnce(Return(aos::ErrorEnum::eNone)); EXPECT_CALL(mFirewall, RemoveInstance(_)).WillOnce(Return(aos::ErrorEnum::eNone)); EXPECT_CALL(mBridgeNetwork, Detach(_, _)).WillOnce(Return(aos::ErrorEnum::eNone)); From c9d105755479b00a81e8e584fe2fce79c99e9d03 Mon Sep 17 00:00:00 2001 From: Mykola Solianko Date: Thu, 30 Jul 2026 21:23:47 +0300 Subject: [PATCH 5/6] sm: networkmanager: reap orphan firewall artifacts on start FirewallItf gains RemoveOrphans so that the firewall no longer has to wipe its whole table on start to get rid of what a crashed SM left behind. Call it from Start with the instances and networks known from storage, so artifacts of anything gone are removed while the rules protecting the instances that kept running stay in place. Signed-off-by: Mykola Solianko Reviewed-by: Mykola Kobets Reviewed-by: Oleksandr Grytsov Reviewed-by: Mykhailo Lohvynenko --- src/core/sm/networkmanager/itf/firewall.hpp | 26 ++++++++++++++- src/core/sm/networkmanager/networkmanager.cpp | 32 +++++++++++++++++++ src/core/sm/networkmanager/networkmanager.hpp | 14 ++++---- .../tests/mocks/firewallmock.hpp | 1 + .../networkmanager/tests/networkmanager.cpp | 2 ++ 5 files changed, 68 insertions(+), 7 deletions(-) diff --git a/src/core/sm/networkmanager/itf/firewall.hpp b/src/core/sm/networkmanager/itf/firewall.hpp index 1520b25a5..3d6af4d28 100644 --- a/src/core/sm/networkmanager/itf/firewall.hpp +++ b/src/core/sm/networkmanager/itf/firewall.hpp @@ -53,6 +53,14 @@ struct InstanceFirewallParams { StaticArray mOutput; }; +/** + * Masquerade rule identity. + */ +struct MasqueradeParams { + StaticString mSubnet; + StaticString mOutIfName; +}; + /** * Firewall interface. * @@ -69,7 +77,8 @@ class FirewallItf { /** * Starts the firewall: creates the `inet aos` table, base chains and - * netfilter hooks (forward filter, postrouting nat). + * netfilter hooks (forward filter, postrouting nat) when they are absent. + * Rules that are already there are left alone. * * @return Error. */ @@ -82,6 +91,21 @@ class FirewallItf { */ virtual Error Stop() = 0; + /** + * Removes the instance chains and masquerade rules that no longer belong + * to anything known, keeping the rest in place. + * + * Called on start so that artifacts left by a crashed SM are reaped + * without touching the rules of the instances that kept running. + * + * @param knownInstanceIDs instance ids whose chains must be kept. + * @param knownMasquerades masquerade rules that must be kept. + * @return Error. + */ + virtual Error RemoveOrphans( + const Array>& knownInstanceIDs, const Array& knownMasquerades) + = 0; + /** * Adds a per-instance chain with the given input/output access rules. * diff --git a/src/core/sm/networkmanager/networkmanager.cpp b/src/core/sm/networkmanager/networkmanager.cpp index cfe992d48..201ad7863 100644 --- a/src/core/sm/networkmanager/networkmanager.cpp +++ b/src/core/sm/networkmanager/networkmanager.cpp @@ -105,6 +105,10 @@ Error NetworkManager::Start() } }); + if (err = RemoveFirewallOrphans(); !err.IsNone()) { + return err; + } + if (err = RemoveDNSOrphans(); !err.IsNone()) { return err; } @@ -1124,6 +1128,34 @@ Error NetworkManager::ReconcileInstances() return ErrorEnum::eNone; } +Error NetworkManager::RemoveFirewallOrphans() +{ + auto knownInstanceIDs = MakeUnique, cMaxNumInstances>>(&mAllocator); + auto knownMasquerades = MakeUnique>(&mAllocator); + + { + LockGuard lock {mMutex}; + + for (const auto& [instanceID, _] : mInstanceNetworkInfos) { + if (auto err = knownInstanceIDs->PushBack(instanceID); !err.IsNone()) { + return AOS_ERROR_WRAP(err); + } + } + + for (const auto& [_, network] : mNetworkProviders) { + if (auto err = knownMasquerades->PushBack({network.mSubnet, network.mBridgeIfName}); !err.IsNone()) { + return AOS_ERROR_WRAP(err); + } + } + } + + if (auto err = mFirewall->RemoveOrphans(*knownInstanceIDs, *knownMasquerades); !err.IsNone()) { + return AOS_ERROR_WRAP(err); + } + + return ErrorEnum::eNone; +} + Error NetworkManager::RemoveDNSOrphans() { auto known = MakeUnique, cMaxNumOwners>>(&mAllocator); diff --git a/src/core/sm/networkmanager/networkmanager.hpp b/src/core/sm/networkmanager/networkmanager.hpp index e364ded38..0c12f6500 100644 --- a/src/core/sm/networkmanager/networkmanager.hpp +++ b/src/core/sm/networkmanager/networkmanager.hpp @@ -202,14 +202,15 @@ class NetworkManager : public NetworkManagerItf { // Start()/OnConnect() run once, outside the concurrent instance-operation hot path, so their // allocations are added rather than multiplied by cMaxNumConcurrentItems: RemoveDNSOrphans' known - // networks list, ReconcileInstances' leftover entries (instance/network IDs plus host/bridge - // interface names, so four ID-sized strings per instance) plus, alive at the same time, either the - // InstanceNetworkInfo DeleteInstanceNetworkConfig allocates while clearing a host interface or the - // config and hosts AdoptInstance allocates while restoring the runtime cache, and OnConnect's state - // sync snapshot. + // networks list, RemoveFirewallOrphans' known instance and masquerade lists, ReconcileInstances' + // leftover entries (instance/network IDs plus host/bridge interface names, so four ID-sized strings + // per instance) plus, alive at the same time, either the InstanceNetworkInfo + // DeleteInstanceNetworkConfig allocates while clearing a host interface or the config and hosts + // InitInstance allocates while restoring the runtime cache, and OnConnect's state sync snapshot. static constexpr auto cAllocatorSize = cMaxOperationAllocatorSize * cMaxNumConcurrentItems + sizeof(StaticArray, cMaxNumOwners>) - + sizeof(StaticArray, cMaxNumInstances>) * 4 + sizeof(InstanceNetworkInfo) + + sizeof(StaticArray, cMaxNumInstances>) * 5 + + sizeof(StaticArray) + sizeof(InstanceNetworkInfo) + sizeof(InstanceNetworkConfig) + sizeof(InstanceHosts) + sizeof(StaticArray); static constexpr auto cNumAllocations = 8 * cMaxNumConcurrentItems; @@ -235,6 +236,7 @@ class NetworkManager : public NetworkManagerItf { const String& instanceID, const String& hostIfName, const String& bridgeIfName) const; Error InitInstance(const String& instanceID, const String& networkID); Error ReconcileInstances(); + Error RemoveFirewallOrphans(); Error RemoveDNSOrphans(); Error AdoptDNSServer(const String& networkID); Error PrepareBridgeParams( diff --git a/src/core/sm/networkmanager/tests/mocks/firewallmock.hpp b/src/core/sm/networkmanager/tests/mocks/firewallmock.hpp index 3f66c52cc..db9f668d4 100644 --- a/src/core/sm/networkmanager/tests/mocks/firewallmock.hpp +++ b/src/core/sm/networkmanager/tests/mocks/firewallmock.hpp @@ -17,6 +17,7 @@ class FirewallMock : public FirewallItf { public: MOCK_METHOD(Error, Start, (), (override)); MOCK_METHOD(Error, Stop, (), (override)); + MOCK_METHOD(Error, RemoveOrphans, (const Array>&, const Array&), (override)); MOCK_METHOD(Error, AddInstance, (const String&, const InstanceFirewallParams&), (override)); MOCK_METHOD(Error, RemoveInstance, (const String&), (override)); MOCK_METHOD(Error, UpdateInstance, (const String&, const InstanceFirewallParams&), (override)); diff --git a/src/core/sm/networkmanager/tests/networkmanager.cpp b/src/core/sm/networkmanager/tests/networkmanager.cpp index c1588ca7f..db6fdc061 100644 --- a/src/core/sm/networkmanager/tests/networkmanager.cpp +++ b/src/core/sm/networkmanager/tests/networkmanager.cpp @@ -42,6 +42,7 @@ class NetworkManagerTest : public Test { std::filesystem::create_directories(mWorkingDir.CStr()); EXPECT_CALL(mFirewall, Start()).WillOnce(Return(aos::ErrorEnum::eNone)); + EXPECT_CALL(mFirewall, RemoveOrphans(_, _)).WillOnce(Return(aos::ErrorEnum::eNone)); EXPECT_CALL(mTrafficMonitor, Start()).WillOnce(Return(aos::ErrorEnum::eNone)); // NetworkManager::Start reaps DNS orphans from a previous SM lifetime @@ -249,6 +250,7 @@ class NetworkManagerTest : public Test { mNetManager.reset(); EXPECT_CALL(mFirewall, Start()).WillOnce(Return(aos::ErrorEnum::eNone)); + EXPECT_CALL(mFirewall, RemoveOrphans(_, _)).WillOnce(Return(aos::ErrorEnum::eNone)); EXPECT_CALL(mTrafficMonitor, Start()).WillOnce(Return(aos::ErrorEnum::eNone)); EXPECT_CALL(mStorage, GetNetworksInfo(_)) From beaa5b289591d6e0843256972964955bba1244b7 Mon Sep 17 00:00:00 2001 From: Mykola Solianko Date: Fri, 31 Jul 2026 09:21:44 +0300 Subject: [PATCH 6/6] sm: networkmanager: restore traffic accounting for adopted instances Signed-off-by: Mykola Solianko Reviewed-by: Mykola Kobets Reviewed-by: Oleksandr Grytsov Reviewed-by: Mykhailo Lohvynenko --- src/core/sm/networkmanager/networkmanager.cpp | 21 +++++++++++++++++-- .../networkmanager/tests/networkmanager.cpp | 19 +++++++++++------ 2 files changed, 32 insertions(+), 8 deletions(-) diff --git a/src/core/sm/networkmanager/networkmanager.cpp b/src/core/sm/networkmanager/networkmanager.cpp index 201ad7863..f9863b38a 100644 --- a/src/core/sm/networkmanager/networkmanager.cpp +++ b/src/core/sm/networkmanager/networkmanager.cpp @@ -1031,7 +1031,8 @@ Error NetworkManager::InitInstance(const String& instanceID, const String& netwo } }); - auto config = MakeUnique(&mAllocator); + auto config = MakeUnique(&mAllocator); + StaticString instanceIP; { LockGuard lock {mMutex}; @@ -1043,7 +1044,8 @@ Error NetworkManager::InitInstance(const String& instanceID, const String& netwo return err; } - *config = it->mSecond.mNetworkConfig; + *config = it->mSecond.mNetworkConfig; + instanceIP = it->mSecond.mAllocatedParams.mIP; } auto hosts = MakeUnique(&mAllocator); @@ -1052,6 +1054,21 @@ Error NetworkManager::InitInstance(const String& instanceID, const String& netwo return err; } + if (err + = mNetMonitor->StartInstanceMonitoring(instanceID, instanceIP, config->mDownloadLimit, config->mUploadLimit); + !err.IsNone()) { + return AOS_ERROR_WRAP(err); + } + + auto cleanupMonitoring = DeferRelease(&instanceID, [this, &err](const String* id) { + if (!err.IsNone()) { + if (auto errStop = mNetMonitor->StopInstanceMonitoring(*id); !errStop.IsNone()) { + LOG_ERR() << "Failed to stop instance monitoring on rollback" << Log::Field("instanceID", *id) + << Log::Field(errStop); + } + } + }); + if (err = UpdateInstanceNetworkCache(instanceID, networkID, *hosts); !err.IsNone()) { return err; } diff --git a/src/core/sm/networkmanager/tests/networkmanager.cpp b/src/core/sm/networkmanager/tests/networkmanager.cpp index db6fdc061..382f351f5 100644 --- a/src/core/sm/networkmanager/tests/networkmanager.cpp +++ b/src/core/sm/networkmanager/tests/networkmanager.cpp @@ -275,12 +275,14 @@ class NetworkManagerTest : public Test { const aos::sm::networkmanager::NetworkInfo& network) { aos::sm::networkmanager::InstanceNetworkInfo leftover; - leftover.mInstanceID = "leftover-instance"; - leftover.mNetworkID = network.mNetworkID; - leftover.mNetworkConfig.mHostname = "leftover-host"; - leftover.mAllocatedParams.mIP = "192.168.1.5"; - leftover.mAllocatedParams.mSubnet = network.mSubnet; - leftover.mHostIfName = "veth-leftover"; + leftover.mInstanceID = "leftover-instance"; + leftover.mNetworkID = network.mNetworkID; + leftover.mNetworkConfig.mHostname = "leftover-host"; + leftover.mNetworkConfig.mDownloadLimit = 4096; + leftover.mNetworkConfig.mUploadLimit = 2048; + leftover.mAllocatedParams.mIP = "192.168.1.5"; + leftover.mAllocatedParams.mSubnet = network.mSubnet; + leftover.mHostIfName = "veth-leftover"; return leftover; } @@ -1435,6 +1437,11 @@ TEST_F(NetworkManagerTest, Start_KeepsLeftoverInstanceWithLiveInterface) ExpectLeftoverInstanceUntouched(); + EXPECT_CALL(mTrafficMonitor, + StartInstanceMonitoring(leftover.mInstanceID, leftover.mAllocatedParams.mIP, + leftover.mNetworkConfig.mDownloadLimit, leftover.mNetworkConfig.mUploadLimit)) + .WillOnce(Return(aos::ErrorEnum::eNone)); + ASSERT_EQ(mNetManager->Start(), aos::ErrorEnum::eNone); EXPECT_TRUE(