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.

Sign up for GitHub

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

Jump to bottom

DNS manual cleanup mechanism added #11535

Merged
merged 3 commits into from
Sep 30, 2019
Merged

DNS manual cleanup mechanism added #11535

merged 3 commits into from
Sep 30, 2019

Conversation

AnttiKauppila
Copy link

@AnttiKauppila AnttiKauppila commented Sep 20, 2019 •
edited by 0xc0170
Loading

Description

Added a mechanism to clean up DNS cache when not needed.
DNS test cases updated to cleanup cache before running those.

Pull request type

[ ] Fix
[ ] Refactor
[ ] Target update
[X] Functionality change
[ ] Docs update
[ ] Test update
[ ] Breaking change

Reviewers

@SeppoTakalo @jamesbeyond @ARMmbed/mbed-os-ipcore

Release Notes

Add nsapi_dns_reset() function to be able to clear a DNS cache when it is not needed anymore.

SeppoTakalo
SeppoTakalo suggested changes Sep 20, 2019
View reviewed changes
features/netsocket/nsapi_dns.h
* NOTE: Does NOT clear asynchronous ongoing operations!
* Currently only cleans up DNS cache (if used)
*/
void nsapi_dns_reset();
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this in the public API?

I don't see any real use cases for resetting the cache, other than to test it.

Copy link
Author

Choose a reason for hiding this comment

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

We don't have, but some application developer might want to use it so why should we prevent that?

Copy link
Author

Choose a reason for hiding this comment

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

The real solution should be that when last NetworkStack is freed, we should free all DNS related stuff as well. Or if permanent DNS cache is needed, we could flag that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Such automatic purging should be optional. Cached information is still valid even if a network interface goes down.

No objection to adding a public API for test and/or application use though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Our DNS greentea tests use cache, even though perhaps sometimes they shouldn't. For example, if you run the whole suite, then the _SIMULTANOUS tests which are supposed to run 5 DNS requests at the same time, will in fact take three addresses resolved in previous tests out of cache and only open two more sockets to resolve the non-cached addresses.
With this API in place we can clear cache where we don't want the test to rely on it.

@ciarmcom
Copy link
Member

@AnttiKauppila, thank you for your changes.
@SeppoTakalo @jamesbeyond @ARMmbed/mbed-os-test @ARMmbed/mbed-os-ipcore @ARMmbed/mbed-os-maintainers please review.

@AnttiKauppila
Copy link
Author

@ARMmbed/mbed-os-maintainers Please proceed

0xc0170
0xc0170 requested changes Sep 25, 2019
View reviewed changes
Copy link
Contributor

@0xc0170 0xc0170 left a comment

Choose a reason for hiding this comment

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

Changes look good to me.

Please add "Release section"

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 25, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Sep 25, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 1
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM

@AnttiKauppila
Copy link
Author

Added

@0xc0170 0xc0170 self-requested a review September 26, 2019 10:20
@0xc0170
Copy link
Contributor

0xc0170 commented Sep 26, 2019

I edited the release notes.

CI started

@mbed-ci
Copy link

mbed-ci commented Sep 26, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 2
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM

@michalpasztamobica
Copy link
Contributor

@AnttiKauppila , I was trying out your branch to check my K64F failures and I got the code to compile by adding

#include "nsapi_dns.h"

into dns_tests.h.
I think it somehow got lost in this PR...

michalpasztamobica
michalpasztamobica suggested changes Sep 26, 2019
View reviewed changes
Copy link
Contributor

@michalpasztamobica michalpasztamobica left a comment

Choose a reason for hiding this comment

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

I did some debugging on top of your PR within dns test suite (for ESP8266 issues) and while doing that I found some improvements in the test code, which I didn't notice earlier...

TESTS/netsocket/dns/asynchronous_dns_invalid_host.cpp Outdated Show resolved Hide resolved
michalpasztamobica
michalpasztamobica approved these changes Sep 27, 2019
View reviewed changes
Copy link
Contributor

@michalpasztamobica michalpasztamobica left a comment

Choose a reason for hiding this comment

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

Thanks, @AnttiKauppila . I am looking forward to having this on master, as this makes things much easier for debugging dns tests 😃

@AnttiKauppila
Copy link
Author

Yes, the tests have initially been made quite badly because those were dependent on previous tests. If someone not knowing that (like me) would have added or modified the tests some other test could have got broken! We should avoid having such test cases

@AnttiKauppila
Copy link
Author

@ARMmbed/mbed-os-maintainers Could you proceed with this

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 30, 2019

CI started

@mbed-ci
Copy link

mbed-ci commented Sep 30, 2019

Test run: FAILED

Summary: 3 of 4 test jobs failed
Build number : 3
Build artifacts

Failed test jobs:

  • jenkins-ci/mbed-os-ci_build-IAR
  • jenkins-ci/mbed-os-ci_build-GCC_ARM
  • jenkins-ci/mbed-os-ci_build-ARM

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 30, 2019

Failures look related:

[DEBUG] Output: ./TESTS/network/multihoming/multihoming_asynchronous_dns.cpp:54:5: error: use of undeclared identifier 'nsapi_dns_reset'

@AnttiKauppila
Copy link
Author

@0xc0170 Added the missing include

@0xc0170
Copy link
Contributor

0xc0170 commented Sep 30, 2019

CI restarted

@mbed-ci
Copy link

mbed-ci commented Sep 30, 2019

Test run: SUCCESS

Summary: 11 of 11 test jobs passed
Build number : 4
Build artifacts

@0xc0170 0xc0170 merged commit c385e14 into ARMmbed:master Sep 30, 2019
@AnttiKauppila AnttiKauppila deleted the DNS_cleanup branch October 1, 2019 08:17
@adbridge adbridge added release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 and removed release-version: 5.15.0-rc1 labels Oct 3, 2019
@adbridge adbridge mentioned this pull request Nov 18, 2019
Merged
@0xc0170 0xc0170 added release-version: 5.15.0-rc1 and removed release-version: 6.0.0-alpha-1 First pre-release version of 6.0.0 labels Nov 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kjbracey kjbracey kjbracey left review comments

@michalpasztamobica michalpasztamobica michalpasztamobica approved these changes

@AriParkkila AriParkkila AriParkkila approved these changes

@0xc0170 0xc0170 0xc0170 approved these changes

@OPpuolitaival OPpuolitaival OPpuolitaival approved these changes

@jamesbeyond jamesbeyond Awaiting requested review from jamesbeyond

@SeppoTakalo SeppoTakalo Awaiting requested review from SeppoTakalo

Assignees
No one assigned
Labels
release-version: 5.15.0-rc1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@AnttiKauppila @ciarmcom @0xc0170 @mbed-ci @michalpasztamobica @OPpuolitaival @SeppoTakalo @kjbracey @AriParkkila @adbridge