-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 panic in linuxNodeHandler.NodeDelete #25851
Conversation
c870d57
to
918c3b9
Compare
918c3b9
to
fc180f2
Compare
Can you provide details on how this causes a panic? Do you have a stacktrace for the panic and could you include it in the commit description? |
See #25543 for details on the panic. The commit msg could use a link too, though :) |
At least for the datapath, we try to have the commit descriptions as self contained as possible, so I'd rather have the full stack trace in the description than a link. |
fc180f2
to
a985715
Compare
hi @pchaigno, I have updated the commit message, Please tell me if you feel this is good. |
I think the formatting is broken as there are no newlines. You will also need to squash your two commits in one. |
e6a02f2
to
3b097d6
Compare
Commit 37604abb942d388f92f8583ec20d4f7dd96073f7 does not contain "Signed-off-by". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
1 similar comment
Commit 37604abb942d388f92f8583ec20d4f7dd96073f7 does not contain "Signed-off-by". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
It seems |
37604ab
to
e129e9d
Compare
Cilium uses main and your commit now looks good to me. Do you know under what conditions the panic happens? I'd like to write a release note and that requires understanding why our CI didn't catch this. Is there something special about your deployment? Why don't nodes have IP addresses? 🤔 |
I am the reporter of #25543. I can't reproduce the issue, but it happened two times during the abnormal shutdown of hypervisor:
|
Thanks for the additional context @egsysoev!
So the VM wasn't fully provisioned yet when it was deleted? If so, that could explain how the deleted node doesn't have IP addresses. |
e129e9d
to
5de857a
Compare
fa6225f
to
254d49b
Compare
@pchaigno Hi, It's time to merge this PR? |
254d49b
to
7ee1f10
Compare
@cyclinder Note that every time you push to the branch, we will need to restart the tests. So I would only push if there are conflicts or if we ask you to because some test is failing. Not sure if that was the case here, but I thought I'd notify you 🙂 |
I see, thanks for the clarification @pchaigno |
@cyclinder Sorry to pull you in the other direction; would you be able to rebase to pick up the recent changes to CI? (Jenkins is no longer) |
Observed a panic: runtime.boundsError{x:0, y:0, signed:true, code:0x0} (runtime error: index out of range [0] with length 0) github.com/cilium/cilium/pkg/datapath/linux.(*linuxNodeHandler).deallocateIDForNode(0xc003e871f0?, 0xc003e87108?) /go/src/github.com/cilium/cilium/pkg/datapath/linux/node_ids.go:110 +0x335 Signed-off-by: qifeng guo <qifeng.guo@daocloud.io>
7ee1f10
to
52560f5
Compare
@ti-mo Sure, Updated. |
/test |
Two ci-gingo jobs failed with |
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXX
line if the commit addresses a particularGitHub issue.
Fixes: <commit-id>
tag, thenplease add the commit author[s] as reviewer[s] to this issue.
Fixes: #25543