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

Fixes salt.utils.EtcdClient.write so it distinguishes between the different python-etcd logic for files/directories #51538

Merged
merged 4 commits into from
Feb 22, 2019

Conversation

arizvisa
Copy link
Contributor

@arizvisa arizvisa commented Feb 7, 2019

What does this PR do?

Fixes some oversights in the salt.utils.etcd_util module when creating directories vs files.

What issues does this PR fix or reference?

Closes issue #51537

Previous Behavior

When creating a directory an error message is logged saying the path is not a file.

New Behavior

Fixes the prior mentioned behaviour.

Tests written?

I'm tired.

Commits signed with GPG?

No.

@arizvisa
Copy link
Contributor Author

arizvisa commented Feb 7, 2019

This PR was only written for the file-directory duality in the salt.utils.etcd_util module and does not affect error messages that are emitted when an empty directory entry is attempted to be read.

This is done by the salt.states.etcd_mod in order to determine the original value to compare to. This function will pass the recurse=True option to salt.utils.etcd_util.read() in an attempt to read the contents of the directory. Due to the directory being empty, the etcd client will raise an EtcdKeyNotFound exception, which will then get logged before the .read() function properly returns None. This is a red herring in that no error has happened, there are just no "files" in the directory.

Nonetheless, this only fixes the salt.utils.EtcdClient.write() method since other things depend on it.

@dwoz
Copy link
Contributor

dwoz commented Feb 8, 2019

@arizvisa are you able to create a test that verifies this functionality? I think it will prevent us from having a regression in the future.

@arizvisa
Copy link
Contributor Author

arizvisa commented Feb 9, 2019

I'll see what I can do in the future, it'll take some time though as my primary job isn't related to development or even close to this sorta devops stuff so it's hard to find business cause for this sorta thing.

Honestly I expected using saltstack was to be a quick effort, but I ended up spending a lot more time on this due to the Etcd support apparently being underused (Such as #51345 which you were a part of, then PRs #51363 and #51346). Still a fan of the whole design of salt, just wasn't expecting to do so much work for my one very-specific configuration.

@arizvisa
Copy link
Contributor Author

arizvisa commented Feb 9, 2019

Oh also, I think I mentioned the issue with the salt.state.etcd_mod.directory state calling the .get() method on a directory that doesn't exist and it not catching the expected error. That will probably need to get caught at some point and demoted to a log.trace as well.

I didn't create the issue yet because I don't have too much time to fix and test it. So just a heads up if someone else gets to writing tests for this PR before I do, they'll still see an error in the logs when using etcd.directory on a path that needs to be created.

@dwoz
Copy link
Contributor

dwoz commented Feb 13, 2019

@arizvisa One of the existing Etcd unit tests is failing, This needs to be fixed before we can merge.

@arizvisa
Copy link
Contributor Author

force-push due to a rebase against develop. added unit tests for both keys and directories. we'll see how it turns out..

@arizvisa
Copy link
Contributor Author

Grr...first force-push was against develop instead of origin/develop. This next push will be against the correct branch...

…t it handles directories properly instead of assuming that everything is a file by default.
…ry already exists. Updated salt.utils.etcd_util to catch it for write_directory and return True if so.
@arizvisa
Copy link
Contributor Author

Um...am I reading this error right from py2-windows-2016? It seems to have terminated before it even ran my tests?

>>>>>> ------Exception-------
>>>>>> Class: Kitchen::ActionFailed
>>>>>> Message: 1 actions failed.
>>>>>>     Failed to complete #verify action: [[WSMAN ERROR CODE: 2152992672]: <f:WSManFault Code='2152992672' Machine='10.200.20.199' xmlns:f='http://schemas.microsoft.com/wbem/wsman/1/wsmanfault'><f:Message><f:ProviderFault path='C:\Windows\system32\pwrshplugin.dll' provider='microsoft.powershell'>Exception of type &apos;System.OutOfMemoryException&apos; was thrown.</f:ProviderFault></f:Message></f:WSManFault>] on py2-windows-2016

@dwoz
Copy link
Contributor

dwoz commented Feb 22, 2019

Um...am I reading this error right from py2-windows-2016? It seems to have terminated before it even ran my tests?

>>>>>> ------Exception-------
>>>>>> Class: Kitchen::ActionFailed
>>>>>> Message: 1 actions failed.
>>>>>>     Failed to complete #verify action: [[WSMAN ERROR CODE: 2152992672]: <f:WSManFault Code='2152992672' Machine='10.200.20.199' xmlns:f='http://schemas.microsoft.com/wbem/wsman/1/wsmanfault'><f:Message><f:ProviderFault path='C:\Windows\system32\pwrshplugin.dll' provider='microsoft.powershell'>Exception of type &apos;System.OutOfMemoryException&apos; was thrown.</f:ProviderFault></f:Message></f:WSManFault>] on py2-windows-2016

Yes, we still have some issues in the test runner. Having said that, it looks like the next test run passed. :) Merging.

@dwoz dwoz merged commit 2ea49a4 into saltstack:develop Feb 22, 2019
@arizvisa
Copy link
Contributor Author

Thx.

garethgreenaway added a commit to garethgreenaway/salt that referenced this pull request Sep 19, 2019
dwoz added a commit that referenced this pull request Dec 23, 2019
arizvisa added a commit to arizvisa/lolfuzz3 that referenced this pull request May 24, 2020
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.

2 participants