Fixes #29246 - introduce new feature to handle foreman service #321

kgaikwad · 2020-03-03T07:06:06Z

adding @sthirugn into loop.
@sthirugn, I have mentioned service priority as 30. Please correct me if any modifications required.
It would be good to have your review on overall changes.

Reference links for these changes:
theforeman/puppet-foreman#802
https://github.com/theforeman/foreman/blob/develop/extras/systemd/foreman.service

theforeman-bot · 2020-03-03T07:06:18Z

Issues: #29246

ehelms · 2020-03-03T12:44:00Z

definitions/features/puma.rb

@@ -0,0 +1,18 @@
+module ForemanMaintain
+  module Features
+    class Puma < ForemanMaintain::Feature


Is it possible to name this something related to foreman-service given thats the systemd service?

I agree here too, we don't need to call out puma.

Renamed feature puma to foreman_service.

upadhyeammit · 2020-03-03T13:54:43Z

Few thoughts on this one,

The maintain already has foreman_server feature which has httpd as service. Here we consider passenger needs the httpd service and we dont have separate service for passenger.
Can we keep the foreman_server as parent class and have two children, one for httpd and other for puma ? The common things of both features goes in foreman_server and we delegate the methods, variables etc to either puma, httpd(passenger) as per what is available ?

This way we still refer to foreman_server while doing or adding changes and at same time maintain both of these ?

sthirugn · 2020-03-03T15:26:22Z

Few thoughts on this one,

1. The maintain already has foreman_server feature which has httpd as service. Here we consider passenger needs the httpd service and we dont have separate service for passenger.

The passenger is not a separate service but it is embedded into httpd itself. But the new foreman.service is a separate service outside of httpd. Also note that we will drop passenger in the near future.

2. Can we keep the foreman_server as parent class and have two children, one for httpd and other for puma ? The common things of both features goes in foreman_server and we delegate the methods, variables etc to either puma, httpd(passenger) as per what is available ?

This way we still refer to foreman_server while doing or adding changes and at same time maintain both of these ?

I recommend keeping the new foreman.service separate as it is intended to be separate from httpd.

ehelms · 2020-03-03T15:35:43Z

Starting with Foreman 2.1 (Sat 6.8) the architecture will be a separate foreman service with Apache as a reverse proxy. So httpd will no longer need to be restarted to restart Foreman with the Foreman server.

kgaikwad · 2020-03-04T12:29:14Z

Starting with Foreman 2.1 (Sat 6.8) the architecture will be a separate foreman service with Apache as a reverse proxy. So httpd will no longer need to be restarted to restart Foreman with the Foreman server.

@ehelms, currently httpd service added in three features i.e foreman, pulp2, pulpcore. Doesn't that mean it will no longer required in all or only applicable for Foreman?

kgaikwad · 2020-03-04T12:38:24Z

Can we keep the foreman_server as parent class and have two children, one for httpd and other for puma ? The common things of both features goes in foreman_server and we delegate the methods, variables etc to either puma, httpd(passenger) as per what is available ?
This way we still refer to foreman_server while doing or adding changes and at same time maintain both of these ?
I recommend keeping the new foreman.service separate as it is intended to be separate from httpd.

@upadhyeammit, @sthirugn,
I have kept foreman_service as separate feature but modified services list in foreman_server based on foreman_service feature presence.
Will it be okay?

ehelms · 2020-03-04T12:39:03Z

Apache remains the application server for Pulp 2. For Foreman and Pulpcore Apache is a reverse proxy to their services.

…

On Wed, Mar 4, 2020, 7:29 AM kgaikwad ***@***.***> wrote: Starting with Foreman 2.1 (Sat 6.8) the architecture will be a separate foreman service with Apache as a reverse proxy. So httpd will no longer need to be restarted to restart Foreman with the Foreman server. Currently httpd service added in three features i.e foreman, pulp2, pulpcore. Doesn't that mean it will no longer required in all or only applicable for Foreman? — You are receiving this because you commented. Reply to this email directly, view it on GitHub <#321?email_source=notifications&email_token=AACHT47OLYEQ53PPCUVMEXDRFZCRVA5CNFSM4LAD57NKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOENXTXOI#issuecomment-594492345>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACHT45SCS5XT7K6GHQPRETRFZCRVANCNFSM4LAD57NA> .

definitions/features/foreman_server.rb

sthirugn · 2020-03-04T18:54:27Z

definitions/features/foreman_service.rb

+  module Features
+    class ForemanService < ForemanMaintain::Feature
+      metadata do
+        label :puma


I think you renamed everything but missed this puma?

My bad. Fixed!

no worries, thank you for the quick fix

sthirugn · 2020-03-04T18:56:48Z

@upadhyeammit, @sthirugn,
I have kept foreman_service as separate feature but modified services list in foreman_server based on foreman_service feature presence.
Will it be okay?

I am not fully aware of foreman_maintain architecture, so the changes here look sane to me in general. I will let @upadhyeammit weigh in with respect to architecture.

kgaikwad · 2020-03-05T10:55:35Z

Tests are failing due to current changes. I will update PR.

ekohl · 2020-03-20T17:59:31Z

As part of theforeman/foreman#7536 we'll also add foreman.socket

kgaikwad · 2020-03-24T09:30:17Z

For Foreman and Pulpcore Apache is a reverse proxy to their services.

@ehelms, @sthirugn,
Does that mean httpd service will be there?
Only difference is foreman/pulpcore will have different services to restart.
I am asking this thing repeatitively because want to understand that along with required changes foreman-maintain should/should not show httpd service in service list as output of command foreman-maintain service list.

ehelms · 2020-03-24T18:57:46Z

Yes, httpd should still be shown.

kgaikwad · 2020-03-25T15:00:52Z

@ehelms, @upadhyeammit,
I have removed feature foreman_service and added apache feature just to handle httpd service.

As part of theforeman/foreman#7536 we'll also add foreman.socket

@ekohl, does it mean like services we need to provide way to apply actions (start/stop/status) for socket?

ehelms · 2020-04-01T12:54:15Z

definitions/features/foreman_server.rb

-          system_service('httpd', 30)
-        ]
+        if foreman_service_installed?
+          [system_service('foreman', 30)]


We have introduced a foreman.socket that works like a service but does not need to be restarted. However, I think it would be helpful to ensure that the socket is running whenever we perform service operations. @ekohl what do you think regarding handling the socket?

ekohl · 2020-04-01T13:15:25Z

If the socket is started and Apache is as well, it'll automatically start foreman.service. Probably good to ensure it's stopped as well when stopping foreman.service. Note that foreman.service requires foreman.socket so there's no need to start both.

Short summary:

Full stop: systemctl stop foreman.socket foreman.service
Start lazy (when the first request comes in): systemctl start foreman.socket
Start eager: systemctl start foreman.service (foreman.socket can be added but is redundant)
Restart: systemctl restart foreman.service
Reload: systemctl reload foreman.service

kgaikwad · 2020-04-02T11:06:40Z

Thank you @ehelms, @ekohl for providing more details on it.
I have removed WIP status as I think current changes will sufficient. It would be good to have your review on this.

kgaikwad · 2020-04-13T07:07:00Z

@sthirugn,
To test these changes, what kind of setup I will require?

ekohl · 2020-04-13T10:18:53Z

All changes for the service are in nightly.

sthirugn · 2020-04-14T20:06:30Z

@sthirugn, could you verify updated changes? please let me know if any change required.

My test worked fine, pending @ekohl's new comments.

ekohl · 2020-04-14T21:19:11Z

IIRC, there is change in priorities in case of dynflow-sidekiq services.

Ah, that makes sense.

ekohl · 2020-04-21T12:43:07Z

definitions/features/foreman_server.rb

-        [
-          system_service('httpd', 30)
-        ]
+        if foreman_service_installed?


If foreman-service is installed, Apache is usually still used as a reverse proxy. The installer can set it up without Apache, but I personally haven't used that and we don't recommend it unless you know what you're doing.

If foreman-service is installed, Apache is usually still used as a reverse proxy.

Yes, Apache will be there. To handle this, I have added Apache feature separately which having httpd in services list.

here -> https://github.com/theforeman/foreman_maintain/pull/321/files#diff-9c2e92406f1636a749c776ff0cd1b1ceR12

kgaikwad · 2020-04-23T10:59:45Z

Resolved Rubocop warnings.
Also replaced package checking with systemctl is-enabled foreman check in foreman_server feature.

upadhyeammit

Added one comment, request to check.

definitions/features/service.rb

ekohl

Small nit, but overall 👍

definitions/features/service.rb

With this commit, renamed feature puma to foreman_service and modified the hammer setup tests.

kgaikwad · 2020-05-05T15:18:29Z

@upadhyeammit, @ekohl,
Updated PR with suggested changes.

sthirugn · 2020-05-05T15:43:03Z

looks great

ekohl

Untested, but 👍 from a conceptual perspective. I'll leave it for other reviewers.

theforeman-bot added Needs testing Not yet reviewed labels Mar 3, 2020

ehelms reviewed Mar 3, 2020

View reviewed changes

kgaikwad commented Mar 4, 2020

View reviewed changes

definitions/features/foreman_server.rb Show resolved Hide resolved

sthirugn reviewed Mar 4, 2020

View reviewed changes

kgaikwad force-pushed the 29246_foreman_service branch 2 times, most recently from 35cafd5 to 686487e Compare March 5, 2020 03:30

kgaikwad changed the title ~~[WIP]Fixes #29246 - introduce puma feature to handle foreman service~~ [WIP]Fixes #29246 - introduce new feature to handle foreman service Mar 5, 2020

kgaikwad force-pushed the 29246_foreman_service branch from 038ce71 to fb3c9d2 Compare March 25, 2020 14:55

kgaikwad requested review from ehelms and upadhyeammit March 30, 2020 15:19

ehelms reviewed Apr 1, 2020

View reviewed changes

kgaikwad changed the title ~~[WIP]Fixes #29246 - introduce new feature to handle foreman service~~ Fixes #29246 - introduce new feature to handle foreman service Apr 2, 2020

kgaikwad requested review from ekohl and ehelms April 16, 2020 15:57

ekohl reviewed Apr 21, 2020

View reviewed changes

kgaikwad force-pushed the 29246_foreman_service branch from f5eddbe to 5dc2073 Compare April 23, 2020 10:35

kgaikwad requested a review from ekohl April 23, 2020 10:59

upadhyeammit reviewed Apr 28, 2020

View reviewed changes

definitions/features/service.rb Outdated Show resolved Hide resolved

kgaikwad force-pushed the 29246_foreman_service branch from 4250319 to 11a104c Compare April 28, 2020 07:09

ekohl reviewed May 5, 2020

View reviewed changes

definitions/features/service.rb Outdated Show resolved Hide resolved

kgaikwad added 9 commits May 5, 2020 10:19

Fixes #29246 - introduce new feature to handle foreman service

53f3b1b

Refs #29246 - handle hammer setup based on service change

97fe2f8

With this commit, renamed feature puma to foreman_service and modified the hammer setup tests.

Refs #29246 - removes foreman-service & adds apache feature

2359c34

Refs #29246 - override stop service options when foreman present

8a722ac

Refs #29246 - add generic handling for sockets

e09ca12

Refs #29246 - refactored socket handling changes

82bbfe5

Refs #29246 - fix rubocop warnings

cf59708

Refs #29246 - remove dead code

1b2d083

Refs #29246 - refactored code while extending list with sockets

53faf85

kgaikwad force-pushed the 29246_foreman_service branch from 11a104c to 53faf85 Compare May 5, 2020 14:52

ekohl approved these changes May 5, 2020

View reviewed changes

theforeman-bot removed the Not yet reviewed label May 5, 2020

kgaikwad added the 0.6.x changes targeted for 6.8 label May 7, 2020

kgaikwad requested a review from upadhyeammit May 12, 2020 13:07

ehelms approved these changes May 12, 2020

View reviewed changes

upadhyeammit approved these changes May 13, 2020

View reviewed changes

upadhyeammit merged commit 675680f into theforeman:master May 13, 2020

Fixes #29246 - introduce new feature to handle foreman service #321

Fixes #29246 - introduce new feature to handle foreman service #321

Conversation

kgaikwad commented Mar 3, 2020 • edited Loading

theforeman-bot commented Mar 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

upadhyeammit commented Mar 3, 2020

sthirugn commented Mar 3, 2020

ehelms commented Mar 3, 2020

kgaikwad commented Mar 4, 2020 • edited Loading

kgaikwad commented Mar 4, 2020

ehelms commented Mar 4, 2020 via email

sthirugn Mar 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sthirugn commented Mar 4, 2020 • edited Loading

kgaikwad commented Mar 5, 2020

ekohl commented Mar 20, 2020

kgaikwad commented Mar 24, 2020 • edited Loading

ehelms commented Mar 24, 2020

kgaikwad commented Mar 25, 2020

Choose a reason for hiding this comment

ekohl commented Apr 1, 2020

kgaikwad commented Apr 2, 2020

kgaikwad commented Apr 13, 2020

ekohl commented Apr 13, 2020

sthirugn commented Apr 14, 2020 • edited Loading

ekohl commented Apr 14, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kgaikwad commented Apr 23, 2020

upadhyeammit left a comment

Choose a reason for hiding this comment

ekohl left a comment

Choose a reason for hiding this comment

kgaikwad commented May 5, 2020

sthirugn commented May 5, 2020

ekohl left a comment

Choose a reason for hiding this comment

kgaikwad commented Mar 3, 2020 •

edited

Loading

kgaikwad commented Mar 4, 2020 •

edited

Loading

sthirugn Mar 4, 2020 •

edited

Loading

sthirugn commented Mar 4, 2020 •

edited

Loading

kgaikwad commented Mar 24, 2020 •

edited

Loading

sthirugn commented Apr 14, 2020 •

edited

Loading