-
Notifications
You must be signed in to change notification settings - Fork 366
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
supportbundle: fix nil pointer error #2598
supportbundle: fix nil pointer error #2598
Conversation
Codecov Report
@@ Coverage Diff @@
## main #2598 +/- ##
==========================================
+ Coverage 60.27% 60.62% +0.34%
==========================================
Files 282 285 +3
Lines 22345 23050 +705
==========================================
+ Hits 13469 13973 +504
- Misses 7452 7610 +158
- Partials 1424 1467 +43
Flags with carried forward coverage won't be shown. Click here to find out more.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it's not great to reference line numbers in a commit message, as code changes over time and it's not helpful when searching through the git history
if b != nil { | ||
r.clean(ctx, b.Filepath, bundleExpireDuration) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
shouldn't we check the value of err
after calling collectAgent
/ collectController
above?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks, I moved it into the annomous function which wraps to handle error returned from collectAgent / collectController
func() {
r.statusLocker.Lock()
defer r.statusLocker.Unlock()
if err != nil {
klog.Errorf("Error when collecting supportBundle: %v", err)
r.cache.Status = systemv1beta1.SupportBundleStatusNone
return
}
...
}()
7b32019
to
9f91972
Compare
/test-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TestAntctl failed because of this change on all testbeds.
Looking at r.clean
, it also acquired r.statusLocker
, so the change leads to a deadlock.
when iproute2 is not installed in docker image, and run `antctl supportbundle` in antrea-agent, it will panic, this is because, collectAgent will return a nil pointer for systemv1beta1.SupportBundle, and in `func (r *supportBundleREST) Create`, antrea handles the nil pointer directly which leads to the panic. Signed-off-by: Bin Liu <biliu@vmware.com>
9f91972
to
3a5402c
Compare
/test-all |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, will defer to @antoninbas since he had comment.
When iproute2 is not installed in docker image, and one runs `antctl supportbundle` in antrea-agent, it will panic. This is because collectAgent will return a nil pointer for systemv1beta1.SupportBundle, and in `func (r *supportBundleREST) Create`, antrea calls a method on the nil pointer directly which leads to the panic. Signed-off-by: Bin Liu <biliu@vmware.com>
The code should only schedule file cleanup when the returned error is empty (meaning the returned supportbundle is not nil), otherwise the supportbundle file would never be deleted and the program would panic when there is any error encountered during supportbundle collection. antrea-io#2598 tried to fix it but did the opposite. Signed-off-by: Quan Tian <qtian@vmware.com>
The code should only schedule file cleanup when the returned error is empty (meaning the returned supportbundle is not nil), otherwise the supportbundle file would never be deleted and the program would panic when there is any error encountered during supportbundle collection. antrea-io#2598 tried to fix it but did the opposite. Signed-off-by: Quan Tian <qtian@vmware.com>
The code should only schedule file cleanup when the returned error is empty (meaning the returned supportbundle is not nil), otherwise the supportbundle file would never be deleted and the program would panic when there is any error encountered during supportbundle collection. antrea-io#2598 tried to fix it but did the opposite. Signed-off-by: Quan Tian <qtian@vmware.com>
The code should only schedule file cleanup when the returned error is empty (meaning the returned supportbundle is not nil), otherwise the supportbundle file would never be deleted and the program would panic when there is any error encountered during supportbundle collection. #2598 tried to fix it but did the opposite. Signed-off-by: Quan Tian <qtian@vmware.com>
…ea-io#4306) The code should only schedule file cleanup when the returned error is empty (meaning the returned supportbundle is not nil), otherwise the supportbundle file would never be deleted and the program would panic when there is any error encountered during supportbundle collection. antrea-io#2598 tried to fix it but did the opposite. Signed-off-by: Quan Tian <qtian@vmware.com>
…ea-io#4306) The code should only schedule file cleanup when the returned error is empty (meaning the returned supportbundle is not nil), otherwise the supportbundle file would never be deleted and the program would panic when there is any error encountered during supportbundle collection. antrea-io#2598 tried to fix it but did the opposite. Signed-off-by: Quan Tian <qtian@vmware.com>
when iproute2 is not installed in docker image, and run
antctl supportbundle
inantrea-agent, it will panic, this is because, collectAgent will return a nil pointer
for systemv1beta1.SupportBundle, and
func (r *supportBundleREST) Create
,antrea handles the nil pointer directly which leads to the panic.