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

Fixes SmartOS grains when using py3 #53794

Merged
merged 5 commits into from
Jul 31, 2019
Merged

Fixes SmartOS grains when using py3 #53794

merged 5 commits into from
Jul 31, 2019

Conversation

sjorge
Copy link
Contributor

@sjorge sjorge commented Jul 10, 2019 •
edited
Loading

What does this PR do?

The salt package in pkgsrc was moved to py3, this broke some grains on SmartOS.
For now the current release 2019.3 is back on py27. But in preperation for the next release, this PR fixes those issues.

What issues does this PR fix or reference?

#53740

Previous Behavior

Missing grains on SmartOS (Global Zone) when using py3

New Behavior

Grains are no longer missing

Tests written?

Yes

Commits signed with GPG?

No

@sjorge
Copy link
Contributor Author

sjorge commented Jul 10, 2019

Some data we can use to mock mdata-list:

  1. failure case
[root@carbon ~]# mdata-list > /tmp/out 2> /tmp/err
[root@carbon ~]# cat /tmp/out
[root@carbon ~]# cat /tmp/err
ERROR: could not initialise protocol: I don't know how to get metadata on this system.
  1. success case
isotope# mdata-list > /tmp/out 2> /tmp/err
isotope# cat /tmp/out
primary_mac
user-script
minion_key
minion_pub
isotope# cat /tmp/err

(yes, sdc:XXX values are not listed)

  1. mdata-get failure missing prop
isotope# mdata-get sdc:alaisx > /tmp/out 2> /tmp/err
isotope# cat /tmp/out
isotope# cat /tmp/err
No metadata for 'sdc:alaisx'
  1. mdata-get OK
isotope# mdata-get sdc:alias > /tmp/out 2> /tmp/err
isotope# cat /tmp/err
isotope# cat /tmp/out
isotope
  1. mdata-get hard errro
[isotope :: sjorge][~]
[.]$ mdata-get sdc:alias > /tmp/out 2> /tmp/err
[isotope :: sjorge][~]
[!]$ cat /tmp/out
[isotope :: sjorge][~]
[.]$ cat /tmp/err
ERROR: could not initialise protocol: Could not find metadata socket.

@sjorge
Copy link
Contributor Author

sjorge commented Jul 10, 2019

Ideally the test should just run through the grain mdata grain parsing and with all the output combinations and we should get the expected grains or lack of the grains (but no exceptions)

@sjorge
Copy link
Contributor Author

sjorge commented Jul 13, 2019

Some more fixes were needed in salt.utils.platform as the is_smartos_zone and is_smartos_globalzone were failing on bytes vs string literal on py3.

@sjorge
Copy link
Contributor Author

sjorge commented Jul 23, 2019 •
edited
Loading

So I talked with @Ch3LL and we need the following tests:

  • unit for grains.mdata
  • unit for grains.smartos

Probably something for salt.utils.platform too as that was the read cause of the issue, it just expose the other to grains to some undefined behavior which we cannot hit otherwise.

@sjorge
Copy link
Contributor Author

sjorge commented Jul 23, 2019

Tests for the functions in both grain modules are done, and some small more tweaks to salt.grains.smartos to make it more efficient.

@Ch3LL Ch3LL self-requested a review July 23, 2019 13:41
@sjorge sjorge requested a review from a team as a code owner July 29, 2019 13:21
@ghost ghost requested a review from dwoz July 29, 2019 13:21
waynew
waynew suggested changes Jul 29, 2019
View reviewed changes
salt/utils/platform.py Outdated Show resolved Hide resolved
salt/utils/platform.py Outdated Show resolved Hide resolved
@sjorge
py3 results in broken mdata grains
0fea6b3 
When running salt on SmartOS with py3, the mdata grains are broken.
Both stem from the same underlying issue that stderr is mixed in
 with stdout for the `mdata-list` and `mdata-get`. (Is this new?)

The solution is to ignore the output if it starts with 'ERROR:'.
I couldn't find a way to filter out stderr via cmd.run directly.
@sjorge
Missing grains on SmartoS GZ under py3
93d7a93 
Some grains were broken in the GZ too,
 also do some mindor cleanup.

1. /etc/pkgsrc_version
Does not exist in GZ and is a non-supported interface for zones,
 we can get this information from the pkg_install.conf file too.
This should also more accuratly reflect the correct value.

The path for the file is different in the GZ so we need to support both locations

2. pkgin repos are missing in GZ only
Same as above, the path is different so we need to support both.
@sjorge
py3 salt.utils.platform.is_smartos_zone/global fix
2a729f3 
The output of Popen is bytes not string so the checks below for 'global' was not matching.

- cleanup the Popen bits a bit
- decode('utf8') the output
@sjorge
Add tests for salt.grains.mdata
e088fea
waynew
waynew suggested changes Jul 29, 2019
View reviewed changes
Copy link
Contributor

@waynew waynew left a comment

Choose a reason for hiding this comment

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

Small typo, otherwise this looks good from my perspective.

salt/grains/smartos.py Outdated Show resolved Hide resolved
@waynew waynew merged commit cb6cfcc into saltstack:develop Jul 31, 2019
@sjorge sjorge deleted the py3_smartos_grains branch July 31, 2019 21:14
xeacott pushed a commit to xeacott/salt that referenced this pull request Apr 24, 2020
@sjorge @xeacott
Port saltstack#53794 to master
b45b5d1
dwoz pushed a commit that referenced this pull request Apr 26, 2020
@sjorge @dwoz
Port #53794 to master
480e6b5
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@waynew waynew waynew approved these changes

@Ch3LL Ch3LL Ch3LL approved these changes

@dwoz dwoz Awaiting requested review from dwoz

Assignees
No one assigned
Labels
code-jam
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@sjorge @waynew @Ch3LL @sagetherage