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

Automated cherry pick of #72856: Fix nil panic propagation #72860

Merged
merged 2 commits into from Jan 16, 2019

Conversation

@liggitt
Copy link
Member

liggitt commented Jan 12, 2019

Cherry pick of #72856 on release-1.10.

also includes a commit updating copyright date in generated files

#72856: Fix nil panic propagation

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 12, 2019

/sig api-machinery
/kind bug
/priority critical-urgent
/milestone v1.10
/assign @sttts @dims @MaciekPytel

cc @kubernetes/sig-release @kubernetes/sig-testing for ruling on a fixup in 1.10. This is fixing a regression that got picked to the last 1.10 release :-/

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 12, 2019

@liggitt: You must be a member of the kubernetes/kubernetes-milestone-maintainers github team to set the milestone.

In response to this:

/sig api-machinery
/kind bug
/priority critical-urgent
/milestone v1.13
/assign @sttts @dims @MaciekPytel

cc @kubernetes/sig-release @kubernetes/sig-testing for ruling on a fixup in 1.10. This is fixing a regression that got picked to the last 1.10 release :-/

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 14, 2019

looks like a golang version changed in the verify job that affected release-1.10... @BenTheElder do you know where that is specified / how to re-pin it to the expected release-1.10 level?

@MaciekPytel

This comment has been minimized.

Copy link
Contributor

MaciekPytel commented Jan 14, 2019

How often does the panic actually happen? If it happens often enough to be a very serious issue I can cut an extra 1.10 release with the fix.
That being said so far we refused to issue extra patch releases for older versions even for the most serious issues (for example your own comment on why we won't cherry-pick fix for CVE-2018-1002105 to 1.9 and 1.8: #71671 (comment)). I've already closed a bunch of cherry-picks to 1.10, because the version is no longer supported. Are we sure this issue is really critical enough to break our own rules? This is setting a potentially dangerous precedent.

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 14, 2019

How often does the panic actually happen? If it happens often enough to be a very serious issue I can cut an extra 1.10 release with the fix.

Rarely enough that most CI was green most of the time, but often enough to be quite noticeable, especially in tests that load the API server (which appears to trigger the bug more).

That being said so far we refused to issue extra patch releases for older versions even for the most serious issues (for example your own comment on why we won't cherry-pick fix for CVE-2018-1002105 to 1.9 and 1.8: #71671 (comment)). I've already closed a bunch of cherry-picks to 1.10, because the version is no longer supported. Are we sure this issue is really critical enough to break our own rules? This is setting a potentially dangerous precedent.

I am not sure, but wanted to have the discussion with sig-release/sig-testing. The difference I see here is that this is fixing a regression introduced in the last 1.10 patch release. I am willing to close this, document the issue, and rely on fixed in 1.11+, but wanted to make sure there was consensus either way.

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Jan 14, 2019

looks like a golang version changed in the verify job that affected release-1.10... @BenTheElder do you know where that is specified / how to re-pin it to the expected release-1.10 level?

yeah, I think support for 1.10 just got dropped in those scripts, the image should still be around, we can poke test-infra and fix that, opening an issue.

@aleksandra-malinowska

This comment has been minimized.

Copy link
Contributor

aleksandra-malinowska commented Jan 14, 2019

Accidentally approved this one instead of 1.13... disregard please :/

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Jan 14, 2019

/retest

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Jan 14, 2019

Brought this up in SIG-release slack, consensus seems to be we should do this, +1
Edit: and consensus coming from the SIG leads + release team

one more time around the sun
Change-Id: I43df65fa4571609423785c334c855cfd21a3d0b8

@k8s-ci-robot k8s-ci-robot added size/L and removed size/XS approved labels Jan 14, 2019

@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 14, 2019

added a commit fixing date in generated files.

the typecheck failure looks like go1.10 is being used (error is related to https://golang.org/doc/go1.10#cgo), but release-1.10 is supposed to be using go1.9.3[1][2][3], and when I run hack/verify-typecheck.sh locally with go 1.9.x, it passes:

$ go version
go version go1.9.4 linux/amd64
$ hack/verify-typecheck.sh 
type-checking:  linux/amd64, windows/386, darwin/amd64, linux/arm, linux/386, windows/amd64, linux/arm64, linux/ppc64le, linux/s390x, darwin/386
16150/16150 linux/s390x   translations/test/en_US/LC_MESSAGES                                             
$ echo $?
0

[1] https://github.com/kubernetes/kubernetes/blob/release-1.10/build/build-image/cross/Dockerfile#L18
[2] https://github.com/kubernetes/kubernetes/blob/release-1.10/build/root/WORKSPACE#L49
[3] https://github.com/kubernetes/kubernetes/blob/release-1.10/test/images/Makefile#L18

@ixdy

This comment has been minimized.

Copy link
Member

ixdy commented Jan 14, 2019

I think we deleted the release-1.10-branched typecheck job in kubernetes/test-infra#10523 because we weren't expecting any more patches to 1.10. 🤷‍♂️

It's probably easy enough to add it back...

@BenTheElder

This comment has been minimized.

Copy link
Member

BenTheElder commented Jan 15, 2019

/retest

  - name: pull-kubernetes-typecheck
    path_alias: "k8s.io/kubernetes"
    decorate: true
    always_run: true
    skip_report: false
    branches:
    - release-1.10
    spec:
      containers:
      - name: main
        command:
        - make
        image: gcr.io/k8s-testimages/kubekins-e2e:v20190110-49573aea5-1.10
        args:
        - verify
        env:
        # Space separated list of the checks to run
        - name: WHAT
          value: typecheck
$ docker run --rm -it --entrypoint /bin/bash gcr.io/k8s-testimages/kubekins-e2e:v20190110-49573aea5-1.10
root@08564b8d576b:/workspace# go version
go version go1.9.3 linux/amd64
@liggitt

This comment has been minimized.

Copy link
Member Author

liggitt commented Jan 15, 2019

summarizing discussion from the sig-release meeting:

Decision was to move forward with this PR

Discussed criteria for acceptance:

  • Supporting CI jobs were still in place and had not yet been removed
  • The issue being fixed was a regression introduced in a patch release
  • The issue being fixed is of sufficient severity
  • The fix is well understood and contained (doesn’t introduce risk of additional regressions)
@dims

This comment has been minimized.

Copy link
Member

dims commented Jan 15, 2019

@liggitt thanks for driving this! yes the criteria is 👍

@dims

This comment has been minimized.

Copy link
Member

dims commented Jan 15, 2019

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm label Jan 15, 2019

@smarterclayton

This comment has been minimized.

Copy link
Contributor

smarterclayton commented Jan 16, 2019

/approve

@k8s-ci-robot

This comment has been minimized.

Copy link
Contributor

k8s-ci-robot commented Jan 16, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, smarterclayton

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 954ff68 into kubernetes:release-1.10 Jan 16, 2019

18 checks passed

cla/linuxfoundation liggitt 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 Skipped
pull-kubernetes-e2e-gce-device-plugin-gpu Job succeeded.
Details
pull-kubernetes-e2e-kops-aws Job succeeded.
Details
pull-kubernetes-e2e-kubeadm-gce Skipped
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-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