Skip to content
This repository was archived by the owner on Apr 26, 2024. It is now read-only.

Add synctl warning when no process is stopped and fix restart#6598

Merged
clokep merged 9 commits intomatrix-org:developfrom
romainbou:5166
May 19, 2020
Merged

Add synctl warning when no process is stopped and fix restart#6598
clokep merged 9 commits intomatrix-org:developfrom
romainbou:5166

Conversation

@romainbou
Copy link
Contributor

@romainbou romainbou commented Dec 27, 2019 •
edited
Loading

As suggested in #5166, I added a warning when no process is killed for synctl stop and synctl restart
For synctl restart if no process is killed don't attempt to start anyway.

Example stopping synapse twice:

> synctl stop 
stopped synapse.app.homeserver

> synctl stop
No running worker found. (from '~/synapse/homeserver.pid')
The process might be managed by another controller (e.g. systemd)

> synctl restart
No running worker found. (from '~/synapse/homeserver.pid')
The process might be managed by another controller (e.g. systemd)

clokep
clokep suggested changes Apr 2, 2020
View reviewed changes
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! Sorry for the delay in reviewing this! Looks pretty good overall, but I left a few comments.

@clokep
Copy link
Member

clokep commented Apr 6, 2020

@romainbou Can you fix the lint issues? You can run them locally with: ./scripts-dev/lint.sh

@anoadragon453 anoadragon453 requested a review from clokep April 9, 2020 15:34
clokep
clokep reviewed Apr 9, 2020
View reviewed changes
@clokep
Copy link
Member

clokep commented May 15, 2020

@romainbou Do you have plans to take a look back at this PR? It'd be great to finish it up!

@clokep
Copy link
Member

clokep commented May 18, 2020

@romainbou Can you merge develop into this branch? There's been some changes to CI and such since the PR was first opened!

@romainbou
Copy link
Contributor Author

romainbou commented May 19, 2020

@clokep cool thanks yes indeed. After merging develop again, now all checks are passing!

@clokep clokep self-requested a review May 19, 2020 11:32
clokep
clokep reviewed May 19, 2020
View reviewed changes
clokep
clokep reviewed May 19, 2020
View reviewed changes
clokep
clokep approved these changes May 19, 2020
View reviewed changes
Copy link
Member

@clokep clokep left a comment

Choose a reason for hiding this comment

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

Looks good! Thanks for making those changes! 👍

@clokep clokep merged commit a57863d into matrix-org:develop May 19, 2020
@richvdh
Copy link
Member

richvdh commented Jun 1, 2020

this seems to have had the side-effect of changing the behaviour of synctl restart so that it doesn't start synapse if it wasn't running previously. Was this a deliberate change? IMHO it's somewhat unintuitive and inconsistent with the behaviour of other process managers.

@richvdh richvdh mentioned this pull request Jun 3, 2020
Merged
@richvdh
Copy link
Member

richvdh commented Jun 3, 2020

it seems that it was a deliberate change, and indeed that was somewhat suggested by #6994.

On reflection, I feel like the downsides of that change outweigh the upsides (specifically: it's a surprise to anyone who has ever used synctl or any other process manager that synctl restart does not start synapse if one was not previously running). Accordingly, I have reverted that part of the change in #7624. The new warning text is retained.

@richvdh
Copy link
Member

richvdh commented Jun 3, 2020

I should also mention: I don't think that changing the behaviour of synctl restart in this way brought any particular advantages. (The goal was to help people who blindly assumed that the stop part of synctl restart had worked: whether or not we go on to do a start after a failed stop has no bearing on that.)

An even better fix here is to sort out #4641, but for now the warning text is an improvement.

@richvdh richvdh mentioned this pull request Jun 3, 2020
Closed
phil-flex pushed a commit to phil-flex/synapse that referenced this pull request Jun 16, 2020
@romainbou @phil-flex
synctl warns when no process is stopped and avoids start (matrix-org#…
576998a 
…6598)

* If an error occurs when stopping a process synctl now logs a warning.
* During a restart, synctl will avoid attempting to start Synapse if an error
  occurs during stopping Synapse.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@clokep clokep clokep approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@romainbou @clokep @richvdh