improve logging for scale set size changes #4541
Conversation
k8s-ci-robot
added
cncf-cla: yes
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: marwanad 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 |
k8s-ci-robot
added
the
approved
| sizeMutex sync.Mutex | ||
| curSize int64 | ||
| lastSizeRefresh time.Time | ||
| sizeRefreshPeriod time.Duration |
There was a problem hiding this comment.
The sizeRefreshPeriod was always 0 after #3717. The TTL for VMSS cache refreshes is in honoured there where we depend on the CA core cloudprovider.Refresh() calls to update the caches.
| @@ -142,35 +140,30 @@ func (scaleSet *ScaleSet) getCurSize() (int64, error) { | |||
| scaleSet.sizeMutex.Lock() | |||
| defer scaleSet.sizeMutex.Unlock() | |||
|
|
|||
| if scaleSet.lastSizeRefresh.Add(scaleSet.sizeRefreshPeriod).After(time.Now()) { | |||
There was a problem hiding this comment.
this was always no-op after #3717 as lastSizeRefresh + 0 will always be in the past so this conditional was skipped.
| if scaleSet.curSize != curSize { | ||
| // Invalidate the instance cache if the capacity has changed. | ||
| klog.V(5).Infof("VMSS %q size changed from: %d to %d, invalidating instance cache", scaleSet.Name, scaleSet.curSize, curSize) |
There was a problem hiding this comment.
keeping it as v=5 intentionally because it can be inferred from L64.
|
/assign @nilo19 |
marwanad
changed the title
marwanad
force-pushed
the
improve-azure-cache-logging
branch
from
e0e2a5b to
091c72c
Compare
| @@ -128,11 +126,11 @@ func (scaleSet *ScaleSet) MaxSize() int { | |||
| return scaleSet.maxSize | |||
| } | |||
|
|
|||
| func (scaleSet *ScaleSet) getVMSSFromCache() (compute.VirtualMachineScaleSet, *retry.Error) { | |||
| func (scaleSet *ScaleSet) getVMSSFromCache() (compute.VirtualMachineScaleSet, error) { | |||
There was a problem hiding this comment.
no reason to return the ARM error type here. The call to scaleSet.manager.azureCache.getScaleSets() is always a cache read call.
| err = ss.IncreaseSize(100) | ||
| expectedErr := fmt.Errorf("the scale set test-asg is under initialization, skipping IncreaseSize") | ||
| expectedErr := fmt.Errorf("could not find vmss: test-asg-doesnt-exist") |
There was a problem hiding this comment.
technically with the changes in #3717, we'll never end with the "-1" initialization case because the cache wont have the VMSS and we exit earlier. (keeping it there for now though)
|
/area provider/azure |
|
@marwanad: The label(s) In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
This adds logging around previous + refreshed VMSS size and lowers the verbosity for one other log.
It also removes some fields that are no longer used after #3717 move to global azure_manager cache