Remove the RemoteFactory class from the python module#4059
Open
clumens wants to merge 15 commits intoClusterLabs:mainfrom
Open
Remove the RemoteFactory class from the python module#4059clumens wants to merge 15 commits intoClusterLabs:mainfrom
clumens wants to merge 15 commits intoClusterLabs:mainfrom
Conversation
It's always False. Ref T680
There's no need for these functions now that they're just pass throughs. Ref T680
It's only ever True in one place, and I don't think I care about that. We can just always log. Ref T680
It defines some fairly common function names like "run" that I want to use, so it makes sense to import the whole module and force us to do "subprocess.popen" instead.
This is only ever used in one place. It can just be an argument array instead. And while I'm at it, do what the TODO suggested and use subprocess instead. Ref T680
If we pass universal_newlines=True when we create a Popen object, the resulting streams will be text instead of bytes (when we support python >= 3.7, this is also called text=True). Additionally, it's unclear to me that this was actually useful in the first place. We were logging the byte strings before converting them to text. Ref T680
This is also used in one place, so just define it as a private variable in the class itself. Note that it would really make a lot of sense to also have this be a list of arguments, but in order to do that properly we also need to change everywhere that runs a command to pass a list as well. That's a project for another day. Ref T680
* It completely duplicates what the async_call method does. * There's one caller in LogAudit that was using it totally incorrectly. verbose=0 doesn't matter because that argument was never getting passed to AsyncCmd, so it would log however it wanted regardless. And, rc would always be 0 because we don't wait around for the process to give us a valid return code so the error message would never be logged. Ref T680
This doesn't really do anything interesting now. It just returns the singleton instance of RemoteExec, but I don't think there's much savings to be had in doing this instead of just creating a new instance each time. It's not a very complicated class with lots of variables that take time to set up. This also allows us to get rid of all of the pylint not-callable pragmas since that level of indirection has been removed. Ref T680
I don't know if there's any reason to use one over the other, but personally I like the more explicit function name. Ref T680
What we really care about here is whether or not one system can ssh into another, so all we need to do is just try to ssh without all those extra options. Ref T680
In general, you definitely want to do host key checking and this option is set to "ask" by default. However, in CTS, this means that if you haven't ssh'd between systems first and done host key checking (or at least responded to the ssh prompt), it will stop and ask. This results in test case failures. So add this argument to just skip the host key checking and prevent those failures. I think this is reasonable to do in CTS, but obviously you wouldn't want to do this elsewhere.
This fixes a pylint warning regarding naming.
TCPKeepAlive=yes and ServerAliveCountMax=3 are ssh defaults, and appear to have been ssh defaults for a very long time. The only reason to specify these would be if you had your ctslab machines configured differently, which I don't expect anyone is doing. Thus we can just remove these.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.