cros/recovery: Support RPM power cycling in PASIT [infra/infra : main]
Mark Yacoub (Gerrit)
Mark Yacoub added 1 comment![]( "Open in Gerrit")
is this so i can reset the UCD on repair?
Related details
- Alexandr Astrouski
- Jason Stanko
- 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. |
Jason Stanko (Gerrit)
Jason Stanko added 1 comment![]( "Open in Gerrit")
is this so i can reset the UCD on repair?
Yes it will:
1. Shut off UCD via RPM
2. Reboot DUT
3. Plug UCD back in via RPM
Related details
- Alexandr Astrouski
- Mark Yacoub
- 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. |
Mark Yacoub (Gerrit)
Mark Yacoub added 2 comments![]( "Open in Gerrit")
initCtx, cancel := context.WithTimeout(ctx, 3*time.Second)A 3-second timeout for establishing all host proxies might be a bit short in environments with high latency or many resources. Was this value chosen based on a specific requirement?
scheme = "http"Why is the scheme changed to HTTP when a proxy is used?
If the proxy is a standard SSH tunnel (TCP forwarding), it will pass the data as-is. If the RPM unit expects HTTPS (as the original code implied by hardcoding https), switching to http here will cause the connection to fail unless the proxy is specifically terminating SSL/TLS.
Jason Stanko (Gerrit)
Jason Stanko added 2 comments![]( "Open in Gerrit")
initCtx, cancel := context.WithTimeout(ctx, 3*time.Second)A 3-second timeout for establishing all host proxies might be a bit short in environments with high latency or many resources. Was this value chosen based on a specific requirement?
This is just needed because the connection is optional when running locally. Without setting a timeout it will hang for like 10 minutes until the context expires. This only initializes the local TLW client struct and attempts a single gRPC connection (it doesn't open SSH host tunnels/proxies). I bumped it up to 10 seconds though.
scheme = "http"Why is the scheme changed to HTTP when a proxy is used?
If the proxy is a standard SSH tunnel (TCP forwarding), it will pass the data as-is. If the RPM unit expects HTTPS (as the original code implied by hardcoding https), switching to http here will cause the connection to fail unless the proxy is specifically terminating SSL/TLS.
IIUC IPPower doesn't actually use HTTPS directly when we tunnel to it via port80, we only need HTTPS when we the infra talks to it since it has to go through a HTTPS gateway.
Otabek Kasimov (Gerrit)
Otabek Kasimov added 3 comments![]( "Open in Gerrit")
initCtx, cancel := context.WithTimeout(ctx, 3*time.Second)Jason StankoA 3-second timeout for establishing all host proxies might be a bit short in environments with high latency or many resources. Was this value chosen based on a specific requirement?
This is just needed because the connection is optional when running locally. Without setting a timeout it will hang for like 10 minutes until the context expires. This only initializes the local TLW client struct and attempts a single gRPC connection (it doesn't open SSH host tunnels/proxies). I bumped it up to 10 seconds though.
why do we set timeout for the context here?
func rpmActionPasit(ctx context.Context, info execs.ExecInfo) error {can we reuse rpmPowerOnDeviceExec/rpmPowerOffDeviceExec and rpmPowerCycleDeviceExec instead of this function? i feel we need adjust the device type to include pasit devices.
Pasit_Device_VIDEO_TESTER Pasit_Device_Type = 13please extract it as separate cl.
Related details
- Alexandr Astrouski
- Jason Stanko
- Mark Yacoub
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jason Stanko (Gerrit)
Jason Stanko added 3 comments![]( "Open in Gerrit")
initCtx, cancel := context.WithTimeout(ctx, 3*time.Second)Jason StankoA 3-second timeout for establishing all host proxies might be a bit short in environments with high latency or many resources. Was this value chosen based on a specific requirement?
Otabek KasimovThis is just needed because the connection is optional when running locally. Without setting a timeout it will hang for like 10 minutes until the context expires. This only initializes the local TLW client struct and attempts a single gRPC connection (it doesn't open SSH host tunnels/proxies). I bumped it up to 10 seconds though.
why do we set timeout for the context here?
Without a timeout, local CLI runs (paris local) hang indefinitely waiting for connection timeout if the lab service isn't reachable. Setting a 10s timeout fails fast so local recovery can fall back to local SSH proxies.
func rpmActionPasit(ctx context.Context, info execs.ExecInfo) error {can we reuse rpmPowerOnDeviceExec/rpmPowerOffDeviceExec and rpmPowerCycleDeviceExec instead of this function? i feel we need adjust the device type to include pasit devices.
`deviceHostnameAndRPMOutlet` is set up to return a single RPM outlet for a single device (like dut or chameleon).
Because PASIT is a topology of multiple peripherals (docks, USB hubs, switches), we need to power cycle all of their RPM outlets together during repair. We would need to refactor `deviceHostnameAndRPMOutlet ` to return a list rather than a single item.
please extract it as separate cl.
Done, moved to: chromium-review.googlesource.com/c/infra/infra/+/8134286
Related details
- Alexandr Astrouski
- Mark Yacoub
- 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. |
Otabek Kasimov (Gerrit)
Otabek Kasimov added 6 comments![]( "Open in Gerrit")
please separate it as execs better to change sepaartely from cli changes
initCtx, cancel := context.WithTimeout(ctx, 3*time.Second)Jason StankoA 3-second timeout for establishing all host proxies might be a bit short in environments with high latency or many resources. Was this value chosen based on a specific requirement?
Otabek KasimovThis is just needed because the connection is optional when running locally. Without setting a timeout it will hang for like 10 minutes until the context expires. This only initializes the local TLW client struct and attempts a single gRPC connection (it doesn't open SSH host tunnels/proxies). I bumped it up to 10 seconds though.
Jason Stankowhy do we set timeout for the context here?
Without a timeout, local CLI runs (paris local) hang indefinitely waiting for connection timeout if the lab service isn't reachable. Setting a 10s timeout fails fast so local recovery can fall back to local SSH proxies.
Acknowledged. Please add comments to clarify the implementation and move this logic into a separate function to ensure the context is properly created and destroyed, avoiding potential misuse in the future.
func rpmActionPasit(ctx context.Context, info execs.ExecInfo) error {Jason Stankocan we reuse rpmPowerOnDeviceExec/rpmPowerOffDeviceExec and rpmPowerCycleDeviceExec instead of this function? i feel we need adjust the device type to include pasit devices.
`deviceHostnameAndRPMOutlet` is set up to return a single RPM outlet for a single device (like dut or chameleon).
Because PASIT is a topology of multiple peripherals (docks, USB hubs, switches), we need to power cycle all of their RPM outlets together during repair. We would need to refactor `deviceHostnameAndRPMOutlet ` to return a list rather than a single item.
I approve of this change, provided that we maintain the current variety of execs. I am concerned that reducing them now could lead to further questions and issues in the future. please update if possible. it can be series of changes
1) change single to slice of rpms
2) refactor rpm_execs.go to add pasit option
scheme = "http"Jason StankoWhy is the scheme changed to HTTP when a proxy is used?
If the proxy is a standard SSH tunnel (TCP forwarding), it will pass the data as-is. If the RPM unit expects HTTPS (as the original code implied by hardcoding https), switching to http here will cause the connection to fail unless the proxy is specifically terminating SSL/TLS.
IIUC IPPower doesn't actually use HTTPS directly when we tunnel to it via port80, we only need HTTPS when we the infra talks to it since it has to go through a HTTPS gateway.
i think if we do it only for local then we can use dev.IsActive(ctx) which is true when run with paris locally.
if localproxy.IsProxyAddr(r.BaseRequest.PowerUnitHostname) {
scheme = "http"
}same here
if args.DevHostProxyAddresses != nil {this covered by condition already.
why we need it here if we have `collectResourcesForPlan(planName, execArgs.DUT)` below
Related details
- Alexandr Astrouski
- Jason Stanko
- Mark Yacoub
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Otabek Kasimov (Gerrit)
Otabek Kasimov added 1 comment![]( "Open in Gerrit")
These changes seem problematic, as the hosts were expected to be reachable via a jump host but are currently unreachable. This is triggering numerous host connection errors due to the specific local configuration.
Jason Stanko (Gerrit)
Jason Stanko added 8 comments![]( "Open in Gerrit")
Jason Stankois this so i can reset the UCD on repair?
Yes it will:
1. Shut off UCD via RPM
2. Reboot DUT
3. Plug UCD back in via RPM
Confirmed offline in chat with mark
These changes seem problematic, as the hosts were expected to be reachable via a jump host but are currently unreachable. This is triggering numerous host connection errors due to the specific local configuration.
This is triggering numerous host connection errors due to the specific local configuration.
How did you test that so I can reproduce it?
I'm not quite sure how this would impact jump host behavior since the change should only apply when passing in the proxy cli arg.
please separate it as execs better to change sepaartely from cli changes
initCtx, cancel := context.WithTimeout(ctx, 3*time.Second)Jason StankoA 3-second timeout for establishing all host proxies might be a bit short in environments with high latency or many resources. Was this value chosen based on a specific requirement?
Otabek KasimovThis is just needed because the connection is optional when running locally. Without setting a timeout it will hang for like 10 minutes until the context expires. This only initializes the local TLW client struct and attempts a single gRPC connection (it doesn't open SSH host tunnels/proxies). I bumped it up to 10 seconds though.
Jason Stankowhy do we set timeout for the context here?
Otabek KasimovWithout a timeout, local CLI runs (paris local) hang indefinitely waiting for connection timeout if the lab service isn't reachable. Setting a 10s timeout fails fast so local recovery can fall back to local SSH proxies.
Acknowledged. Please add comments to clarify the implementation and move this logic into a separate function to ensure the context is properly created and destroyed, avoiding potential misuse in the future.
Acknowledged. Code moved to: https://chromium-review.git.corp.google.com/c/infra/infra/+/8134740/1
func rpmActionPasit(ctx context.Context, info execs.ExecInfo) error {Jason Stankocan we reuse rpmPowerOnDeviceExec/rpmPowerOffDeviceExec and rpmPowerCycleDeviceExec instead of this function? i feel we need adjust the device type to include pasit devices.
Otabek Kasimov`deviceHostnameAndRPMOutlet` is set up to return a single RPM outlet for a single device (like dut or chameleon).
Because PASIT is a topology of multiple peripherals (docks, USB hubs, switches), we need to power cycle all of their RPM outlets together during repair. We would need to refactor `deviceHostnameAndRPMOutlet ` to return a list rather than a single item.
I approve of this change, provided that we maintain the current variety of execs. I am concerned that reducing them now could lead to further questions and issues in the future. please update if possible. it can be series of changes
1) change single to slice of rpms
2) refactor rpm_execs.go to add pasit option
Done
scheme = "http"Jason StankoWhy is the scheme changed to HTTP when a proxy is used?
If the proxy is a standard SSH tunnel (TCP forwarding), it will pass the data as-is. If the RPM unit expects HTTPS (as the original code implied by hardcoding https), switching to http here will cause the connection to fail unless the proxy is specifically terminating SSL/TLS.
Otabek KasimovIIUC IPPower doesn't actually use HTTPS directly when we tunnel to it via port80, we only need HTTPS when we the infra talks to it since it has to go through a HTTPS gateway.
i think if we do it only for local then we can use dev.IsActive(ctx) which is true when run with paris locally.
Done
if localproxy.IsProxyAddr(r.BaseRequest.PowerUnitHostname) {
scheme = "http"
}Jason Stankosame here
Done
if args.DevHostProxyAddresses != nil {this covered by condition already.
why we need it here if we have `collectResourcesForPlan(planName, execArgs.DUT)` below
collectResourcesForPlan only grabs main testbed devices (DUT, Servo, etc.). It doesn't include RPM or PDU IP addresses. Registering proxies upfront is the only way local RPM SSH tunnels get added to localproxy.
Related details
- Alexandr Astrouski
- Mark Yacoub
- 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. |
Jason Stanko (Gerrit)
Jason Stanko added 2 comments![]( "Open in Gerrit")
These changes seem problematic, as the hosts were expected to be reachable via a jump host but are currently unreachable. This is triggering numerous host connection errors due to the specific local configuration.
This is triggering numerous host connection errors due to the specific local configuration.
How did you test that so I can reproduce it?
I'm not quite sure how this would impact jump host behavior since the change should only apply when passing in the proxy cli arg.
Moved all this code to: https://chromium-review.git.corp.google.com/c/infra/infra/+/8135044
can follow up there.
if args.DevHostProxyAddresses != nil {Jason Stankothis covered by condition already.
why we need it here if we have `collectResourcesForPlan(planName, execArgs.DUT)` below
collectResourcesForPlan only grabs main testbed devices (DUT, Servo, etc.). It doesn't include RPM or PDU IP addresses. Registering proxies upfront is the only way local RPM SSH tunnels get added to localproxy.
Moved all proxy code to: https://chromium-review.git.corp.google.com/c/infra/infra/+/8135044
can follow up there.
Related details
- Alexandr Astrouski
- Mark Yacoub
- Otabek Kasimov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Otabek Kasimov (Gerrit)
Otabek Kasimov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Otabek Kasimov
| Code-Review | +1 |
1 comment
LGTM % please add test for new section.
Related details
- Alexandr Astrouski
- Jason Stanko
- Mark Yacoub
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jason Stanko (Gerrit)
Jason Stanko added 1 comment![]( "Open in Gerrit")
LGTM % please add test for new section.
- Alexandr Astrouski
- Mark Yacoub
- Otabek Kasimov
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Otabek Kasimov (Gerrit)
Otabek Kasimov voted and added 1 comment![]( "Open in Gerrit")
Votes added by Otabek Kasimov
| Code-Review | +1 |
1 comment
case "pasit", "pasit_peripherals":q: do we need both of them? why not a single key?
Related details
- Alexandr Astrouski
- Jason Stanko
- Mark Yacoub
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jason Stanko (Gerrit)
Jason Stanko added 1 comment![]( "Open in Gerrit")
q: do we need both of them? why not a single key?
- Alexandr Astrouski
- Mark Yacoub
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Otabek Kasimov (Gerrit)
Otabek Kasimov voted Code-Review+1![]( "Open in Gerrit")
- Alexandr Astrouski
- Jason Stanko
- Mark Yacoub
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jason Stanko (Gerrit)
Jason Stanko voted Commit-Queue+2![]( "Open in Gerrit")
- Alexandr Astrouski
- Mark Yacoub
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Jason Stanko (Gerrit)
Jason Stanko voted Commit-Queue+2![]( "Open in Gerrit")
| Commit-Queue | +2 |
Related details
- Alexandr Astrouski
- Mark Yacoub
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
LUCI CQ (Gerrit)
LUCI CQ submitted the change![]( "Open in Gerrit")
Unreviewed changes
30 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
Change information
cros/recovery: Support RPM power cycling in PASIT
Add support for resolution of PASIT peripheral RPM targets in getRPMTargets.
BUG=b:536041360
- M go/src/infra/cros/recovery/internal/execs/rpm/rpm_execs.go
- M go/src/infra/cros/recovery/internal/execs/rpm/rpm_execs_test.go
Code-Review: +1 by Otabek Kasimov
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |