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

[V2V] Enhance throttler logging #18929

Merged
merged 3 commits into from
Jul 2, 2019

Conversation

ghost
Copy link

@ghost ghost commented Jul 2, 2019

While trying to identify the reason for odd balancing on a migration plan execution, we realized that we didn't have enough information to nail the cause. This PR adds extra logging in debug mode, so that we can understand the current limits at the EMS and Conversion Host levels.

The PR also changes the configuration of the SSH connection in ConversionHost.verify_credentials, because it currently overrides the log level of the global throttler. We will need another PR to keep the SSH errors, but the current setting is counter productive.

RHBZ: https://bugzilla.redhat.com/show_bug.cgi?id=1726186

@ghost
Copy link
Author

ghost commented Jul 2, 2019

@miq-bot add-label transformation, hammer/yes, enhancement
@miq-bot add-reviewer @djberg96
@miq-bot add-reviewer @agrare

@@ -1,24 +1,42 @@
class InfraConversionThrottler
def self.start_conversions
$log&.debug("InfraConversionThrottler.start_conversions")
Copy link
Member

Choose a reason for hiding this comment

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

I know the previous code had this...but why do you need the &. for $log? This is a global var that is set up by vmdb_loggers and will never be nil

Copy link
Author

Choose a reason for hiding this comment

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

Included Vmdb::Logging and updated to use _log.

@djberg96
Copy link
Contributor

djberg96 commented Jul 2, 2019

Does this also address the race condition you mentioned?

@ghost
Copy link
Author

ghost commented Jul 2, 2019

@djberg96 nope, but it logs the job state, so we know it if it happens again.

vm_name = job.migration_task.source.name
_log.debug("- Looking for a conversion host for task for #{vm_name}")

if slots <= 0
Copy link
Member

Choose a reason for hiding this comment

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

Could this be outside the jobs.each loop? It looks like there is no context in this loop that impacts the slots check and it breaks out anyway

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it could. We talked about it with Keenan, but I wanted to limit the change to new log messages.
Anyway, it's a good point, so I updated the code.

@agrare
Copy link
Member

agrare commented Jul 2, 2019

Looks good to me, @djberg96 ?

@djberg96
Copy link
Contributor

djberg96 commented Jul 2, 2019

👍

@agrare agrare merged commit fd70427 into ManageIQ:master Jul 2, 2019
@agrare agrare added this to the Sprint 115 Ending Jul 8, 2019 milestone Jul 2, 2019
simaishi pushed a commit that referenced this pull request Jul 2, 2019
@simaishi
Copy link
Contributor

simaishi commented Jul 2, 2019

Hammer backport details:

$ git log -1
commit 99bbf47929122fdebc7fc77d53e52206fac11a54
Author: Adam Grare <[email protected]>
Date:   Tue Jul 2 12:46:57 2019 -0400

    Merge pull request #18929 from fdupont-redhat/v2v_enhance_throttler_logging
    
    [V2V] Enhance throttler logging
    
    (cherry picked from commit fd7042726763fa37450efd587a83dbad575d68ec)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1726394

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants