From c4ee82c4dcae562e409e3c08e49977f28e722fae Mon Sep 17 00:00:00 2001 From: Dane Everitt Date: Sun, 30 Jan 2022 12:56:25 -0500 Subject: [PATCH] Code cleanup, providing better commentary to decisions --- environment/docker/container.go | 10 ++++---- environment/docker/environment.go | 42 +++++++++++++------------------ environment/docker/power.go | 7 ++++++ server/server.go | 8 ------ 4 files changed, 30 insertions(+), 37 deletions(-) diff --git a/environment/docker/container.go b/environment/docker/container.go index 858a082..525f9a1 100644 --- a/environment/docker/container.go +++ b/environment/docker/container.go @@ -38,13 +38,13 @@ func (nw noopWriter) Write(b []byte) (int, error) { } // Attach attaches to the docker container itself and ensures that we can pipe -// data in and out of the process stream. This should not be used for reading -// console data as you *will* miss important output at the beginning because of -// the time delay with attaching to the output. +// data in and out of the process stream. This should always be called before +// you have started the container, but after you've ensured it exists. // // Calling this function will poll resources for the container in the background -// until the provided context is canceled by the caller. Failure to cancel said -// context will cause background memory leaks as the goroutine will not exit. +// until the container is stopped. The context provided to this function is used +// for the purposes of attaching to the container, a seecond context is created +// within the function for managing polling. func (e *Environment) Attach(ctx context.Context) error { if e.IsAttached() { return nil diff --git a/environment/docker/environment.go b/environment/docker/environment.go index 4142f09..a0b7e71 100644 --- a/environment/docker/environment.go +++ b/environment/docker/environment.go @@ -27,7 +27,6 @@ var _ environment.ProcessEnvironment = (*Environment)(nil) type Environment struct { mu sync.RWMutex - eventMu sync.Once // The public identifier for this environment. In this case it is the Docker container // name that will be used for all instances created under it. @@ -73,6 +72,7 @@ func New(id string, m *Metadata, c *environment.Configuration) (*Environment, er meta: m, client: cli, st: system.NewAtomicString(environment.ProcessOfflineState), + emitter: events.NewBus(), } return e, nil @@ -86,34 +86,33 @@ func (e *Environment) Type() string { return "docker" } -// Set if this process is currently attached to the process. +// SetStream sets the current stream value from the Docker client. If a nil +// value is provided we assume that the stream is no longer operational and the +// instance is effectively offline. func (e *Environment) SetStream(s *types.HijackedResponse) { e.mu.Lock() - defer e.mu.Unlock() - e.stream = s + e.mu.Unlock() } -// Determine if the this process is currently attached to the container. +// IsAttached determine if the this process is currently attached to the +// container instance by checking if the stream is nil or not. func (e *Environment) IsAttached() bool { e.mu.RLock() defer e.mu.RUnlock() - return e.stream != nil } +// Events returns an event bus for the environment. func (e *Environment) Events() *events.Bus { - e.eventMu.Do(func() { - e.emitter = events.NewBus() - }) - return e.emitter } -// Determines if the container exists in this environment. The ID passed through should be the -// server UUID since containers are created utilizing the server UUID as the name and docker -// will work fine when using the container name as the lookup parameter in addition to the longer -// ID auto-assigned when the container is created. +// Exists determines if the container exists in this environment. The ID passed +// through should be the server UUID since containers are created utilizing the +// server UUID as the name and docker will work fine when using the container +// name as the lookup parameter in addition to the longer ID auto-assigned when +// the container is created. func (e *Environment) Exists() (bool, error) { _, err := e.ContainerInspect(context.Background()) if err != nil { @@ -122,10 +121,8 @@ func (e *Environment) Exists() (bool, error) { if client.IsErrNotFound(err) { return false, nil } - return false, err } - return true, nil } @@ -146,7 +143,7 @@ func (e *Environment) IsRunning(ctx context.Context) (bool, error) { return c.State.Running, nil } -// Determine the container exit state and return the exit code and whether or not +// ExitState returns the container exit state, the exit code and whether or not // the container was killed by the OOM killer. func (e *Environment) ExitState() (uint32, bool, error) { c, err := e.ContainerInspect(context.Background()) @@ -163,15 +160,13 @@ func (e *Environment) ExitState() (uint32, bool, error) { if client.IsErrNotFound(err) { return 1, false, nil } - return 0, false, err } - return uint32(c.State.ExitCode), c.State.OOMKilled, nil } -// Returns the environment configuration allowing a process to make modifications of the -// environment on the fly. +// Config returns the environment configuration allowing a process to make +// modifications of the environment on the fly. func (e *Environment) Config() *environment.Configuration { e.mu.RLock() defer e.mu.RUnlock() @@ -179,12 +174,11 @@ func (e *Environment) Config() *environment.Configuration { return e.Configuration } -// Sets the stop configuration for the environment. +// SetStopConfiguration sets the stop configuration for the environment. func (e *Environment) SetStopConfiguration(c remote.ProcessStopConfiguration) { e.mu.Lock() - defer e.mu.Unlock() - e.meta.Stop = c + e.mu.Unlock() } func (e *Environment) SetImage(i string) { diff --git a/environment/docker/power.go b/environment/docker/power.go index 7c22cd3..6f7fdc4 100644 --- a/environment/docker/power.go +++ b/environment/docker/power.go @@ -111,6 +111,13 @@ func (e *Environment) Start(ctx context.Context) error { actx, cancel := context.WithTimeout(ctx, time.Second*30) defer cancel() + // You must attach to the instance _before_ you start the container. If you do this + // in the opposite order you'll enter a deadlock condition where we're attached to + // the instance successfully, but the container has already stopped and you'll get + // the entire program into a very confusing state. + // + // By explicitly attaching to the instance before we start it, we can immediately + // react to errors/output stopping/etc. when starting. if err := e.Attach(actx); err != nil { return err } diff --git a/server/server.go b/server/server.go index 47f2f8c..af60daa 100644 --- a/server/server.go +++ b/server/server.go @@ -239,14 +239,6 @@ func (s *Server) ReadLogfile(len int) ([]string, error) { return s.Environment.Readlog(len) } -// Determine if the server is bootable in it's current state or not. This will not -// indicate why a server is not bootable, only if it is. -func (s *Server) IsBootable() bool { - exists, _ := s.Environment.Exists() - - return exists -} - // Initializes a server instance. This will run through and ensure that the environment // for the server is setup, and that all of the necessary files are created. func (s *Server) CreateEnvironment() error {