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

common: fix that $host always expands to localhost instead of actual hostname #12998

Merged
merged 1 commit into from Mar 1, 2017

Conversation

Projects
None yet
2 participants
@Liuchang0812
Copy link
Contributor

Liuchang0812 commented Jan 19, 2017

  • add function : ceph_get_hostname, ceph_get_short_hostname
  • add unittest for hostname function
  • expand $host in config.cc

TODO: update documentation about this feature

Fixes: http://tracker.ceph.com/issues/11081

Signed-off-by: liuchang0812 liuchang0812@gmail.com

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Jan 19, 2017

The following tests FAILED:
69 - unittest_config (Failed)
Errors while running CTest
Build step 'Execute shell' marked build as failure

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Jan 19, 2017

/home/liuchang/WorkSpace/ceph/src/test/common/test_config.cc:69: Failure
      Expected: public_network + " " + public_network + " " + lockdep + " " + "localhost"
      Which is: "NETWORK NETWORK true localhost"
To be equal to: val
      Which is: "NETWORK NETWORK true liuchang-VirtualBox"

@Liuchang0812 Liuchang0812 force-pushed the Liuchang0812:wip-11081 branch from e2412d9 to a43bada Jan 19, 2017

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Jan 19, 2017

I have tested this in my pc. It works ok.

# ceph.conf
[osd.0]                                                                                                             
        osd crush location = host=${host}lc root=not-default                     
[osd.1]                                                                                                                  
        osd crush location = host=lc${host}lc root=not-default                   
[osd.2]                                                                          
        host = osd2                                                              
        osd crush location = host=${host} root=not-default  

# query crush location 
➜  build git:(wip-11081) ✗ ./bin/ceph-conf --name=osd.0 --lookup=osd_crush_location
host=liuchang-VirtualBoxlc root=not-default
➜  build git:(wip-11081) ✗ ./bin/ceph-conf --name=osd.1 --lookup=osd_crush_location
host=lcliuchang-VirtualBoxlc root=not-default
➜  build git:(wip-11081) ✗ ./bin/ceph-conf --name=osd.2 --lookup=osd_crush_location
host=osd2 root=not-default

@liewegas @tchaikov mind taking a look. very thanks

@@ -13,7 +13,7 @@
*/

/* note: no header guard */
OPTION(host, OPT_STR, "localhost")
OPTION(host, OPT_STR, "default") // "default" means that host will be `hostname -s`

This comment has been minimized.

Copy link
@liewegas

liewegas Jan 19, 2017

Member

can we make it $hostname and $fqdn perhaps? if it's magic it should be a normal substitution variable..

This comment has been minimized.

Copy link
@liewegas

liewegas Jan 19, 2017

Member

hrm, maybe what we should really do is eliminate 'host' as a config option and turn it into a proper meta. i'm not sure why users would be configuring this?

This comment has been minimized.

Copy link
@liewegas

liewegas Jan 19, 2017

Member

if we think it is important, than we should make "" the value that looks at the real hostname. meaning the config option is nothing more than an override

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jan 20, 2017

Author Contributor

hi, @liewegas thanks for your comments. I'm agree with your advice that use $hostname and $fqdn.

but, there are a lot of $host config in user product env probably. Eliminating $host must be carefully.

in RGW, there is a option called rgw dns name, that is used in s3 authorization.In my dev/test env, I would like to config this option to really hostname. so providing a $hostname is useful.

This comment has been minimized.

Copy link
@liewegas

liewegas Jan 20, 2017

Member

I think if we just change teh default from "localhost" to "", and change "" to fill in the real hostname, we'll be pretty safe:

  • if someone configured a host, that won't change
  • if someone is using $host but is getting the default of 'localhost', they probably haven't noticed.. and hopefully won't notice if it starts getting a real hostname. We can add something to the release notes.
  • Same for if 'host = ' and they are using $host

This comment has been minimized.

Copy link
@Liuchang0812

Liuchang0812 Jan 20, 2017

Author Contributor

OK! sounds perfect, I am updating it.

so, we don't need to provide ${hostname} and ${fdqn} now?

@liewegas liewegas self-assigned this Jan 19, 2017

@liewegas

This comment has been minimized.

Copy link
Member

liewegas commented Jan 20, 2017

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Jan 20, 2017

@liewegas OK!

common: fix that $host always expands to localhost instead of actual …
…hostname

* add function : ceph_get_hostname, ceph_get_short_hostname
* add unittest for hostname function
* expand $host in config.cc
* remove hardcode hostname in unittest_config

TODO: update documentation about this feature

Fixes: http://tracker.ceph.com/issues/11081

Signed-off-by: liuchang0812 <liuchang0812@gmail.com>

@Liuchang0812 Liuchang0812 force-pushed the Liuchang0812:wip-11081 branch from a43bada to 95598d6 Jan 20, 2017

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Jan 20, 2017

@liewegas updated! take a look please.

@Liuchang0812

This comment has been minimized.

Copy link
Contributor Author

Liuchang0812 commented Feb 27, 2017

@liewegas liewegas merged commit 8d179d2 into ceph:master Mar 1, 2017

3 checks passed

Signed-off-by all commits in this PR are signed
Details
Unmodifed Submodules submodules for project are unmodified
Details
default Build finished.
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.