-
Notifications
You must be signed in to change notification settings - Fork 4k
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
Provider/Azure: Add VM cache. #2683
Conversation
var instances []compute.VirtualMachine | ||
var err error | ||
|
||
if virtualMachinesStatusCache.lastRefresh.Add(vmInstancesRefreshPeriod).After(time.Now()) { |
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.
there're three places of such codes, let's extract a new func for getting VMs from cache and refresh if the cache is outdated.
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.
Finished.
9572001
to
ca65d81
Compare
return virtualMachinesStatusCache.virtualMachines, nil | ||
} | ||
|
||
vms, err := as.GetVirtualMachines() |
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.
should check err before the cache update. or else, the cache may be cleaned on errors
// GetVMIndexes gets indexes of all virtual machines belonging to the agent pool. | ||
func (as *AgentPool) GetVMIndexes() ([]int, map[int]string, error) { | ||
instances, err := as.GetVirtualMachines() | ||
virtualMachinesStatusCache.mutex.Lock() | ||
defer virtualMachinesStatusCache.mutex.Unlock() |
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.
virtualMachinesStatusCache
has already been locked within getAllVirtualMachinesWithCache
, hence it shouldn't be locked again
@@ -117,9 +131,35 @@ func (as *AgentPool) MaxSize() int { | |||
return as.maxSize | |||
} | |||
|
|||
func (as *AgentPool) getAllVirtualMachinesWithCache() ([]compute.VirtualMachine, 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.
getAllVirtualMachinesWithCache
-> getVirtualMachinesFromCache
ca65d81
to
306cece
Compare
The function's been tested. |
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.
/lgtm
/approve
@nilo19 please cherry pick the fix to old branches |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: feiskyer The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Cherry pick of #2683 to 1.15: Add VM cache.
Cherry pick of #2683 to 1.14: Add VM cache.
Cherry pick of #2683 to 1.16: Add VM cache.
Cherry pick of #2683 to 1.17: Add VM cache.
What this PR does:
/area provider/azure
/kind feature