Bug 1800346: lib/resourcemerge/core: Fix panic on container removal #314
Conversation
|
@wking: This pull request references Bugzilla bug 1800346, which is invalid:
Comment DetailsIn 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
the
bugzilla/invalid-bug
openshift-ci-robot
added
approved
wking
force-pushed
the
resource-merge-index-mutating-existing-4.2
branch
from
23b65b6 to
1563b4a
Compare
|
/bugzilla refresh |
openshift-ci-robot
added
bugzilla/valid-bug
|
@sdodson: This pull request references Bugzilla bug 1800346, which is valid. DetailsIn 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. |
|
/lgtm |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
1 similar comment
|
/retest Please review the full test history for this PR and help us cut down flakes. |
bparees
added
the
cherry-pick-approved
|
/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. |
openshift-ci-robot
added
the
needs-rebase
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.
wking
force-pushed
the
resource-merge-index-mutating-existing-4.2
branch
from
1563b4a to
cba1d64
Compare
openshift-ci-robot
removed
lgtm
|
/lgtm |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: sdodson, wking The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
sdodson
removed
the
cherry-pick-approved
|
Patch Manager, I've yanked cherry-pick-approved just so this doesn't merge outside of the merge window. Please re-add it when the window opens again. |
|
/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. |
knobunc
added
the
cherry-pick-approved
|
@wking: All pull requests linked via external trackers have merged. Bugzilla bug 1800346 has been moved to the MODIFIED state. DetailsIn 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. |
7 participants
This pull-request cherry-picks e8e80c2 (#282) back to the 4.2 branch. But since f268b6d (#272) and its
EnsureServicePortswas never backported to the 4.2 branch, I've only kept the container portion of e8e80c2 in this PR.