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

Fix minion start issue #51274

Merged
merged 2 commits into from Jan 23, 2019

Conversation

@twangboy
Copy link
Contributor

commented Jan 22, 2019

What does this PR do?

Fixes an issue introduced by #49755 which was causing the minion to fail to start on Windows with the following error:

[ERROR   ] Master address: '104.236.251.190' could not be resolved. Invalid or unresolveable address. Set 'master' value in minion config.
[INFO    ] Shutting down the Salt Minion
The Salt Minion is shutdown.
Master address: '104.236.251.190' could not be resolved. Invalid or unresolveable address. Set 'master' value in minion config.

The issue is in a new function in the salt.utils.network salt util. It was setting the host to with the following command:

host_ip = ipaddress.ip_address(host)

Which sets the host_ip to an IPv4Address object. This PR adds .compressed that sets host_ip to the actual string value instead of an IPv4Address object

host_ip = ipaddress.ip_address(host).compressed

It does this on the IPv4 portion and the IPv6 portion as well.

@aphor Please test this. I don't know how it affects the issue you were trying to fix.

.. note::
I found this issue by doing a git bisect which brought me to this commit: df73388

Tests written?

No

Commits signed with GPG?

Yes

twangboy added a commit to twangboy/salt that referenced this pull request Jan 22, 2019
This is fixed via another PR (saltstack#51274)
@aphor

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

I'll look at it, but I don't have time until next week.

@aphor

This comment has been minimized.

Copy link
Contributor

commented Jan 22, 2019

Can you add a stack trace for this error? I don't have a Windows test VM handy.

@aphor

This comment has been minimized.

Copy link
Contributor

commented Jan 23, 2019

I went to a lot of trouble at the request of @isbm to move things towards passing around IP addresses as IP address objects instead of passing around strings. I'd like to understand how your error is happening, and why there is an attempt to do DNS resolution on an IP address.

This change works like duct tape, and I'd like to get closer to the root if we can, and fix the unit tests so we don't end up going in circles trying to fix things naïvely. This is breaking tests which run on Linux.

@s0undt3ch

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

Leaving ipaddress instances in opts brings several problems.

I tried leaving them and only casting to string(or actually calling .compressed as on this PR) when required and see when it kept breaking and got to the port where the minion does start but:

11:28:33,187 [salt.minion                                    :1780][WARNING ][ProcessPayload(27042)] [salt-minion/minion] Passed invalid arguments to saltutil.sync_all: can not serialize 'IPv4Address' object 
@s0undt3ch

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

Test failures are valid. Tests need to be updated.

@s0undt3ch s0undt3ch force-pushed the twangboy:fix_network branch from 088f02b to 344bf65 Jan 23, 2019
@s0undt3ch s0undt3ch merged commit aacc047 into saltstack:2018.3 Jan 23, 2019
10 checks passed
10 checks passed
WIP Ready for review
Details
continuous-integration/jenkins/pr-merge This commit looks good
Details
jenkins/pr/docs The docs job has passed
Details
jenkins/pr/lint Python lint test has passed
Details
jenkins/pr/py2-centos-7 The py2-centos-7 job has passed
Details
jenkins/pr/py2-ubuntu-1604 The py2-ubuntu-1604 job has passed
Details
jenkins/pr/py2-windows-2016 The py2-windows-2016 job has passed
Details
jenkins/pr/py3-centos-7 The py3-centos-7 job has passed
Details
jenkins/pr/py3-ubuntu-1604 The py3-ubuntu-1604 job has passed
Details
jenkins/pr/py3-windows-2016 The py3-windows-2016 job has passed
Details
@s0undt3ch

This comment has been minimized.

Copy link
Member

commented Jan 23, 2019

2018.3.4 PR #51290
This code does not exist on 2019.2.0.rc1

Ch3LL added a commit to Ch3LL/salt that referenced this pull request Jan 23, 2019
This is fixed via another PR (saltstack#51274)
@twangboy twangboy deleted the twangboy:fix_network branch Jan 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.