Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ModifyTime to Allocation and update it both on plan applies and c… #3446

Merged
merged 2 commits into from
Oct 26, 2017
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion api/allocations.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ type Allocation struct {
ModifyIndex uint64
AllocModifyIndex uint64
CreateTime int64
ModifyTime int64
}

// AllocationMetric is used to deserialize allocation metrics.
Expand Down Expand Up @@ -132,11 +133,12 @@ type AllocationListStub struct {
CreateIndex uint64
ModifyIndex uint64
CreateTime int64
ModifyTime int64
}

// AllocDeploymentStatus captures the status of the allocation as part of the
// deployment. This can include things like if the allocation has been marked as
// heatlhy.
// healthy.
type AllocDeploymentStatus struct {
Healthy *bool
ModifyIndex uint64
Expand Down
4 changes: 4 additions & 0 deletions nomad/node_endpoint.go
Original file line number Diff line number Diff line change
Expand Up @@ -776,6 +776,10 @@ func (n *Node) UpdateAlloc(args *structs.AllocUpdateRequest, reply *structs.Gene

// Add this to the batch
n.updatesLock.Lock()
now := time.Now().UTC().UnixNano()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you add a comment on what this code is doing

for _, alloc := range args.Alloc {
alloc.ModifyTime = now
}
n.updates = append(n.updates, args.Alloc...)

// Start a new batch if none
Expand Down
10 changes: 10 additions & 0 deletions nomad/node_endpoint_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1406,8 +1406,10 @@ func TestClientEndpoint_GetAllocs_Blocking(t *testing.T) {
node.ModifyIndex = resp.Index

// Inject fake evaluations async
now := time.Now().UTC().UnixNano()
alloc := mock.Alloc()
alloc.NodeID = node.ID
alloc.ModifyTime = now
state := s1.fsm.State()
state.UpsertJobSummary(99, mock.JobSummary(alloc.JobID))
start := time.Now()
Expand Down Expand Up @@ -1445,6 +1447,10 @@ func TestClientEndpoint_GetAllocs_Blocking(t *testing.T) {
t.Fatalf("bad: %#v", resp2.Allocs)
}

if resp2.Allocs[0].ModifyTime != now {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure this is testing anything?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had added this test to debug serialization (before I realized there was generated code that needed to be updated). Left it here because it helps catch the problem if you update the struct but not the serializer.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Although, now that I think about this uses msgpack codec and not the generated codec so I don't know where I was going with that. Seems like this test is still useful as it verifies that the modtime persisted correctly in the state store.

t.Fatalf("Invalid modify time %v", resp2.Allocs[0].ModifyTime)
}

// Alloc updates fire watches
time.AfterFunc(100*time.Millisecond, func() {
allocUpdate := mock.Alloc()
Expand Down Expand Up @@ -1536,6 +1542,10 @@ func TestClientEndpoint_UpdateAlloc(t *testing.T) {
if out.ClientStatus != structs.AllocClientStatusFailed {
t.Fatalf("Bad: %#v", out)
}

if out.ModifyTime <= 0 {
t.Fatalf("must have valid modify time but was %v", out.ModifyTime)
}
}

func TestClientEndpoint_BatchUpdate(t *testing.T) {
Expand Down
2 changes: 2 additions & 0 deletions nomad/plan_apply.go
Original file line number Diff line number Diff line change
Expand Up @@ -149,6 +149,8 @@ func (s *Server) applyPlan(plan *structs.Plan, result *structs.PlanResult, snap
for _, alloc := range req.Alloc {
if alloc.CreateTime == 0 {
alloc.CreateTime = now
} else {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this should always be updated (remove the else and always run). The reason I say this is that when the scheduler first creates the allocation we want CreateTime == ModifyTime

alloc.ModifyTime = now
}
}

Expand Down
4 changes: 4 additions & 0 deletions nomad/plan_apply_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -226,6 +226,10 @@ func TestPlanApply_applyPlan(t *testing.T) {
t.Fatalf("missing job")
}

if out.ModifyTime <= 0 {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you test that when it is created it gets a modify time too

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

t.Fatalf("must have valid modify time but was %v", out.ModifyTime)
}

// Lookup the allocation
out, err = s1.fsm.State().AllocByID(ws, alloc2.ID)
if err != nil {
Expand Down
3 changes: 3 additions & 0 deletions nomad/state/state_store.go
Original file line number Diff line number Diff line change
Expand Up @@ -1718,6 +1718,9 @@ func (s *StateStore) nestedUpdateAllocFromClient(txn *memdb.Txn, index uint64, a
// Update the modify index
copyAlloc.ModifyIndex = index

// Update the modify time
copyAlloc.ModifyTime = alloc.ModifyTime

if err := s.updateDeploymentWithAlloc(index, copyAlloc, exist, txn); err != nil {
return fmt.Errorf("error updating deployment: %v", err)
}
Expand Down
Loading