Skip to content
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.

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

[Feature](network interface) Support network interface #16617

Closed
wants to merge 12 commits into from

Conversation

ZashJie
Copy link
Contributor

@ZashJie ZashJie commented Feb 10, 2023

Proposed changes

Support network_interface in Apache Doris, the main changes are:

  1. If network_interface is configured in the configuration file, use network_interface configuration

Issue Number: close #xxx

Problem summary

Describe your changes.

Checklist(Required)

  1. Does it affect the original behavior:
    • Yes
    • No
    • I don't know
  2. Has unit tests been added:
    • Yes
    • No
    • No Need
  3. Has document been added or modified:
    • Yes
    • No
    • No Need
  4. Does it need to update dependencies:
    • Yes
    • No
  5. Are there any changes that cannot be rolled back:
    • Yes (If Yes, please explain WHY)
    • No

Further comments

If this is a relatively large or complex change, kick off the discussion at dev@doris.apache.org by explaining why you chose the solution you did and what alternatives you considered, etc...

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@hf200012
Copy link
Contributor

hf200012 commented Feb 10, 2023

LOG(INFO) << "priority cidrs in conf: " << config::priority_networks;

// 解析IP为cidr表达方式
bool BackendOptions::analyze_cidrs(const std::string & ip) {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no need to modify this function.

be/src/service/backend_options.cpp Outdated Show resolved Hide resolved
}
LOG(INFO) << "priority cidrs in conf: " << config::priority_networks;

// 解析IP为cidr表达方式
Copy link
Collaborator

Choose a reason for hiding this comment

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

please use english comment.

Copy link
Collaborator

@Yukang-Lian Yukang-Lian left a comment

Choose a reason for hiding this comment

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

please change your PR title format.

@@ -101,6 +100,61 @@ bool BackendOptions::analyze_priority_cidrs() {
return true;
}


//从network_interface中获取IP
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

// }
// _s_priority_cidrs.push_back(cidr);
// }
// return true;
Copy link
Collaborator

Choose a reason for hiding this comment

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

useless code?

// _s_priority_cidrs.push_back(cidr);
// }
// return true;
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

conf/be.conf Outdated

# data root path, separate by ';'
# you can specify the storage medium of each root path, HDD or SSD
# you can add capacity limit at the end of each root path, separate by ','
# eg:
# storage_root_path = /home/disk1/doris.HDD,50;/home/disk2/doris.SSD,1;/home/disk2/doris
# storage_root_path = /root/git/output/be/storage;/home/disk2/doris.SSD,1;/home/disk2/doris
# /home/disk1/doris.HDD, capacity limit is 50GB, HDD;
# /home/disk2/doris.SSD, capacity limit is 1GB, SSD;
Copy link
Collaborator

Choose a reason for hiding this comment

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

modify this file in your environment, but do not commit it.

@@ -52,7 +52,7 @@ mysql_service_nio_enabled = true
# If no ip match this rule, will choose one randomly.
# use CIDR format, e.g. 10.10.10.0/24
# Default value is empty.
# priority_networks = 10.10.10.0/24;192.168.0.0/16
priority_networks = 127.0.0.1/16

# Advanced configurations
# log_roll_size_mb = 1024
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto

@github-actions
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@morningman
Copy link
Contributor

Please amend your PR title and add describe in your PR comment.
And I suggest to wait #14063 merge

@ZashJie ZashJie changed the title ZhangShunjieNet interface [Feature](network interface) Support network interface Feb 13, 2023
@@ -49,6 +49,8 @@ CONF_Int32(single_replica_load_brpc_num_threads, "64");
// If no ip match this rule, will choose one randomly.
CONF_String(priority_networks, "");

CONF_Strings(network_interface, "");
Copy link
Contributor

Choose a reason for hiding this comment

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

String is enough.

return false;
}
LOG(INFO) << "network name in conf: " << config::network_interface[0] << "\tpriority cidrs in conf: " << config::network_interface[1];

Copy link
Contributor

Choose a reason for hiding this comment

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

we should not print cidrs here, it is confusing.

}
LOG(INFO) << "network name in conf: " << config::network_interface[0] << "\tpriority cidrs in conf: " << config::network_interface[1];

std::vector<std::string> cidr_strs =
Copy link
Contributor

Choose a reason for hiding this comment

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

variable name is confusing.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

}
LOG(INFO) << "network name in conf: " << config::network_interfaces;

struct ifaddrs * ifAddrStruct = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: use nullptr [modernize-use-nullptr]

Suggested change
struct ifaddrs * ifAddrStruct = NULL;
struct ifaddrs * ifAddrStruct = nullptr;

LOG(INFO) << "network name in conf: " << config::network_interfaces;

struct ifaddrs * ifAddrStruct = NULL;
void * tmpAddrPtr = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: use nullptr [modernize-use-nullptr]

Suggested change
void * tmpAddrPtr = NULL;
void * tmpAddrPtr = nullptr;

bool flag = false;

for (auto& name_str : name_strs) {
while (ifAddrStruct!=NULL) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: use nullptr [modernize-use-nullptr]

Suggested change
while (ifAddrStruct!=NULL) {
while (ifAddrStruct!=nullptr) {

if (config::network_interfaces == "") {
return true;
}
LOG(INFO) << "network name in conf: " << config::network_interfaces;
Copy link
Contributor

Choose a reason for hiding this comment

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

network interfaces in conf:


std::vector<std::string> name_strs =
strings::Split(config::network_interfaces, PRIORITY_CIDR_SEPARATOR);
bool flag = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

found may be a better name here.


getifaddrs(&ifAddrStruct);

std::vector<std::string> name_strs =
Copy link
Contributor

Choose a reason for hiding this comment

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

nic_names is a better var name.


for (auto& name_str : name_strs) {
while (ifAddrStruct!=NULL) {
if (ifAddrStruct->ifa_addr->sa_family==AF_INET) { // check it is IP4
Copy link
Contributor

Choose a reason for hiding this comment

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

we should put a space around !=, ==, +, - etc. liking while (ifAddrStruct != nullptr)

Copy link
Contributor

Choose a reason for hiding this comment

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

for (; if_addr_struct != nullptr; if_addr_struct = if_addr_struct->ifa_next) {
...
}

Copy link
Contributor

Choose a reason for hiding this comment

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

use if_addr_struct instead of ifAddrStruct.

if (ifAddrStruct->ifa_addr->sa_family==AF_INET) { // check it is IP4
// is a valid IP4 Address
if (name_str == ifAddrStruct->ifa_name) {
tmpAddrPtr=&((struct sockaddr_in *)ifAddrStruct->ifa_addr)->sin_addr;
Copy link
Contributor

Choose a reason for hiding this comment

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

tmpAddrPtr = &...

} else if (!_s_net_interfaces.empty()) {
if (is_in_net_interface(addr_it->get_host_address_v4())) {
_s_localhost = addr_it->get_host_address_v4();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

We should not follow cidrs here.

There is a problem here, e.g. There are 2 nics, eth0 and eth1, we configed eth1 and cidrs, if we tranverse eth0 first then cidr is used. we should let network interface is selected in priority.

So we should handle net_interface before this while.

@@ -21,6 +21,8 @@

#include <string>
#include <vector>
#include <ifaddrs.h>
#include <arpa/inet.h>
Copy link
Contributor

Choose a reason for hiding this comment

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

put the two .h to .cpp .

if (isInNetInterface(addr.getHostAddress())) {
localAddr = addr;
break;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

same as be.

Copy link
Contributor

@dataroaring dataroaring left a comment

Choose a reason for hiding this comment

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

Please have a look at the comments.

Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

}
LOG(INFO) << "network interfaces in conf: " << config::network_interfaces;

struct ifaddrs * if_Addr_Struct = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: use nullptr [modernize-use-nullptr]

Suggested change
struct ifaddrs * if_Addr_Struct = NULL;
struct ifaddrs * if_Addr_Struct = nullptr;

@github-actions github-actions bot added the kind/docs Categorizes issue or PR as related to documentation. label Mar 4, 2023
Copy link
Contributor

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

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

clang-tidy made some suggestions

}
LOG(INFO) << "network interfaces in conf: " << config::network_interfaces;

struct ifaddrs * if_addr_struct = NULL;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: use nullptr [modernize-use-nullptr]

Suggested change
struct ifaddrs * if_addr_struct = NULL;
struct ifaddrs * if_addr_struct = nullptr;

@github-actions
Copy link
Contributor

github-actions bot commented Mar 5, 2023

clang-tidy review says "All clean, LGTM! 👍"

dataroaring
dataroaring previously approved these changes Mar 5, 2023
Copy link
Contributor

@dataroaring dataroaring left a comment

Choose a reason for hiding this comment

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

LGTM

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2023

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2023

clang-tidy review says "All clean, LGTM! 👍"

1 similar comment
@github-actions
Copy link
Contributor

github-actions bot commented Mar 6, 2023

clang-tidy review says "All clean, LGTM! 👍"

@github-actions
Copy link
Contributor

github-actions bot commented Sep 3, 2023

We're closing this PR because it hasn't been updated in a while.
This isn't a judgement on the merit of the PR in any way. It's just a way of keeping the PR queue manageable.
If you'd like to revive this PR, please reopen it and feel free a maintainer to remove the Stale tag!

@github-actions github-actions bot added the Stale label Sep 3, 2023
@github-actions github-actions bot closed this Sep 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doris-future kind/docs Categorizes issue or PR as related to documentation. Stale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants