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

Use getent instead of nslookup for starting scripts #243

Merged
merged 1 commit into from
Aug 23, 2021

Conversation

jinfwhuang
Copy link
Contributor

The nslookup only exit with status 0 with successful lookup of dns record for both forward and backward lookups. It would mean that the cluster's DNS setup has to support. getent only performs a forward lookup.

#76

@codecov-commenter
Copy link

codecov-commenter commented Sep 16, 2020

Codecov Report

Merging #243 (f25be3b) into master (c62716b) will not change coverage.
The diff coverage is n/a.

❗ Current head f25be3b differs from pull request most recent head a454656. Consider uploading reports for the commit a454656 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master     #243   +/-   ##
=======================================
  Coverage   86.00%   86.00%           
=======================================
  Files          11       11           
  Lines        1572     1572           
=======================================
  Hits         1352     1352           
  Misses        147      147           
  Partials       73       73           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update c62716b...a454656. Read the comment docs.

@amuraru
Copy link
Contributor

amuraru commented Oct 9, 2020

@jinfwhuang can you please rebase your patch on top of latest master? thanks

@spiegela spiegela requested a review from anishakj October 9, 2020 14:25
@jinfwhuang jinfwhuang force-pushed the fix-zk-startup-script branch from cfd3571 to b5268fc Compare February 1, 2021 05:36
@jinfwhuang
Copy link
Contributor Author

@amuraru Updated this PR.

@derekm
Copy link
Contributor

derekm commented Mar 11, 2021

I think the claim that nslookup requires both forward A/AAAA records and reverse PTR records is false.

Rather, nslookup uses DNS-only lookups and does not return entries from /etc/hosts. Whereas, getent hosts does resolution according to Name Service Switch configuration.

This being the case, using nslookup does hinder /etc/hosts-only deployments.

@amuraru
Copy link
Contributor

amuraru commented Mar 12, 2021

@derekm thanks for your analysis.
I don't have a strong opinion for this patch - we've seen this on some of our clusters but those are not critical atm so we can drop this.

@jinfwhuang
Copy link
Contributor Author

I think getent is simpler than nslookup. It works for more cases. Why not?

@anishakj
Copy link
Contributor

@jinfwhuang Are you still facing issues with nslookup?

@jinfwhuang
Copy link
Contributor Author

@jinfwhuang Are you still facing issues with nslookup?

We have been running our zookeeper clusters with the patch in the past year.

@anishakj
Copy link
Contributor

@jinfwhuang Are you still facing issues with nslookup?

We have been running our zookeeper clusters with the patch in the past year.

@jinfwhuang Could you please sign-off the commit?

Copy link
Contributor Author

@jinfwhuang jinfwhuang left a comment

Choose a reason for hiding this comment

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

Looks good to me

@anishakj
Copy link
Contributor

Looks good to me

@jinfwhuang If you see DCO check is failing, because commits are not signed off

@jinfwhuang jinfwhuang force-pushed the fix-zk-startup-script branch from 29cfbd3 to f25be3b Compare August 19, 2021 16:53
@jinfwhuang jinfwhuang force-pushed the fix-zk-startup-script branch from f25be3b to a454656 Compare August 19, 2021 16:55
@jinfwhuang
Copy link
Contributor Author

jinfwhuang commented Aug 19, 2021

Looks good to me

@jinfwhuang If you see DCO check is failing, because commits are not signed off

Okay. Updated the commit.

Question, why was signoff necessary for this project?

@anishakj
Copy link
Contributor

Looks good to me

@jinfwhuang If you see DCO check is failing, because commits are not signed off

Okay. Updated the commit.

Question, why was signoff necessary for this project?

In Pravega, we are using DCO instead of CLA, so authors acknowledge and retain their copyrights

Copy link
Contributor

@anishakj anishakj left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@derekm derekm left a comment

Choose a reason for hiding this comment

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

It will be nice to help a user get off of a private fork!

@anishakj anishakj merged commit c4113df into pravega:master Aug 23, 2021
@amuraru
Copy link
Contributor

amuraru commented Sep 3, 2021

@jinfwhuang I still see a reference to nslookup here

elif nslookup $DOMAIN | grep -q "server can't find $DOMAIN"; then

Should this be updated as well?
thanks

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.

5 participants