recovery: Add retries and increase timeout for SSH client creation [infra/infra : main]
Alexandr Astrouski (Gerrit)
Alexandr Astrouski added 2 comments![]( "Open in Gerrit")
Timeout: 5 * time.Second,This wouldn't be approved, need to modify that value somewhere closer to wifi/bt preipherals code.
for curAttempt := range 3 {As you mention in proposal, pls make that 3 as sshConfig option, so it could be provided, and for wifi/bt peripherals set that value to 3 and keep default to 1.
Related details
- Bryan Ni
- Chen-Jie Tsai (xWF)
- Otabek Kasimov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexandr Astrouski (Gerrit)
Alexandr Astrouski added 1 comment![]( "Open in Gerrit")
if curAttempt < 2 {here change 2 to be maxRetries - 1
Alexandr Astrouski (Gerrit)
Alexandr Astrouski added 1 comment![]( "Open in Gerrit")
To clarify that CL.
There is a comment https://buganizer.corp.google.com/issues/524118960#comment8 which shows that adding clientPool was a very good change, but we still hit that issue sometimes, so adding some retries or longer delay should completely eliminate that issue from repair jobs.
Otabek Kasimov (Gerrit)
Otabek Kasimov added 3 comments![]( "Open in Gerrit")
please address all Alexandr comments first.
Timeout: 5 * time.Second,This wouldn't be approved, need to modify that value somewhere closer to wifi/bt preipherals code.
+1 do not change it. if we need then we have issue in the lab.
for curAttempt := range 3 {As you mention in proposal, pls make that 3 as sshConfig option, so it could be provided, and for wifi/bt peripherals set that value to 3 and keep default to 1.
why we changing the core here? is there issue in the lab with client creation?
Related details
- Bryan Ni
- Chen-Jie Tsai (xWF)
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexandr Astrouski (Gerrit)
Alexandr Astrouski added 1 comment![]( "Open in Gerrit")
for curAttempt := range 3 {Otabek KasimovAs you mention in proposal, pls make that 3 as sshConfig option, so it could be provided, and for wifi/bt peripherals set that value to 3 and keep default to 1.
why we changing the core here? is there issue in the lab with client creation?
I share details on why we would like to change current approach a bit. We still hit client creation from time to time (much lower rate then it was after clientPool added). Repair jobs shows that it may timeout to create client on first call, but after 2 sec it was managed to create a client, that's why it would be nice to retry couple times. To affect only wifi/bt peripherals we suggest to add that option in sshConfiguration, so we can easily control that.
Otabek Kasimov (Gerrit)
Otabek Kasimov added 1 comment![]( "Open in Gerrit")
for curAttempt := range 3 {Otabek KasimovAs you mention in proposal, pls make that 3 as sshConfig option, so it could be provided, and for wifi/bt peripherals set that value to 3 and keep default to 1.
Alexandr Astrouskiwhy we changing the core here? is there issue in the lab with client creation?
I share details on why we would like to change current approach a bit. We still hit client creation from time to time (much lower rate then it was after clientPool added). Repair jobs shows that it may timeout to create client on first call, but after 2 sec it was managed to create a client, that's why it would be nice to retry couple times. To affect only wifi/bt peripherals we suggest to add that option in sshConfiguration, so we can easily control that.
I recommend limiting the retries to exactly one to avoid excessive attempts. Please refer to the sshpool implementation within the infra codebase for a cleaner structural approach. Let me know if you would prefer that I adjust this implementation myself.
Alexandr Astrouski (Gerrit)
Alexandr Astrouski added 1 comment![]( "Open in Gerrit")
for curAttempt := range 3 {Otabek KasimovAs you mention in proposal, pls make that 3 as sshConfig option, so it could be provided, and for wifi/bt peripherals set that value to 3 and keep default to 1.
Alexandr Astrouskiwhy we changing the core here? is there issue in the lab with client creation?
Otabek KasimovI share details on why we would like to change current approach a bit. We still hit client creation from time to time (much lower rate then it was after clientPool added). Repair jobs shows that it may timeout to create client on first call, but after 2 sec it was managed to create a client, that's why it would be nice to retry couple times. To affect only wifi/bt peripherals we suggest to add that option in sshConfiguration, so we can easily control that.
I recommend limiting the retries to exactly one to avoid excessive attempts. Please refer to the sshpool implementation within the infra codebase for a cleaner structural approach. Let me know if you would prefer that I adjust this implementation myself.
There is already a ssh with retries for proxies, why can't we have similar for regular clients? https://source.corp.google.com/h/chromium/infra/infra_superproject/+/main:infra/go/src/infra/cros/recovery/internal/localtlw/ssh/ssh_client.go;l=175
Otabek Kasimov (Gerrit)
Otabek Kasimov added 1 comment![]( "Open in Gerrit")
for curAttempt := range 3 {Otabek KasimovAs you mention in proposal, pls make that 3 as sshConfig option, so it could be provided, and for wifi/bt peripherals set that value to 3 and keep default to 1.
Alexandr Astrouskiwhy we changing the core here? is there issue in the lab with client creation?
Otabek KasimovI share details on why we would like to change current approach a bit. We still hit client creation from time to time (much lower rate then it was after clientPool added). Repair jobs shows that it may timeout to create client on first call, but after 2 sec it was managed to create a client, that's why it would be nice to retry couple times. To affect only wifi/bt peripherals we suggest to add that option in sshConfiguration, so we can easily control that.
Alexandr AstrouskiI recommend limiting the retries to exactly one to avoid excessive attempts. Please refer to the sshpool implementation within the infra codebase for a cleaner structural approach. Let me know if you would prefer that I adjust this implementation myself.
There is already a ssh with retries for proxies, why can't we have similar for regular clients? https://source.corp.google.com/h/chromium/infra/infra_superproject/+/main:infra/go/src/infra/cros/recovery/internal/localtlw/ssh/ssh_client.go;l=175
i am not telling we cannot not, just need to make it more explicit and clear to the reader.
Chen-Jie Tsai (xWF) (Gerrit)
Chen-Jie Tsai (xWF) added 2 comments![]( "Open in Gerrit")
Timeout: 5 * time.Second,Otabek KasimovThis wouldn't be approved, need to modify that value somewhere closer to wifi/bt preipherals code.
+1 do not change it. if we need then we have issue in the lab.
Updated, the timeout will be extend to 5 on router/pcap/btpeer only
for curAttempt := range 3 {Otabek KasimovAs you mention in proposal, pls make that 3 as sshConfig option, so it could be provided, and for wifi/bt peripherals set that value to 3 and keep default to 1.
Alexandr Astrouskiwhy we changing the core here? is there issue in the lab with client creation?
Otabek KasimovI share details on why we would like to change current approach a bit. We still hit client creation from time to time (much lower rate then it was after clientPool added). Repair jobs shows that it may timeout to create client on first call, but after 2 sec it was managed to create a client, that's why it would be nice to retry couple times. To affect only wifi/bt peripherals we suggest to add that option in sshConfiguration, so we can easily control that.
Alexandr AstrouskiI recommend limiting the retries to exactly one to avoid excessive attempts. Please refer to the sshpool implementation within the infra codebase for a cleaner structural approach. Let me know if you would prefer that I adjust this implementation myself.
Otabek KasimovThere is already a ssh with retries for proxies, why can't we have similar for regular clients? https://source.corp.google.com/h/chromium/infra/infra_superproject/+/main:infra/go/src/infra/cros/recovery/internal/localtlw/ssh/ssh_client.go;l=175
i am not telling we cannot not, just need to make it more explicit and clear to the reader.
- Alexandr Astrouski
- Bryan Ni
- Otabek Kasimov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Alexandr Astrouski (Gerrit)
Alexandr Astrouski added 1 comment![]( "Open in Gerrit")
peripheralSSHConfig := `this wouldn't be approved, pls update Config creation somewhere closer too wifi/btpeers execs.
Related details
- Bryan Ni
- Chen-Jie Tsai (xWF)
- Otabek Kasimov
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |