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 for Solaris virtual grain detection #55491

Merged
merged 7 commits into from
May 22, 2020
Merged

Fix for Solaris virtual grain detection #55491

merged 7 commits into from
May 22, 2020

Conversation

clallen
Copy link
Contributor

@clallen clallen commented Dec 2, 2019

What does this PR do?

Fixes a bug that I inadvertently introduced in commit f7f8a8f, as well as adds better detection for zones on Solaris 11.

What issues does this PR fix or reference?

Issue #55444

Previous Behavior

When on a non-LDOM platform, the virtual grain would always detect as "LDOM", due to lack of adequate parsing of "virtinfo" command output.

New Behavior

Output of the "virtinfo" command is now parsed correctly, and detection of zones is more accurate.

Tests written?

Output of nox test unit.grains.test_core:

~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 * Python Version: 2.7.15+ (default, Oct 7 2019, 17:39:04) [GCC 7.4.0]
 * Transplanting configuration files to '/tmp/salt-tests-tmpdir/config'
 * Transplanting multimaster configuration files to '/tmp/salt-tests-tmpdir/config'
 * Current Directory: /salt-fork
 * Test suite is running under PID 575
 * Logging tests on artifacts/logs/runtests-20191203010050.773101.log
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
Starting unit.grains.test_core Tests
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
.s...............s......................s.
----------------------------------------------------------------------
Ran 42 tests in 0.112s

OK (skipped=3)

==========================================================================================================  Overall Tests Report  ==========================================================================================================
*** unit.grains.test_core Tests  ***********************************************************************************************************************************************************************************************************
 --------  Skipped Tests  ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
   -> unit.grains.test_core.CoreGrainsTestCase.test__windows_platform_data    ->  System is not Windows
   -> unit.grains.test_core.CoreGrainsTestCase.test_locale_info_no_tz_tzname  ->  Not Missing dateutil.tz
   -> unit.grains.test_core.CoreGrainsTestCase.test_windows_platform_data     ->  System is not Windows
 -------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
============================================================================================================================================================================================================================================
OK (total=42, skipped=3, passed=39, failures=0, errors=0)
==========================================================================================================  Overall Tests Report  ==========================================================================================================
Test suite execution finalized with exit code: 0
nox > Session runtests-parametrized-2.7(coverage=False, crypto=None, transport='zeromq') was successful.
nox > Ran multiple sessions:
nox > * runtests-zeromq-2.7(coverage=False): success
nox > * runtests-parametrized-2.7(coverage=False, crypto=None, transport='zeromq'): success

Commits signed with GPG?

No

Clint Allen added 6 commits November 29, 2019 22:11
Removed virtinfo "-a" arg, it is not supported in Solaris 11.4

Added virtinfo args to output a more exact virt type

Added parsing of virtinfo output for "virtual" grain

Moved subtype detection with the rest of the detection code

Fixed "virtual_subtype" command to use correct command variable
("command" instead of "cmd")

Added detection of zone subtypes
Re-added detection for Solaris 8/9/10 zones
Fixed call to os.path.isdir
@clallen clallen marked this pull request as ready for review December 3, 2019 20:21
@clallen clallen requested a review from a team as a code owner December 3, 2019 20:21
@ghost ghost requested a review from DmitryKuzmenko December 3, 2019 20:21
@sagetherage sagetherage added Bug broken, incorrect, or confusing behavior ZRelease-Sodium retired label labels Mar 13, 2020
@Ch3LL Ch3LL removed the request for review from a team April 15, 2020 14:31
@DmitryKuzmenko DmitryKuzmenko added the Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases label May 8, 2020
@DmitryKuzmenko
Copy link
Contributor

@clallen sorry for the long delay. I've updated your PR: merged to the head of the current master resolving conflicts. Let's wait for tests.
Are you still interested in this? Could you please write a test for this?

@clallen
Copy link
Contributor Author

clallen commented May 14, 2020

I don't mind writing a test, but I'm not sure what is required. Could you point me to an example?

@DmitryKuzmenko
Copy link
Contributor

DmitryKuzmenko commented May 15, 2020

@clallen we already have a unit test for grains tests/unit/grains/test_core.py. All you need is to ensure your code is covered with tests.
Thank you for keeping in touch.

Copy link
Contributor

@dwoz dwoz left a comment

Choose a reason for hiding this comment

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

@clallen I'm going to merge this in for Sodium. In the future we'll definitely need tests along with bug fixes like this.

@dwoz dwoz merged commit 4c8a6f6 into saltstack:master May 22, 2020
@clallen
Copy link
Contributor Author

clallen commented May 22, 2020

@dwoz My apologies, I have not had much time to get familiar with the tests system. If there is anything else you need from me let me know.

@sjorge
Copy link
Contributor

sjorge commented Jun 18, 2020

This seems to have completely broken virtual detection on illumos :(
Everything ends up as physical now, still investigate as 3001 overall is a disaster for illumos.

Nevermind, definately broken as it 'physical' now but doesn't seem to be this one that broke it. Can't figure out which one it is though 🤔

@DmitryKuzmenko
Copy link
Contributor

@sjorge is it easily reproducible? Could you try to bisect it and create a separate issue?

@DmitryKuzmenko
Copy link
Contributor

Or either just create an issue describing reproduction steps, and we'll do it.

@sjorge
Copy link
Contributor

sjorge commented Jun 18, 2020

Yep, already did #57714

I think I found what is going wrong, not yet why/when it changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug broken, incorrect, or confusing behavior Needs-Testcase PR needs test cases written, or the issue is about a bug/feature that needs test cases ZRelease-Sodium retired label
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants