Fix for UI hangs if errors are encountered during Git Import #12964

eclarizio · 2016-12-02T16:01:05Z

The main issue is that if github (or whatever site they're trying to use for an import) is down, it takes a really long time to timeout. @mkanoor and I figured out that in production, the apache timeout is 2 minutes, as is the timeout for the backend to decide that it should give up on trying to refresh the repo. This, coupled with the queue latency causes a 302 redirect to come back from the rails server, but apache instead serves up a 502 proxy error.

This fix aims to address this issue by instead not waiting for the task to finish on the server side, but on the client side. When we queue the task, we immediately pass the task id back to the client, which uses that to poll the server for completeness. I've currently set the poll to 1.5 seconds, if you think it should be longer or shorter, please let me know.

Links

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

Steps for Testing/QA

To mimic this in development, you can either add something to /etc/hosts like 8.8.8.8 github.com and continue using github.com. Or, when importing, just use a local IP that doesn't resolve to anything in the url field, e.g. https://192.168.1.42/test_org/test_repo.

@miq-bot add_label blocker, bug

@miq-bot assign @gmcculloug

@mkanoor Can you test and make sure everything appears to be working the same as it was before? I ran through a bunch of scenarios and it seemed to be behaving nicely, but I'd like a sanity check.
@syncrou Can you review as there's a lot of javascript changes I made here that I'd like your eye on.

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

https://bugzilla.redhat.com/show_bug.cgi?id=1393982 retrieve_git_datastore now returns a task id that the javascript then uses to poll to see if the task is done. When it is, it runs through the rendering of the git import stuff.

mkanoor · 2016-12-02T18:54:32Z

@eclarizio
Shouldn't this be implemented for the repository fetch as well as for the import since either of those activities can taker over 2 minutes depending on the size of the repository. Also In the app/services/git_based_domain_import_service we have split some of the calls to just submit the task and get the task_id.

https://github.com/ManageIQ/manageiq/blob/master/app/services/git_based_domain_import_service.rb#L51

Should we be using that approach since we already do it for the REST API which always runs async task requests via the queue and waits for the task to end by letting the user do the polling.

eclarizio · 2016-12-02T19:14:06Z

@mkanoor By the "repository fetch" do you mean when you put in the URL and hit submit? That is what this fix is on, but it's not on the actual import (yet) because they haven't ran into that issue. I'd say since this is a blocker and no one has tried to import a large enough repo that it takes longer than 2 minutes to import, that for now we should just be focused on the repository fetch.

As far as the functionality added to the git_based_domain_import_service, I'll use that for queuing the task, good catch.

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

syncrou · 2016-12-02T21:04:13Z

app/assets/javascripts/git_import.js

+    $.get('check_git_task', gitData, function(data) {
+      var parsedData = JSON.parse(data);
+      if (parsedData.state) {
+        setTimeout(GitImport.pollForGitTaskCompletion, 1500, gitData);


@eclarizio - It might be nice to set a constant for this timeout value. With that value's importance to this fix - the 1500 is hard to find buried in this function.

syncrou · 2016-12-02T21:24:54Z

app/controllers/miq_ae_tools_controller.rb

    else
      begin
-        git_repo = GitRepository.find_or_create_by!(:url => git_url) { new_git_repo = true }


@eclarizio - Nice all this was moved into the GitRepositoryService

syncrou · 2016-12-02T21:43:28Z

app/assets/javascripts/git_import.js

+      $.post('retrieve_git_datastore', $('#retrieve-git-datastore-form').serialize(), function(data) {
+        var parsedData = JSON.parse(data);
+        var messages = parsedData.message;
+        if (messages && messages.level === "error") {


@eclarizio - Code climate is asking for single quotes here - That would be consistent with the rest of the file

syncrou

Looks good minus a few small comments. I tested it as well - and didn't run into any timeout issues - It worked as advertised.

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

mkanoor · 2016-12-02T22:26:22Z

@eclarizio Looks good, I ran some tests with it. Can you implement @syncrou 's recommendations

eclarizio · 2016-12-05T16:09:47Z

@mkanoor Sorry, forgot to respond, but the changes @syncrou recommended have been made. Thanks 😄 !

syncrou

Looks Great to Me! - 👍

mkanoor · 2016-12-05T16:30:56Z

app/controllers/miq_ae_tools_controller.rb

      rescue => err
-        git_repo.destroy if git_repo && new_git_repo


@eclarizio
Shouldn't we be deleting the repository if we get a failure and its a new repo that the user was trying to create.
If the user creates bogus entries we would create repositories for each one and leave them around

@mkanoor It's being taken care of in the check_git_task action now: https://github.com/ManageIQ/manageiq/pull/12964/files/dbff1b629627610e45ed00d9f7bc9728ba6205e4#diff-6a59cc1ea26326d3a83ec0fddaf40487R251

Fix for UI hangs if errors are encountered during Git Import (cherry picked from commit f698c9e)

chessbyte · 2016-12-05T22:45:02Z

Euwe Backport conflict:

$ git cherry-pick -e -x -m 1  f698c9e                   
error: could not apply f698c9e... Merge pull request #12964 from eclarizio/BZ1393982_async
hint: after resolving the conflicts, mark the corrected paths
hint: with 'git add <paths>' or 'git rm <paths>'
hint: and commit the result with 'git commit'

$ git status
On branch euwe
Your branch is up-to-date with 'upstream/euwe'.
You are currently cherry-picking commit f698c9e.
  (fix conflicts and run "git cherry-pick --continue")
  (use "git cherry-pick --abort" to cancel the cherry-pick operation)

Changes to be committed:

	modified:   app/assets/javascripts/application.js
	new file:   app/assets/javascripts/git_import.js
	modified:   app/assets/javascripts/import.js
	new file:   app/services/git_repository_service.rb
	modified:   app/views/miq_ae_tools/_import_export.html.haml
	modified:   config/routes.rb
	modified:   spec/controllers/miq_ae_tools_controller_spec.rb
	modified:   spec/javascripts/automate_import_export_spec.js
	new file:   spec/javascripts/git_import_spec.js
	modified:   spec/javascripts/import_spec.js
	new file:   spec/services/git_repository_service_spec.rb

Unmerged paths:
  (use "git add <file>..." to mark resolution)

	both modified:   app/assets/javascripts/automate_import_export.js
	both modified:   app/controllers/miq_ae_tools_controller.rb

$ git diff

diff --cc app/assets/javascripts/automate_import_export.js
index 55921cb,b1d71b4..0000000
--- a/app/assets/javascripts/automate_import_export.js
+++ b/app/assets/javascripts/automate_import_export.js
@@@ -33,10 -33,11 +33,16 @@@ var Automate = 
  
    renderGitImport: function(branches, tags, gitRepoId, messages) {
      clearMessages();
++<<<<<<< HEAD
 +    message = JSON.parse(messages).message;
 +    messageLevel = JSON.parse(messages).level;
++=======
+ 
+     var message = messages.message;
+     var messageLevel = messages.level;
++>>>>>>> f698c9e... Merge pull request #12964 from eclarizio/BZ1393982_async
  
 -    if (messageLevel === 'error') {
 +    if (messageLevel === "error") {
        showErrorMessage(message);
      } else {
        $('.hidden-git-repo-id').val(gitRepoId);
@@@ -50,17 -51,14 +56,28 @@@
  
        var addToDropDown = function(identifier, child) {
          $('select.git-' + identifier).append(
++<<<<<<< HEAD
 +          $('<option>', {
 +            value: child,
 +            text: child
 +          })
 +        );
 +      };
 +
 +      $.each(JSON.parse(branches), function(index, child) {
 +        addToDropDown('branches', child);
 +      });
 +      $.each(JSON.parse(tags), function(index, child) {
++=======
+           $('<option>', {value: child, text: child})
+         );
+       };
+ 
+       $.each(branches, function(_index, child) {
+         addToDropDown('branches', child);
+       });
+       $.each(tags, function(_index, child) {
++>>>>>>> f698c9e... Merge pull request #12964 from eclarizio/BZ1393982_async
          addToDropDown('tags', child);
        });
  
diff --cc app/controllers/miq_ae_tools_controller.rb
index 06959ac,6569c52..0000000
--- a/app/controllers/miq_ae_tools_controller.rb
+++ b/app/controllers/miq_ae_tools_controller.rb
@@@ -205,46 -203,26 +204,31 @@@ Methods updated/added: %{method_stats}"
  
      if git_url.blank?
        add_flash(_("Please provide a valid git URL"), :error)
+       response_json = {:message => @flash_array.first}
      elsif !GitBasedDomainImportService.available?
        add_flash(_("Please enable the git owner role in order to import git repositories"), :error)
+       response_json = {:message => @flash_array.first}
      else
        begin
-         git_repo = GitRepository.find_or_create_by!(:url => git_url) { new_git_repo = true }
-         git_repo.update_attributes(:verify_ssl => verify_ssl)
-         if params[:git_username] && params[:git_password]
-           git_repo.update_authentication(:values => {:userid   => params[:git_username],
-                                                      :password => params[:git_password]})
-         end
+         setup_results = git_repository_service.setup(
+           git_url,
+           params[:git_username],
+           params[:git_password],
+           params[:git_verify_ssl]
+         )
+         git_repo_id = setup_results[:git_repo_id]
+         new_git_repo = setup_results[:new_git_repo?]
  
-         task_options = {
-           :action => "Retrieve git repository",
-           :userid => current_user.userid
-         }
-         queue_options = {
-           :class_name  => "GitRepository",
-           :method_name => "refresh",
-           :instance_id => git_repo.id,
-           :role        => "git_owner",
-           :args        => []
-         }
- 
-         task_id = MiqTask.generic_action_with_callback(task_options, queue_options)
-         task = MiqTask.wait_for_taskid(task_id)
- 
-         raise task.message unless task.status == "Ok"
- 
-         branch_names = git_repo.git_branches.collect(&:name)
-         tag_names = git_repo.git_tags.collect(&:name)
-         redirect_options[:git_branches] = branch_names.to_json
-         redirect_options[:git_tags] = tag_names.to_json
-         redirect_options[:git_repo_id] = git_repo.id
-         flash_message = "Successfully found git repository, please choose a branch or tag"
-         add_flash(_(flash_message), :success)
+         task_id = git_based_domain_import_service.queue_refresh(git_repo_id)
+         response_json = {:task_id => task_id, :git_repo_id => git_repo_id, :new_git_repo => new_git_repo}
        rescue => err
++<<<<<<< HEAD
 +        git_repo.destroy if git_repo && new_git_repo
 +        add_flash(_("Error during repository fetch: #{err.message}"), :error)
++=======
+         add_flash(_("Error during repository setup: %{error_message}") % {:error_message => err.message}, :error)
+         response_json = {:message => @flash_array.first}
++>>>>>>> f698c9e... Merge pull request #12964 from eclarizio/BZ1393982_async
        end
      end

eclarizio · 2016-12-05T22:46:14Z

@chessbyte The backport is here: #12995

chessbyte · 2016-12-05T22:49:16Z

Backported to Euwe via #12995

eclarizio added 3 commits December 2, 2016 07:42

Add new retrieve git datastore click handler in GitImport javascript

ae62daf

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

Add GitRepositoryService in preparation for extracting logic

436969b

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

miq-bot added blocker bug labels Dec 2, 2016

miq-bot assigned gmcculloug Dec 2, 2016

gmcculloug added automate euwe/yes labels Dec 2, 2016

Utilize queue_refresh call within GitBasedDomainImportService

d5ee1dd

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

gmcculloug assigned mkanoor and unassigned gmcculloug Dec 2, 2016

syncrou reviewed Dec 2, 2016

View reviewed changes

Add constant for the poll timeout value and fix quotes

dbff1b6

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

syncrou approved these changes Dec 5, 2016

View reviewed changes

mkanoor reviewed Dec 5, 2016

View reviewed changes

mkanoor approved these changes Dec 5, 2016

View reviewed changes

mkanoor merged commit f698c9e into ManageIQ:master Dec 5, 2016

mkanoor added this to the Sprint 50 Ending Dec 5, 2016 milestone Dec 5, 2016

eclarizio pushed a commit to eclarizio/manageiq that referenced this pull request Dec 5, 2016

Merge pull request ManageIQ#12964 from eclarizio/BZ1393982_async

06fdcf6

Fix for UI hangs if errors are encountered during Git Import (cherry picked from commit f698c9e)

eclarizio mentioned this pull request Dec 5, 2016

[EUWE] Backport for Fix for UI hangs if errors are encountered during Git Import #12995

Merged

chessbyte added the euwe/conflict label Dec 5, 2016

chessbyte added euwe/backported and removed euwe/conflict labels Dec 5, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix for UI hangs if errors are encountered during Git Import #12964

Fix for UI hangs if errors are encountered during Git Import #12964

eclarizio commented Dec 2, 2016 •

edited by miq-bot

Loading

mkanoor commented Dec 2, 2016

eclarizio commented Dec 2, 2016

syncrou Dec 2, 2016 •

edited

Loading

syncrou Dec 2, 2016

syncrou Dec 2, 2016

syncrou left a comment •

edited

Loading

mkanoor commented Dec 2, 2016

eclarizio commented Dec 5, 2016

syncrou left a comment

mkanoor Dec 5, 2016

eclarizio Dec 5, 2016 •

edited

Loading

chessbyte commented Dec 5, 2016

eclarizio commented Dec 5, 2016

chessbyte commented Dec 5, 2016

Fix for UI hangs if errors are encountered during Git Import #12964

Fix for UI hangs if errors are encountered during Git Import #12964

Conversation

eclarizio commented Dec 2, 2016 • edited by miq-bot Loading

Links

Steps for Testing/QA

mkanoor commented Dec 2, 2016

eclarizio commented Dec 2, 2016

syncrou Dec 2, 2016 • edited Loading

Choose a reason for hiding this comment

syncrou Dec 2, 2016

Choose a reason for hiding this comment

syncrou Dec 2, 2016

Choose a reason for hiding this comment

syncrou left a comment • edited Loading

Choose a reason for hiding this comment

mkanoor commented Dec 2, 2016

eclarizio commented Dec 5, 2016

syncrou left a comment

Choose a reason for hiding this comment

mkanoor Dec 5, 2016

Choose a reason for hiding this comment

eclarizio Dec 5, 2016 • edited Loading

Choose a reason for hiding this comment

chessbyte commented Dec 5, 2016

eclarizio commented Dec 5, 2016

chessbyte commented Dec 5, 2016

eclarizio commented Dec 2, 2016 •

edited by miq-bot

Loading

syncrou Dec 2, 2016 •

edited

Loading

syncrou left a comment •

edited

Loading

eclarizio Dec 5, 2016 •

edited

Loading