reboot operation #11
reboot operation #11
Conversation
| } | ||
|
|
||
| // IgnoreUnavailableErr ignores unavailable errors returned by reboot status. | ||
| func (r *RebootOperation) IgnoreUnavailableErr(ignoreUnavailableErr bool) *RebootOperation { |
There was a problem hiding this comment.
I think we just drop this and always ignore UnavailableErrors when waiting for active
There was a problem hiding this comment.
hmmm.. is UnavailableError always acceptable after reboot?
Having a function to ignore the error gives the flexibility to ignore errors based on the usecase.
I do see a test here which performs a reboot and expects error to be nil -> https://github.com/openconfig/featureprofiles/blob/main/feature/gnoi/system/tests/chassis_reboot_status_and_cancel_test/chassis_reboot_status_and_cancel_test.go#L168
There was a problem hiding this comment.
Yeah, I see the pros and cons. Three comments:
First, It just seems to me that the likelihood that we would see an unexpected Unavailable error it is very low, especially given that the prior RebootRequest must have succeeded by then. Other errors would obviously still be reported.
Second, almost all uses of WaitForActive(true) is going to want to be accompanied by IgnoreUnavailableErr(true), because nearly all will be rebooting the chassis, and if not that the management linecard. So I feel like IgnoreUnavailableErr(true) is going to be one of these lines people just copy and paste everywhere anyway, regardless of whether they need it, because that's what most calls look like.
Third, I'm holding out some hope that we can extend the RebootRequestResponse to make IgnoreUnavailableErr eventually unnecessary. I'm going to be starting a thread on the OpenConfig chat about that.
Due to those three, I was feeling like IgnoreUnavailableErr maybe isn't offering enough to pull its weight. All that said, I still see the reasons for including it, so I'm fine with you leaving it in if you prefer to keep it.
| } | ||
|
|
||
| // IgnoreUnavailableErr ignores unavailable errors returned by reboot status. | ||
| func (r *RebootOperation) IgnoreUnavailableErr(ignoreUnavailableErr bool) *RebootOperation { |
There was a problem hiding this comment.
Yeah, I see the pros and cons. Three comments:
First, It just seems to me that the likelihood that we would see an unexpected Unavailable error it is very low, especially given that the prior RebootRequest must have succeeded by then. Other errors would obviously still be reported.
Second, almost all uses of WaitForActive(true) is going to want to be accompanied by IgnoreUnavailableErr(true), because nearly all will be rebooting the chassis, and if not that the management linecard. So I feel like IgnoreUnavailableErr(true) is going to be one of these lines people just copy and paste everywhere anyway, regardless of whether they need it, because that's what most calls look like.
Third, I'm holding out some hope that we can extend the RebootRequestResponse to make IgnoreUnavailableErr eventually unnecessary. I'm going to be starting a thread on the OpenConfig chat about that.
Due to those three, I was feeling like IgnoreUnavailableErr maybe isn't offering enough to pull its weight. All that said, I still see the reasons for including it, so I'm fine with you leaving it in if you prefer to keep it.
PR for the Reboot Operation