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

Add framework for managing startup dependencies and shutdown #153

Merged
merged 2 commits into from
Jul 14, 2021
Merged

Conversation

fzdarsky
Copy link
Contributor

@fzdarsky fzdarsky commented Jul 9, 2021

Adds a "Service interface" that when implemented by embedded component services allows generic management of service startup (incl. waiting on dependencies to become ready), readiness, and cancellation.

Adds such a generic service manager that works based on expressing service dependencies as directed acyclic graph.
Implements signaling readiness to systemd and cancellation of embedded services after interrupts from the system.

Refactors the first two services (etcd, kube-apiserver) to illustrate the pattern, using a GenericService implementation to call the legacy code for bringing up non-ported services.

Signed-off-by: Frank A. Zdarsky fzdarsky@redhat.com

Closes #152

Adds a "Service interface" that when implemented by embedded component services
allows generic management of service startup (incl. waiting on dependencies to
become ready), readiness, and cancellation.
Adds such a generic service manager that works based on expressing service
dependencies as directed acyclic graph.
Implements signaling readiness to systemd and cancellation of embedded services
after interrupts from the system.
Refactors the first two services (etcd, kube-apiserver) to illustrate the
pattern, using a GenericService implementation to call the legacy code for
bringing up non-ported services.

Signed-off-by: Frank A. Zdarsky <fzdarsky@redhat.com>
@openshift-ci openshift-ci bot requested review from oglok and rootfs July 9, 2021 14:54
@fzdarsky fzdarsky requested a review from copejon July 9, 2021 14:55
@fzdarsky
Copy link
Contributor Author

fzdarsky commented Jul 9, 2021

The PR starts all the embedded services, but some of them haven't been refactored yet and don't implement cancellation (they just execute forever until MicroShift's process terminates). As a result, the behavior (incl. when receiving interrupts) should still be the same.

To get a feeling for the UX after refactoring, comment out L80-91 and L95-110 of pkg/cmd/run.go and run.

You may need to wipe /var/lib/microshift before.

Signed-off-by: Frank A. Zdarsky <fzdarsky@redhat.com>
}
}

m.services = append(m.services, s)
Copy link
Member

Choose a reason for hiding this comment

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

need a mutex here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the review, @rootfs. You mean for hedging against AddService() being called concurrently?
Note that at least currently, the order of adding services is significant (to enforce the invariant of dependencies being added before dependents), so AddService() shouldn't be called concurrently. Otherwise, we'd have to enable topo sorting. See also the comments in AddService() and Run().

How should we proceed? Add mutex proactively or add note in method description?

@rootfs
Copy link
Member

rootfs commented Jul 12, 2021

this looks good, please finish it up and also run some benchmark to see if the new way accelerates the startup as well.

@fzdarsky
Copy link
Contributor Author

this looks good, please finish it up and also run some benchmark to see if the new way accelerates the startup as well.

As long as the services other than etcd and kube-apiserver (who have a startup dependency already) haven't been ported, I don't expect to see much diff (other than seeing a few less error messages in the log).
Would you propose I port over the other services and run the benchmark or shall we merge, then port, then benchmark for control?

@rootfs
Copy link
Member

rootfs commented Jul 14, 2021

Would you propose I port over the other services and run the benchmark or shall we merge, then port, then benchmark for control?

let's merge this one first and please go ahead with another PR for the rest of the services.

@rootfs
Copy link
Member

rootfs commented Jul 14, 2021

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 14, 2021
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jul 14, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rootfs

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jul 14, 2021
@rootfs rootfs merged commit 74c05ad into main Jul 14, 2021
@fzdarsky fzdarsky deleted the servicemgr branch July 21, 2021 18:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Refactor embedded services, improve startup and shutdown behavior
2 participants