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

Annotate errors in apimachinery e2e tests #69583

Conversation

@audreylim
Copy link
Contributor

audreylim commented Oct 9, 2018

Related to #34059

What this PR does / why we need it:
This PR makes errors more helpful in the apimachinery e2e tests.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:
My first contribution to Kubernetes

Release note:

NONE
@wgliang

This comment has been minimized.

Copy link
Member

wgliang commented Oct 10, 2018

/ok-to-test

@audreylim audreylim force-pushed the audreylim:annotate-apimachinery-e2e-test-errors branch from 7b3cc7a to a65f212 Oct 10, 2018

@audreylim

This comment has been minimized.

Copy link
Contributor Author

audreylim commented Oct 10, 2018

/retest

@neolit123

This comment has been minimized.

Copy link
Member

neolit123 commented Oct 10, 2018

/area test
/kind cleanup
/retest

@audreylim audreylim force-pushed the audreylim:annotate-apimachinery-e2e-test-errors branch from a65f212 to 070ee53 Oct 10, 2018

@audreylim

This comment has been minimized.

Copy link
Contributor Author

audreylim commented Oct 10, 2018

/retest

@audreylim audreylim force-pushed the audreylim:annotate-apimachinery-e2e-test-errors branch from 070ee53 to b4ebfdf Oct 10, 2018

@ixdy

This comment has been minimized.

Copy link
Member

ixdy commented Oct 11, 2018

/assign
/retest

@audreylim audreylim force-pushed the audreylim:annotate-apimachinery-e2e-test-errors branch 2 times, most recently from c85d764 to 6ae0ff8 Oct 11, 2018

@audreylim audreylim force-pushed the audreylim:annotate-apimachinery-e2e-test-errors branch from 6ae0ff8 to 22526fa Oct 12, 2018

@nikhita

This comment has been minimized.

Copy link
Member

nikhita commented Oct 19, 2018

/assign @spiffxp

@@ -130,7 +131,7 @@ func checkExistingRCRecovers(f *framework.Framework) {
framework.ExpectNoError(wait.Poll(time.Millisecond*500, time.Second*60, func() (bool, error) {
options := metav1.ListOptions{LabelSelector: rcSelector.String()}
pods, err := podClient.List(options)
Expect(err).NotTo(HaveOccurred())
Expect(err).NotTo(HaveOccurred(), "failed to list pods in namespace: %s, that match label selector: %s", f.Namespace.Name, rcSelector.String())

This comment has been minimized.

Copy link
@spiffxp

spiffxp Oct 19, 2018

Member

Overall this PR LGTM, but I'm curious if there is a way we could get the namespace name from podClient here? Something about having to refer back N>10 lines to see why/how to craft this message seems less than ideal

This comment has been minimized.

Copy link
@spiffxp

spiffxp Oct 20, 2018

Member

ok, looks like no

@spiffxp
Copy link
Member

spiffxp left a comment

Thanks so much for PR'ing this, this is a lot of lines touched!

I reviewed about halfway before stopping, so keep in mind there are more examples of the following than I explicitly called out:

  • I think we want to be consistent about logging the namespace being used for namespace-scoped objects
  • I think we generally don't want to dump an entire pod spec with +%v, but there were some cases that may make sense (ie: error updating pod "foo" to )
  • I'm not sure it's worth calling out "or pods went to Failed state" if you're err'ing on WaitForPod, but I could be convinced either way
@@ -120,7 +121,7 @@ func checkExistingRCRecovers(f *framework.Framework) {
}
for _, pod := range pods.Items {
err = podClient.Delete(pod.Name, metav1.NewDeleteOptions(0))
Expect(err).NotTo(HaveOccurred())
Expect(err).NotTo(HaveOccurred(), "failed to delete pod: %s", pod.Name)

This comment has been minimized.

Copy link
@spiffxp

spiffxp Oct 20, 2018

Member

missing namespace

pod3 := newGCPod("pod3")
pod3, err = podClient.Create(pod3)
Expect(err).NotTo(HaveOccurred())
Expect(err).NotTo(HaveOccurred(), "failed to create pod: %+v", pod3)

This comment has been minimized.

Copy link
@spiffxp

spiffxp Oct 20, 2018

Member

For these three pods:

  • why the %+v instead of the pod's name
  • missing namespace
patch := fmt.Sprintf(`{"metadata":{"ownerReferences":[{"apiVersion":"v1","kind":"ReplicationController","name":"%s","uid":"%s"}]}}`, rc2.ObjectMeta.Name, rc2.ObjectMeta.UID)
for i := 0; i < halfReplicas; i++ {
pod := pods.Items[i]
_, err := podClient.Patch(pod.Name, types.StrategicMergePatchType, []byte(patch))
Expect(err).NotTo(HaveOccurred())
Expect(err).NotTo(HaveOccurred(), "failed to apply to pod %s a strategic merge patch: %s", pod.Name, patch)

This comment has been minimized.

Copy link
@spiffxp

spiffxp Oct 20, 2018

Member

missing namespace

framework.Logf("pod3.ObjectMeta.OwnerReferences=%#v", pod3.ObjectMeta.OwnerReferences)
// delete one pod, should result in the deletion of all pods
deleteOptions := getForegroundOptions()
deleteOptions.Preconditions = metav1.NewUIDPreconditions(string(pod1.UID))
err = podClient.Delete(pod1.ObjectMeta.Name, deleteOptions)
Expect(err).NotTo(HaveOccurred())
Expect(err).NotTo(HaveOccurred(), "failed to delete pod: %s", pod1.ObjectMeta.Name)

This comment has been minimized.

Copy link
@spiffxp

spiffxp Oct 20, 2018

Member

nit: why pod1.ObjectMeta.Name instead of pod1.Name as used above?

missing namespace

This comment has been minimized.

Copy link
@audreylim

audreylim Oct 23, 2018

Author Contributor

I referenced the previous line which used pod1.ObjectMeta.Name but I think both refer to the same since ObjectMeta.Name is a promoted field (https://github.com/kubernetes/api/blob/master/core/v1/types.go#L3116). I've changed it to pod1.Name here and left the other references to pod1.ObjectMeta.Name alone, maybe the author wanted more clarity.

Expect(err).NotTo(HaveOccurred())
patch3 := addRefPatch(pod2.Name, pod2.UID)
pod3, err = podClient.Patch(pod3.Name, types.StrategicMergePatchType, patch3)
Expect(err).NotTo(HaveOccurred(), "failed to apply to pod %s a strategic merge patch: %s", pod3.Name, patch3)

This comment has been minimized.

Copy link
@spiffxp

spiffxp Oct 20, 2018

Member

each of these patch failures are missing namespace

pod.Annotations = map[string]string{"update-1": "test"}
pod, err = c.CoreV1().Pods(ns).Update(pod)
Expect(err).NotTo(HaveOccurred())
Expect(err).NotTo(HaveOccurred(), "failed to update pod %s to: %+v", pod.Name, pod)

This comment has been minimized.

Copy link
@spiffxp

spiffxp Oct 20, 2018

Member

missing namespace


// pod should now start running
err = framework.WaitForPodRunningInNamespace(c, pod)
Expect(err).NotTo(HaveOccurred())
Expect(err).NotTo(HaveOccurred(), "timeout while waiting for pod %s to run in namespace: %s, or pod is in a Failed state", pod.Name, pod.Namespace)

This comment has been minimized.

Copy link
@spiffxp

spiffxp Oct 20, 2018

Member

maybe "error while waiting for pod %s to go to Running phase in namespace %s"


// ensure create call returns
<-ch

// verify that we cannot start the pod initializing again
pod, err = c.CoreV1().Pods(ns).Get(podName, metav1.GetOptions{})
Expect(err).NotTo(HaveOccurred())
Expect(err).NotTo(HaveOccurred(), "failed to get pod: %s", podName)

This comment has been minimized.

Copy link
@spiffxp

spiffxp Oct 20, 2018

Member

missing namespace

Expect(err).NotTo(HaveOccurred())
pod := newInitPod(podName)
_, err := c.CoreV1().Pods(ns).Create(pod)
Expect(err).NotTo(HaveOccurred(), "failed to create pod: %+v", pod)

This comment has been minimized.

Copy link
@spiffxp

spiffxp Oct 20, 2018

Member

missing namespace, and consider using the pod's name with a %s instead


// verify the list call filters out uninitialized pods
pods, err := c.CoreV1().Pods(ns).List(metav1.ListOptions{IncludeUninitialized: true})
Expect(err).NotTo(HaveOccurred())
Expect(err).NotTo(HaveOccurred(), "failed to list pods in namespace: %s", ns)

This comment has been minimized.

Copy link
@spiffxp

spiffxp Oct 20, 2018

Member

I saw earlier messages included the list options, maybe here as well?

@audreylim

This comment has been minimized.

Copy link
Contributor Author

audreylim commented Oct 23, 2018

Thanks for your feedback! I've:

  • added namespaces where its scoped
  • replaced spec dumps with object names
  • addressed the other comments

Notes:

@spiffxp
Copy link
Member

spiffxp left a comment

few tiny nits to fix, otherwise lgtm

and yeah feel free to squash

@@ -672,15 +672,15 @@ func testWebhook(f *framework.Framework) {
cm.Data["webhook-e2e-test"] = "webhook-disallow"
}
_, err = updateConfigMap(client, f.Namespace.Name, allowedConfigMapName, toNonCompliantFn)
Expect(err).NotTo(BeNil())
Expect(err).To(HaveOccurred(), "update (PUT) admitted configmap %s to a non-compliant one should be rejected by webhook", allowedConfigMapName)

This comment has been minimized.

Copy link
@spiffxp

spiffxp Oct 23, 2018

Member

missing namespace


By("creating a new configmap")
testConfigMap, err = c.CoreV1().ConfigMaps(ns).Create(testConfigMap)
Expect(err).NotTo(HaveOccurred())
Expect(err).NotTo(HaveOccurred(), "failed to create configmap %s", configMapName, configMapName)

This comment has been minimized.

Copy link
@spiffxp

spiffxp Oct 23, 2018

Member

missing namespace

@@ -261,23 +263,23 @@ var _ = SIGDescribe("Watchers", func() {

By("creating a watch on configmaps with a certain label")
testWatch, err := watchConfigMaps(f, "", toBeChangedLabelValue)
Expect(err).NotTo(HaveOccurred())
Expect(err).NotTo(HaveOccurred(), "failed to create configmap with label: %s", toBeChangedLabelValue)

This comment has been minimized.

Copy link
@spiffxp

spiffxp Oct 23, 2018

Member

failed to create a watch on configmaps with label

@@ -197,17 +198,17 @@ var _ = SIGDescribe("Watchers", func() {

By("creating a watch on configmaps")
testWatchBroken, err := watchConfigMaps(f, "", watchRestartedLabelValue)
Expect(err).NotTo(HaveOccurred())
Expect(err).NotTo(HaveOccurred(), "failed to create a watch on configmaps: %s", watchRestartedLabelValue)

This comment has been minimized.

Copy link
@spiffxp

spiffxp Oct 23, 2018

Member

configmaps with with label

@audreylim audreylim force-pushed the audreylim:annotate-apimachinery-e2e-test-errors branch from 7421b57 to ad3bca0 Oct 23, 2018

@audreylim

This comment has been minimized.

Copy link
Contributor Author

audreylim commented Oct 23, 2018

Fixed

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Oct 24, 2018

/lgtm
/approve
Awesome first contribution, thanks!

@k8s-ci-robot k8s-ci-robot added the lgtm label Oct 24, 2018

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Oct 24, 2018

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: audreylim, spiffxp

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

@audreylim audreylim force-pushed the audreylim:annotate-apimachinery-e2e-test-errors branch from ad3bca0 to 3eaab70 Oct 24, 2018

@k8s-ci-robot k8s-ci-robot removed the lgtm label Oct 24, 2018

@audreylim

This comment has been minimized.

Copy link
Contributor Author

audreylim commented Oct 24, 2018

oops, made a slight change here: https://github.com/kubernetes/kubernetes/pull/69583/files#diff-b3ef5995f1063613af33e678183dc03dR57

removed pod := newTablePod(podName) since pod spec wasn't used in error message anymore

@spiffxp

This comment has been minimized.

Copy link
Member

spiffxp commented Nov 1, 2018

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Nov 1, 2018

@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 1, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

1 similar comment
@fejta-bot

This comment has been minimized.

Copy link

fejta-bot commented Nov 2, 2018

/retest
This bot automatically retries jobs that failed/flaked on approved PRs (send feedback to fejta).

Review the full test history for this PR.

Silence the bot with an /lgtm cancel comment for consistent failures.

@k8s-ci-robot k8s-ci-robot merged commit 808557e into kubernetes:master Nov 2, 2018

18 checks passed

cla/linuxfoundation audreylim authorized
Details
pull-kubernetes-bazel-build Job succeeded.
Details
pull-kubernetes-bazel-test Job succeeded.
Details
pull-kubernetes-cross Skipped
pull-kubernetes-e2e-gce Job succeeded.
Details
pull-kubernetes-e2e-gce-100-performance Job succeeded.
Details
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-gke Skipped
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
pull-kubernetes-integration Job succeeded.
Details
pull-kubernetes-kubemark-e2e-gce-big Job succeeded.
Details
pull-kubernetes-local-e2e Skipped
pull-kubernetes-local-e2e-containerized Skipped
pull-kubernetes-node-e2e Job succeeded.
Details
pull-kubernetes-typecheck Job succeeded.
Details
pull-kubernetes-verify Job succeeded.
Details
tide In merge pool.
Details
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.