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

Fixes #13571 - Remote execution integration #5756

Merged
merged 1 commit into from Feb 16, 2016

Conversation

iNecas
Copy link
Member

@iNecas iNecas commented Feb 4, 2016

Add ability to use the Remote Execution tooling for the content actions
on hosts.

First, we need to use the developer API to define the remote execution features
and templates.

Then, there is ability in UI to jump into the remote execution UI
from the appropriate places.

katello-rex-1

katello-rex-2

Depends on theforeman/foreman_remote_execution#149.

  • - detect if Remote Execution plugin is installed and make the option available only when presented
  • - errata support

@jlsherrill
Copy link
Member

I'm okay if we assume remote execution is installed (and start requiring it by default)

we're going to do that eventually to support things like errata application so might as well do that now?

@jlsherrill
Copy link
Member

Also, curious if you think it makes sense to be able to specify a default value for whether you want to use remote execution or not. If someone really likes remote execution they may want to do that.

@jlsherrill
Copy link
Member

[test]

@ehelms
Copy link
Member

ehelms commented Feb 5, 2016

When viewing the gif, what I missed was how a user does a package install as we do them today with gofer/katello-agent. I saw the ability to choose how to perform but both looked via remote execution only.

@ehelms
Copy link
Member

ehelms commented Feb 5, 2016

I would prefer we not assume remote execution is installed and either make it a hard dependency or not. I can see value in both cases:

  1. Hard dependency on remote execution for Katello to work
  2. Optional dependency that unlocks features when present

Option 2 provides more composibility and lighter-weightness to Katello as it can manage your content but can be enhanced by adding remote execution to unlock functionality.

@@ -0,0 +1,48 @@
module Katello
class RemoteExecutionController < JobInvocationsController
Copy link
Member

Choose a reason for hiding this comment

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

Which screen in the diff is this being used at?

Copy link
Member

Choose a reason for hiding this comment

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

Looking further down the diff, it appears this is providing a controller to hit remotely but not any UI components itself?

Copy link
Member Author

Choose a reason for hiding this comment

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

It mostly uses the present UI in JobInvocationsController (from remote execution). This provides the mapping between the Katello UI and the remote execution

@iNecas
Copy link
Member Author

iNecas commented Feb 8, 2016

@ehelms @jlsherrill I have both:
- detect if Remote Execution plugin is installed and make the option available only when presented
- errata support
as a todo in this PR, just wanted to get the initial code into review earlier. So the plan is optional dependency and unlocking the new features when rex is available. The errata support should arrive today.

The original use for katello-agent is still triggered by the original buttons itselfs. I can make that as another option in the poppup, and make the default configurable in settings: seems like something people, that prefer remote execution would prefer anyway.

@iNecas
Copy link
Member Author

iNecas commented Feb 8, 2016

I've made the remote execution optiona + a setting option to make it the default action. The popup now contains "run via Katello agent" option as well

@ehelms
Copy link
Member

ehelms commented Feb 9, 2016

@jlsherrill currently this defaults to and shows Katello Agent as the top choice no matter if katello agent is installed - I lean towards not showing that as an option or simply not defaulting to it if not present. What do you think?

@iNecas consider me a REX noob here, I selected install via ssh and was taken to a job invocation screen where it failed, but I can't tell why it failed.

Question -- does the SSH option require the content host is registered against a particular proxy or the main proxy with the SSH feature enabled on it? If yes, should that option be displayed if the content host is or the main proxy doesn't have that feature?

screenshot from 2016-02-09 11-40-09

@iNecas
Copy link
Member Author

iNecas commented Feb 9, 2016

@ehelms the failure is due to host coming from Katello does not have a operating system assigned. I would like to see that fixed in the Katelo (I can do that) if you think it's a good idea? We could fallback in our template to handle this case though, @stbenjam ideas?

@iNecas
Copy link
Member Author

iNecas commented Feb 9, 2016

@ehelms ad. the proxy selection, see http://theforeman.org/plugins/foreman_remote_execution/0.2/#4.1Determiningthesmartproxyforhost : there are multiple strategies for selecting one

@stbenjam
Copy link
Contributor

stbenjam commented Feb 9, 2016

@ehelms the failure is due to host coming from Katello does not have a operating system assigned. I would like to see that fixed in the Katelo (I can do that) if you think it's a good idea? We could fallback in our template to handle this case though, @stbenjam ideas?

In a real world situation Katello hosts created through subscription-manager will have an Operating System since #5719. I think it'd be better if the test objects reflected that.

It raises an issue though that the template doesn't support anything other than Red Hat and Debian, so we should handle that case - by like as you suggested setting a default (e.g. yum), or failing better. I think I'd prefer the latter, because someone running Package Action on FreeBSD should get some helpful message.

@stbenjam
Copy link
Contributor

stbenjam commented Feb 9, 2016

Oh I see, not a test object but that was @ehelms's real host - how come it doesn't have an OS assigned? Both puppet reported hosts and subscription-manager reported hosts should have one.

@ehelms
Copy link
Member

ehelms commented Feb 9, 2016

Is this rebased to include #5719? I am not seeing in the git logs and I registered this host against this PR.

@stbenjam
Copy link
Contributor

stbenjam commented Feb 9, 2016

Yea, it's not, it was a recent merge. Rebasing should solve the problem.

The error message should be more helpful anyway, I filed http://projects.theforeman.org/issues/13632.

@iNecas iNecas force-pushed the remote-execution-integration branch from 38856a6 to 82ce68d Compare February 10, 2016 16:50
@iNecas
Copy link
Member Author

iNecas commented Feb 10, 2016

I've rebased the stuff and converted the content_host_ids

@iNecas
Copy link
Member Author

iNecas commented Feb 11, 2016

Ad. not showing the stuff when the rex/agent is not available for the host: it's a bit tricky, especially for the bulk action, where we would need to check if at least on from the group has the feature. Also, in case you want to plan the execution in the future, in theory, you would not need the host to have the proxy available right now. I would rather see this handled in remote execution by enhancing the error messages, when no proxy is available for the execution

@iNecas
Copy link
Member Author

iNecas commented Feb 11, 2016

I've taken @waldenraines suggestion and improved the JS code, it looks a bit more as Angular code now. The CI should get green now.

@iNecas iNecas force-pushed the remote-execution-integration branch from 3fc7399 to 8ebef38 Compare February 12, 2016 08:42
@iNecas
Copy link
Member Author

iNecas commented Feb 12, 2016

Tests are green now

@ehelms
Copy link
Member

ehelms commented Feb 12, 2016

I will take a look at this again today - thanks for indulging us with getting the UI code up to speed!

@iNecas
Copy link
Member Author

iNecas commented Feb 12, 2016

I've done some updates to the developer api on rex side. If you rebase theforeman/foreman_remote_execution#149 on already migrated system, you should follow theforeman/foreman_remote_execution#149 to get up to date with the schema (or just resetting the databases form scratch).

@iNecas iNecas force-pushed the remote-execution-integration branch 2 times, most recently from a2dd020 to 4266e37 Compare February 15, 2016 15:05
@ehelms
Copy link
Member

ehelms commented Feb 15, 2016

screenshot from 2016-02-15 13-18-18

I see the above error when templates aren't associated which is kind of ugly from a user perspective. Further, after a complete reset with migrate and seed I do not see the templates auto-associated to the various features. Is that intentional? I thought when I previously reviewed it they were automatically associated.

@iNecas
Copy link
Member Author

iNecas commented Feb 16, 2016

It gets associated only at a time of first seeding the templates: the reason for this is we don't know, if no template is assigned to the template on purpose or accidentally.

We could say though that we don't allow no template to be assigned to the feature, in which case we would assign it to the seeded one when seeding.

@ares
Copy link
Contributor

ares commented Feb 16, 2016

after short discussion with Ivan, I lean to that we should re-associate the template as the on purpose disassociation is quite edge case and I'd recommend using permissions for this use case

@ehelms
Copy link
Member

ehelms commented Feb 16, 2016

  1. So don't allow it to be unset? or default to the base if not otherwise specified? I just want to clarify what we are landing on so I can properly review the behavior.

  2. Are the test failures due to needing a new release of foreman_tasks?

@iNecas
Copy link
Member Author

iNecas commented Feb 16, 2016

  1. the desired behaviour will be, if unset, re-attach the original template when seeding, /me waiting for fixes #13579 - sync templates during seed theforeman/foreman_remote_execution#150 to finish tests so that I can push my changes in
  2. the issues were fixed by fixes #13723 - use git ls for gemspec theforeman/foreman-tasks#173 (comment), once I update this PR with latest changes, it should get green

@ehelms
Copy link
Member

ehelms commented Feb 16, 2016

Ok - can a template be complete unset from a feature? In other words, can the template for a feature be set to blank by the user?

@iNecas
Copy link
Member Author

iNecas commented Feb 16, 2016

yes

Add ability to use the Remote Execution tooling for the content actions
on hosts.

First, we need to use the developer API to define the remote execution features
and templates.

Then, there is ability in UI to jump into the remote execution UI
from the appropriate places (package an errata operations)

It detects, whether the remote execution feature is there.
@iNecas iNecas force-pushed the remote-execution-integration branch from 4266e37 to cda2cf7 Compare February 16, 2016 17:07
@iNecas
Copy link
Member Author

iNecas commented Feb 16, 2016

I've updated from latest PR in remote execution theforeman/foreman_remote_execution#149: now the unassigned feature should get assigned to the default template when re-seeding. Still I would consider this as a corner case. The tests should get green as well now.

@iNecas
Copy link
Member Author

iNecas commented Feb 16, 2016

@ehelms
Copy link
Member

ehelms commented Feb 16, 2016

ACK - thanks @iNecas !

ehelms added a commit that referenced this pull request Feb 16, 2016
Fixes #13571 - Remote execution integration
@ehelms ehelms merged commit aeafa56 into Katello:master Feb 16, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants