Skip to content

Commit

Permalink
perf(providers): retrieve state on start instead of assuming starting (…
Browse files Browse the repository at this point in the history 
…#350)

When an instance does not exist yet and needs to be started, its status is not assumed to be starting anymore.

Instead, the statue will be retrieved from the provider. This changes one thing, it's that you may be able to start and access your services instantly because they'll be instantly seen as ready.

With this change, you might want to make sure that your containers have a proper healthcheck used to determine when the application is able to process incoming requests.

* refactor: add interface guards

* refactor(providers): remove instance.State as a return value from Stop and Start

* test(e2e): add healthcheck on nginx container

Because now the container check is so fast, we need to add a delay on which the container is considered started and healthy to have a proper waiting page.

* fix(tests): using acouvreur/whoami:v1.10.2 instead of containous/whoami:v1.5.0

This image simply retrieve the curl binary from curlimages/curl:8.8.0 to be able to add proper docker healthcheck commands.

Once this is merged with traefik/whoami, I'll update back to the original image.

See traefik/whoami#33
  • Loading branch information
@acouvreur
acouvreur authored Jul 8, 2024
1 parent 3b34054 commit be7ace0
Show file tree
Hide file tree
Showing 31 changed files with 135 additions and 229 deletions.
2 changes: 1 addition & 1 deletion app/discovery/autostop.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func StopAllUnregisteredInstances(ctx context.Context, provider providers.Provid
func stopFunc(ctx context.Context, name string, provider providers.Provider) func() error {
return func() error {
log.Tracef("Stopping %v...", name)
_, err := provider.Stop(ctx, name)
err := provider.Stop(ctx, name)
if err != nil {
log.Errorf("Could not stop %v: %v", name, err)
return err
Expand Down
9 changes: 4 additions & 5 deletions app/discovery/autostop_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@ import (
"context"
"errors"
"github.com/acouvreur/sablier/app/discovery"
"github.com/acouvreur/sablier/app/instance"
"github.com/acouvreur/sablier/app/providers"
"github.com/acouvreur/sablier/app/providers/mock"
"github.com/acouvreur/sablier/app/types"
Expand All @@ -30,8 +29,8 @@ func TestStopAllUnregisteredInstances(t *testing.T) {
}).Return(instances, nil)

// Set up expectations for Stop
mockProvider.On("Stop", ctx, "instance2").Return(instance.State{}, nil)
mockProvider.On("Stop", ctx, "instance3").Return(instance.State{}, nil)
mockProvider.On("Stop", ctx, "instance2").Return(nil)
mockProvider.On("Stop", ctx, "instance3").Return(nil)

// Call the function under test
err := discovery.StopAllUnregisteredInstances(ctx, mockProvider, registered)
Expand Down Expand Up @@ -62,8 +61,8 @@ func TestStopAllUnregisteredInstances_WithError(t *testing.T) {
}).Return(instances, nil)

// Set up expectations for Stop with error
mockProvider.On("Stop", ctx, "instance2").Return(instance.State{}, errors.New("stop error"))
mockProvider.On("Stop", ctx, "instance3").Return(instance.State{}, nil)
mockProvider.On("Stop", ctx, "instance2").Return(errors.New("stop error"))
mockProvider.On("Stop", ctx, "instance3").Return(nil)

// Call the function under test
err := discovery.StopAllUnregisteredInstances(ctx, mockProvider, registered)
Expand Down
35 changes: 8 additions & 27 deletions app/providers/docker/docker.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"fmt"
"github.com/acouvreur/sablier/app/discovery"
"github.com/acouvreur/sablier/app/providers"
"io"
"strings"

Expand All @@ -18,6 +19,9 @@ import (
log "github.com/sirupsen/logrus"
)

// Interface guard
var _ providers.Provider = (*DockerClassicProvider)(nil)

type DockerClassicProvider struct {
Client client.APIClient
desiredReplicas int
Expand Down Expand Up @@ -71,39 +75,16 @@ func (provider *DockerClassicProvider) GetGroups(ctx context.Context) (map[strin
return groups, nil
}

func (provider *DockerClassicProvider) Start(ctx context.Context, name string) (instance.State, error) {
err := provider.Client.ContainerStart(ctx, name, container.StartOptions{})

if err != nil {
return instance.ErrorInstanceState(name, err, provider.desiredReplicas)
}

return instance.State{
Name: name,
CurrentReplicas: 0,
DesiredReplicas: provider.desiredReplicas,
Status: instance.NotReady,
}, err
func (provider *DockerClassicProvider) Start(ctx context.Context, name string) error {
return provider.Client.ContainerStart(ctx, name, container.StartOptions{})
}

func (provider *DockerClassicProvider) Stop(ctx context.Context, name string) (instance.State, error) {
err := provider.Client.ContainerStop(ctx, name, container.StopOptions{})

if err != nil {
return instance.ErrorInstanceState(name, err, provider.desiredReplicas)
}

return instance.State{
Name: name,
CurrentReplicas: 0,
DesiredReplicas: provider.desiredReplicas,
Status: instance.NotReady,
}, nil
func (provider *DockerClassicProvider) Stop(ctx context.Context, name string) error {
return provider.Client.ContainerStop(ctx, name, container.StopOptions{})
}

func (provider *DockerClassicProvider) GetState(ctx context.Context, name string) (instance.State, error) {
spec, err := provider.Client.ContainerInspect(ctx, name)

if err != nil {
return instance.ErrorInstanceState(name, err, provider.desiredReplicas)
}
Expand Down
38 changes: 2 additions & 36 deletions app/providers/docker/docker_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,6 @@ func TestDockerClassicProvider_Stop(t *testing.T) {
name string
fields fields
args args
want instance.State
wantErr bool
err error
}{
Expand All @@ -283,13 +282,6 @@ func TestDockerClassicProvider_Stop(t *testing.T) {
args: args{
name: "nginx",
},
want: instance.State{
Name: "nginx",
CurrentReplicas: 0,
DesiredReplicas: 1,
Status: instance.Unrecoverable,
Message: "container with name \"nginx\" was not found",
},
wantErr: true,
err: fmt.Errorf("container with name \"nginx\" was not found"),
},
Expand All @@ -301,12 +293,6 @@ func TestDockerClassicProvider_Stop(t *testing.T) {
args: args{
name: "nginx",
},
want: instance.State{
Name: "nginx",
CurrentReplicas: 0,
DesiredReplicas: 1,
Status: instance.NotReady,
},
wantErr: false,
err: nil,
},
Expand All @@ -320,14 +306,11 @@ func TestDockerClassicProvider_Stop(t *testing.T) {

tt.fields.Client.On("ContainerStop", mock.Anything, mock.Anything, mock.Anything).Return(tt.err)

got, err := provider.Stop(context.Background(), tt.args.name)
err := provider.Stop(context.Background(), tt.args.name)
if (err != nil) != tt.wantErr {
t.Errorf("DockerClassicProvider.Stop() error = %v, wantErr %v", err, tt.wantErr)
return
}
if !reflect.DeepEqual(got, tt.want) {
t.Errorf("DockerClassicProvider.Stop() = %v, want %v", got, tt.want)
}
})
}
}
Expand All @@ -343,7 +326,6 @@ func TestDockerClassicProvider_Start(t *testing.T) {
name string
fields fields
args args
want instance.State
wantErr bool
err error
}{
Expand All @@ -355,13 +337,6 @@ func TestDockerClassicProvider_Start(t *testing.T) {
args: args{
name: "nginx",
},
want: instance.State{
Name: "nginx",
CurrentReplicas: 0,
DesiredReplicas: 1,
Status: instance.Unrecoverable,
Message: "container with name \"nginx\" was not found",
},
wantErr: true,
err: fmt.Errorf("container with name \"nginx\" was not found"),
},
Expand All @@ -373,12 +348,6 @@ func TestDockerClassicProvider_Start(t *testing.T) {
args: args{
name: "nginx",
},
want: instance.State{
Name: "nginx",
CurrentReplicas: 0,
DesiredReplicas: 1,
Status: instance.NotReady,
},
wantErr: false,
err: nil,
},
Expand All @@ -392,14 +361,11 @@ func TestDockerClassicProvider_Start(t *testing.T) {

tt.fields.Client.On("ContainerStart", mock.Anything, mock.Anything, mock.Anything).Return(tt.err)

got, err := provider.Start(context.Background(), tt.args.name)
err := provider.Start(context.Background(), tt.args.name)
if (err != nil) != tt.wantErr {
t.Errorf("DockerClassicProvider.Start() error = %v, wantErr %v", err, tt.wantErr)
return
}
if !reflect.DeepEqual(got, tt.want) {
t.Errorf("DockerClassicProvider.Start() = %v, want %v", got, tt.want)
}
})
}
}
Expand Down
23 changes: 12 additions & 11 deletions app/providers/dockerswarm/docker_swarm.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"errors"
"fmt"
"github.com/acouvreur/sablier/app/discovery"
"github.com/acouvreur/sablier/app/providers"
"io"
"strings"

Expand All @@ -16,6 +17,9 @@ import (
log "github.com/sirupsen/logrus"
)

// Interface guard
var _ providers.Provider = (*DockerSwarmProvider)(nil)

type DockerSwarmProvider struct {
Client client.APIClient
desiredReplicas int
Expand All @@ -41,40 +45,37 @@ func NewDockerSwarmProvider() (*DockerSwarmProvider, error) {

}

func (provider *DockerSwarmProvider) Start(ctx context.Context, name string) (instance.State, error) {
func (provider *DockerSwarmProvider) Start(ctx context.Context, name string) error {
return provider.scale(ctx, name, uint64(provider.desiredReplicas))
}

func (provider *DockerSwarmProvider) Stop(ctx context.Context, name string) (instance.State, error) {
func (provider *DockerSwarmProvider) Stop(ctx context.Context, name string) error {
return provider.scale(ctx, name, 0)
}

func (provider *DockerSwarmProvider) scale(ctx context.Context, name string, replicas uint64) (instance.State, error) {
func (provider *DockerSwarmProvider) scale(ctx context.Context, name string, replicas uint64) error {
service, err := provider.getServiceByName(name, ctx)

if err != nil {
return instance.ErrorInstanceState(name, err, provider.desiredReplicas)
return err
}

foundName := provider.getInstanceName(name, *service)

if service.Spec.Mode.Replicated == nil {
return instance.UnrecoverableInstanceState(foundName, "swarm service is not in \"replicated\" mode", provider.desiredReplicas)
return errors.New("swarm service is not in \"replicated\" mode")
}

service.Spec.Mode.Replicated.Replicas = &replicas

response, err := provider.Client.ServiceUpdate(ctx, service.ID, service.Meta.Version, service.Spec, types.ServiceUpdateOptions{})

if err != nil {
return instance.ErrorInstanceState(foundName, err, provider.desiredReplicas)
return err
}

if len(response.Warnings) > 0 {
return instance.UnrecoverableInstanceState(foundName, strings.Join(response.Warnings, ", "), provider.desiredReplicas)
return fmt.Errorf("warning received updating swarm service [%s]: %s", foundName, strings.Join(response.Warnings, ", "))
}

return instance.NotReadyInstanceState(foundName, 0, provider.desiredReplicas)
return nil
}

func (provider *DockerSwarmProvider) GetGroups(ctx context.Context) (map[string][]string, error) {
Expand Down
Loading

0 comments on commit be7ace0

Please sign in to comment.