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

Include socket services while stopping services during restart #653

Closed

Conversation

Gauravtalreja1
Copy link
Contributor

@Gauravtalreja1 Gauravtalreja1 commented Oct 12, 2022

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

Results:

# foreman-maintain service restart --only foreman
Running Restart Services
================================================================================
Check if command is run as root user:                                 [OK]
--------------------------------------------------------------------------------
Restart applicable services:

Stopping the following service(s):
foreman, foreman.socket
\ All services stopped

Starting the following service(s):
foreman
/ All services started                                                [OK]
--------------------------------------------------------------------------------

@ehelms
Copy link
Member

ehelms commented Oct 25, 2022

@evgeni @ekohl do y'all remember where we landed on this?

Comment on lines 16 to 17
run_service_action('stop', common_options.merge(:include_sockets => true))
run_service_action('start', common_options)
Copy link
Member

Choose a reason for hiding this comment

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

I really wonder why we don't call the restart action. I mean, systemctl is perfectly capable of calling stop and start itself if there's no restart defined. That also gets rid of the warning.

Copy link
Member

Choose a reason for hiding this comment

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

Mind filing an issue for that? we could ask to include that change here but it sorta muddies the waters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ehelms Could you please create a redmine issue for this? Thanks

Copy link
Contributor Author

@Gauravtalreja1 Gauravtalreja1 Mar 29, 2023

Choose a reason for hiding this comment

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

@ehelms @ekohl Could you please take a look at this? It has been pending for a few months now

@evgeni
Copy link
Member

evgeni commented Mar 30, 2023

Two things:

/packit build

@packit-as-a-service
Copy link

No config file for packit (e.g. .packit.yaml) found in theforeman/foreman_maintain on commit 42adca6

For more info, please check out the documentation or contact the Packit team.

@Gauravtalreja1
Copy link
Contributor Author

Gauravtalreja1 commented Aug 17, 2023

@ehelms Thanks for addressing the comment above in #733, I've updated this PR to use restart and include sockets for restart action to avoid this socket warnings, please can you re-visit this PR, if you think this PR is still valid, else we can close this. Thank you!

@ehelms
Copy link
Member

ehelms commented Aug 17, 2023

From my quick test, when we switched to using systemd restart, this was inherently fixed, here is output without this fix:

# foreman-maintain service restart --only foreman
Running Restart Services
================================================================================
Check if command is run as root user:                                 [OK]
--------------------------------------------------------------------------------
Restart applicable services: 

Restarting the following service(s):
foreman
/ All services restarted                                              [OK]      
--------------------------------------------------------------------------------

@Gauravtalreja1
Copy link
Contributor Author

@ehelms Yes, I've also tried, this warning disappears when using restart, but sockets aren't included in it, so do you think if it makes sense to include sockets while restart? if not, then we're good to close this PR I think, Thanks!

@ehelms
Copy link
Member

ehelms commented Aug 29, 2023

I do not think we need to include sockets as systemd handles this for us.

@ehelms ehelms closed this Aug 29, 2023
@Gauravtalreja1 Gauravtalreja1 deleted the fix-bz-2067120 branch November 15, 2023 17:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants