Model change for Ansible Tower Credential #13773

jameswnl · 2017-02-06T16:39:28Z

We're duplicating this api_creat code. We need to refactor this (next round)

jameswnl · 2017-02-06T16:40:00Z

@miq-bot add_labels wip, enhancement, providers/ansible_tower

jameswnl · 2017-02-06T20:52:02Z

@miq-bot remove_label wip

Fryguy · 2017-02-07T18:03:54Z

@bdunne Please review.

bdunne · 2017-02-07T18:53:25Z

app/models/manageiq/providers/ansible_tower/automation_manager/machine_credential/api_create.rb

+    def create_in_provider(manager_id, params)
+      manager = ExtManagementSystem.find(manager_id)
+      credential = manager.with_provider_connection do |connection|
+        connection.api.credentials.create!(params)


Should we verify that the kind is ssh? Or merge that in as a default option?

Yeah, that was the same thing I am not sure about. Actually not just kind, my question is do we want to validate the input? and how much we want to validate? Or let Tower's validation does its work?

My concern about kind is that this is a create on a particular leaf class. If you want to move this logic up to the superclass so that it is shared by all AutomationManager::Authentications, then I would be okay with letting Tower do all of the data validation. Is there a reason for this method to be on the leaf class rather than the superclass? (I don't think there's anything specific to this leaf class here)

I don't like this at all and that's why I have only done this to MachineCredential. I prefer this to be done at a higher level class. The existing AutomationManager::Authentication is not the right place because this particular behavior is Tower specific.
We need to insert another Authentication in the name space of Tower. Are you onboard with that?

I agree, this is Tower specific logic and should live in ManageIQ::Providers::AnsibleTower::AutomationManager::Authentication

Great. It's in now

bdunne · 2017-02-07T19:10:00Z

...ls/manageiq/providers/ansible_tower/automation_manager/machine_credential/api_create_spec.rb

+      {
+        :description  => "Description",
+        :name         => "My Credential",
+        :related      => {}


I would expect to see :username here and maybe some other fields too.

…al and move api_create there

blomquisg · 2017-02-15T15:16:07Z

Holding off merging this until #13879 is merged. I believe that #13879 is adding many of the automate models added here, too.

Fryguy · 2017-02-15T21:54:06Z

app/models/manageiq/providers/ansible_tower/automation_manager/credential/api_create.rb

+      # TODO: This needs to be targeted refresh so it doesn't take too long
+      EmsRefresh.queue_refresh(manager, nil, true) if !manager.missing_credentials? && manager.authentication_status_ok?
+
+      find_by(:resource_id => manager.id, :manager_ref => credential.id)


I don't understand this line. You shouldn't query against the resource_id as it's part of a polymorphic pair. You have to query with:

find_by(:resource => manager, :manager_ref => credential.id)

Fryguy · 2017-02-15T21:54:52Z

app/models/manageiq/providers/ansible_tower/automation_manager/credential/api_create.rb

+
+      # Get the record in our database
+      # TODO: This needs to be targeted refresh so it doesn't take too long
+      EmsRefresh.queue_refresh(manager, nil, true) if !manager.missing_credentials? && manager.authentication_status_ok?


I really don't like this blocking call, but it is what it is for the existing pattern. cc @bdunne @agrare

Typically CRUD of native is done inside of a state machine like how provisioning does it where there is a step to create natively, poke the refresh asynchronously, then transition to a state where you can check asynchronously. All of these seems to be happening directly in this method, which means a worker is going to be completely locked waiting for refresh to complete.

I feel like a better approach for ANY CRUD of native objects is to use some sort of MiqTask, but not as it's done here. This task should call a method that natively creates the object, then pokes the refresh and then immediately returns. The MiqTask is then responsible for the next step of seeing if the refresh has completed. Once the MiqTask completes, it can use UI Notifications to tell the user that the object is ready.

So, for the use case of creating a credential and then using it during creating a repository, I would see:

User creates the credential

Creation request put in queue via a task and user is notified that "Request to create credential has started"

Queue picks up the message, executes the native create, pokes the EmsRefresh

Task then puts polling on the queue until the expected object is in the database.

Task then notifies via UI notification, and marks itself as done.

The User will then see the notification and can go create his repository. If he tries any earlier, the new cred just won't appear.

Or perhaps a ::Job is a better choice and create a "real" state machine

Another option is doing what's similar to the rhev create_vm event where we inject ourselves to create the object on the fly without impacting the EMS Refresh. cc @agrare

The api_create method that this is based on is changing too, I agree with splitting the PR.

This PR is following the pattern established in #13495. And API create is the key goal of this PR. The modeling code will need refresh once GregB's new models PR is merged.

I can close this for now and re-create once the new pattern is established -- @bdunne is there a PR for the new changes?

If you have to return the id of the object that is created (and looks like you are) then the rhev refresh new vm is probably the right approach, just create the basic object kick off a refresh and return the new id.

Otherwise I'd love to put anything waiting for a refresh in a Task/Job state machine.

We just have to weigh time to implement versus current implementation. If we already do this "wait for refresh" elsewhere, we can continue that for now, with the plan to get away from it.

@jameswnl No need to close, just split the PR. This PR can just be the new models, and ApiCreate can be split out.

Fryguy · 2017-02-15T22:10:54Z

@jameswnl If you split the modeling from the ApiCreate stuff, then I can merge that independent. They are not related in this PR.

blomquisg · 2017-02-16T15:54:51Z

So, this PR is

APICreate, and
Some model hierarchy cleanup and adding some automate models.

For #1 we want to hold off and talk about the APICreate approach.

For #2, those models are part of #13879.

So, for now, I guess this PR can just be closed until we figure out the right approach for APICreate.

Thoughts? @jameswnl @Fryguy @bdunne

jameswnl · 2017-02-16T15:59:20Z

@blomquisg yes, that's what I think too. closing it.

miq-bot · 2017-02-16T18:42:39Z

Checked commits jameswnl/manageiq@fe4fd78~...c8e2fe2 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
9 files checked, 1 offense detected

spec/lib/miq_automation_engine/service_models/miq_ae_service_manageiq-providers-ansible_tower-automation_manager-credential_spec.rb

❗ - Line 1, Col 1 - Style/FileName - The name of this source file (miq_ae_service_manageiq-providers-ansible_tower-automation_manager-credential_spec.rb) should use snake_case.

jameswnl changed the title ~~Allow MachineCredential to create in Tower~~ [WIP] Allow MachineCredential to create in Tower Feb 6, 2017

miq-bot added enhancement providers/ansible_tower wip labels Feb 6, 2017

jameswnl force-pushed the credential-crud branch 2 times, most recently from 91984ac to bdedab2 Compare February 6, 2017 20:51

jameswnl changed the title ~~[WIP] Allow MachineCredential to create in Tower~~ Allow MachineCredential to create in Tower Feb 6, 2017

miq-bot removed the wip label Feb 6, 2017

chessbyte assigned blomquisg Feb 6, 2017

jameswnl force-pushed the credential-crud branch from bdedab2 to 3cdc450 Compare February 6, 2017 22:00

Allow MachineCredential to create in Tower

fe4fd78

jameswnl force-pushed the credential-crud branch from 3cdc450 to fe4fd78 Compare February 6, 2017 23:04

jntullo mentioned this pull request Feb 7, 2017

Authentications Read and Delete api #13780

Merged

bdunne reviewed Feb 7, 2017

View reviewed changes

jameswnl force-pushed the credential-crud branch 3 times, most recently from 31bb1db to d330632 Compare February 8, 2017 18:56

Create ManageIQ::Providers::AnsibleTower::AutomationManager::Credenti…

55427c6

…al and move api_create there

jameswnl force-pushed the credential-crud branch from d330632 to 55427c6 Compare February 8, 2017 19:32

jameswnl changed the title ~~Allow MachineCredential to create in Tower~~ Allow Ansible Tower Credential to create in Tower Feb 8, 2017

Fryguy reviewed Feb 15, 2017

View reviewed changes

jameswnl closed this Feb 16, 2017

remove api_create of credential

c8e2fe2

jameswnl reopened this Feb 16, 2017

jameswnl changed the title ~~Allow Ansible Tower Credential to create in Tower~~ Model change for Ansible Tower Credential Feb 16, 2017

Fryguy merged commit 7034341 into ManageIQ:master Feb 16, 2017

Fryguy added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 16, 2017

jameswnl deleted the credential-crud branch February 16, 2017 20:00

Fryguy added the providers/automation label Jan 24, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Model change for Ansible Tower Credential #13773

Model change for Ansible Tower Credential #13773

jameswnl commented Feb 6, 2017 •

edited

Loading

jameswnl commented Feb 6, 2017

jameswnl commented Feb 6, 2017

Fryguy commented Feb 7, 2017

bdunne Feb 7, 2017

jameswnl Feb 7, 2017

bdunne Feb 7, 2017

jameswnl Feb 8, 2017

bdunne Feb 8, 2017

jameswnl Feb 8, 2017

bdunne Feb 7, 2017

blomquisg commented Feb 15, 2017

Fryguy Feb 15, 2017

Fryguy Feb 15, 2017 •

edited

Loading

Fryguy Feb 15, 2017

Fryguy Feb 15, 2017

Fryguy Feb 15, 2017

bdunne Feb 15, 2017

jameswnl Feb 15, 2017

agrare Feb 16, 2017

Fryguy Feb 16, 2017

Fryguy commented Feb 15, 2017

blomquisg commented Feb 16, 2017

jameswnl commented Feb 16, 2017

miq-bot commented Feb 16, 2017

Model change for Ansible Tower Credential #13773

Model change for Ansible Tower Credential #13773

Conversation

jameswnl commented Feb 6, 2017 • edited Loading

jameswnl commented Feb 6, 2017

jameswnl commented Feb 6, 2017

Fryguy commented Feb 7, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blomquisg commented Feb 15, 2017

Choose a reason for hiding this comment

Fryguy Feb 15, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Fryguy commented Feb 15, 2017

blomquisg commented Feb 16, 2017

jameswnl commented Feb 16, 2017

miq-bot commented Feb 16, 2017

jameswnl commented Feb 6, 2017 •

edited

Loading

Fryguy Feb 15, 2017 •

edited

Loading