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

Propagate errors from DSW to pod events #80369

Merged
merged 3 commits into from Aug 5, 2019

Conversation

@jsafrane
Copy link
Member

commented Jul 19, 2019

What this PR does / why we need it:
Goal: tell users that pod cannot run because:

  • PVC does not exist
  • PVC is not bound
  • PVC uses a different volumeMode than the pod expects.
  • ...

The first two bullets are checked also by scheduler, but kubelet should report errors here too, in case a pod runs with a different scheduler or was scheduled manually.

I added list of errors for each pod to DesiredStateOfWorld (DSW), where DSW populator (DSWP) stores them and kubelet picks them up via WaitForAttachAndMount and creates pod event(s) from them.

I did not want to add errors to dsw.volumesToMount, because that would create volumesToMount entries with incomplete / erroneous volumes (e.g. PVC was not bound yet and we don't know unique volume name).

I hope I got the cleaning right - DSWP periodically checks all stored errors and removes those for deleted pods.

I cleaned some error messages on the way, so the events are nice and tidy (well, at least they're better than before, see below).

Natural extension would be to report also mount / set up errors from reconciler ((not part of this PR). By placing the list of errors to DSW, there is no way to pass errors from unmount / teardown (there is no entry in DSW for pods that are being torn down).

Examples (simulating narrow terminal window):

  • PVC does not exist:

    Type     Reason       Age   From                Message
    ----     ------       ----  ----                -------
    Warning  FailedMount  9s    kubelet, 127.0.0.1  Unable to attach or mount volumes:
    error processing PVC default/myclaim: failed to fetch PVC from API server:
    persistentvolumeclaims "myclaim" not found. List of unmounted volumes=[vol]. List 
    of unattached volumes=[vol default-token-4pt45].
    
  • PVC is not bound:

    Type     Reason       Age   From                Message
    ----     ------       ----  ----                -------
    Warning  FailedMount  4s    kubelet, 127.0.0.1  Unable to attach or mount volumes:
    error processing PVC default/myclaim: PVC is not bound. List of unmounted volumes=
    [vol]. List of unattached volumes=[vol default-token-4pt45]. 
    
  • Wrong mode:

    Type     Reason                  Age   From                     Message
    ----     ------                  ----  ----                     -------
    Normal   SuccessfulAttachVolume  4s    attachdetach-controller  AttachVolume.Attach 
    succeeded for volume "mypv"
    Warning  FailedMount             4s    kubelet, 127.0.0.1       Unable to attach or
    mount volumes: volume vol has volumeMode Filesystem, but is specified in 
    volumeDevices. List of unmounted volumes=[vol default-token-4pt45]. List of 
    unattached volumes=[vol default-token-4pt45].
    
  • Multiple errors:

    Type     Reason       Age   From                Message
    ----     ------       ----  ----                -------
    Warning  FailedMount  1s    kubelet, 127.0.0.1  Unable to attach or mount volumes: 
    error processing PVC default/myclaim2: failed to fetch PVC from API server: 
    persistentvolumeclaims "myclaim2" not found; error processing PVC default/myclaim: 
    failed to fetch PVC from API server: persistentvolumeclaims "myclaim" not found. 
    List of unmounted volumes=[vol vol2 default-token-5jpm2]. List of unattached 
    volumes=[vol vol2 default-token-5jpm2].
    

I might send every error as a separate event. Would it be too many events? Does
List of unmounted volumes=[vol vol2 default-token-5jpm2]. List of unattached volumes=[vol vol2 default-token-5jpm2]. have any value?

Which issue(s) this PR fixes:
Fixes #79794

Does this PR introduce a user-facing change?:

Errors from pod volume set up are now propagated as pod events.

/kind bug
/sig storage

@msau42 @jingxu97 @gnufied, PTAL

@mkimuram, I am afraid that checking for events in #79796 is the best we can do here.

pod.Namespace,
pod.Name,
err,

This comment has been minimized.

Copy link
@mkimuram

mkimuram Jul 19, 2019

Contributor

For readability in multiple errors case, would it be better to output errors after lists of volumes?
(It might be too much to add "List of errors=" before errors, though.)

This comment has been minimized.

Copy link
@jsafrane

jsafrane Jul 22, 2019

Author Member

Ack, I reordered the messages as you suggest:

Events:
  Type     Reason       Age   From                Message
  ----     ------       ----  ----                -------
  Warning  FailedMount  1s    kubelet, 127.0.0.1  Unable to attach or mount 
volumes: unmounted volumes=[vol], unattached volumes=[vol default-token-p67zc]: 
error processing PVC default/myclaim: failed to fetch PVC from API server: 
persistentvolumeclaims "myclaim" not found
@msau42

This comment has been minimized.

Copy link
Member

commented Jul 19, 2019

/assign @jingxu97

@@ -402,6 +402,9 @@ func (vm *volumeManager) getUnattachedVolumes(expectedVolumes []string) []string
// volumes are mounted.
func (vm *volumeManager) verifyVolumesMountedFunc(podName types.UniquePodName, expectedVolumes []string) wait.ConditionFunc {
return func() (done bool, err error) {
if errs := vm.desiredStateOfWorld.PopPodErrors(podName); len(errs) > 0 {
return true, errors.New(strings.Join(errs, "; "))

This comment has been minimized.

Copy link
@gnufied

gnufied Jul 19, 2019

Member

should it return false from here?

This comment has been minimized.

Copy link
@jsafrane

jsafrane Jul 22, 2019

Author Member

It does not really matter, wait.Poll will report error in both cases.

podVolume.Name,
volumeSpec.Name(),
uniquePodName,
err)
dswp.desiredStateOfWorld.AddErrorToPod(uniquePodName, err.Error())

This comment has been minimized.

Copy link
@gnufied

gnufied Jul 19, 2019

Member

If for some reason if these errors were transient and volume eventually did get added to DSOW.. should we remove the errors from pod if allVolumesAdded is true in lin#343?

This comment has been minimized.

Copy link
@jsafrane

jsafrane Jul 22, 2019

Author Member

Good idea, I removed errors if allVolumesAdded.

@jsafrane jsafrane force-pushed the jsafrane:dswp-error branch 2 times, most recently from 2f6aa64 to a4bac55 Jul 22, 2019

@k8s-ci-robot k8s-ci-robot added size/M approved and removed size/L labels Jul 22, 2019

@jsafrane jsafrane force-pushed the jsafrane:dswp-error branch from a4bac55 to 1a99a14 Jul 22, 2019

@k8s-ci-robot k8s-ci-robot added size/L and removed size/M approved labels Jul 22, 2019

@jsafrane jsafrane force-pushed the jsafrane:dswp-error branch from 1a99a14 to af0c2fe Jul 22, 2019

@jsafrane

This comment has been minimized.

Copy link
Member Author

commented Jul 22, 2019

/retest

@gnufied

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

Looks good to me. but I want @jingxu97 to have a look as well.

/lgtm
/hold

@gnufied

This comment has been minimized.

Copy link
Member

commented Jul 23, 2019

/priority important-longterm

@gnufied

This comment has been minimized.

Copy link
Member

commented Jul 25, 2019

/hold cancel

Assigning to @derekwaynecarr for approval

/assign @derekwaynecarr

@@ -124,9 +125,16 @@ func TestInitialPendingVolumesForPodAndGetVolumesInUse(t *testing.T) {
// delayed claim binding
go delayClaimBecomesBound(kubeClient, claim.GetNamespace(), claim.ObjectMeta.Name)

err = manager.WaitForAttachAndMount(pod)
err = wait.Poll(100*time.Millisecond, 1*time.Second, func() (bool, error) {

This comment has been minimized.

Copy link
@jingxu97

jingxu97 Jul 26, 2019

Contributor

without this wait.Poll, the test should be flaky?

This comment has been minimized.

Copy link
@jsafrane

jsafrane Jul 29, 2019

Author Member

Yes, because the PVC is bound after a short delay (see above) and the first few calls return error that the PVC is not bound yet.

@jingxu97

This comment has been minimized.

Copy link
Contributor

commented Jul 26, 2019

just one thing since the error might be transient, for example, PVC is not created when pod is created. But then PVC is added to desired state, but fail to attach/mount for other reason. When user see the events, will it cause some confusion on what is the reason that pod fails to start?

@jsafrane

This comment has been minimized.

Copy link
Member Author

commented Jul 29, 2019

I thinks that regular volume operation errors are reported as events too, so user will see say "PVC does not exist" and "SetUp failed: volume not found" events and should know where is the pod stuck at.

@jingxu97

This comment has been minimized.

Copy link
Contributor

commented Jul 29, 2019

/lgtm

@jsafrane

This comment has been minimized.

Copy link
Member Author

commented Jul 30, 2019

/assign @derekwaynecarr @tallclair
for approval on sig-node side (verifyVolumesMountedFunc changed a bit).

@derekwaynecarr
Copy link
Member

left a comment

have a question, once clarified, this is good from my side.

@@ -1673,8 +1673,8 @@ func (kl *Kubelet) syncPod(o syncPodOptions) error {
if !kl.podIsTerminated(pod) {
// Wait for volumes to attach/mount
if err := kl.volumeManager.WaitForAttachAndMount(pod); err != nil {
kl.recorder.Eventf(pod, v1.EventTypeWarning, events.FailedMountVolume, "Unable to mount volumes for pod %q: %v", format.Pod(pod), err)
klog.Errorf("Unable to mount volumes for pod %q: %v; skipping pod", format.Pod(pod), err)
kl.recorder.Eventf(pod, v1.EventTypeWarning, events.FailedMountVolume, "Unable to attach or mount volumes: %v", err)

This comment has been minimized.

Copy link
@derekwaynecarr

derekwaynecarr Aug 2, 2019

Member

if this recurs multiple times, we will just fall back to event spam protection, so i think this is fine.

@@ -412,3 +430,36 @@ func (dsw *desiredStateOfWorld) isDeviceMountableVolume(volumeSpec *volume.Spec)

return false
}

func (dsw *desiredStateOfWorld) AddErrorToPod(podName types.UniquePodName, err string) {

This comment has been minimized.

Copy link
@derekwaynecarr

derekwaynecarr Aug 2, 2019

Member

how big can this get? is it possible for this to grow unbounded ?

This comment has been minimized.

Copy link
@jsafrane

jsafrane Aug 5, 2019

Author Member

Good question, errors are stored in sets.String, so it can grow if every error has a different text. I added a limit of 10, just in case there is a bug in the populator.

Add limit of stored errors
There should be a limit so the set of errors does not grow too much in
case of some bug in the populator.

@k8s-ci-robot k8s-ci-robot removed the lgtm label Aug 5, 2019

@jsafrane

This comment has been minimized.

Copy link
Member Author

commented Aug 5, 2019

/retest

@derekwaynecarr

This comment has been minimized.

Copy link
Member

commented Aug 5, 2019

thanks for bounding the set.

/approve
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Aug 5, 2019

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: derekwaynecarr, jsafrane

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 merged commit 1fcd3d6 into kubernetes:master Aug 5, 2019

23 checks passed

cla/linuxfoundation jsafrane authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-conformance-image-test Skipped.
pull-kubernetes-cross Skipped.
pull-kubernetes-dependencies Job succeeded.
Details
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-csi-serial Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gce-iscsi Skipped.
pull-kubernetes-e2e-gce-iscsi-serial Skipped.
pull-kubernetes-e2e-gce-storage-slow Job succeeded.
Details
pull-kubernetes-godeps Skipped.
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped.
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-node-e2e-containerd Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
pull-publishing-bot-validate Skipped.
tide In merge pool.
Details

@k8s-ci-robot k8s-ci-robot added this to the v1.16 milestone Aug 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.