Reduce scope of holding ServiceManager.planLock

In most cases we're just reading from m.plan fields (Services,
DefaultServiceNames, and so on), so we can use m.Plan(), which holds
the plan lock only for the duration of fetch m.plan -- this is safe,
because we never mutate what's inside m.plan, we only replace it (in
updatePlan).

For the cases we are updating the plan (AppendLayer, CombineLayer,
SetServiceArgs), we still need acquirePlan.

In ServiceManager.ServiceLogs, we don't need to acquire the plan lock
at all (m.plan isn't used).

internals/overlord/servstate/handlers.go

@@ -114,12 +114,11 @@ func (m *ServiceManager) doStart(task *state.Task, tomb *tomb.Tomb) error {
 		return err
 	}
 
-	releasePlan, err := m.acquirePlan()
+	mgrPlan, err := m.Plan()
 	if err != nil {
-		return fmt.Errorf("cannot acquire plan lock: %w", err)
+		return fmt.Errorf("cannot fetch plan: %w", err)
 	}
-	config, ok := m.plan.Services[request.Name]
-	releasePlan()
+	config, ok := mgrPlan.Services[request.Name]
 	if !ok {
 		return fmt.Errorf("cannot find service %q in plan", request.Name)
 	}

internals/overlord/servstate/manager.go

@@ -275,11 +275,10 @@ const (
 // Services returns the list of configured services and their status, sorted
 // by service name. Filter by the specified service names if provided.
 func (m *ServiceManager) Services(names []string) ([]*ServiceInfo, error) {
-	releasePlan, err := m.acquirePlan()
+	mgrPlan, err := m.Plan()
 	if err != nil {
 		return nil, err
 	}
-	defer releasePlan()
 
 	m.servicesLock.Lock()
 	defer m.servicesLock.Unlock()
@@ -291,7 +290,7 @@ func (m *ServiceManager) Services(names []string) ([]*ServiceInfo, error) {
 
 	var services []*ServiceInfo
 	matchNames := len(names) > 0
-	for name, config := range m.plan.Services {
+	for name, config := range mgrPlan.Services {
 		if matchNames && !requested[name] {
 			continue
 		}
@@ -355,56 +354,47 @@ func stateToStatus(state serviceState) ServiceStatus {
 // DefaultServiceNames returns the name of the services set to start
 // by default.
 func (m *ServiceManager) DefaultServiceNames() ([]string, error) {
-	releasePlan, err := m.acquirePlan()
+	mgrPlan, err := m.Plan()
 	if err != nil {
 		return nil, err
 	}
-	defer releasePlan()
 
 	var names []string
-	for name, service := range m.plan.Services {
+	for name, service := range mgrPlan.Services {
 		if service.Startup == plan.StartupEnabled {
 			names = append(names, name)
 		}
 	}
 
-	return m.plan.StartOrder(names)
+	return mgrPlan.StartOrder(names)
 }
 
 // StartOrder returns the provided services, together with any required
 // dependencies, in the proper order for starting them all up.
 func (m *ServiceManager) StartOrder(services []string) ([]string, error) {
-	releasePlan, err := m.acquirePlan()
+	mgrPlan, err := m.Plan()
 	if err != nil {
 		return nil, err
 	}
-	defer releasePlan()
 
-	return m.plan.StartOrder(services)
+	return mgrPlan.StartOrder(services)
 }
 
 // StopOrder returns the provided services, together with any dependants,
 // in the proper order for stopping them all.
 func (m *ServiceManager) StopOrder(services []string) ([]string, error) {
-	releasePlan, err := m.acquirePlan()
+	mgrPlan, err := m.Plan()
 	if err != nil {
 		return nil, err
 	}
-	defer releasePlan()
 
-	return m.plan.StopOrder(services)
+	return mgrPlan.StopOrder(services)
 }
 
 // ServiceLogs returns iterators to the provided services. If last is negative,
 // return tail iterators; if last is zero or positive, return head iterators
 // going back last elements. Each iterator must be closed via the Close method.
 func (m *ServiceManager) ServiceLogs(services []string, last int) (map[string]servicelog.Iterator, error) {
-	releasePlan, err := m.acquirePlan()
-	if err != nil {
-		return nil, err
-	}
-	defer releasePlan()
-
 	requested := make(map[string]bool, len(services))
 	for _, name := range services {
 		requested[name] = true
@@ -434,19 +424,18 @@ func (m *ServiceManager) ServiceLogs(services []string, last int) (map[string]se
 // Replan returns a list of services to stop and services to start because
 // their plans had changed between when they started and this call.
 func (m *ServiceManager) Replan() ([]string, []string, error) {
-	releasePlan, err := m.acquirePlan()
+	mgrPlan, err := m.Plan()
 	if err != nil {
 		return nil, nil, err
 	}
-	defer releasePlan()
 
 	m.servicesLock.Lock()
 	defer m.servicesLock.Unlock()
 
 	needsRestart := make(map[string]bool)
 	var stop []string
 	for name, s := range m.services {
-		if config, ok := m.plan.Services[name]; ok {
+		if config, ok := mgrPlan.Services[name]; ok {
 			if config.Equal(s.config) {
 				continue
 			}
@@ -457,13 +446,13 @@ func (m *ServiceManager) Replan() ([]string, []string, error) {
 	}
 
 	var start []string
-	for name, config := range m.plan.Services {
+	for name, config := range mgrPlan.Services {
 		if needsRestart[name] || config.Startup == plan.StartupEnabled {
 			start = append(start, name)
 		}
 	}
 
-	stop, err = m.plan.StopOrder(stop)
+	stop, err = mgrPlan.StopOrder(stop)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -473,7 +462,7 @@ func (m *ServiceManager) Replan() ([]string, []string, error) {
 		}
 	}
 
-	start, err = m.plan.StartOrder(start)
+	start, err = mgrPlan.StartOrder(start)
 	if err != nil {
 		return nil, nil, err
 	}
@@ -531,8 +520,8 @@ func (m *ServiceManager) SetServiceArgs(serviceArgs map[string][]string) error {
 	defer releasePlan()
 
 	newLayer := &plan.Layer{
-		// TODO: Consider making any labels starting with
-		// the "pebble-" prefix reserved.
+		// Labels with "pebble-*" prefix are (will be) reserved, see:
+		// https://github.com/canonical/pebble/issues/220
 		Label:    "pebble-service-args",
 		Services: make(map[string]*plan.Service),
 	}
@@ -564,20 +553,19 @@ func (m *ServiceManager) SetServiceArgs(serviceArgs map[string][]string) error {
 // exits), it would generate a runtime error as the reaper would already be dead.
 // This function returns a slice of service names to stop, in dependency order.
 func servicesToStop(m *ServiceManager) ([]string, error) {
-	releasePlan, err := m.acquirePlan()
+	mgrPlan, err := m.Plan()
 	if err != nil {
 		return nil, err
 	}
-	defer releasePlan()
 
 	// Get all service names in plan.
-	var services []string
-	for name := range m.plan.Services {
+	services := make([]string, 0, len(mgrPlan.Services))
+	for name := range mgrPlan.Services {
 		services = append(services, name)
 	}
 
 	// Order according to dependency order.
-	stop, err := m.plan.StopOrder(services)
+	stop, err := mgrPlan.StopOrder(services)
 	if err != nil {
 		return nil, err
 	}