When volume is not marked in-use, do not backoff #106853
Conversation
k8s-ci-robot
added
size/M
k8s-ci-robot
added
the
sig/node
|
/triage accepted |
k8s-ci-robot
added
triage/accepted
|
/assign @jingxu97 |
|
/approve Maybe a little context: right now, when a pod lands on a node, kubelet does two things in parallel:
With this PR, the exp. backoff starts when the VolumeManager knows the volume already is in @jingxu97, PTAL |
k8s-ci-robot
added
approved
gnufied
force-pushed
the
disable-exp-backoff-volume-not-inuse
branch
2 times, most recently
from
f7d760a
to
90bb32f
Compare
|
/retest |
The current logic in VerifyControllerAttachedVolumeFunc, we check both whether ReportedInUse and VolumesAttached. To avoid checking ReportedInUse trigger backoff, now we can move the check ReportedInUse in reconciler before calling VerifyControllerAttachedVolumeFunc. Only after ReportedInUse is added, we can go ahead to continue the previous logic to check volumeAttached. |
Yes I already did that. See - https://github.com/kubernetes/kubernetes/pull/106853/files#diff-e9392bf9a117fa5eda2756ca13783db044a18ad4585b9b1fad776f660dc13068R206 |
|
@jingxu97 if the PR looks alright can you plz lgtm? |
| assert.NoError(t, volumetesting.VerifyWaitForAttachCallCount( | ||
| 0 /* expectedWaitForAttachCallCount */, fakePlugin)) | ||
| assert.NoError(t, volumetesting.VerifyMountDeviceCallCount( | ||
| 0 /* expectedMountDeviceCallCount */, fakePlugin)) |
There was a problem hiding this comment.
without this code change to pre-check volumeInUse and volumeattached, the test can also pass? It will fail VerifyControllerAttachedVolume, it will not go to waitforattachcall too?
There was a problem hiding this comment.
yes that sounds right. But then it will have IsOperationSafeToRetry check failing. But since we can't count VerifyControllerAttachedVolume calls, this test here should be fine?
There was a problem hiding this comment.
yeah, it is ok. we could try to improve some test logic later.
pkg/kubelet/volume_host.go
Outdated
| @@ -270,6 +270,20 @@ func (kvh *kubeletVolumeHost) GetNodeLabels() (map[string]string, error) { | |||
| return node.Labels, nil | |||
| } | |||
|
|
|||
| func (kvh *kubeletVolumeHost) GetAttachedVolumes() (map[v1.UniqueVolumeName]string, error) { | |||
There was a problem hiding this comment.
GetAttachedVolumes() were used in actual_state_world for both controller and kubelet side.
how about use a different name like GetAttachedVolumesFromNodeStatus?
| @@ -1514,6 +1515,16 @@ func (og *operationGenerator) GenerateVerifyControllerAttachedVolumeFunc( | |||
| return volumetypes.GeneratedOperations{}, volumeToMount.GenerateErrorDetailed("VerifyControllerAttachedVolume.FindPluginBySpec failed", err) | |||
| } | |||
|
|
|||
| if volumeToMount.PluginIsAttachable { | |||
| cachedAttachedVolumes, _ := og.volumePluginMgr.Host.GetAttachedVolumes() | |||
There was a problem hiding this comment.
maybe add comment indicating this is early check attached volume from node status using nodelister to avoid back off? Later we will directly call api server to get the latest node status to confirm the VolumeAttached list to avoid race condition.
|
the code looks good to me, just a few small comments. Thank you for the PR! I think this change can really improve some performance. Hopefully we can get some data about it. We should also consider to cherrypick this. |
gnufied
force-pushed
the
disable-exp-backoff-volume-not-inuse
branch
from
90bb32f
to
7989f27
Compare
k8s-ci-robot
removed
the
lgtm
|
The usual improvement from measurments Before: volume_operation_total_seconds_bucket{operation_name="volume_mount",plugin_name="kubernetes.io/vsphere-volume",le="0.1"} 0 After So overall 10.25 vs 8.23. Also as you can see most operations now complete within 15s whereas before some operations took 20s to complete. |
|
/lgtm |
k8s-ci-robot
added
the
lgtm
that's great, already 20% improvement! |
|
/assign @Random-Liu could you help review and approve this PR? Thanks! |
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: gnufied, jingxu97, jsafrane, Random-Liu 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
|
Unrelated flake in pull-kubernetes-integration: FAIL: TestCronJobLaunchesPodAndCleansUp /retest |
…853-upstream-release-1.22 Automated cherry pick of #106853: When volume is not marked in-use, do not backoff
…853-upstream-release-1.23 Automated cherry pick of #106853: When volume is not marked in-use, do not backoff
|
@gnufied should we consider cherrypick this change? |
|
@jingxu97 I already backported to 1.23 and 1.22 , were you thinking earlier versions than those? |
|
oh, I missed it. |
We unnecessarily trigger exp. backoff when volume is not marked in-use. Instead we can wait for volume to be marked as in-use before triggering operation_executor. This could result in reduced time when mounting attached volumes.
/sig storage
/kind bug
cc @jsafrane @jingxu97