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

Do domain resolution for portal #461

Merged
merged 2 commits into from
Jun 12, 2020
Merged

Do domain resolution for portal #461

merged 2 commits into from
Jun 12, 2020

Conversation

chiehmin
Copy link
Contributor

@chiehmin chiehmin commented Jun 5, 2020

iscsiadm support domain resolution (ex: iscsiadm -m discovery -t sendtargets -p iscsi.chiehmin.com).
However, open_iscsi module will try to match the portal with discovered results which will never
matched cause the discovered results use IP to represent node.

This patch do portal DNS resolution first to solve this situation.

Signed-off-by: Chieh-Min Wang chiehminw@synology.com

SUMMARY
ISSUE TYPE
  • Bugfix Pull Request
  • Docs Pull Request
  • Feature Pull Request
  • New Module Pull Request
COMPONENT NAME
ADDITIONAL INFORMATION

@ansibullbot
Copy link
Collaborator

cc @MorrisA @bcoca @d-little @flynn1973 @gforster @kairoaraujo @marvin-sinister @mator @molekuul @ramooncamacho @srvg @wtcross
click here for bot help

@ansibullbot ansibullbot added affects_2.10 bug This issue/PR relates to a bug module module new_contributor Help guide this first time contributor labels Jun 5, 2020
@ansibullbot
Copy link
Collaborator

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

plugins/modules/system/open_iscsi.py:8:0: import-before-documentation: Import found before documentation variables. All imports must appear below DOCUMENTATION/EXAMPLES/RETURN.

click here for bot help

@ansibullbot ansibullbot added the ci_verified Push fixes to PR branch to re-run CI label Jun 5, 2020
@chiehmin chiehmin force-pushed the open-iscsi-portal-dns-resolution branch from ffc3f5d to 16a8d5d Compare June 5, 2020 08:45
@ansibullbot ansibullbot removed the ci_verified Push fixes to PR branch to re-run CI label Jun 5, 2020
Andersson007
Andersson007 reviewed Jun 9, 2020
View reviewed changes
Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

please add a changelog fragment https://docs.ansible.com/ansible/devel/community/development_process.html#changelogs-how-to

@Andersson007
Copy link
Contributor

does the pr change the portal parameter behavior? if yes, add info about this to the section

    portal:
        description:
        - The IP address of the iSCSI target.
        type: str
        aliases: [ ip ]

e.g. - The IP address or domain name of the iSCSI target.
also it's maybe worth changing the examples in the EXAMPLE block

@chiehmin
Do domain resolution for portal
15ce9d2 
iscsiadm support domain resolution (ex: iscsiadm -m discovery -t sendtargets -p iscsi.chiehmin.com).
However, open_iscsi module will try to match the portal with discovered results which will never
matched cause the discovered results use IP to represent node.

This patch do portal DNS resolution first to solve this situation.

Signed-off-by: Chieh-Min Wang <chiehminw@synology.com>
@chiehmin chiehmin force-pushed the open-iscsi-portal-dns-resolution branch from 16a8d5d to 15ce9d2 Compare June 9, 2020 08:41
@chiehmin
Copy link
Contributor Author

chiehmin commented Jun 9, 2020

@Andersson007 Just added a changelog fragment and updated the example.

@chiehmin @Andersson007
Update changelogs/fragments/461-resolve-domain-for-iscsi-portal.yml
e73efca 
Co-authored-by: Andrew Klychkov <aaklychkov@mail.ru>
@Andersson007
Copy link
Contributor

@srvg as the module's author, could you please take a look?

@Andersson007
Copy link
Contributor

I just emailed the author. Let's wait a bit.

Andersson007
Andersson007 approved these changes Jun 11, 2020
View reviewed changes
Copy link
Contributor

@Andersson007 Andersson007 left a comment

Choose a reason for hiding this comment

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

LGTM

@srgvg
Copy link

srgvg commented Jun 11, 2020

It has been 7 years since I wrote this. I currently have no means to test the code, but looking at the changeset, this LGTM.

I assume

portal = socket.getaddrinfo(portal, None)[0][4][0]

just returns the same ip address when portal is set to an ip?

Thanks,

@Andersson007
Copy link
Contributor

@chiehmin ^. @srgvg i examined the function, yes, it should return ip whatever (an ip addr or domain name) we pass

@srgvg
Copy link

srgvg commented Jun 12, 2020

shipit

@Andersson007 Andersson007 merged commit 2aa84f0 into ansible-collections:master Jun 12, 2020
@Andersson007
Copy link
Contributor

@chiehmin thanks for the contribution!
@srgvg thanks for reviewing!

@Andersson007
Copy link
Contributor

merged #461 into master

@chiehmin chiehmin deleted the open-iscsi-portal-dns-resolution branch June 15, 2020 01:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Andersson007 Andersson007 Andersson007 approved these changes

Assignees
No one assigned
Labels
bug This issue/PR relates to a bug module module new_contributor Help guide this first time contributor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@chiehmin @ansibullbot @Andersson007 @srgvg