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

Fix: karmada-controller-manager: panic: runtime error: index out of range #4145

Merged
merged 1 commit into from
Oct 24, 2023

Conversation

halfrost
Copy link
Contributor

@halfrost halfrost commented Oct 18, 2023

What type of PR is this?

What this PR does / why we need it:
Fix issue #3854
karmada-controller-manager: panic: runtime error: index out of range

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

Special notes for your reviewer:

`karmada-controller-manager`: Fixed a panic issue due to `index out of range` in resource model functionality.

@karmada-bot karmada-bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Oct 18, 2023
@RainbowMango
Copy link
Member

Thanks @halfrost, appreciate it!

cc @tedli @liangyuanpeng @NickYadance @jwcesign @chaosi-zju help to take a look~ Thanks.

@jwcesign
Copy link
Member

/lgtm

These two functions are not used:

func (rs *ResourceSummary) UpdateInResourceSummary(oldNode, newNode ClusterResourceNode) error {

func (rs *ResourceSummary) DeleteFromResourceSummary(crn ClusterResourceNode) error {

Can we delete them? @halfrost

@karmada-bot karmada-bot added lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Oct 23, 2023
@halfrost
Copy link
Contributor Author

@jwcesign Hi, I have already removed these two functions that are currently not in use. This delete function took me some time to debug when it was initially developed. Now, they are no longer needed. Could you please review it again?

@jwcesign
Copy link
Member

Can you help squash the multiple commits into one? other lgtm

cc @chaosi-zju for take another look

@halfrost
Copy link
Contributor Author

@jwcesign Done

Copy link
Member

@chaosi-zju chaosi-zju left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

generally looks good to me

pkg/modeling/modeling.go Outdated Show resolved Hide resolved
… solve multithreading issues

2. Delete unused functions

Signed-off-by: halfrost <ydz627@gmail.com>
@chaosi-zju
Copy link
Member

/lgtm

@karmada-bot karmada-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 24, 2023
@jwcesign
Copy link
Member

/cc @RainbowMango

Copy link
Member

@RainbowMango RainbowMango left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/approve

@karmada-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: RainbowMango

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

@karmada-bot karmada-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 24, 2023
@karmada-bot karmada-bot merged commit 086cbcc into karmada-io:master Oct 24, 2023
12 checks passed
@liangyuanpeng
Copy link
Contributor

liangyuanpeng commented Oct 25, 2023

Sorry for my late and I have tried triggering ResourceSummary.getIndex continuously in an environment with 15 members of the cluster, but cannot reproduce the problem. I believe this PR can fix the problem.
/lgtm

@tedli
Copy link
Contributor

tedli commented Dec 7, 2023

Hi, @halfrost

It's this issue fixed by a single commit which asscoiated with this pr? Is there any other commits related?

I didn't use the latest release, I have a my own branch, I cherry-pick this commit, it builds, run, and works.

However, I run into another index out of range panic, so I thought whether my cherry pick lack some commits.

I1207 10:05:29.583572       7 controller.go:118] "Observed a panic in reconciler: runtime error: index out of range [2] with length 2" controller="cluster" controllerGroup="cluster.karmada.io" controllerKind="Cluster" Cluster="db-shbt" namespace="" name="db-shbt" reconcileID=5fff6af8-e7dc-4716-a6cf-34c4e9a6a0e5
panic: runtime error: index out of range [2] with length 2 [recovered]
	panic: runtime error: index out of range [2] with length 2

goroutine 1559 [running]:
sigs.k8s.io/controller-runtime/pkg/internal/controller.(*Controller).Reconcile.func1()
	C:/Users/lizhen/Documents/Go/src/github.com/karmada-io/karmada/vendor/sigs.k8s.io/controller-runtime/pkg/internal/controller/controller.go:119 +0x1e5
panic({0x22b3fc0?, 0xc01306bc80?})
	C:/Program Files/Go/src/runtime/panic.go:914 +0x21f
github.com/karmada-io/karmada/pkg/modeling.(*ResourceSummary).getIndex(0xc00e7730a8, {0x2?, 0xc02121cf60?})
	C:/Users/lizhen/Documents/Go/src/github.com/karmada-io/karmada/pkg/modeling/modeling.go:125 +0x185
github.com/karmada-io/karmada/pkg/modeling.(*ResourceSummary).AddToResourceSummary(0xc00e7730a8, {0x1, 0xc02121cf60})
	C:/Users/lizhen/Documents/Go/src/github.com/karmada-io/karmada/pkg/modeling/modeling.go:181 +0x37
github.com/karmada-io/karmada/pkg/controllers/status.getAllocatableModelings(0xc02609e000, {0xc025e15800, 0x84, 0xc025e6d000?}, {0xc025e6d000, 0x532, 0x9f877eb7cc?})
func (rs *ResourceSummary) getIndex(crn ClusterResourceNode) int {
	index := math.MaxInt
	for i, m := range defaultModelSorting {
		tmpIndex := searchLastLessElement(rs.modelSortings[i], crn.resourceList[m])
		if tmpIndex < index {
			index = tmpIndex
		}
	}
	return index
}

@jwcesign
Copy link
Member

jwcesign commented Dec 7, 2023

Hi @tedli
Here is another PR: #4176

@tedli
Copy link
Contributor

tedli commented Dec 8, 2023

Hi @tedli Here is another PR: #4176

Thx 🥰

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

karmada-controller-manager: panic: runtime error: index out of range
7 participants