[WIP] Add ServiceAnsiblePlaybook.check_connection #20645

NickLaMuro · 2020-10-02T16:49:54Z

Allows a ServiceAnsiblePlaybook instance to check the remote endpoint for a repo to see if it can connect prior to attempting a clone.

This PR also creates some internals in GitRepository and GitWorktree to facilitate this and call into the rugged/libgit2 internals (all sharing the same method name: .check_connection)

Links

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

Steps for Testing/QA

I tested this by running it in a jansa VM with this patch applied:

From a jansa appliance UI, , and create + run a playbook service
- Login to the UI
- Enable EmbeddedAnsible
- Add an Ansible playbook repository (example: https://github.com/NickLaMuro/ansible-tower-samples.git )
- Create a catalog and add an Ansible Playbook service (example: 'hello_world.yml')
- Run the playbook service
SSH into the vm, apply the changes, and test in a rails console:

$ ssh root@[MIQ_VM]
root@MIQ_VM # vmdb
root@MIQ_VM # git init
root@MIQ_VM # git apply <(curl -L https://github.com/ManageIQ/manageiq/pull/20645.patch)
root@MIQ_VM # bin/rails c
irb> ServiceAnsiblePlaybook.first.check_connection(:provision)
#=> true

This method takes a `url` and uses that to create an anonymous remote to use the `rugged`/`libgit2` to check the connection of a given remote. Will make use of any credentials that are configured for the worktree.

For a given repository record, check the connection for the configured url. In this case, an existing repository is not used or created, and simply a "stub repository" is created in a tmpdir to allow calling `check_connection` with the configured url for the repo record.

Allows calling `.check_connection` from a given `ServiceAnsiblePlaybook` instance. A new `.repository` method was created to allow fetching the `GitRepository` record and calling `GitRepository#check_connection` from the `ServiceAnsiblePlaybook`.

NickLaMuro · 2020-10-02T16:51:03Z

@tinaafitz @d-m-u Hopefully this helps with the BZ you are working on.

miq-bot · 2020-10-02T16:55:35Z

Checked commits NickLaMuro/manageiq@f65b139~...ff7eb86 with ruby 2.6.3, rubocop 0.69.0, haml-lint 0.28.0, and yamllint
4 files checked, 0 offenses detected
Everything looks fine. ⭐

tinaafitz · 2020-10-02T18:40:04Z

Thanks so much @NickLaMuro. We'll start testing the changes.

NickLaMuro

Just a few notes on what I did for context.

NickLaMuro · 2020-10-02T18:56:54Z

app/models/service_ansible_playbook.rb

@@ -18,6 +18,10 @@ def execute(action)
    launch_ansible_job_queue(action)
  end

+  def check_connection(action)


The action argument here is just in line with how playbook also works. If this interface, and the one in ServiceTemplateAnsiblePlaybook, are incorrect, we can change, but it seems like this is how these methods function.

(by that, I mean they pass around a :provision keyword to access specific attributes in config_info)

NickLaMuro · 2020-10-02T18:59:25Z

app/models/git_repository.rb

+  # repo in a temp path to check the remote to see if it can accept
+  # connections.
+  def check_connection
+    Dir.mktmpdir do |tmp_repo_dir|


Doing this as a block to make sure the tmp dir is removed once we are done with the .check_connection call.

The result of the block is what is returned, so the directory being deleted is just a side effect of that.

app/models/git_repository.rb

NickLaMuro · 2020-10-02T19:03:53Z

lib/git_worktree.rb

+    with_remote_options do |remote_options|
+      @repo.remotes
+           .create_anonymous(url)
+           .check_connection(:fetch, remote_options)


remote_options here has our credentials and proxy info, so if that is configured, it will be used as it is with other methods in this class.

Is check_connection here a rugged method?

Yes, it is:

https://www.rubydoc.info/github/libgit2/rugged/Rugged%2FRemote:check_connection

kbrock

code checks out for me.

Like the approach and am happy we are not over optimizing this before we even get the thing up and running. The point that seals the deal for me is knowing that if there were a performance problem, Nick would not be the only one able to diagnose and fix this problem.

This looks great.

Tina - let me know if this checks out on your side and I can merge.
Or if you are not able to test, ping me and I can take a look

d-m-u · 2020-10-07T19:07:26Z

Not sure why the enhancement label, pretty sure this is a bug: see the linked bz in the description (https://bugzilla.redhat.com/show_bug.cgi?id=1835226).

NickLaMuro · 2020-10-07T19:08:56Z

@d-m-u while this PR itself is addressing a bug, but the addition is a enhancement that didn't exist previously.

NickLaMuro · 2020-10-07T19:14:35Z

Marking this as [WIP] for now while we determine what is causing a segfault with this code.

NickLaMuro · 2020-10-08T23:52:00Z

So I wanted to give an update on this...

I have been unable to replicate the segfault on another VM, and importing the automate domain properly was a research project in it of itself, so I am a bit stuck.

I think Drew has replicated this issue multiple times now, but I honestly don't know where to start on fixing the segfault and have been spinning my wheels a bit on where to start. I might consider a shell out to git ls-remote if it comes to that, though that would be a much more involved process to do just that, and probably deserves it's own separate class for handling it.

Anyway, thought I would give a bit of an update since this has been staled as a result of me not being able to figure this bug out... 😞

NickLaMuro · 2020-10-09T21:40:11Z

Leaving this as a note for myself. I think this might be a inter-opt issue with DRb, but not positive. If we are trying to work with a c-extension over DRb, I could see that potentially causing weird issues with this code and causing said segfault to happen.

That said, I currently haven't been able to recreate and setting something up to do just that has been a bit of a nightmare, so will probably work with Drew and Tina next week to work on this.

tinaafitz · 2020-10-12T13:51:41Z

Hi @NickLaMuro, Thanks so much for your work on this and for the update. I know we discussed it briefly on our call on Friday, but I'm happy to get on a call to discuss it whenever it works best for you.

I was hoping this simplified way of running the Automate check_connection method through the rails console would reproduce the segfault, but it didn't. :-(

#setup the Automate workspace $evm=MiqAeMethodService::MiqAeService.new(MiqAeEngine::MiqAeWorkspaceRuntime.new)

#get the service model wrapped Service object
sm_s = $evm.vmdb('service', 80)

sm_s.check_connection("provision")

NickLaMuro · 2020-10-26T19:49:53Z

After some major troubles with segfaults that @tinaafitz and Co have been having, I think we are going to abandon this PR in favor of an alternative solution. Going to close, and we can re-open if we change our minds.

d-m-u · 2020-10-27T14:45:09Z

lib/git_worktree.rb

@@ -258,6 +258,14 @@ def checkout
    end
  end

+  def check_connection(url)


I know this is closed, but just as a note for me so I don't lose context, we were in the process of replacing this method with Net::Ping::HTTP.new(url).ping? when we hit the subsequent error about the job not existing.

NickLaMuro added 3 commits October 2, 2020 10:40

[GitWorktree] Add #check_connection

f65b139

This method takes a `url` and uses that to create an anonymous remote to use the `rugged`/`libgit2` to check the connection of a given remote. Will make use of any credentials that are configured for the worktree.

NickLaMuro requested review from agrare, Fryguy, gtanzillo and kbrock as code owners October 2, 2020 16:49

NickLaMuro commented Oct 2, 2020

View reviewed changes

kbrock approved these changes Oct 2, 2020

View reviewed changes

chessbyte added the enhancement label Oct 6, 2020

chessbyte assigned kbrock Oct 6, 2020

NickLaMuro changed the title ~~Add ServiceAnsiblePlaybook.check_connection~~ [WIP] Add ServiceAnsiblePlaybook.check_connection Oct 7, 2020

miq-bot added the wip label Oct 7, 2020

NickLaMuro closed this Oct 26, 2020

djberg96 mentioned this pull request Oct 27, 2020

Update playbook_log_stdout to handle possible nil job #20738

Merged

d-m-u reviewed Oct 27, 2020

View reviewed changes

djberg96 mentioned this pull request Oct 29, 2020

Add git remote connection check code #20759

Merged

NickLaMuro mentioned this pull request Oct 29, 2020

[RFE][EmbeddedAnsible] SSH key credential check/validation #20760

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Add ServiceAnsiblePlaybook.check_connection #20645

[WIP] Add ServiceAnsiblePlaybook.check_connection #20645

NickLaMuro commented Oct 2, 2020 •

edited

Loading

NickLaMuro commented Oct 2, 2020

miq-bot commented Oct 2, 2020

tinaafitz commented Oct 2, 2020

NickLaMuro left a comment

NickLaMuro Oct 2, 2020

NickLaMuro Oct 2, 2020

NickLaMuro Oct 2, 2020

Fryguy Oct 7, 2020

NickLaMuro Oct 7, 2020

kbrock left a comment

d-m-u commented Oct 7, 2020

NickLaMuro commented Oct 7, 2020

NickLaMuro commented Oct 7, 2020

NickLaMuro commented Oct 8, 2020

NickLaMuro commented Oct 9, 2020

tinaafitz commented Oct 12, 2020 •

edited

Loading

NickLaMuro commented Oct 26, 2020

d-m-u Oct 27, 2020

[WIP] Add ServiceAnsiblePlaybook.check_connection #20645

[WIP] Add ServiceAnsiblePlaybook.check_connection #20645

Conversation

NickLaMuro commented Oct 2, 2020 • edited Loading

Links

Steps for Testing/QA

NickLaMuro commented Oct 2, 2020

miq-bot commented Oct 2, 2020

tinaafitz commented Oct 2, 2020

NickLaMuro left a comment

Choose a reason for hiding this comment

NickLaMuro Oct 2, 2020

Choose a reason for hiding this comment

NickLaMuro Oct 2, 2020

Choose a reason for hiding this comment

NickLaMuro Oct 2, 2020

Choose a reason for hiding this comment

Fryguy Oct 7, 2020

Choose a reason for hiding this comment

NickLaMuro Oct 7, 2020

Choose a reason for hiding this comment

kbrock left a comment

Choose a reason for hiding this comment

d-m-u commented Oct 7, 2020

NickLaMuro commented Oct 7, 2020

NickLaMuro commented Oct 7, 2020

NickLaMuro commented Oct 8, 2020

NickLaMuro commented Oct 9, 2020

tinaafitz commented Oct 12, 2020 • edited Loading

NickLaMuro commented Oct 26, 2020

d-m-u Oct 27, 2020

Choose a reason for hiding this comment

NickLaMuro commented Oct 2, 2020 •

edited

Loading

tinaafitz commented Oct 12, 2020 •

edited

Loading