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

mgr: add the ip addr of standbys #16476

Merged
merged 1 commit into from Sep 5, 2017

Conversation

Projects
None yet
5 participants
@renhwztetecs
Copy link
Member

commented Jul 21, 2017

we need to manage the ip addr of the "standbys" state,
because the hostname/gid is insufficient to locate the
Standby node.

[root@node173 build]# ceph mgr metadata
[
{
"id": "node173",
"addr": "10.118.202.173",
"arch": "x86_64",
"ceph_version": "ceph version 12.1.2-589-gc8c837f (c8c837f) luminous (rc)",
"cpu": "Intel(R) Core(TM) i5-2400 CPU @ 3.10GHz",
"distro": "centos",
"distro_description": "CentOS Linux 7 (Core)",
"distro_version": "7",
"hostname": "node173",
"kernel_description": "#1 SMP Mon Jun 30 12:09:22 UTC 2014",
"kernel_version": "3.10.0-123.el7.x86_64",
"mem_swap_kb": "4079612",
"mem_total_kb": "3768768",
"os": "Linux"
}
]

Signed-off-by: huanwen ren ren.huanwen@zte.com.cn

@jcsp

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

I'm not sure why this is needed -- what is it that needs to see the address of the client?

For human eyes, the name is what is meant to act as the identifier.

@renhwztetecs renhwztetecs force-pushed the renhwztetecs:renhw-wip-mgr-standby-ip branch 3 times, most recently from 21dec25 to b29a658 Jul 21, 2017

@renhwztetecs

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2017

@jcsp
Usually host hostname only one, but the corresponding IP address may exist more than one, we do not know the use of Mgr is that ip address

@liuchang0812

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2017

I think it would be better to put the IP informations to ceph mgr metadata. see liuchang0812#4

@renhwztetecs renhwztetecs force-pushed the renhwztetecs:renhw-wip-mgr-standby-ip branch from b29a658 to 9cd8c73 Jul 21, 2017

@renhwztetecs

This comment has been minimized.

Copy link
Member Author

commented Jul 21, 2017

@liuchang0812 👍
I would like to get the ip adds information from monc in simple way.

@liuchang0812

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2017

sage merged metadata command PR already, you could use pybind to get standby's addresses

@jcsp

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2017

I'm still not clear why this is needed. Your mgr daemon name is unique, and if it is a hostname then you can resolve it to an IP address (externally) if you need to.

Can you explain (beyond just saying that you want to know the IP address), why you need it. Is there some external piece of software that wants to know?

@renhwztetecs

This comment has been minimized.

Copy link
Member Author

commented Jul 27, 2017

@liuchang0812
thank you for your hint
@jcsp
yes, we have their own management system need to know all the state of mgr, including standby, and for our management system ip address is more intuitive than the host name

@tchaikov

This comment has been minimized.

Copy link
Contributor

commented Jul 27, 2017

@renhwztetecs why it's more intuitive ? is there any additional information that you cannot get by resolving the IP addresss?

@renhwztetecs

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2017

@tchaikov I'm later
If the standby provides ip address, we can directly through the MgrMap to obtain all the standby IP address in the cluster, if there is no standby IP address, you also need to go to hostname to find IP

@@ -157,7 +157,7 @@ void MgrStandby::send_beacon()
// as available in the map)
bool available = active_mgr != nullptr && active_mgr->is_initialized();

auto addr = available ? active_mgr->get_server_addr() : entity_addr_t();
auto addr = available ? active_mgr->get_server_addr() : monc.get_my_addr();

This comment has been minimized.

Copy link
@liewegas

liewegas Aug 8, 2017

Member

The risk here seems to be that someone might assume the standby addr will become the active addr when that's not actually the case. That aside I do'nt have a philosophical problem with this

This comment has been minimized.

Copy link
@renhwztetecs

renhwztetecs Aug 10, 2017

Author Member

thanks for your suggest

@liewegas liewegas added mgr mon labels Aug 8, 2017

@jcsp

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2017

Since 1bf4a89, the mgr has sent metadata to the mons -- I would be okay with adding the IP address there. But let's make it just the IP address, and not the whole entity_addr_t that comes out of monc.get_my_addr().

I prefer to use metadata rather than mgrmap, because the metadata's purpose is to hold advisory information that may be interesting to external systems, whereas the mgrmap is for information that Ceph needs internally.

@liewegas

This comment has been minimized.

Copy link
Member

commented Aug 9, 2017

👍

@renhwztetecs

This comment has been minimized.

Copy link
Member Author

commented Aug 10, 2017

I will change to metadata

@renhwztetecs renhwztetecs force-pushed the renhwztetecs:renhw-wip-mgr-standby-ip branch from 9cd8c73 to be62513 Aug 12, 2017

@renhwztetecs

This comment has been minimized.

Copy link
Member Author

commented Aug 12, 2017

jenkins retest this please

@renhwztetecs

This comment has been minimized.

Copy link
Member Author

commented Aug 14, 2017

changed to metadata



const char *host_ip = NULL;
char addr_buf[129];

This comment has been minimized.

Copy link
@jcsp

jcsp Aug 14, 2017

Contributor

Avoid hardcoded buffer size, I guess this should be MAX(INET_ADDRSTRLEN, INET6_ADDRSTRLEN) + 1?

This comment has been minimized.

Copy link
@renhwztetecs

renhwztetecs Aug 15, 2017

Author Member

Yes, this is more elegant.
For INET and INET6 do a different definition, but also do not need "+1"

default:
break;
};
metadata["addr"] = host_ip;

This comment has been minimized.

Copy link
@jcsp

jcsp Aug 14, 2017

Contributor

inet_ntop returns null on errors -- must check the return value to avoid possibility of assigning null to a std string.

This comment has been minimized.

Copy link
@renhwztetecs

renhwztetecs Aug 15, 2017

Author Member

@jcsp
I assigned a value of “0.0.0.0” when "host_ip" is NULL, and log it,
I'm not sure if there is a better way.
thank you

@renhwztetecs renhwztetecs force-pushed the renhwztetecs:renhw-wip-mgr-standby-ip branch from be62513 to 97ff0c7 Aug 15, 2017

break;
};
if (NULL == host_ip) {
metadata["addr"] = "0.0.0.0";

This comment has been minimized.

Copy link
@jcsp

jcsp Aug 15, 2017

Contributor

If the addr isn't AF_INT or AF_INET6, then it's probably not IP at all (an RDMA messenger or something like that), so I'd just return an empty string rather than something IP-like.

This comment has been minimized.

Copy link
@renhwztetecs

renhwztetecs Aug 15, 2017

Author Member

yes,""is good


switch(addr_standby.get_family()) {
case AF_INET:
char addr_buf_inet[INET_ADDRSTRLEN];

This comment has been minimized.

Copy link
@jcsp

jcsp Aug 15, 2017

Contributor

Defining the buffer inside the switch {} and then using the host_ip pointer to it from outside the {} seems incorrect -- I think the compiler is allowed to re-use that space after the end of the {}

This comment has been minimized.

Copy link
@renhwztetecs

renhwztetecs Aug 15, 2017

Author Member

@jcsp
Yes has this problem,thanks.
unified use char addr_buf[INET6_ADDRSTRLEN],
because INET6_ADDRSTRLEN=46 > INET_ADDRSTRLEN=16.
reference:https://linux.die.net/man/3/inet_pton

This comment has been minimized.

Copy link
@renhwztetecs

renhwztetecs Aug 15, 2017

Author Member

@jcsp
All changed it.

@renhwztetecs renhwztetecs force-pushed the renhwztetecs:renhw-wip-mgr-standby-ip branch 2 times, most recently from c1fc759 to c56a279 Aug 15, 2017

@jcsp

jcsp approved these changes Aug 24, 2017

@renhwztetecs

This comment has been minimized.

Copy link
Member Author

commented Aug 25, 2017

jenkins retest this please

INET6_ADDRSTRLEN);
break;
default:
break;

This comment has been minimized.

Copy link
@liewegas

liewegas Aug 30, 2017

Member

Can you move this into an entity_addr_t method (msg/msg_types.h) called something like std::string ip_only_to_str() const?

This comment has been minimized.

Copy link
@liewegas

liewegas Aug 30, 2017

Member

Then the change to this files becomes a single line,

metadata["addr"] = monc.get_my_addr().ip_only_to_str();

This comment has been minimized.

Copy link
@renhwztetecs

renhwztetecs Aug 31, 2017

Author Member

@liewegas
I have changed it

@renhwztetecs renhwztetecs force-pushed the renhwztetecs:renhw-wip-mgr-standby-ip branch from c56a279 to fa107a5 Aug 30, 2017

} else {
ip_str = host_ip;
}
return ip_str;

This comment has been minimized.

Copy link
@liewegas

liewegas Aug 31, 2017

Member

this can be simplified to


return host_ip ? host_ip : "";

and drop the ip_str decl

Otherwise, looks good, thanks!

This comment has been minimized.

Copy link
@renhwztetecs

renhwztetecs Aug 31, 2017

Author Member

@liewegas
“Less is More” :-)
updated it

@renhwztetecs renhwztetecs force-pushed the renhwztetecs:renhw-wip-mgr-standby-ip branch 2 times, most recently from ae4c79a to 6fb8e48 Aug 31, 2017

@@ -384,6 +385,24 @@ struct entity_addr_t {
}
}

std::string ip_only_to_str() {

This comment has been minimized.

Copy link
@tchaikov

tchaikov Aug 31, 2017

Contributor

please mark this method const. and move the implementation to .cc file. as msg_types.h is included by quite a few places, put the implementation here could prolong the compiling time.

This comment has been minimized.

Copy link
@renhwztetecs

renhwztetecs Sep 1, 2017

Author Member

At first I also considered this problem, but found that all the implementation in "msg_types.h"

This comment has been minimized.

Copy link
@tchaikov

tchaikov Sep 1, 2017

Contributor

i don't get you. could you rephrase you reply?

This comment has been minimized.

Copy link
@renhwztetecs

renhwztetecs Sep 1, 2017

Author Member

sorry, I may not have described it clearly, I mean whether all the implementations in msg_types.h are placed in msg_types.cc?

This comment has been minimized.

Copy link
@tchaikov

tchaikov Sep 1, 2017

Contributor

not all.

This comment has been minimized.

Copy link
@liewegas

liewegas Sep 1, 2017

Member

let's put this one in the .cc file. we can move others later.

This comment has been minimized.

Copy link
@renhwztetecs

renhwztetecs Sep 4, 2017

Author Member

@liewegas
ok! updated it

@renhwztetecs renhwztetecs force-pushed the renhwztetecs:renhw-wip-mgr-standby-ip branch 2 times, most recently from da39c8b to d52781a Sep 1, 2017

@@ -290,3 +290,22 @@ void entity_addrvec_t::generate_test_instances(list<entity_addrvec_t*>& ls)
ls.back()->v.push_back(entity_addr_t());
ls.back()->v.push_back(entity_addr_t());
}

const std::string ip_only_to_str()

This comment has been minimized.

Copy link
@tchaikov

tchaikov Sep 1, 2017

Contributor

i mean, std::string ip_only_to_str() const. as this method is not changing in4_addr, IIUC.

This comment has been minimized.

Copy link
@renhwztetecs

renhwztetecs Sep 4, 2017

Author Member

@tchaikov
updated it, thank you

mgr: add the ip addr of standbys
we need to manage the ip addr of the "standbys" state,
because the hostname/gid is insufficient to locate the
Standby node. we add ip of the mgr standby to metadata.

Signed-off-by: huanwen ren <ren.huanwen@zte.com.cn>

@renhwztetecs renhwztetecs force-pushed the renhwztetecs:renhw-wip-mgr-standby-ip branch from d52781a to 389d676 Sep 4, 2017

@tchaikov tchaikov added the needs-qa label Sep 4, 2017

comments addressed

@jcsp

jcsp approved these changes Sep 4, 2017

@tchaikov tchaikov merged commit c46220e into ceph:master Sep 5, 2017

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.