Bug 1783221: lib/resourcemerge/core: Fix panic on container/port removal #282
Conversation
openshift-ci-robot
added
do-not-merge/work-in-progress
|
@wking: This pull request references Bugzilla bug 1783221, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker. In response to this:
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. |
openshift-ci-robot
added
approved
wking
force-pushed
the
resource-merge-index-mutating-existing
branch
from
252c62b
to
caec9c2
Compare
wking
changed the title
openshift-ci-robot
removed
the
do-not-merge/work-in-progress
|
Fixed by iterating through |
|
/cherrypick release-4.3 |
|
@wking: once the present PR merges, I will cherry-pick it on top of release-4.3 in a new PR and assign it to you. In response to this:
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. |
|
/cherrypick release-4.2 |
|
@wking: once the present PR merges, I will cherry-pick it on top of release-4.2 in a new PR and assign it to you. In response to this:
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. |
|
/cherrypick release-4.1 |
|
@wking: once the present PR merges, I will cherry-pick it on top of release-4.1 in a new PR and assign it to you. In response to this:
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. |
Avoid:
$ go test ./lib/resourcemerge/
panic: runtime error: index out of range [recovered]
panic: runtime error: index out of range
goroutine 38 [running]:
testing.tRunner.func1(0xc0001ab000)
.../sdk/go1.12.9/src/testing/testing.go:830 +0x392
panic(0xccb520, 0x163f880)
.../sdk/go1.12.9/src/runtime/panic.go:522 +0x1b5
github.com/openshift/cluster-version-operator/lib/resourcemerge.ensureContainers(0xc0000bbd57, 0xc0001d4040, 0xc0001cd760, 0x1, 0x1)
.../lib/go/src/github.com/openshift/cluster-version-operator/lib/resourcemerge/core.go:69 +0x840
github.com/openshift/cluster-version-operator/lib/resourcemerge.ensurePodSpec(0xc0001c5d57, 0xc0001d4010, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc0001cd760, 0x1, ...)
.../lib/go/src/github.com/openshift/cluster-version-operator/lib/resourcemerge/core.go:28 +0xc6
github.com/openshift/cluster-version-operator/lib/resourcemerge.TestEnsurePodSpec.func1(0xc0001ab000)
.../lib/go/src/github.com/openshift/cluster-version-operator/lib/resourcemerge/core_test.go:276 +0xc7
testing.tRunner(0xc0001ab000, 0xc0001d8770)
.../sdk/go1.12.9/src/testing/testing.go:865 +0xc0
created by testing.(*T).Run
.../sdk/go1.12.9/src/testing/testing.go:916 +0x35a
FAIL github.com/openshift/cluster-version-operator/lib/resourcemerge 0.010s
(with the core_test.go but the old core.go) when removing an entry
mutated the existing slice without re-entering the:
for i, whatever := range *existing
With this commit, we iterate from the back of the existing slice, so
any removals affect indexes that we've already covered. For both
containers and service ports, any appends happen later in the
function, so we don't need to worry about slice expansion at this
point.
The buggy logic was originally from 3d1ad76 (Remove containers if
requested in Update, 2019-04-26, openshift#178).
wking
force-pushed
the
resource-merge-index-mutating-existing
branch
from
caec9c2
to
e8e80c2
Compare
|
Hmm, buggy logic was originally from 3d1ad76 (#178), so we may not need to backport to 4.1. Not sure why that change wasn't backported, but it was linked to rhbz#1710172, which ended up getting closed NOTABUG. I'm happy manually adjusting the 4.1 backport to include both #178 and this change. |
|
/lgtm I might have suggested a comment to explain reverse iteration but git blame plus comprehensive commit message is probably better. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: patrickdillon, wking 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 |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
5 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
3 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/hold No need for retest noise with intergration broken: |
openshift-ci-robot
added
the
do-not-merge/hold
|
/retest |
|
@wking PTAL, unsure if anything has changed in the 1.5 months since a hold was put on this or if we're just hoping to stop retests |
|
Hah, yeah, probably fixed integration by now :p. /hold cancel |
openshift-ci-robot
removed
the
do-not-merge/hold
|
/retest Please review the full test history for this PR and help us cut down flakes. |
7 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@wking: All pull requests linked via external trackers have merged. Bugzilla bug 1783221 has been moved to the MODIFIED state. In response to this:
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. |
|
@wking: new pull request created: #313 In response to this:
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. |
|
@wking: #282 failed to apply on top of branch "release-4.2": In response to this:
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. |
|
@wking: #282 failed to apply on top of branch "release-4.1": In response to this:
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. |
Avoid:
$ go test ./lib/resourcemerge/
panic: runtime error: index out of range [recovered]
panic: runtime error: index out of range
goroutine 38 [running]:
testing.tRunner.func1(0xc0001ab000)
.../sdk/go1.12.9/src/testing/testing.go:830 +0x392
panic(0xccb520, 0x163f880)
.../sdk/go1.12.9/src/runtime/panic.go:522 +0x1b5
github.com/openshift/cluster-version-operator/lib/resourcemerge.ensureContainers(0xc0000bbd57, 0xc0001d4040, 0xc0001cd760, 0x1, 0x1)
.../lib/go/src/github.com/openshift/cluster-version-operator/lib/resourcemerge/core.go:69 +0x840
github.com/openshift/cluster-version-operator/lib/resourcemerge.ensurePodSpec(0xc0001c5d57, 0xc0001d4010, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc0001cd760, 0x1, ...)
.../lib/go/src/github.com/openshift/cluster-version-operator/lib/resourcemerge/core.go:28 +0xc6
github.com/openshift/cluster-version-operator/lib/resourcemerge.TestEnsurePodSpec.func1(0xc0001ab000)
.../lib/go/src/github.com/openshift/cluster-version-operator/lib/resourcemerge/core_test.go:276 +0xc7
testing.tRunner(0xc0001ab000, 0xc0001d8770)
.../sdk/go1.12.9/src/testing/testing.go:865 +0xc0
created by testing.(*T).Run
.../sdk/go1.12.9/src/testing/testing.go:916 +0x35a
FAIL github.com/openshift/cluster-version-operator/lib/resourcemerge 0.010s
(with the core_test.go but the old core.go) when removing an entry
mutated the existing slice without re-entering the:
for i, whatever := range *existing
With this commit, we iterate from the back of the existing slice, so
any removals affect indexes that we've already covered. For both
containers and service ports, any appends happen later in the
function, so we don't need to worry about slice expansion at this
point.
The buggy logic was originally from 3d1ad76 (Remove containers if
requested in Update, 2019-04-26, openshift#178).
This commit cherry-picks e8e80c2 (lib/resourcemerge/core: Fix panic
on container/port removal, 2019-12-16, openshift#282) back to the 4.2 branch.
But since f268b6d (Add support to EnsureServicePorts, 2019-11-25,
branch, I've only kept the container portion of e8e80c2 in this
commit.
Avoid:
$ go test ./lib/resourcemerge/
panic: runtime error: index out of range [recovered]
panic: runtime error: index out of range
goroutine 38 [running]:
testing.tRunner.func1(0xc0001ab000)
.../sdk/go1.12.9/src/testing/testing.go:830 +0x392
panic(0xccb520, 0x163f880)
.../sdk/go1.12.9/src/runtime/panic.go:522 +0x1b5
github.com/openshift/cluster-version-operator/lib/resourcemerge.ensureContainers(0xc0000bbd57, 0xc0001d4040, 0xc0001cd760, 0x1, 0x1)
.../lib/go/src/github.com/openshift/cluster-version-operator/lib/resourcemerge/core.go:69 +0x840
github.com/openshift/cluster-version-operator/lib/resourcemerge.ensurePodSpec(0xc0001c5d57, 0xc0001d4010, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc0001cd760, 0x1, ...)
.../lib/go/src/github.com/openshift/cluster-version-operator/lib/resourcemerge/core.go:28 +0xc6
github.com/openshift/cluster-version-operator/lib/resourcemerge.TestEnsurePodSpec.func1(0xc0001ab000)
.../lib/go/src/github.com/openshift/cluster-version-operator/lib/resourcemerge/core_test.go:276 +0xc7
testing.tRunner(0xc0001ab000, 0xc0001d8770)
.../sdk/go1.12.9/src/testing/testing.go:865 +0xc0
created by testing.(*T).Run
.../sdk/go1.12.9/src/testing/testing.go:916 +0x35a
FAIL github.com/openshift/cluster-version-operator/lib/resourcemerge 0.010s
(with the core_test.go but the old core.go) when removing an entry
mutated the existing slice without re-entering the:
for i, whatever := range *existing
With this commit, we iterate from the back of the existing slice, so
any removals affect indexes that we've already covered. For both
containers and service ports, any appends happen later in the
function, so we don't need to worry about slice expansion at this
point.
The buggy logic was originally from 3d1ad76 (Remove containers if
requested in Update, 2019-04-26, openshift#178).
This commit cherry-picks e8e80c2 (lib/resourcemerge/core: Fix panic
on container/port removal, 2019-12-16, openshift#282) back to the 4.2 branch.
But since f268b6d (Add support to EnsureServicePorts, 2019-11-25, openshift#272)
and its EnsureServicePorts was never backported to the 4.2 branch,
I've only kept the container portion of e8e80c2 in this commit.
Avoid:
$ go test ./lib/resourcemerge/
panic: runtime error: index out of range [recovered]
panic: runtime error: index out of range
goroutine 38 [running]:
testing.tRunner.func1(0xc0001ab000)
.../sdk/go1.12.9/src/testing/testing.go:830 +0x392
panic(0xccb520, 0x163f880)
.../sdk/go1.12.9/src/runtime/panic.go:522 +0x1b5
github.com/openshift/cluster-version-operator/lib/resourcemerge.ensureContainers(0xc0000bbd57, 0xc0001d4040, 0xc0001cd760, 0x1, 0x1)
.../lib/go/src/github.com/openshift/cluster-version-operator/lib/resourcemerge/core.go:69 +0x840
github.com/openshift/cluster-version-operator/lib/resourcemerge.ensurePodSpec(0xc0001c5d57, 0xc0001d4010, 0x0, 0x0, 0x0, 0x0, 0x0, 0x0, 0xc0001cd760, 0x1, ...)
.../lib/go/src/github.com/openshift/cluster-version-operator/lib/resourcemerge/core.go:28 +0xc6
github.com/openshift/cluster-version-operator/lib/resourcemerge.TestEnsurePodSpec.func1(0xc0001ab000)
.../lib/go/src/github.com/openshift/cluster-version-operator/lib/resourcemerge/core_test.go:276 +0xc7
testing.tRunner(0xc0001ab000, 0xc0001d8770)
.../sdk/go1.12.9/src/testing/testing.go:865 +0xc0
created by testing.(*T).Run
.../sdk/go1.12.9/src/testing/testing.go:916 +0x35a
FAIL github.com/openshift/cluster-version-operator/lib/resourcemerge 0.010s
(with the core_test.go but the old core.go) when removing an entry
mutated the existing slice without re-entering the:
for i, whatever := range *existing
With this commit, we iterate from the back of the existing slice, so
any removals affect indexes that we've already covered. For both
containers and service ports, any appends happen later in the
function, so we don't need to worry about slice expansion at this
point.
The buggy logic was originally from 3d1ad76 (Remove containers if
requested in Update, 2019-04-26, openshift#178).
This commit cherry-picks e8e80c2 (lib/resourcemerge/core: Fix panic
on container/port removal, 2019-12-16, openshift#282) back to the 4.2 branch.
But since f268b6d (Add support to EnsureServicePorts, 2019-11-25, openshift#272)
and its EnsureServicePorts was never backported to the 4.2 branch,
I've only kept the container portion of e8e80c2 in this commit.
Avoid:
when removing an entry mutated the existing slice without re-entering the
for i, whatever := range *existing.No fix yet, but there's a test that reproduces the bug's panic.