Skip to content

Add config-read and run thread_birds with arguments. #265

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Markuu-s wants to merge 13 commits into
base: bird-import-proto
Choose a base branch
from
Open

Add config-read and run thread_birds with arguments. #265

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
c0cfe2f
Update release workflow
GeorgyKirichenko Aug 14, 2024
dd064b6
Export image to Dockerhub
GeorgyKirichenko Aug 22, 2024
8b9ca68
Add pragma once to config.release.h
ol-imorozko Aug 2, 2024
e50920e
Append yanet-announcer.py into YANET image
ezhk Sep 10, 2024
7c177c7
Add total counters for ingress and egress packet drops
Sep 7, 2024
c5d0822
WIP: bird import proto
GeorgyKirichenko Jun 6, 2024
8581dc1
Start bird import thread
GeorgyKirichenko Nov 22, 2024
17e89f6
Add config-read and run thread_birds with arguments.
Markuu-s Dec 28, 2024
c5aaf0a
clang-format controlplane.cpp
Markuu-s Dec 28, 2024
63e7840
Merge remote-tracking branch 'origin/main' into HEAD
Markuu-s Dec 28, 2024
a9e7fde
Fix includes.
Markuu-s Dec 28, 2024
d7ca9ba
Merge remote-tracking branch 'origin/bird-import-proto' into HEAD
Markuu-s Dec 28, 2024
2d82f7e
Fix PR.
Markuu-s Jan 24, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 16 additions & 1 deletion common/controlplaneconfig.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
#include <nlohmann/json.hpp>

#include "balancer.h"
#include "define.h"
#include "scheduler.h"
#include "type.h"

Expand Down Expand Up @@ -104,6 +105,19 @@ class interface_t
common::globalBase::tFlow flow;
};

class bird_import_t
{
public:
SERIALIZABLE(socket, vrf);

public:
inline static const std::string socketStr = "socket";
inline static const std::string vrfStr = "vrf";
Comment on lines +114 to +115
Copy link
Collaborator

@ol-imorozko ol-imorozko Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
inline static const std::string socketStr = "socket";
inline static const std::string vrfStr = "vrf";
static constexpr auto socketStr = "socket";
static constexpr auto vrfStr = "vrf";

Copy link
Author

@Markuu-s Markuu-s Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. "constexpr" doesn`t include "inline" in all compilers
  2. I thought about "constexpr" instead of "const" buf std::string doesn`t support it
  3. auto give us const char[N], I think in C++ it worst

Copy link
Collaborator

@ol-imorozko ol-imorozko Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. Yeah, I think we do need inline here because we need that field to be ODR-usable: link

A static data member may be declared inline. An inline static data member can be defined in the class definition and may specify an initializer. It does not need an out-of-class definition:

struct X
{
    inline static int fully_usable = 1; // No out-of-class definition required, ODR-usable
    inline static const std::string class_name{"X"}; // Likewise
 
    static const int non_addressable = 1; // C.f. non-inline constants, usable
                                          // for its value, but not ODR-usable
    // static const std::string class_name{"X"}; // Non-integral declaration of this
                                                 // form is disallowed entirely
};

We're binding it to a reference by passing it to exist function => ODR-usage => we must provide exactly one definition of that static data member in the entire program and the simplest way to do so -- use inline here.

  1. Yeah, that's correct. Here you can use std::string_view for string literals, but the issue with it is that nlohmann::json doesn't have an overload for operator[] with std::string_view. However, it does have overloads for std::string and const char*. So you could either use the change I suggested, or go with std::string_view and pass .data() like this:
if (exist(elemJson, BirdImport::socketStr))
{
    import.socket = elemJson[BirdImport::socketStr.data()];
}

if (exist(elemJson, BirdImport::vrfStr))
{
    import.vrf = elemJson[BirdImport::vrfStr.data()];
}

Either way works for me.

  1. Why do you think so? IMHO using array types isn't inherently bad in C++. It's quite common and valid (as long as we don't decay them into pointers and/or perform pointer arithmetic :) ). I don't see any issues with using array types in this context.


std::string socket;
std::string vrf;
};

class config_t
{
public:
Expand All @@ -114,7 +128,7 @@ class config_t
nlohmann::json save() const;
*/

SERIALIZABLE(routeId, to_kernel_prefixes, vrf, tunnel_enabled, ignore_tables, ipv4_source_address, ipv6_source_address, udp_destination_port, local_prefixes, peers, interfaces);
SERIALIZABLE(routeId, to_kernel_prefixes, vrf, tunnel_enabled, ignore_tables, ipv4_source_address, ipv6_source_address, udp_destination_port, local_prefixes, peers, interfaces, bird_imports);

public:
tRouteId routeId;
Expand All @@ -128,6 +142,7 @@ class config_t
std::set<common::ip_prefix_t> local_prefixes; ///< for fallback to default
std::map<uint32_t, std::string> peers;
std::map<std::string, interface_t> interfaces;
std::vector<bird_import_t> bird_imports;
};

}
Expand Down
4 changes: 3 additions & 1 deletion common/icp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -671,7 +671,9 @@ using eor = std::tuple<std::string, ///< protocol
ip_address_t, ///< peer
std::string>; ///< table_name

using request = std::vector<std::variant<insert, remove, clear, eor>>;
using action = std::variant<insert, remove, clear, eor>;

using request = std::vector<action>;
}

namespace rib_summary
Expand Down
33 changes: 33 additions & 0 deletions controlplane/configparser.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,32 @@ controlplane::base_t config_parser_t::loadConfig(const std::string& rootFilePath
return baseNext;
}

void config_parser_t::loadConfig_route_bird([[maybe_unused]]controlplane::base_t& baseNext,
std::vector<controlplane::route::bird_import_t>& birdsImport,
const nlohmann::json& birdJson)
{
using BirdImport = controlplane::route::bird_import_t;
for (const auto& elemJson : birdJson)
{
BirdImport import;

if (exist(elemJson, BirdImport::socketStr))
{
import.socket = elemJson[BirdImport::socketStr];
}

if (exist(elemJson, BirdImport::vrfStr))
{
import.vrf = elemJson[BirdImport::vrfStr];
}
Comment on lines +209 to +217
Copy link
Collaborator

@ol-imorozko ol-imorozko Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if those keys do not exist? We need to use some default value then, right? Otherwise I think it will be two empty strings.
It's better to use value(const typename object_t::key_type& key, const ValueType& default_value) method of nlohmann::json like so:

Suggested change
if (exist(elemJson, BirdImport::socketStr))
{
import.socket = elemJson[BirdImport::socketStr];
}
if (exist(elemJson, BirdImport::vrfStr))
{
import.vrf = elemJson[BirdImport::vrfStr];
}
import.socket = elemJson.value(BirdImport::socketStr, "some_default_value");
import.vrf = elemJson.value(BirdImport::vrfStr, "some_default_value");


YANET_LOG_INFO("loadConfig_route_bird: socket(%s), vrf(%s)\n",
import.socket.data(),
import.vrf.data());
birdsImport.push_back(std::move(import));
}
}

void config_parser_t::loadConfig_logicalPort(controlplane::base_t& baseNext,
const std::string& moduleId,
const nlohmann::json& moduleJson)
Expand Down Expand Up @@ -367,6 +393,13 @@ void config_parser_t::loadConfig_route(controlplane::base_t& baseNext,
route.tunnel_enabled = false;
}

if (exist(moduleJson, "birdImport"))
{
loadConfig_route_bird(baseNext,
route.bird_imports,
moduleJson["birdImport"]);
}

//

route.routeId = routeId;
Expand Down
1 change: 1 addition & 0 deletions controlplane/configparser.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ class config_parser_t
void loadConfig_logicalPort(controlplane::base_t& baseNext, const std::string& moduleId, const nlohmann::json& moduleJson);
void loadConfig_route(controlplane::base_t& baseNext, const std::string& moduleId, const nlohmann::json& moduleJson, const std::string& rootFilePath, const std::map<std::string, nlohmann::json>& jsons);
void loadConfig_route_peers(controlplane::base_t& baseNext, controlplane::route::config_t& route, const nlohmann::json& json, const std::string& rootFilePath, const std::map<std::string, nlohmann::json>& jsons);
void loadConfig_route_bird(controlplane::base_t& baseNext, std::vector<controlplane::route::bird_import_t>& birdsImport, const nlohmann::json& birdJson);
void loadConfig_decap(controlplane::base_t& baseNext, const std::string& moduleId, const nlohmann::json& moduleJson);
void loadConfig_nat64stateful(controlplane::base_t& baseNext, const std::string& moduleId, const nlohmann::json& moduleJson, const std::string& rootFilePath, const std::map<std::string, nlohmann::json>& jsons);
void loadConfig_nat64stateless(controlplane::base_t& baseNext, const std::string& moduleId, const nlohmann::json& moduleJson, const std::string& rootFilePath, const std::map<std::string, nlohmann::json>& jsons);
Expand Down
20 changes: 20 additions & 0 deletions controlplane/controlplane.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -538,6 +538,18 @@ common::icp::controlplane_values::response cControlPlane::controlplane_values()
return response;
}

common::icp::route_config::response cControlPlane::getRoute() const
{
common::icp::route_config::response response;

{
auto current_guard = generations.current_lock_guard();
response = generations.current().routes;
}

return response;
}

common::icp::getDecapPrefixes::response cControlPlane::command_getDecapPrefixes()
{
common::icp::getDecapPrefixes::response response;
Expand Down Expand Up @@ -941,6 +953,14 @@ eResult cControlPlane::loadConfig(const std::string& rootFilePath,
}

YANET_LOG_INFO("dataplane has been updated (stage 7)\n");
for (auto& module : modules)
{
if (rib_t* rib = dynamic_cast<rib_t*>(module))
{
rib->bird_import_get();
rib->moduleStart();
}
}
}
Comment on lines +956 to 964
Copy link
Collaborator

@ol-imorozko ol-imorozko Jan 14, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is every module is also a rib module? I don't quite understand.
Also, dynamic_cast will inevitably lead to some errors, if, for example, we decide to change class hierarchy.
Can we use other things here? Like maybe we can utilize one or two virtual functions?

Copy link
Author

@Markuu-s Markuu-s Jan 24, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At this stage, I need to call one method that is only in rib_t (bird_import_get()), using a virtual function for all classes is somehow strange. I'll think about it, or I'm ready to discuss what's best.

Copy link
Collaborator

@ol-imorozko ol-imorozko Feb 4, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

modules is a vector of cModule pointers. So what you can do is add a new field to the class that will point to the required module, and here just call it's methods.
Something like an iterator to the modules will suffice (but be aware of invalidation). Or just a pointer/reference to rib_t.
Either way it's better than what we have how.

catch (const error_result_t& error)
{
Expand Down
1 change: 1 addition & 0 deletions controlplane/controlplane.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -106,6 +106,7 @@ class cControlPlane
common::icp::acl_unwind::response acl_unwind(const common::icp::acl_unwind::request& request) const;
common::icp::acl_lookup::response acl_lookup(const common::icp::acl_lookup::request& request) const;
common::icp::controlplane_values::response controlplane_values() const;
common::icp::route_config::response getRoute() const;

common::icp::getDecapPrefixes::response command_getDecapPrefixes();
common::icp::getNat64statelessTranslations::response command_getNat64statelessTranslations();
Expand Down
Loading