Skip to content

greendns: Use _compute_times on dnspython 2 (Updated)#722

Merged
temoto merged 8 commits intoeventlet:masterfrom
felixonmars:dnspython2
Sep 1, 2021
Merged

greendns: Use _compute_times on dnspython 2 (Updated)#722
temoto merged 8 commits intoeventlet:masterfrom
felixonmars:dnspython2

Conversation

@felixonmars
Copy link
Copy Markdown
Contributor

@felixonmars felixonmars commented Aug 16, 2021

This PR is based on #639

Rebased against latest master, and updated the GitHub CI instead of Travis.

I have also updated the tests for the last test failure left.

jayvdb and others added 2 commits August 16, 2021 23:19
@jayvdb @felixonmars
setup.py: Unpin
22b5169
@ralonsoh @felixonmars
Replace dnspython "_compute_expiration" by "_compute_times".
51569f3 
In dnspython v2.0.0, "_compute_expiration" was replaced by
"_compute_times". Once the minimum version of dnspython is
v2.0.0, we can remove this wrapping method.

Related to eventlet#629
@codecov

This comment has been minimized.

Sign in to view
temoto
temoto reviewed Aug 16, 2021
View reviewed changes
@temoto temoto requested review from jstasiak and tipabu August 16, 2021 16:05
@felixonmars
Copy link
Copy Markdown
Contributor Author

felixonmars commented Aug 16, 2021

The new test failure was introduced since dnspython 2.1.0. I have bisected to rthalley/dnspython@8c63cfd

@felixonmars
Copy link
Copy Markdown
Contributor Author

felixonmars commented Aug 16, 2021

Okay, found the cause. They are checking whether the QR flag is set now.

All green now :)

temoto
temoto approved these changes Aug 16, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@temoto temoto left a comment

Choose a reason for hiding this comment

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

LGTM, but I'm pretty sure someone reported that compute_time wasn't enough for compatibility with dnspython2.

So to actually merge this, we need to run some real world application using updated code. Testing volunteers are very welcome.

@felixonmars
Copy link
Copy Markdown
Contributor Author

felixonmars commented Aug 16, 2021 •
edited
Loading

LGTM, but I'm pretty sure someone reported that compute_time wasn't enough for compatibility with dnspython2.

So to actually merge this, we need to run some real world application using updated code. Testing volunteers are very welcome.

I took a look at the other related issue: #632

The original logs are not accessible now, but based on nova's bug report this seems to be caused by a module import order issue: https://bugs.launchpad.net/nova/+bug/1888237

In #684 we are patching ssl now, not sure if it helps on this. But IMHO if it's indeed caused by a module import order issue, it should be fixed at the corresponding project (by running eventlet monkeypatch in prior to importing dnspython), and we can do very little here.

@temoto temoto mentioned this pull request Aug 16, 2021
Closed
@temoto temoto mentioned this pull request Aug 16, 2021
Closed
@temoto
Copy link
Copy Markdown
Member

temoto commented Aug 16, 2021

Maintainer reminder before merge: add co-authored-by @ralonsoh and @jayvdb

@rthalley
Copy link
Copy Markdown

rthalley commented Aug 16, 2021

See my comments in #619 here. There were more changes than to just compute_times, as the API of dns.query.udp() and dns.query.tcp() changed too, and eventlet monkeypatching them back to old versions will cause failures with the resolver.

@felixonmars
Copy link
Copy Markdown
Contributor Author

felixonmars commented Aug 17, 2021

@rthalley Thank you, and sorry for missing that comment before. I have added all the missing parameters for udp() and tcp(), up to master. Hopefully the implementation makes sense.

@felixonmars
Add handling for new dnspython arguments
4496d99 
one_rr_per_rrset and ignore_trailing are present very long ago,
raise_on_truncation and sock are added since dnspython 2.0.
@filippog
Copy link
Copy Markdown

filippog commented Aug 31, 2021

I'm using python-eventlet from Debian Bullseye (0.26.1-7) and can confirm that with these changes DNS resolution works again with dnspython 2.0.0 and Openstack Swift 2.26.0-10

@temoto temoto merged commit aeb0390 into eventlet:master Sep 1, 2021
@felixonmars felixonmars deleted the dnspython2 branch September 1, 2021 08:56
@temoto temoto linked an issue Sep 13, 2021 that may be closed by this pull request
eventlet is incompatible with dnspython 2.0.0rc1 #619
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@temoto temoto temoto approved these changes

@tipabu tipabu Awaiting requested review from tipabu

@jstasiak jstasiak Awaiting requested review from jstasiak

Assignees

No one assigned

Labels

feature-dns importance-bug need-feedback

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

eventlet is incompatible with dnspython 2.0.0rc1

6 participants

@felixonmars @temoto @rthalley @filippog @jayvdb @ralonsoh