Skip to content
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

controller: ignore deleted PVs #92

Merged
merged 1 commit into from Oct 16, 2020

Conversation

pohly
Copy link
Contributor

@pohly pohly commented Oct 8, 2020

It is normal that a volume is still in the work queue after it was
deleted, which used to trigger an unnecessary error log message:

    I1008 18:58:53.808993       1 controller.go:1467] delete "pvc-0e8331f7-de7a-4c0c-a678-724e05c3b716": started
    I1008 18:58:54.125842       1 controller.go:1482] delete "pvc-0e8331f7-de7a-4c0c-a678-724e05c3b716": volume deleted
    I1008 18:58:54.159675       1 controller.go:1532] delete "pvc-0e8331f7-de7a-4c0c-a678-724e05c3b716": persistentvolume deleted
    I1008 18:58:54.159706       1 controller.go:1537] delete "pvc-0e8331f7-de7a-4c0c-a678-724e05c3b716": succeeded
    E1008 18:58:54.159752       1 controller.go:1056] volume "pvc-0e8331f7-de7a-4c0c-a678-724e05c3b716" in work queue no longer exists

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Oct 8, 2020
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Oct 8, 2020
@pohly
Copy link
Contributor Author

pohly commented Oct 8, 2020

Potentially related to this PR (I'm currently running an external-provisioner with this patch):


I1008 18:58:53.808993       1 controller.go:1467] delete "pvc-0e8331f7-de7a-4c0c-a678-724e05c3b716": started
I1008 18:58:54.125842       1 controller.go:1482] delete "pvc-0e8331f7-de7a-4c0c-a678-724e05c3b716": volume deleted
I1008 18:58:54.159675       1 controller.go:1532] delete "pvc-0e8331f7-de7a-4c0c-a678-724e05c3b716": persistentvolume deleted
I1008 18:58:54.159706       1 controller.go:1537] delete "pvc-0e8331f7-de7a-4c0c-a678-724e05c3b716": succeeded
E1008 18:58:54.159752       1 controller.go:1056] volume "pvc-0e8331f7-de7a-4c0c-a678-724e05c3b716" in work queue no longer exists

Line numbers are a bit different because of other changes, but I think that's from

// syncVolumeHandler gets the volume from informer's cache then calls syncVolume
func (ctrl *ProvisionController) syncVolumeHandler(ctx context.Context, key string) error {
volumeObj, exists, err := ctrl.volumes.GetByKey(key)
if err != nil {
return err
}
if !exists {
utilruntime.HandleError(fmt.Errorf("volume %q in work queue no longer exists", key))
return nil
}
return ctrl.syncVolume(ctx, volumeObj)
}

Should that really be logged as error? I think it is (now?) normal that some work items in the queue get removed.

Same with

// syncClaimHandler gets the claim from informer's cache then calls syncClaim. A non-nil error triggers requeuing of the claim.
func (ctrl *ProvisionController) syncClaimHandler(ctx context.Context, key string) error {
objs, err := ctrl.claimsIndexer.ByIndex(uidIndex, key)
if err != nil {
return err
}
var claimObj interface{}
if len(objs) > 0 {
claimObj = objs[0]
} else {
obj, found := ctrl.claimsInProgress.Load(key)
if !found {
utilruntime.HandleError(fmt.Errorf("claim %q in work queue no longer exists", key))
return nil
}
claimObj = obj
}
return ctrl.syncClaim(ctx, claimObj)
}

@wongma7
Copy link
Contributor

wongma7 commented Oct 9, 2020

Just merged #89 which is making the same fix.

I agree with you that an object missing from cache should not be logged as an error, it will be logged unnecessarily every single time a volume is deleted+removed from cache since the deletion itself will trigger the informer to call syncVolume again

It is normal that a volume is still in the work queue after it was
deleted, which used to trigger an unnecessary error log message:

I1008 18:58:53.808993       1 controller.go:1467] delete "pvc-0e8331f7-de7a-4c0c-a678-724e05c3b716": started
I1008 18:58:54.125842       1 controller.go:1482] delete "pvc-0e8331f7-de7a-4c0c-a678-724e05c3b716": volume deleted
I1008 18:58:54.159675       1 controller.go:1532] delete "pvc-0e8331f7-de7a-4c0c-a678-724e05c3b716": persistentvolume deleted
I1008 18:58:54.159706       1 controller.go:1537] delete "pvc-0e8331f7-de7a-4c0c-a678-724e05c3b716": succeeded
E1008 18:58:54.159752       1 controller.go:1056] volume "pvc-0e8331f7-de7a-4c0c-a678-724e05c3b716" in work queue no longer exists
@pohly
Copy link
Contributor Author

pohly commented Oct 12, 2020

I agree with you that an object missing from cache should not be logged as an error, it will be logged unnecessarily every single time a volume is deleted+removed from cache since the deletion itself will trigger the informer to call syncVolume again

Let's use this PR to get rid of that log message. Commit added and description updated.

@pohly pohly changed the title controller: fix updating of volume cache controller: ignore deleted PVs Oct 12, 2020
@pohly
Copy link
Contributor Author

pohly commented Oct 12, 2020

/assign @wongma7

@wongma7
Copy link
Contributor

wongma7 commented Oct 13, 2020

/lgtm
/retest
flake issue: #95

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 13, 2020
@pohly
Copy link
Contributor Author

pohly commented Oct 13, 2020

/retest

@wongma7
Copy link
Contributor

wongma7 commented Oct 16, 2020

whoops

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pohly, wongma7

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 16, 2020
@k8s-ci-robot k8s-ci-robot merged commit 1a367dc into kubernetes-sigs:master Oct 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants