From f34703fc63c00987ed3a7659a06a672e4c5bce6c Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Mon, 30 Nov 2020 17:01:37 -0500 Subject: [PATCH] agent: move checkStateSnapshot Move the field into the struct for addServiceLocked. Also don't require setting a default value, so that the callers can leave it as nil if they don't already have a snapshot. --- agent/agent.go | 31 +++++++++++++++++++++---------- agent/service_manager.go | 11 +---------- 2 files changed, 22 insertions(+), 20 deletions(-) diff --git a/agent/agent.go b/agent/agent.go index f20a6b404b..883371d8d8 100644 --- a/agent/agent.go +++ b/agent/agent.go @@ -515,7 +515,8 @@ func (a *Agent) Start(ctx context.Context) error { a.serviceManager.Start() // Load checks/services/metadata. - if err := a.loadServices(c, nil); err != nil { + emptyCheckSnapshot := map[structs.CheckID]*structs.HealthCheck{} + if err := a.loadServices(c, emptyCheckSnapshot); err != nil { return err } if err := a.loadChecks(c, nil); err != nil { @@ -1894,15 +1895,14 @@ func (a *Agent) readPersistedServiceConfigs() (map[structs.ServiceID]*structs.Se // This entry is persistent and the agent will make a best effort to // ensure it is registered func (a *Agent) AddService(req AddServiceRequest) error { + a.stateLock.Lock() + defer a.stateLock.Unlock() + rl := addServiceLockedRequest{ AddServiceRequest: req, serviceDefaults: serviceDefaultsFromCache(a.baseDeps, req), persistServiceConfig: true, } - a.stateLock.Lock() - defer a.stateLock.Unlock() - - rl.snap = a.State.Checks(structs.WildcardEnterpriseMeta()) return a.addServiceLocked(rl) } @@ -1936,6 +1936,11 @@ type addServiceLockedRequest struct { // serviceDefaults is called when the Agent.stateLock is held, so it must // never attempt to acquire that lock. serviceDefaults func(context.Context) (*structs.ServiceConfigResponse, error) + + // checkStateSnapshot may optionally be set to a snapshot of the checks in + // the local.State. If checkStateSnapshot is nil, addServiceInternal will + // callState.Checks to get the snapshot. + checkStateSnapshot map[structs.CheckID]*structs.HealthCheck } // AddServiceRequest is the union of arguments for calling both @@ -1955,7 +1960,6 @@ type AddServiceRequest struct { token string replaceExistingChecks bool Source configSource - snap map[structs.CheckID]*structs.HealthCheck } type addServiceInternalRequest struct { @@ -2001,6 +2005,13 @@ func (a *Agent) addServiceInternal(req addServiceInternalRequest) error { existingChecks[check.CompoundCheckID()] = false } + // Note, this is explicitly a nil check instead of len() == 0 because + // Agent.Start does not have a snapshot, and we don't want to query + // State.Checks each time. + if req.checkStateSnapshot == nil { + req.checkStateSnapshot = a.State.Checks(structs.WildcardEnterpriseMeta()) + } + // Create an associated health check for i, chkType := range req.chkTypes { checkID := string(chkType.CheckID) @@ -2035,7 +2046,7 @@ func (a *Agent) addServiceInternal(req addServiceInternalRequest) error { } // Restore the fields from the snapshot. - prev, ok := req.snap[cid] + prev, ok := req.checkStateSnapshot[cid] if ok { check.Output = prev.Output check.Status = prev.Status @@ -3100,10 +3111,10 @@ func (a *Agent) loadServices(conf *config.RuntimeConfig, snap map[structs.CheckI token: service.Token, replaceExistingChecks: false, // do default behavior Source: ConfigSourceLocal, - snap: snap, }, serviceDefaults: serviceDefaultsFromStruct(persistedServiceConfigs[sid]), persistServiceConfig: false, // don't rewrite the file with the same data we just read + checkStateSnapshot: snap, }) if err != nil { return fmt.Errorf("Failed to register service %q: %v", service.Name, err) @@ -3120,10 +3131,10 @@ func (a *Agent) loadServices(conf *config.RuntimeConfig, snap map[structs.CheckI token: sidecarToken, replaceExistingChecks: false, // do default behavior Source: ConfigSourceLocal, - snap: snap, }, serviceDefaults: serviceDefaultsFromStruct(persistedServiceConfigs[sidecarServiceID]), persistServiceConfig: false, // don't rewrite the file with the same data we just read + checkStateSnapshot: snap, }) if err != nil { return fmt.Errorf("Failed to register sidecar for service %q: %v", service.Name, err) @@ -3218,10 +3229,10 @@ func (a *Agent) loadServices(conf *config.RuntimeConfig, snap map[structs.CheckI token: p.Token, replaceExistingChecks: false, // do default behavior Source: source, - snap: snap, }, serviceDefaults: serviceDefaultsFromStruct(persistedServiceConfigs[serviceID]), persistServiceConfig: false, // don't rewrite the file with the same data we just read + checkStateSnapshot: snap, }) if err != nil { return fmt.Errorf("failed adding service %q: %s", serviceID, err) diff --git a/agent/service_manager.go b/agent/service_manager.go index f43a5a57e7..0a02ed6106 100644 --- a/agent/service_manager.go +++ b/agent/service_manager.go @@ -86,12 +86,7 @@ func (s *ServiceManager) registerOnce(args addServiceInternalRequest) error { s.agent.stateLock.Lock() defer s.agent.stateLock.Unlock() - if args.snap == nil { - args.snap = s.agent.snapshotCheckState() - } - - err := s.agent.addServiceInternal(args) - if err != nil { + if err := s.agent.addServiceInternal(args); err != nil { return fmt.Errorf("error updating service registration: %v", err) } return nil @@ -201,10 +196,6 @@ func (w *serviceConfigWatch) RegisterAndStart(ctx context.Context, wg *sync.Wait req := w.registration req.Service = merged - // TODO: move this line? it seems out of place. Maybe this default should - // be set in addServiceInternal - req.snap = w.agent.snapshotCheckState() // requires Agent.stateLock - err = w.agent.addServiceInternal(addServiceInternalRequest{ addServiceLockedRequest: req, persistService: w.registration.Service,