Skip to content

Commit

Permalink
Merge pull request #6605 from hashicorp/b-allocres-panic
Browse files Browse the repository at this point in the history
client: fix panic from 0.8 -> 0.10 upgrade
  • Loading branch information
schmichael committed Nov 1, 2019
1 parent cd16a7c commit 4dee31e
Show file tree
Hide file tree
Showing 4 changed files with 94 additions and 4 deletions.
6 changes: 4 additions & 2 deletions client/allocrunner/taskrunner/service_hook.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,7 +63,8 @@ func newServiceHook(c serviceHookConfig) *serviceHook {
delay: c.task.ShutdownDelay,
}

// COMPAT(0.10): Just use the AllocatedResources
// COMPAT(0.11): AllocatedResources was added in 0.9 so assume its set
// in 0.11.
if c.alloc.AllocatedResources != nil {
if res := c.alloc.AllocatedResources.Tasks[c.task.Name]; res != nil {
h.networks = res.Networks
Expand Down Expand Up @@ -115,7 +116,8 @@ func (h *serviceHook) Update(ctx context.Context, req *interfaces.TaskUpdateRequ
canary = req.Alloc.DeploymentStatus.Canary
}

// COMPAT(0.10): Just use the AllocatedResources
// COMPAT(0.11): AllocatedResources was added in 0.9 so assume its set
// in 0.11.
var networks structs.Networks
if req.Alloc.AllocatedResources != nil {
if res := req.Alloc.AllocatedResources.Tasks[h.taskName]; res != nil {
Expand Down
2 changes: 1 addition & 1 deletion client/allocrunner/taskrunner/task_runner.go
Original file line number Diff line number Diff line change
Expand Up @@ -301,7 +301,7 @@ func NewTaskRunner(config *Config) (*TaskRunner, error) {
}
tr.taskResources = tres
} else {
// COMPAT(0.10): Upgrade from old resources to new resources
// COMPAT(0.11): Upgrade from 0.8 resources to 0.9+ resources
// Grab the old task resources
oldTr, ok := tr.alloc.TaskResources[tr.taskName]
if !ok {
Expand Down
9 changes: 8 additions & 1 deletion command/agent/consul/client.go
Original file line number Diff line number Diff line change
Expand Up @@ -805,7 +805,9 @@ func (noopRestarter) Restart(context.Context, *structs.TaskEvent, bool) error {
//
//TODO(schmichael) rename TaskServices and refactor this into a New method
func makeAllocTaskServices(alloc *structs.Allocation, tg *structs.TaskGroup) (*TaskServices, error) {
if n := len(alloc.AllocatedResources.Shared.Networks); n == 0 {
//COMPAT(0.11) AllocatedResources is only nil when upgrading directly
// from 0.8.
if alloc.AllocatedResources == nil || len(alloc.AllocatedResources.Shared.Networks) == 0 {
return nil, fmt.Errorf("unable to register a group service without a group network")
}

Expand Down Expand Up @@ -867,6 +869,11 @@ func (c *ServiceClient) UpdateGroup(oldAlloc, newAlloc *structs.Allocation) erro
return fmt.Errorf("task group %q not in old allocation", oldAlloc.TaskGroup)
}

if len(oldTG.Services) == 0 {
// No old group services, simply add new group services
return c.RegisterGroup(newAlloc)
}

oldServices, err := makeAllocTaskServices(oldAlloc, oldTG)
if err != nil {
return err
Expand Down
81 changes: 81 additions & 0 deletions command/agent/consul/group_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -127,3 +127,84 @@ func TestConsul_Connect(t *testing.T) {
time.Sleep(interval >> 2)
}
}

// TestConsul_Update08Alloc asserts that adding group services to a previously
// 0.8 alloc works.
//
// COMPAT(0.11) Only valid for upgrades from 0.8.
func TestConsul_Update08Alloc(t *testing.T) {
// Create an embedded Consul server
testconsul, err := testutil.NewTestServerConfig(func(c *testutil.TestServerConfig) {
// If -v wasn't specified squelch consul logging
if !testing.Verbose() {
c.Stdout = ioutil.Discard
c.Stderr = ioutil.Discard
}
})
if err != nil {
t.Fatalf("error starting test consul server: %v", err)
}
defer testconsul.Stop()

consulConfig := consulapi.DefaultConfig()
consulConfig.Address = testconsul.HTTPAddr
consulClient, err := consulapi.NewClient(consulConfig)
require.NoError(t, err)
serviceClient := NewServiceClient(consulClient.Agent(), testlog.HCLogger(t), true)

// Lower periodicInterval to ensure periodic syncing doesn't improperly
// remove Connect services.
const interval = 50 * time.Millisecond
serviceClient.periodicInterval = interval

// Disable deregistration probation to test syncing
serviceClient.deregisterProbationExpiry = time.Time{}

go serviceClient.Run()
defer serviceClient.Shutdown()

// Create new 0.10-style alloc
alloc := mock.Alloc()
alloc.AllocatedResources.Shared.Networks = []*structs.NetworkResource{
{
Mode: "bridge",
IP: "10.0.0.1",
DynamicPorts: []structs.Port{
{
Label: "connect-proxy-testconnect",
Value: 9999,
To: 9998,
},
},
},
}
tg := alloc.Job.LookupTaskGroup(alloc.TaskGroup)
tg.Services = []*structs.Service{
{
Name: "testconnect",
PortLabel: "9999",
Connect: &structs.ConsulConnect{
SidecarService: &structs.ConsulSidecarService{
Proxy: &structs.ConsulProxy{
LocalServicePort: 9000,
},
},
},
},
}

// Create old 0.8-style alloc from new alloc
oldAlloc := alloc.Copy()
oldAlloc.AllocatedResources = nil
oldAlloc.Job.LookupTaskGroup(alloc.TaskGroup).Services = nil

// Expect new services to get registered
require.NoError(t, serviceClient.UpdateGroup(oldAlloc, alloc))

// Assert the group and sidecar services are registered
require.Eventually(t, func() bool {
services, err := consulClient.Agent().Services()
require.NoError(t, err)
return len(services) == 2
}, 3*time.Second, 100*time.Millisecond)
}

0 comments on commit 4dee31e

Please sign in to comment.