Project

General

Profile

Bug #24081

Interface configuration call timeouts

Added by John Hixson over 3 years ago. Updated over 3 years ago.

Status:
Closed: Behaves correctly
Priority:
No priority
Assignee:
John Hixson
Category:
Middleware
Target version:
Severity:
New
Reason for Closing:
Reason for Blocked:
Needs QA:
Yes
Needs Doc:
Yes
Needs Merging:
Yes
Needs Automation:
No
Support Suite Ticket:
n/a
Hardware Configuration:
ChangeLog Required:
No

Description

When configuring a network interface, call timeouts occur. I removed an ipv6 address from my em0 interface and this happened. I added it back as an alias and this happened as well.

Associated revisions

Revision d6b9b961 (diff)
Added by John Hixson over 3 years ago

Add network node to sysctl tree Ticket: #24081

Revision b263a060 (diff)
Added by John Hixson over 3 years ago

While here, add route node to network sysctl tree Ticket: #24081

Revision e2c5b212 (diff)
Added by John Hixson over 3 years ago

Set interface and route sync call timeouts Ticket: #24081

Revision 3045635d (diff)
Added by John Hixson over 3 years ago

Add network node to sysctl tree Ticket: #24081 (cherry picked from commit d6b9b961ebdb85594200307ca25f6216ec1bcccb)

Revision c94939f4 (diff)
Added by John Hixson over 3 years ago

While here, add route node to network sysctl tree Ticket: #24081 (cherry picked from commit b263a060f47faf3c22d54f102cf9f529bf1b7766)

Revision bb210936 (diff)
Added by John Hixson over 3 years ago

Set interface and route sync call timeouts Ticket: #24081 (cherry picked from commit e2c5b212d004088583f5e7e059497bd863cda707)

History

#1 Updated by John Hixson over 3 years ago

  • Status changed from Screened to Needs Developer Review
  • Assignee changed from John Hixson to Vaibhav Chauhan

#2 Updated by William Grzybowski over 3 years ago

  • Status changed from Needs Developer Review to Unscreened
  • Assignee changed from Vaibhav Chauhan to John Hixson

Did figure out why it happened in the first place?

Network operations like these should not timeout, unless connectivity was lost which is a real issue and in that case I don`t see the reason why we need configurable timeouts for these?

#3 Updated by John Hixson over 3 years ago

  • Status changed from Unscreened to Screened

Yeah. This is related to the listen queue overflow that I saw over the weekend, which we sorted out. Still,we should probably at a minimum set a timeout and wrap in a try/catch block to avoid ugliness should some network issues occur.

#4 Updated by William Grzybowski over 3 years ago

John Hixson wrote:

Yeah. This is related to the listen queue overflow that I saw over the weekend, which we sorted out. Still,we should probably at a minimum set a timeout and wrap in a try/catch block to avoid ugliness should some network issues occur.

I don`t agree with that course of action. If an ugliness occur in that case it should be shown, not hidden and silently ignored.

#5 Updated by John Hixson over 3 years ago

William Grzybowski wrote:

John Hixson wrote:

Yeah. This is related to the listen queue overflow that I saw over the weekend, which we sorted out. Still,we should probably at a minimum set a timeout and wrap in a try/catch block to avoid ugliness should some network issues occur.

I don`t agree with that course of action. If an ugliness occur in that case it should be shown, not hidden and silently ignored.

I'm not saying not to ignore the error silently, i just don't agree with showing a stack trace. I think that under no circumstances should we ever show a stacktrace in the UI. We can save it to the log or whatever, but for god sake it just doesn't look good when in the UI. Would you rather have stack trace in the UI?

#6 Updated by Vaibhav Chauhan over 3 years ago

  • Target version changed from 11.0-RC3 to 11.0-U1

#7 Updated by William Grzybowski over 3 years ago

John Hixson wrote:

William Grzybowski wrote:

John Hixson wrote:

Yeah. This is related to the listen queue overflow that I saw over the weekend, which we sorted out. Still,we should probably at a minimum set a timeout and wrap in a try/catch block to avoid ugliness should some network issues occur.

I don`t agree with that course of action. If an ugliness occur in that case it should be shown, not hidden and silently ignored.

I'm not saying not to ignore the error silently, i just don't agree with showing a stack trace. I think that under no circumstances should we ever show a stacktrace in the UI. We can save it to the log or whatever, but for god sake it just doesn't look good when in the UI. Would you rather have stack trace in the UI?

How else would you present it and make users report the issue if not by showing them some error occurred and not let them think everything is super cool?

#8 Updated by John Hixson over 3 years ago

William Grzybowski wrote:

John Hixson wrote:

William Grzybowski wrote:

John Hixson wrote:

Yeah. This is related to the listen queue overflow that I saw over the weekend, which we sorted out. Still,we should probably at a minimum set a timeout and wrap in a try/catch block to avoid ugliness should some network issues occur.

I don`t agree with that course of action. If an ugliness occur in that case it should be shown, not hidden and silently ignored.

I'm not saying not to ignore the error silently, i just don't agree with showing a stack trace. I think that under no circumstances should we ever show a stacktrace in the UI. We can save it to the log or whatever, but for god sake it just doesn't look good when in the UI. Would you rather have stack trace in the UI?

How else would you present it and make users report the issue if not by showing them some error occurred and not let them think everything is super cool?

I don't think you are understanding me. I have said nothing indicating to "let them think everything is super cool". We can display errors just like we display errors all over freenas without a stack trace. Are you suggesting we stack trace everywhere instead of showing a well formatted error? Showing a stack trace is tacky and unprofessional. If we know of any area where a stack trace occurs, no matter the circumstances, we should eliminate it. By eliminating it, I do not mean to now show an error or "let them think everything is super cool", I mean eliminate the stack trace and show a well formatted error that indicates there is a problem. If an error is occurring and is unusual or shouldn't be happening, it will still be reported. I'm sure your ticket queue can vouch for that. I don't understand the problem here. A stack trace can happen, one that we know can happen, what is the issue with preventing it? `

#9 Updated by William Grzybowski over 3 years ago

John Hixson wrote:

William Grzybowski wrote:

John Hixson wrote:

William Grzybowski wrote:

John Hixson wrote:

Yeah. This is related to the listen queue overflow that I saw over the weekend, which we sorted out. Still,we should probably at a minimum set a timeout and wrap in a try/catch block to avoid ugliness should some network issues occur.

I don`t agree with that course of action. If an ugliness occur in that case it should be shown, not hidden and silently ignored.

I'm not saying not to ignore the error silently, i just don't agree with showing a stack trace. I think that under no circumstances should we ever show a stacktrace in the UI. We can save it to the log or whatever, but for god sake it just doesn't look good when in the UI. Would you rather have stack trace in the UI?

How else would you present it and make users report the issue if not by showing them some error occurred and not let them think everything is super cool?

I don't think you are understanding me. I have said nothing indicating to "let them think everything is super cool". We can display errors just like we display errors all over freenas without a stack trace. Are you suggesting we stack trace everywhere instead of showing a well formatted error? Showing a stack trace is tacky and unprofessional. If we know of any area where a stack trace occurs, no matter the circumstances, we should eliminate it. By eliminating it, I do not mean to now show an error or "let them think everything is super cool", I mean eliminate the stack trace and show a well formatted error that indicates there is a problem. If an error is occurring and is unusual or shouldn't be happening, it will still be reported. I'm sure your ticket queue can vouch for that. I don't understand the problem here. A stack trace can happen, one that we know can happen, what is the issue with preventing it? `

Right, and how changing the timeout without investigating a root cause or silencing the timeout (via a try/except and I have seen you have done in some commits) working towards displaying the errors in a beautiful way?
If you are simply ignoring the error and "logging it" it will go unnoticed and the user will think the operation succeeded.

#10 Updated by John Hixson over 3 years ago

William Grzybowski wrote:

John Hixson wrote:

William Grzybowski wrote:

John Hixson wrote:

William Grzybowski wrote:

John Hixson wrote:

Yeah. This is related to the listen queue overflow that I saw over the weekend, which we sorted out. Still,we should probably at a minimum set a timeout and wrap in a try/catch block to avoid ugliness should some network issues occur.

I don`t agree with that course of action. If an ugliness occur in that case it should be shown, not hidden and silently ignored.

I'm not saying not to ignore the error silently, i just don't agree with showing a stack trace. I think that under no circumstances should we ever show a stacktrace in the UI. We can save it to the log or whatever, but for god sake it just doesn't look good when in the UI. Would you rather have stack trace in the UI?

How else would you present it and make users report the issue if not by showing them some error occurred and not let them think everything is super cool?

I don't think you are understanding me. I have said nothing indicating to "let them think everything is super cool". We can display errors just like we display errors all over freenas without a stack trace. Are you suggesting we stack trace everywhere instead of showing a well formatted error? Showing a stack trace is tacky and unprofessional. If we know of any area where a stack trace occurs, no matter the circumstances, we should eliminate it. By eliminating it, I do not mean to now show an error or "let them think everything is super cool", I mean eliminate the stack trace and show a well formatted error that indicates there is a problem. If an error is occurring and is unusual or shouldn't be happening, it will still be reported. I'm sure your ticket queue can vouch for that. I don't understand the problem here. A stack trace can happen, one that we know can happen, what is the issue with preventing it? `

Right, and how changing the timeout without investigating a root cause or silencing the timeout (via a try/except and I have seen you have done in some commits) working towards displaying the errors in a beautiful way?
If you are simply ignoring the error and "logging it" it will go unnoticed and the user will think the operation succeeded.

Taking cheap shots at my commits is not necessary. I am "proposing" we do a try/catch to catch the exception, so sigh. And again, WHERE am I saying to just log the error and ignore it? I am "proposing" to DISPLAY THE ERROR IN A WELL FORMATTED MANNER. Re-read what I have previously stated and do so without your continuous assumption that I am stating to log it, ignore it and every other cheap shot you are taking at me. KThanks.

#11 Updated by John Hixson over 3 years ago

and hey, if stack traces are an OK manner to display errors, I'll just close out all my tickets that have stack traces as "behaves correctly" ;-)

#12 Updated by William Grzybowski over 3 years ago

John Hixson wrote:

William Grzybowski wrote:

John Hixson wrote:

William Grzybowski wrote:

John Hixson wrote:

William Grzybowski wrote:

John Hixson wrote:

Yeah. This is related to the listen queue overflow that I saw over the weekend, which we sorted out. Still,we should probably at a minimum set a timeout and wrap in a try/catch block to avoid ugliness should some network issues occur.

I don`t agree with that course of action. If an ugliness occur in that case it should be shown, not hidden and silently ignored.

I'm not saying not to ignore the error silently, i just don't agree with showing a stack trace. I think that under no circumstances should we ever show a stacktrace in the UI. We can save it to the log or whatever, but for god sake it just doesn't look good when in the UI. Would you rather have stack trace in the UI?

How else would you present it and make users report the issue if not by showing them some error occurred and not let them think everything is super cool?

I don't think you are understanding me. I have said nothing indicating to "let them think everything is super cool". We can display errors just like we display errors all over freenas without a stack trace. Are you suggesting we stack trace everywhere instead of showing a well formatted error? Showing a stack trace is tacky and unprofessional. If we know of any area where a stack trace occurs, no matter the circumstances, we should eliminate it. By eliminating it, I do not mean to now show an error or "let them think everything is super cool", I mean eliminate the stack trace and show a well formatted error that indicates there is a problem. If an error is occurring and is unusual or shouldn't be happening, it will still be reported. I'm sure your ticket queue can vouch for that. I don't understand the problem here. A stack trace can happen, one that we know can happen, what is the issue with preventing it? `

Right, and how changing the timeout without investigating a root cause or silencing the timeout (via a try/except and I have seen you have done in some commits) working towards displaying the errors in a beautiful way?
If you are simply ignoring the error and "logging it" it will go unnoticed and the user will think the operation succeeded.

Taking cheap shots at my commits is not necessary. I am "proposing" we do a try/catch to catch the exception, so sigh. And again, WHERE am I saying to just log the error and ignore it? I am "proposing" to DISPLAY THE ERROR IN A WELL FORMATTED MANNER. Re-read what I have previously stated and do so without your continuous assumption that I am stating to log it, ignore it and every other cheap shot you are taking at me. KThanks.

I am not making anything up here: https://github.com/freenas/freenas/commit/ef2c4790117582b5a5af04202dad7eee5810b541
You're simply ignoring it.

THIS IS NOT AN ASSUMPTION. This is not a cheap shot.

#13 Updated by John Hixson over 3 years ago

Read the whole ticket. Code is versatile. We can change it. It isn't set it stone. Showing me code I've already written doesn't prove anything. I don't want to show a stack trace, EVER. I stand by that. You don't approve of my code? Fine, that is okay by me, hence this entire thread. So let's change the error, which is what I have been focused on, but you are so set on proving me wrong and not liking the way I handled it, that it is blinding you to this. I refuse to accept a stack trace. What next, will show you me everywhere in my code where a stack trace occurs to prove that that isn't true? I do not approve of showing a stack trace. So how about we fix this to propagate a proper error to the user, as I have been TRYING to say all along. What next?

#14 Updated by John Hixson over 3 years ago

  • Assignee changed from John Hixson to William Grzybowski
  • Target version changed from 11.0-U1 to N/A

#15 Updated by William Grzybowski over 3 years ago

  • Assignee changed from William Grzybowski to John Hixson
  • Target version changed from N/A to 11.0-U1

John Hixson wrote:

Read the whole ticket. Code is versatile. We can change it. It isn't set it stone. Showing me code I've already written doesn't prove anything. I don't want to show a staAck trace, EVER. I stand by that. You don't approve of my code? Fine, that is okay by me, hence this entire thread. So let's change the error, which is what I have been focused on, but you are so set on proving me wrong and not liking the way I handled it, that it is blinding you to this. I refuse to accept a stack trace. What next, will show you me everywhere in my code where a stack trace occurs to prove that that isn't true? I do not approve of showing a stack trace. So how about we fix this to propagate a proper error to the user, as I have been TRYING to say all along. What next?

You don't want to show a stacktrace, got it. Where is the ticket to handle that?

Also, can you show me how this ticket or these commits you done help with the problem described? You're literally decreasing the default timeout from 60 to 30 seconds.

#16 Updated by John Hixson over 3 years ago

William Grzybowski wrote:

John Hixson wrote:

Read the whole ticket. Code is versatile. We can change it. It isn't set it stone. Showing me code I've already written doesn't prove anything. I don't want to show a staAck trace, EVER. I stand by that. You don't approve of my code? Fine, that is okay by me, hence this entire thread. So let's change the error, which is what I have been focused on, but you are so set on proving me wrong and not liking the way I handled it, that it is blinding you to this. I refuse to accept a stack trace. What next, will show you me everywhere in my code where a stack trace occurs to prove that that isn't true? I do not approve of showing a stack trace. So how about we fix this to propagate a proper error to the user, as I have been TRYING to say all along. What next?

You don't want to show a stacktrace, got it. Where is the ticket to handle that?

https://bugs.freenas.org/issues/24180

Also, can you show me how this ticket or these commits you done help with the problem described? You're literally decreasing the default timeout from 60 to 30 seconds.

The commit is done. As I continue to try and say, let's CHANGE what I did already to something YOU can agree with. Got it? What already is there is not relevant. LET'S CHANGE THE MESSAGE TO SOMETHING YOU CAN AGREE WITH. okay? Understand? As for the the decreasing the timeout, 1) It's a sysctl, which is the entire point, 2) I can easily change the sysctl to have a default value in the code 3) we can easily change the sysctl to have a default value in sysctl.conf. However, I have reverted all the code so everything is fine now. I am only arguing for arguments sake.

#17 Updated by John Hixson over 3 years ago

  • Status changed from Screened to Closed: Behaves correctly
  • Priority changed from Important to No priority
  • Target version changed from 11.0-U1 to N/A

#18 Updated by William Grzybowski over 3 years ago

Thank you.

Also available in: Atom PDF