Fixes #22458 - Add Katello service command #151

johnpmitsch · 2018-03-01T18:55:26Z

Todo

write tests
test on standalone foreman and foreman proxy w/ content

johnpmitsch · 2018-03-01T19:02:44Z

@iNecas can you move this ticket to foreman maintain?

johnpmitsch · 2018-03-01T19:06:17Z

definitions/features/katello.rb

+      'foreman-tasks'            => 30,
+      'goferd'                   => 30
+    }
+  end


@iNecas @mbacovsky Should I split up these services into different features? They are being selected based if they exist on the sytem We could keep one master list of services and it shouldn't make a difference if its standalone foreman/foreman-proxy/etc. Curious to hear your thoughts.

If we do go with one list, I also am wondering where the best home for it would be 😄

What about every relevant feature (foreman, katello, pulp, tasks…) including some module (something like FeatureWithServices) and providing a method (e.g. services_info) that would provide the partial hash for it's part. Alternatively to including some module, we could mark those features with some metadata tag.

We could then get the list of relevant services with something like features.find_all { |f| f.is_a? FeatureWithServices }.map(&:service_info).reduce(&:merge).

WIth this approach, I believe we can achieve better modularity of the scripts

I like the idea of moving the services into the features. I used the same approach with config files in backup (each feature with configs has method config_files returning the list).
I also like the the idea of tagging the features and putting the master list together dynamically. For the config files the HasConfigFiles tag would equal respond_to?(:config_files) but for services it may be different, so +1 for tagging.

The question is how it would look like for features with more than one service such as Pulp. Do we want to create feature for each service?

I agree it may make sense to include some module FeatureWithServices making sure the feature comply with some service related interface that we can relay on.

Having a feature per service would be probably an overkill: the pulp feature could just return something like this:

def services { 'pulp_workers' => 20, 'pulp_celerybeat' => 20, 'pulp_resource_manager' => 20, 'pulp_streamer' => 20 } end

Thanks for the suggestions, I'll add services per feature in a module with methods to return them

Is there a way to say "what features does this machine have?" I imagine that would be helpful not only here but in the other katello scripts as well

ForemanMaintain.available_features should return that. You can also pass filtering params and filter based on class, label or tag. So ForemanMaintain.available_features(:tag => 'HasServices') should do the trick.

I tried with the config files and this worked:

features = ForemanMaintain.available_features(:tags => :has_config_files)

when the features are tagged with

metadata do tags :has_config_files end

@mbacovsky why a tag? Why not just have a method with a blank hash in ForemanMaintain::Feature and override it in the features with services?

Then we could just do ForemanMaintain.available_features.map(&:services).reduce(&:merge)
A simliar strategy could be done for config files too

I updated this part in my latest commit

johnpmitsch · 2018-03-01T19:07:02Z

lib/foreman_maintain/cli/service_command.rb

+        # TODO: DRY up labels
+        label_option
+        tags_option
+        interactive_option


@iNecas @mbacovsky what is the best way to not repeat these option methods each subcommand?

Also do I need all of these? If I didn't include them I got an error (something about "assumeyes" not found iirc), but not sure if I need all of them

Currently, we don't have any way to avoid this repetition. If we want to achieve then we will need to add wrapper on top of it but I am not sure how easy it will be.

It is giving error because when we called method run_scenario from base class we have used assumeyes? method.
Note - For flag options, Clamp provides "#{flag-option-name}?" method.

@johnpmitsch do you need label and tags? It worked for for me when removed. The interactive_option(s) are needed by runner but I'm not sure if --assumeyes and --whitelist makes sense in the scripts. I'd suggest to fix the runner to not depend on these and use it only when available.

Back to your question - if not removed it could be included in common predecessor for the service subcommands. For such case there is another notation for subcommand declaration https://github.com/mdub/clamp#declaring-subcommands.

One thing I noticed and am not sure about is the declaration of --exclude and --only options. The way it is done it allows you to call foreman-maintain service --only httpd status and also foreman-maintain service status --only httpd. Both ways work both make sense but I'd stick with one to avoid confusion. While I like the first notation (reminds me sysv init scripts notation) it is the second one our CLI is using quite uniformly.

@mbacovsky thanks, I can try to remove the label and tags option. I agree that foreman-maintain service status --only httpd would be good syntax and match our CLI usage already, I'll update to that 💯

johnpmitsch · 2018-03-01T19:08:52Z

definitions/procedures/services/list.rb

@@ -0,0 +1,21 @@
+#TODO: figure out why I need this?
+require_relative 'base.rb'


@iNecas @mbacovsky Is there a better way to do this? I would have thought the base class would be required already here, but it wasn't. Maybe I am missing something obvious 🤷‍♂️

@johnpmitsch,

Yes, you don't need to require base class here. Instead of Base class, you just need to inherit List class from ForemanMaintain::Procedure. Example -

module Procedures::Services class List < ForemanMaintain::Procedure # code end

@kgaikwad

The base class is for the services procedures to share methods, and it inherits from ForemanMaintain::Procedure. I'm fine with the require_relative here but I am just wondering if there is a better way such as adding something in `lib/foreman_maintain.rb

iNecas · 2018-03-02T08:56:21Z

@ehelms was faster than me in moving the ticked. I however put you to developers of foreman-maintain in redmine, so you should be able to do the change on your own.

johnpmitsch · 2018-03-20T20:50:15Z

@ntkathole thanks for the review! To confirm, were you able to test out on both 6.1 and 6.2? Also testing RHEL6? I did my best to test this out locally but would like someone to confirm that they tested this w/o issue

ntkathole · 2018-03-21T07:01:47Z

@johnpmitsch Tested on sat 6.1, 6.2 and 6.3. For 6.2, when service are stopped, I faced an issue http://projects.theforeman.org/issues/22945 but, it is not related to this PR. RHEL 6 is not tested as afaik foreman-maintain don't support it (we use clone tool for migration). Please correct me here/ let me know if rhel6 testing is needed/

iNecas · 2018-03-21T09:26:28Z

Since so far, we've been targeting with one release against multiple versions, and 6.1 and 6.2 are supported on rhel6, we should also test this scenario on rhel6

iNecas · 2018-03-21T09:26:42Z

And yes, the regressions you mentioned is not related

mbacovsky · 2018-03-26T10:08:41Z

definitions/features/foreman_server.rb

+
+      def services
+        {
+          'postgresql'               => 5,


Shouldn't postgresql be in foreman_database and candlepin_database features?

mbacovsky · 2018-03-26T12:10:26Z

@johnpmitsch so far it looks good, I've tested it on recent Foreman + Katello and Sat 6.1 on RHEL 6. I' d like to give it a try on plain Foreman.
The only unexpected thing so far was that on Sat 6.1 + RHEL6 the following restarted tomcat6 while the same command on Foreman + Katello restarted only httpd

# ./bin/foreman-maintain service restart --only httpd
Running Restart Services
================================================================================
Check if command is run as root user:                                 [OK]
--------------------------------------------------------------------------------
Restart applicable services: 
Stopping the following service(s):
httpd

/ Services are handled by katello-service in Satellite versions 6.2 and earlier. Redirecting to: 
katello-service stop --exclude mongod,postgresql,qpidd,qdrouterd,pulp_resource_manager,pulp_workers,foreman-proxy,pulp_celerybeat,elasticsearch,foreman-tasks
- Stopping httpd: [  OK  ]                                                      
Stopping tomcat6: waiting for processes 28529 to exit
[  OK  ]
Success!

Starting the following service(s):
httpd

| Services are handled by katello-service in Satellite versions 6.2 and earlier. Redirecting to: 
katello-service start --exclude mongod,postgresql,qpidd,qdrouterd,pulp_resource_manager,pulp_workers,foreman-proxy,pulp_celerybeat,elasticsearch,foreman-tasks
/ [Mon Mar 26 12:00:41 2018] [warn] module passenger_module is already loaded, skipping
Starting tomcat6: [  OK  ]
Starting httpd: [  OK  ]
Success!
                                                                    [OK]
--------------------------------------------------------------------------------

mbacovsky · 2018-03-26T13:04:49Z

@johnpmitsch on Foreman without katello the -p option is not applicable. It may make sense to condition it by presence of feature(:katello).

mbacovsky · 2018-03-28T12:14:00Z

@johnpmitsch I've investigated the issue with restart --only httpd restarting also the tomcat. The reason is when the --only flag is simulated with --exclude we omit the tomcat in the list. The reason is that on Sat 6.1 the tomcat service is called tomcat6. Adding tomcat6 to the list of services solved the issue.

mbacovsky · 2018-03-28T12:23:01Z

definitions/features/candlepin_database.rb

@@ -43,6 +43,10 @@ def env_content_ids_with_null_content
    query(sql).map { |r| r['id'] }
  end

+  def services
+    { 'tomcat' => 20 }


tomcat6 should be listed for Sat 6.1. Also this does not feel like the right place for these services. I'd move them to candlepin or katello. For candlepin_database the related service would be potgresql.

johnpmitsch · 2018-03-28T14:16:06Z

@mbacovsky I've updated the hammer ping part to be restricted by feature(:katello) and was able to use the option_wrapper method I added for this as well, it seems to be hidden properly on foreman and show on katello just fine, but please double check me 😺

@mbacovsky I added tomcat6 to the candlepin_database feature for now, but I am thinking we should make a candlepin feature that is confined by the package candlepin and add the services there?

johnpmitsch · 2018-03-28T14:17:00Z

(sorry changes aren't in a separate commit, I have a bad amend + force push habit) 🚬

johnpmitsch · 2018-03-28T14:21:29Z

actually hold on reviewing plz, for some reason hammer ping is running all the time now

mbacovsky · 2018-03-28T14:29:33Z

@johnpmitsch +1 on adding the candlepin feature

johnpmitsch · 2018-03-28T15:31:12Z

@mbacovsky updated the option_wrapper according to our discussion off thread and also moved the candlepin services to a new candlepin feature

johnpmitsch · 2018-03-28T15:49:11Z

@mbacovsky one update on the option_wrapper, I had to move away from passing in a default argument like option_wrapper("whitelist", []), as this led to whitelist being nil for the commands that have the argument.

i.e. for health check, the options would be
{:assumeyes=>nil, :whitelist=>nil, :force=>nil}

which is why we need something like option_wrapper("whitelist") || [], which will actually pass the empty array when the param is responded to.

I kept the argument there in case its needed, but it defaults to false

Katello-service is added as foreman-maintain service

mbacovsky · 2018-04-03T13:45:34Z

@johnpmitsch, I've tested against plain Foreman and Sat 6.1 and all the issues were fixed. Thanks!

One last thing I'd prefer to get fixed is moving postgres service to candlepin_database and foreman_database features as they are bound closer to the service than the :foreman_server is.

We will need to do this anyway for adding support for remote DBs, so I'm okay to postpone till then if you prefer to no longer block this PR.

mbacovsky · 2018-04-03T15:19:25Z

ACK, given we address the postgres service later. Thanks @johnpmitsch, 🎆 👍

mbacovsky · 2018-04-03T19:37:06Z

@iNecas, If there are no objections could you merge, please?

iNecas · 2018-04-04T06:51:15Z

Thanks @johnpmitsch, @mbacovsky and @ntkathole

theforeman-bot added Waiting on contributor Needs testing Not yet reviewed labels Mar 1, 2018

johnpmitsch force-pushed the katello-service-context branch from bc8c5b9 to 663cbea Compare March 1, 2018 18:59

theforeman-bot added Waiting on contributor Needs testing and removed Waiting on contributor labels Mar 1, 2018

johnpmitsch force-pushed the katello-service-context branch from 663cbea to 7c5cf13 Compare March 1, 2018 19:01

theforeman-bot added Waiting on contributor Needs testing and removed Waiting on contributor labels Mar 1, 2018

johnpmitsch commented Mar 1, 2018

View reviewed changes

theforeman-bot added the Waiting on contributor label Mar 1, 2018

johnpmitsch commented Mar 1, 2018

View reviewed changes

theforeman-bot added Waiting on contributor labels Mar 1, 2018

johnpmitsch commented Mar 1, 2018

View reviewed changes

theforeman-bot added Waiting on contributor labels Mar 1, 2018

theforeman deleted a comment from theforeman-bot Mar 2, 2018

kgaikwad mentioned this pull request Mar 21, 2018

Fixes #22945 - show failure if DB service stopped #160

Merged

mbacovsky reviewed Mar 26, 2018

View reviewed changes

mbacovsky reviewed Mar 28, 2018

View reviewed changes

johnpmitsch force-pushed the katello-service-context branch 3 times, most recently from cea5896 to 80a7757 Compare March 28, 2018 14:09

johnpmitsch force-pushed the katello-service-context branch 2 times, most recently from f0996f3 to ee7ed4d Compare March 28, 2018 15:26

johnpmitsch force-pushed the katello-service-context branch from ee7ed4d to 09d067b Compare March 28, 2018 15:48

johnpmitsch force-pushed the katello-service-context branch from 09d067b to 2bc37ba Compare April 2, 2018 15:24

Fixes #22458 - Add katello-service functionality

2bc37ba

Katello-service is added as foreman-maintain service

iNecas merged commit e4e08d5 into theforeman:master Apr 4, 2018

johnpmitsch deleted the katello-service-context branch April 11, 2018 17:22

		@@ -0,0 +1,21 @@
		#TODO: figure out why I need this?
		require_relative 'base.rb'

Fixes #22458 - Add Katello service command #151

Fixes #22458 - Add Katello service command #151

Conversation

johnpmitsch commented Mar 1, 2018 • edited Loading

johnpmitsch commented Mar 1, 2018 • 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

iNecas Mar 2, 2018 • 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

johnpmitsch Mar 2, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnpmitsch Mar 1, 2018 • 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

iNecas commented Mar 2, 2018

johnpmitsch commented Mar 20, 2018

ntkathole commented Mar 21, 2018

iNecas commented Mar 21, 2018

iNecas commented Mar 21, 2018

Choose a reason for hiding this comment

mbacovsky commented Mar 26, 2018

mbacovsky commented Mar 26, 2018

mbacovsky commented Mar 28, 2018

Choose a reason for hiding this comment

johnpmitsch commented Mar 28, 2018

johnpmitsch commented Mar 28, 2018

johnpmitsch commented Mar 28, 2018

mbacovsky commented Mar 28, 2018

johnpmitsch commented Mar 28, 2018

johnpmitsch commented Mar 28, 2018 • edited Loading

mbacovsky commented Apr 3, 2018

mbacovsky commented Apr 3, 2018

mbacovsky commented Apr 3, 2018

iNecas commented Apr 4, 2018

johnpmitsch commented Mar 1, 2018 •

edited

Loading

johnpmitsch commented Mar 1, 2018 •

edited

Loading

iNecas Mar 2, 2018 •

edited

Loading

johnpmitsch Mar 2, 2018 •

edited

Loading

johnpmitsch Mar 1, 2018 •

edited

Loading

johnpmitsch commented Mar 28, 2018 •

edited

Loading