-
Notifications
You must be signed in to change notification settings - Fork 2k
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
Garbage collector for allocations #2081
Conversation
331dcb9
to
439f283
Compare
49e3a2f
to
abc16a0
Compare
@@ -434,6 +443,23 @@ func (c *Client) Stats() map[string]map[string]string { | |||
return stats | |||
} | |||
|
|||
// CollectAllocation garbage collects a single allocation | |||
func (c *Client) CollectAllocation(allocID string) error { | |||
if err := c.garbageCollector.Collect(allocID); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return c.garbageCollector.Collect(allocID)
And below. Or wrap the errors
inodeUsageThreshold = 70 | ||
|
||
// MB is a constant which converts values in bytes to MB | ||
MB = 1024 * 1024 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
May be worth throwing this in nomad/structs
MB = 1024 * 1024 | ||
) | ||
|
||
type GCAlloc struct { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comments
select { | ||
case <-ticker.C: | ||
if err := a.keepUsageBelowThreshold(); err != nil { | ||
a.logger.Printf("[ERR] client: error GCing allocation: %v", err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
GCing
-> garbage collecting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also add allocation ID?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error could be coming in from the stats collector and not necessarily while destroying a particular allocation runner.
} | ||
|
||
func (a *AllocGarbageCollector) Stop() { | ||
a.shutdownCh <- struct{}{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should close the channel rather than send a struct. If there are ever multiple listeners, only one will get it
} | ||
|
||
// NewHostStatsCollector returns a HostStatsCollector | ||
func NewHostStatsCollector(logger *log.Logger) *HostStatsCollector { | ||
func NewHostStatsCollector(logger *log.Logger, allocDir string) *HostStatsCollector { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment on why the alloc dir is passed in
} | ||
hs.DiskStats = diskStats | ||
|
||
usage, err := disk.Usage(h.allocDir) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Comment here
} | ||
|
||
return nil, CodedError(404, resourceNotFoundErr) | ||
} | ||
|
||
func (s *HTTPServer) ClientGCRequest(resp http.ResponseWriter, req *http.Request) (interface{}, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unit tests for both
statsCollector.usedPercents = []float64{0} | ||
statsCollector.inodePercents = []float64{0} | ||
|
||
alloc := mock.Alloc() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you set the disk un the tests so it is obvious in the test and the tests become resilient to changes to the mock alloc
|
||
if err := gc.keepUsageBelowThreshold(); err != nil { | ||
t.Fatalf("err: %v", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do these two test anything? Don't they need to check if the allocs have been popped
} | ||
|
||
if allocDirStats != nil { | ||
if allocDirStats.Available >= uint64(totalResource.DiskMB*1024*1024) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
1024*1024
9586b5d
to
6186adc
Compare
I'm going to lock this pull request because it has been closed for 120 days ⏳. This helps our maintainers find and focus on the active contributions. |
This PR introduces Garbage Collection to the Nomad client.