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

CP-51870: Refresh toolstack services #6060

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

liulinC
Copy link
Collaborator

@liulinC liulinC commented Oct 17, 2024

  • varstored-guard Before: xenopsd.servcie->xenopsd-xc.service
  • Remove unnecessary xenopsd services
    • xenopsd
    • xenopsd-xenlight
    • xenopsd-simulator
    • xenopsd-libvirt
  • Reorder services to stop/start
    • forkexecd: does not depends on others, move to last
    • xenopsd-xc After xcp-rrdd. so it needs after xcp-rrdd-*

Note: the for loop reverse the TO_RESTART order

@robhoes
Copy link
Member

robhoes commented Oct 17, 2024

The odd thing about xe-toolstack-restart is that is stops and starts services in the same order, whereas it'd better do the restarts in the opposite order. Otherwise you can't really reason about dependencies at all...

Even better may be to have a single "toolstack" service that depends on all of these individual services and restart just that, hoping that systemd would stop and start them things correctly based on dependencies encoded in the service files (as a single systemctl stop x y z command ignores those dependencies).

@robhoes
Copy link
Member

robhoes commented Oct 17, 2024

Also, your branch is out of date, and you are missing some new rrdd plugins.

@liulinC
Copy link
Collaborator Author

liulinC commented Oct 17, 2024

The odd thing about xe-toolstack-restart is that is stops and starts services in the same order, whereas it'd better do the restarts in the opposite order. Otherwise you can't really reason about dependencies at all...

Even better may be to have a single "toolstack" service that depends on all of these individual services and restart just that, hoping that systemd would stop and start them things correctly based on dependencies encoded in the service files (as a single systemctl stop x y z command ignores those dependencies).

It looks systemd can figure out the right order to start the service, so start order does not matter.
However, the stop order matters if the dependent service stop before others.

Yeah, one toolstack service configure the depends, and just let systemd decide the order sounds a better solution.

@liulinC liulinC changed the title CP-51870: Refresh toolstack services CP-51870: Refresh toolstack services [WIP] Oct 17, 2024
@lindig lindig marked this pull request as draft October 18, 2024 10:35
@lindig
Copy link
Contributor

lindig commented Oct 18, 2024

Converted this to draft. This needs to be thought trough and brought up to date before it will be considered for merging. My preference would be for start and restart the sequence being controlled by systemd but this requires testing that dependencies are correct.

@liulinC
Copy link
Collaborator Author

liulinC commented Oct 21, 2024

Converted this to draft. This needs to be thought trough and brought up to date before it will be considered for merging. My preference would be for start and restart the sequence being controlled by systemd but this requires testing that dependencies are correct.

Yeah, I am considering how to use systemd to manage it and got a roughly plan.
I will update this PR later.

@liulinC liulinC marked this pull request as ready for review October 22, 2024 09:15
@liulinC liulinC changed the title CP-51870: Refresh toolstack services [WIP] CP-51870: Refresh toolstack services Oct 22, 2024
services configuration

Systemd services has good support for the services depends and
orders in the Unit file, that is the place the restart order
should be stated.
However, the command `systemd stop foo bar ...` will override
the order in the Unit file. As the number of the services grow
up, it is really hard to manage the order in the systemd command

In order to resolve the issue, `toolstack.target` is created to
group and manage the toolstack services.
- toolstack.target: `Wants: foo.service` will start foo.service
when `systemctl start toolstack.target`
- foo.service: `PartOf: toolstack.target` will restart/stop
foo.service when `systemctl stop/restart toolstack.target`
Note: Above two does not have to match, eg. if we do not want to
start a service during `systemctl start toolstack.target`, we can
remove it from the first list.

- Following xenopsd services are no longer valid, just got removed
  * xenopsd
  * xenopsd-xenlight
  * xenopsd-simulator
  * xenopsd-libvirt

Signed-off-by: Lin Liu <[email protected]>
@liulinC
Copy link
Collaborator Author

liulinC commented Oct 22, 2024

test result refer to xapi.spec repo pull request.

Copy link
Contributor

@lindig lindig left a comment

Choose a reason for hiding this comment

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

Toolstack restart is exercised quite a bit in tests. So I would hope that this would find problems.

@liulinC
Copy link
Collaborator Author

liulinC commented Oct 22, 2024

Toolstack restart is exercised quite a bit in tests. So I would hope that this would find problems.

Yes, XenRT detect a problem so I update Requires -> Wants , otherwise, systemctl restart /stop xcp-rrdd in some case would restart/stop xapi.

@robhoes
Copy link
Member

robhoes commented Oct 22, 2024

The change to use a systemd target looks great, thanks for doing it. As discussed elsewhere, it would be better to include the target file in this repo.

Otherwise, I think a difference between old and the new may be that if any of the services fail to start, now xe-toolstack-restart would not raise an error? At least that is how Wants is defined, so that would be the case for systemctl start toolstack.target. However, the docs for PartOf say that it is similar to Requires (but as a one-way dependency), so does systemctl restart toolstack.target raises an error if any of the contained services fail to start?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants