Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it.

This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

Hi @goushicui. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

@gnufied

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: goushicui, vahan-sahakyan-op
Once this PR has been reviewed and has the lgtm label, please assign thockin for approval. For more information see the Code Review Process.

The full list of commands accepted by this bot can be found here.

/assign @gnufied

/uncc

/assgin @thockin

/assign @thockin

/ok-to-test

@goushicui: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Shouldn't that volume be detached by external-attacher anyways regardless of restart of KCM? Once, deletionTimestamp is set on a VA object, it will get detached by external-attacher.

What exactly did we leak in this case? Are you saying, volume is not detaching in this case?

@gnufied Yes, the kube-controller restarted before the volumeAttachment started deleting.

@gnufied Yes, the kube-controller restarted before the volumeAttachment started deleting.

But you didn't answer rest of my question.

Shouldn't that volume be detached by external-attacher anyways regardless of restart of KCM? Once, deletionTimestamp is set on a VA object, it will get detached by external-attacher.

@gnufied Are you referring to this issue? I suggest you take a closer look at the description above. If deletionTimestamp is set on the volumeAttachment, the external-attacher can indeed perform the detachment operation in a timely manner. However, what if the deletion call fails, or if the KCM (Kubernetes Controller Manager) restarts before detachVolume is executed due to serial processing of the volume?
If it is not re-added here, neither the desired cache nor the actual cache will contain information about this volume. How would the subsequent reconcile operation perform the so-called deletion then?

kcm rebuild the asw from node and va objects, please see https://github.com/carlory/kubernetes/blob/master/pkg/controller/volume/attachdetach/attach_detach_controller.go#L683

https://github.com/carlory/kubernetes/blob/master/pkg/controller/volume/attachdetach/attach_detach_controller.go#L737

attachState := adc.actualStateOfWorld.GetAttachState(volumeName, nodeName)
		if attachState == cache.AttachStateDetached {

Look at the judgment condition here? It has already been removed from the node status attachedvolume before detach. Do you think this judgment can hold? @carlory

This check is correct. If the asw has populated the volume from the node status, the expected state of the volume is attached. if not but found it in the va object, it means that the volume is uncertain. we can not say it is attached or detached. reconciler will take care of it and mark it as attached or detached later. It is no problem.

it is not added to the cache.

It is not correct. MarkVolumeAsUncertain adds the volume to asw but its state is marked as Uncertain

err = rc.actualStateOfWorld.RemoveVolumeFromReportAsAttached(attachedVolume.VolumeName, attachedVolume.NodeName)
			if err != nil {
				logger.V(5).Info("RemoveVolumeFromReportAsAttached failed while removing volume from node",
					"node", klog.KRef("", string(attachedVolume.NodeName)),
					"volumeName", attachedVolume.VolumeName,
					"err", err)
			}

			// Update Node Status to indicate volume is no longer safe to mount.
			err = rc.nodeStatusUpdater.UpdateNodeStatusForNode(logger, attachedVolume.NodeName)

kCM restart, I want to ask how the volume information in ASW can be obtained through reconciliation. If it cannot be retrieved here, how should it be set to Uncertain status?

 If the asw has populated the volume from the node status, the expected state of the volume is attached. 

@carlory

https://github.com/carlory/kubernetes/blob/aab083972dbb5620b6daa62172aa1694e85facd7/pkg/controller/volume/attachdetach/attach_detach_controller.go#L347

If the kcm is restarted, the ADC controller will rebuild its cache before it starts reconciler. If the volume is removed from node's attachedVolumes but the va object still exists, the ADC controller will add the volume to asw and mark it as uncertain. After asw and dsw are populated, the reconciler is started. it will compare the asw and dsw, and then re-do detach operation.

If the volume has already been removed from the node status, it will not trigger a node status update.

@carlory
I see you keep emphasizing that populateActualStateOfWorld -> processVolumeAttachments will handle this logic... As I mentioned earlier, if it is removed from node.status.volumeAttached, the execution logic of populateActualStateOfWorld is as follows:

for _, node := range nodes {
    nodeName := types.NodeName(node.Name)

    for _, attachedVolume := range node.Status.VolumesAttached {
        uniqueName := attachedVolume.Name

First question: Won't this be added to the asw cache here?

attachState := adc.actualStateOfWorld.GetAttachState(volumeName, nodeName)
    if attachState == cache.AttachStateDetached {

Second question: Can it be marked as Uncertain here?

Third question: If it is not marked as Uncertain, will reconcile continue to process this volumeattachment?

Could you please refer to the code and answer the above questions 1, 2, and 3 respectively? Thank you.

First question: Won't this be added to the asw cache here?

It won't add the volume to cache. If the volume can be found in node status, it means that the volume should be added to asw and its state is attached.

Second question: Can it be marked as Uncertain here?

Yes, it should be Uncertain. we don't know whether the detach operation is called. If the detach operation is called and fails due to timeout, the volume may be detached. If not, the volume is attached. So it's state is Uncertain. We can not mark the attached volume as Uncertain if the volume is found in the node status. So we need this check.

Third question: If it is not marked as Uncertain, will reconcile continue to process this volume attachment?

No. If it is not in aws and dsw, the VA won't be handled. the reconciler doesn't know the VA concept.

Yes, it should be Uncertain. we don't know whether the detach operation is called. If the detach operation is called and fails due to timeout, the volume may be detached. If not, the volume is attached. So it's state is Uncertain. We can not mark the attached volume as Uncertain if the volume is found in the node status. So we need this check.

@carlory I am not asking whether it should be marked as Uncertain, but rather whether the attachState := adc.actualStateOfWorld.GetAttachState(volumeName, nodeName) can be retrieved from the cache here. Can we proceed further?

/assign jsafrane

@yuga711

The Kubernetes project currently lacks enough contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle stale
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle stale

PR needs rebase.

The Kubernetes project currently lacks enough active contributors to adequately respond to all PRs.

This bot triages PRs according to the following rules:

• After 90d of inactivity, lifecycle/stale is applied
• After 30d of inactivity since lifecycle/stale was applied, lifecycle/rotten is applied
• After 30d of inactivity since lifecycle/rotten was applied, the PR is closed

You can:

• Mark this PR as fresh with /remove-lifecycle rotten
• Close this PR with /close
• Offer to help out with Issue Triage

Please send feedback to sig-contributor-experience at kubernetes/community.

/lifecycle rotten