Skip to content

Check dmidecoder executable on each "smbios" call to avoid race condition#48781

Closed
meaksh wants to merge 3 commits into
saltstack:2018.3from
meaksh:2018.3-fix-dmidecode-race-condition
Closed

Check dmidecoder executable on each "smbios" call to avoid race condition#48781
meaksh wants to merge 3 commits into
saltstack:2018.3from
meaksh:2018.3-fix-dmidecode-race-condition

Conversation

@meaksh
Copy link
Copy Markdown
Contributor

@meaksh meaksh commented Jul 26, 2018

What does this PR do?

This PR fixes a race condition happening during core grains generation in case dmidecode or smbios are installed/removed during salt-minion execution.

As described on #35505, smbios.DMIDECODER is fixed to the dmidecode or smbios executables, and this is happening only once while the module is loaded. Then there are some calls to smbios.get performed while calculating the "core" grains which produces a race condition in case the results of salt.utils.path.which_bin(['dmidecode', 'smbios']) changes during execution time.

What issues does this PR fix or reference?

Hopefully fixes #35505

Previous Behavior

The situation appears in case neither dmidecode nor smbios are installed at the time of salt-minion is started, therefore smbios.DMIDECODER is set to None. Then, at some point, either dmidecode or smbios are installed on the minion and eventually the grains will be recalculated.
At this particular moment, when the "core" grains are being recalculated, https://github.com/saltstack/salt/blob/2018.3/salt/grains/core.py#L2310 is checking again if executables are now available, and if so, it would produce a call to smbios.get which will crash since smbios.DMIDECODER is set to None:

ERROR: Unable to run command '['None', '-s', 'bios-version']' with the context '{'timeout': None, 'with_communicate': True, 'shell': False, 'bg': False, 'stderr': -2, 'env': {'LC_NUMERIC': 'C', 'NOTIFY_SOCKET': '/run/systemd/notify', 'LC_MESSAGES': 'C', 'LC_IDENTIFICATION': 'C', 'LC_MONETARY': 'C', 'LC_COLLATE': 'C', 'LC_CTYPE': 'C', 'LC_ADDRESS': 'C', 'LC_MEASUREMENT': 'C', 'LC_TELEPHONE': 'C', 'LC_PAPER': 'C', 'LC_NAME': 'C', 'PATH': '/usr/local/sbin:/usr/local/bin:/usr/sbin:/usr/bin:/sbin:/bin', 'LC_TIME': 'C'}, 'stdout': -1, 'close_fds': True, 'stdin': None, 'cwd': '/root'}', reason: [Errno 2] No such file or directory

New Behavior

Now smbios.DMIDECODER is refreshed on each smbios module call to prevent the race condition with the "core" grains. Besides of that, this PR introduces also a comprehensive error message in case of crashing:

2018-07-26 17:04:40,063 [salt.modules.smbios:337 ][ERROR   ][16331] SMBIOS: neither dmidecode nor smbios found!

Tests written?

No

Commits signed with GPG?

Yes

@rallytime
Copy link
Copy Markdown
Contributor

@meaksh Can you fix up the lint failures listed here?

https://jenkinsci.saltstack.com/job/pr-lint/job/PR-48781/1/warnings52Result/package.2029219534/

@rallytime rallytime requested a review from isbm July 26, 2018 20:40
@meaksh
Copy link
Copy Markdown
Contributor Author

meaksh commented Jul 27, 2018

@rallytime - pylint issues fixed 👍

Copy link
Copy Markdown
Contributor

@isbm isbm left a comment

Choose a reason for hiding this comment

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

I will follow-up on this PR differently.

Comment thread salt/modules/smbios.py

def _refresh_dmidecoder():
global DMIDECODER
DMIDECODER = salt.utils.path.which_bin(['dmidecode', 'smbios'])
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why there is a need of this global variable at all? And this function is also not needed. I would discard this PR entirely and just refactor it the way that which_bin is just called.

@isbm
Copy link
Copy Markdown
Contributor

isbm commented Jul 29, 2018

🛑
@rallytime Please close this. There is a better version of it here: #48814

@rallytime
Copy link
Copy Markdown
Contributor

Closed in favor of #48814

@rallytime rallytime closed this Jul 30, 2018
@meaksh meaksh deleted the 2018.3-fix-dmidecode-race-condition branch August 6, 2018 10:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants