Add git remote connection check code

[djberg96]

Slight reworking of #20645

This is basically @NickLaMuro's PR, but with the check_connection method modified. Instead of making a Rugged connection on the API endpoint, we just perform an http ping on the repo.

I made these changes to Nick's PR for a couple reasons. First and foremost, the Rugged code was segfaulting with a difficult to diagnose error that indicates that, somewhere in the guts of either rugged or libgit2, it's trying to free something that it didn't allocate. Issue reported here: libgit2/rugged#859. Unable to solve that, we had to find a different way.

Since the original issue was really a generic connectivity issue, and not specifically about the github API failing to return expected data, I felt the original rugged code was too heavy. It's the equivalent of a git ls-remote, which fetches data under the hood, and we don't need to fetch data, we just need to test for connectivity.

I chose net-ping as the solution because it was already a dependency in ManageIQ, because I'm the original author, and because it solves our needs. Although I have not been the maintainer for many years now, the code hasn't changed dramatically, so I have good familiarity with it.

@tinaafitz, @billfitzgerald0120, @d-m-u and I tested this on a vmware host where we temporarily disabled the network in the middle of provisioning a catalog item and it worked as expected, i.e. it would check the connection X number of times, and proceed if the connection was restored.

Original bug: https://bugzilla.redhat.com/show_bug.cgi?id=1835226

[d-m-u]

thanks for doing this

[tinaafitz]

Thanks so much, @djberg96

[Fryguy]

Cool idea. @NickLaMuro Please review.

[Fryguy]

This set ups the framework, but how do you envision this method to be used? (I don't see anything ultimately calling check_connection in a flow).

[djberg96]

This set ups the framework, but how do you envision this method to be used? (I don't see anything ultimately calling check_connection in a flow).

I think @tinaafitz, @billfitzgerald0120 or @d-m-u can provide an example.

[d-m-u]

@Fryguy this is intended to be called from the generic state machine, the automate side:

module ManageIQ
  module Automate
    module Service
      module Generic
        module StateMachines
          module GenericLifecycle
            class CheckConnection
...

              def check_connection(service)
                rc = service.check_connection(action = "Provision")
                #  check_connection returns true if connection is successful, false otherwise
                if !rc
                  @handle.root['ae_result'] = 'retry'
                  retry_interval
                  else
                  @handle.root['ae_result'] = 'ok'
                end
              end

so that when running a playbook if the connectivity isn't present we can handle the retry more gracefully.

[NickLaMuro]

A few changes.

---

Also, I wanted to clarify something that was mentioned in the PR description:

Since the original issue was really a generic connectivity issue, and not specifically about the github API failing to return expected data, I felt the original rugged code was too heavy. It's the equivalent of a git ls-remote, which fetches data under the hood, and we don't need to fetch data, we just need to test for connectivity.

The intent with going with the check_connection code, while yes, a bit heavier than your standard ping, would avoid false positives or false negatives (compared to a ping) depending on how the endpoint's firewall is configured.

For example: If the remote host is super locked down, most likely only git protocols would be allowed, and a simple ping might not get through. Or, conversely, if it is a little more permissive and allows a ping, but git is run on a non-standard port (but that isn't configured in MIQ), that won't be tested here and the ping would pass, but the eventual clone would still fail.

That said, not invalidating where is here and what is here is better than nothing, just giving some more context as to why the .check_connection was originally chosen for a solution.

[Fryguy]

rc = service.check_connection(action = "Provision")

Then I wonder if it's better to rename the method to be a predicate. Something like check_connection? valid_connection?, connection_valid?

[djberg96]

rc = service.check_connection(action = "Provision")

Then I wonder if it's better to rename the method to be a predicate. Something like check_connection? valid_connection?, connection_valid?

I renamed it to check_connection?.

[NickLaMuro]

This looks fine to me now. Stinks we can't use rugged, but we have mucked around with that segfault long enough. 👍

[miq-bot]

Checked commits https://github.com/djberg96/manageiq/compare/439b41f938fe2cb219e24831ed585be2c2d25fa4~...874902f033528ff1d6e14c2f5f285d3929b5d1a0 with ruby 2.6.3, rubocop 0.82.0, haml-lint 0.35.0, and yamllint
4 files checked, 21 offenses detected

spec/models/git_repository_spec.rb

• ❗ - Line 354, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 355, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 359, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 360, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 361, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 365, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 366, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 367, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 368, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 372, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 373, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 374, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 375, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 379, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 380, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 381, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 382, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 386, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 387, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 388, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.
• ❗ - Line 389, Col 9 - Style/MethodCallWithArgsParentheses - Use parentheses for method calls with arguments.

[simaishi]

@djberg96 Can this be ivanchuk/yes (which implies jansa/yes and kasparov/yes)?

[djberg96]

@dmetzger57 what say you?

[dmetzger57]

Yes to ivanchuk/yes

[djberg96]

@miq-bot add_label ivanchuk/yes

[simaishi]

Ivanchuk backport details:

$ git log -1
commit 0ce905f5f9515848587f6cc3225ef9f89881f86c
Author: Keenan Brock <[email protected]>
Date:   Wed Nov 4 11:39:58 2020 -0500

    Merge pull request #20759 from djberg96/better_check_connection

    Add git remote connection check code

    (cherry picked from commit 41f40f81f619562e5334f94543dba8e0da42adb3)

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

[simaishi]

Jansa backport details:

$ git log -1
commit 47b8134287e94cfe3b88be5500468615cabd03bf
Author: Keenan Brock <[email protected]>
Date:   Wed Nov 4 11:39:58 2020 -0500

    Merge pull request #20759 from djberg96/better_check_connection

    Add git remote connection check code

    (cherry picked from commit 41f40f81f619562e5334f94543dba8e0da42adb3)

[simaishi]

Kasparov backport details:

$ git log -1
commit bb658d4f210764c81c1437b8e705816875fbcb27
Author: Keenan Brock <[email protected]>
Date:   Wed Nov 4 11:39:58 2020 -0500

    Merge pull request #20759 from djberg96/better_check_connection

    Add git remote connection check code

    (cherry picked from commit 41f40f81f619562e5334f94543dba8e0da42adb3)