[V2V] Use authentication_check instead of verify credentials #18880
Conversation
|
@fdupont-redhat What do you think? |
djberg96
changed the title
|
@miq-bot add_labels transformation, changelog/yes, hammer/yes |
|
@miq-bot remove_label wip |
|
@djberg96 'agrare kbrock' is an invalid reviewer, ignoring... |
|
@djberg96 'agrare, kbrock' is an invalid reviewer, ignoring... |
|
@fdupont-redhat 'agree' is an invalid reviewer, ignoring... |
|
@djberg96 shouldn't verify_credentials return false if authentication was not invalid within the last X minutes ? X could be 2, to not attempt connection too frequently, but also not leave the host as ineligible during too long. |
There was a problem hiding this comment.
I sure am getting distracted a lot by this code.
verify_credentials: this looks like it is copy/pasted across the code. Encouraging us to have different implementations when we answer the question "Are these credentials are valid?". This seems like a support nightmare.
heck connecting to ssh in general looks like it is scattered across a number of classes. Sure feels like it should be in helper class. grep 'uname -a'. Implemented 2 totally different ways.
It feels like 15 minutes will need to be configurable. Don't want to deal with it here, but it sure feels like an obvious request right around the corner.
And to be fair, just looking at the verify_credentials method has me wanting to refactor out the count(*) up front, move the require ssh down a few lines. And also understand why the 2 uname calls seem to connect to ssh in different ways.
eligible? is too high level and low level at the same time. (total tangent: I sure wish the task count wasn't in that method.)
| @@ -25,18 +25,20 @@ | |||
| allow(ems).to receive(:conversion_hosts).and_return([conversion_host1, conversion_host2]) | |||
| allow(conversion_host1).to receive(:check_ssh_connection).and_return(true) | |||
| allow(conversion_host2).to receive(:check_ssh_connection).and_return(true) | |||
| allow(conversion_host1).to receive(:authentication_check).and_return([true, 'passed']) | |||
| allow(conversion_host2).to receive(:authentication_check).and_return([true, 'passed']) | |||
There was a problem hiding this comment.
keep this
but this stubbing is skipping actually testing the code.
try and keep stubbing to a method that provides just an external interface (e.g.: connect to ssh - or maybe even connect to the database, though that is suspect)
There was a problem hiding this comment.
Those particular tests are just checking the throttling, not sure what you mean.
|
@fdupont-redhat Not sure I'm following. Do you want me to add logic that checks |
Yes. Otherwise for a host with invalid authentication, we'll do SSH connections until it becomes valid. I think we should evince it for a period of time. Maybe we could have the same check interval for both, no ? What about 5 minutes ? That would still lower the number of SSH connections, while limiting the risk of assigning an invalid host to a task. |
|
@fdupont-redhat I split the difference and changed it to 10 minutes. Is that ok? 5 minutes seems too short. I also updated it so that if it was invalid in the last 10 minutes it will still retry, even if it was also valid within that time. I updated the specs, too. |
|
@kbrock Are you ok with it now? |
|
@miq-bot remove_label hammer/yes |
|
@miq-bot add_label hammer/no |
|
This looks better, @kbrock you good with this? |
… 15 minutes. Update specs to reflect changes in eligible? method. Add v2v tag to InfraConversionThrottler specs. Update host and vm in specs to be provider-specific, change custom task name to avoid deprecation warnings. Stub the authentication_check method. Add more specs for verify_credentials to validate that ssh calls are not made if already valid within last 15 minutes.
…ith validation if invalid within last 10 minutes.
|
Checked commits https://github.com/djberg96/manageiq/compare/5cd804f742be40b7c4af6e050322a72b3ba29a2d~...51bca563276aed4824113d500d2dd8d61fc03a1a with ruby 2.4.6, rubocop 0.69.0, haml-lint 0.20.0, and yamllint 1.10.0 |
This alters the
ConversionHost#eligible?method so that it calls theauthentication_checkmethod instead ofverify_credentialsdirectly.Internally the
authentication_checkmethod calls theverify_credentialsmethod, and then updates the authentications object, including thelast_valid_onfield. We use that field to skip the ssh connection check if it was valid within the last 15 minutes. This will help reduce unnecessary ssh connections if the method happens to be called multiple times in a short period of time, e.g.InfraConversionThrottler.start_conversions. Plus it reduces the chance of unexpected timeout errors, etc.Update: The 15 minute idea was dropped in favor of just using the
authentication_status_ok?method that already exists in the Authentication mixin.WIP until I can get some tests added.Some other updates included in this PR:
I set the default auth_type to "v2v" in the
verify_credentialsmethod.I changed the resources in the throttler specs to be provider-specific.
I added an "RSpec" to the outer describe for future rspec 4 compliance.
I added the "v2v" tag to the throttler specs.
I changed "Max Transformation Runners" to "MaxTransformationRunners" because it was causing a deprecation warning:
DEPRECATION WARNING: Invalid custom attribute: 'Max Transformation Runners'. A custom attribute name must begin with a letter (a-z, but also letters with diacritical marks and non-Latin letters) or an underscore (_). Subsequent characters can be letters, underscores, digits (0-9), or dollar signs ($) (called from block (3 levels) in <top (required)> at /Users/dberger/Dev/manageiq-djberg96/spec/lib/infra_conversion_throttler_spec.rb:31)Not exactly related, but does obliquely help with #18880 a bit.
Addresses https://bugzilla.redhat.com/show_bug.cgi?id=1723420