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

Fixing issue with multiple interface of brick and peer probe #815

Merged
merged 1 commit into from May 21, 2018

Conversation

GowthamShanmugam
Copy link
Contributor

@GowthamShanmugam GowthamShanmugam commented May 16, 2018

tendrl-bug-id: #814
bugzilla: 1573075, 1564510

Signed-off-by: GowthamShanmugasundaram gshanmug@redhat.com

@GowthamShanmugam GowthamShanmugam requested a review from a team as a code owner May 16, 2018 15:23
@GowthamShanmugam
Copy link
Contributor Author

@r0h4n @shtripat @nthomas-redhat please review

ip = socket.gethostbyname(brick_host)
node_id = etcd_client.read("indexes/ip/%s" % ip).value
_key = "/nodes/%s/NodeContext/fqdn" % node_id
fqdn = etcd_client.read(_key).value
Copy link
Contributor

Choose a reason for hiding this comment

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

before reading fqdn

ensure "node_id" is present at ""indexes/tags/tendrl/integration/:integration_id" ,

otherwise there can be clash between IP address in multi cluster scenario

brick_ip = socket.gethostbyname(brick_hostname)
else:
brick_ip = socket.gethostbyname(brick['hostname'])
brick_hostname = brick['hostname']
Copy link
Contributor

Choose a reason for hiding this comment

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

what is the purpose of L#553? , you are simply assigning new value to `brick_ip", the why the if/else above

brick_ip = socket.gethostbyname(
brick.get('hostname')
)
brick_hostname = brick.get('hostname')
Copy link
Contributor

Choose a reason for hiding this comment

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

This code snippet seems similar to the one I have commented below, can you please check if both are the same and make a common util or something if they are doing the same thing

brick_hostname = tendrl_glusterfs_utils.find_brick_host(
self.etcd_client, brick['hostname']
)
if brick_hostname:
Copy link
Contributor

Choose a reason for hiding this comment

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

again, this piece of code seems common, please dont duplicate this everywhere use single function

def __init__(self):
self.provisioner_only_plugin = False
TendrlGlusterfsMonitoringBase.__init__(self)
if not self.etcd_client:
_etcd_args = dict(
host=self.CONFIG['etcd_host'],
Copy link
Contributor

Choose a reason for hiding this comment

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

does the plugin config template have these etcd_* variables?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, it has, i verified

tendrl-bug-id: Tendrl#814
bugzilla: 1573075

Signed-off-by: GowthamShanmugasundaram <gshanmug@redhat.com>
@r0h4n r0h4n merged commit c8938e3 into Tendrl:master May 21, 2018
@TimothyAsirJeyasing
Copy link
Contributor

TimothyAsirJeyasing commented Jun 1, 2018

bz#bug-id 1573075 does not have all the required acks. So I am using bug-id 1564510 for downstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants