This repository was archived by the owner on Jan 23, 2026. It is now read-only.
Merged
Conversation
Otherwise, in labs where ntp is not available for the DUTs to use, time setting will be wrong and fls, or CA cert checking will fail.
✅ Deploy Preview for jumpstarter-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
Contributor
WalkthroughRemote time synchronization in the flasher client was relocated from the SSL setup phase to occur after network configuration during the flash operation, ensuring the DUT time matches the local system before flashing begins. Changes
Sequence DiagramsequenceDiagram
participant Local as Local System
participant Remote as Remote DUT
rect rgb(220, 240, 255)
Note over Local,Remote: Old flow: time sync during SSL setup
Local->>Remote: _setup_flasher_ssl()
Remote-->>Local: (time sync occurred here)
end
rect rgb(240, 255, 240)
Note over Local,Remote: New flow: time sync after network setup
Local->>Remote: _perform_flash_operation()
Local->>Remote: udhcpc (network setup)
Remote-->>Local: network ready
Local->>Remote: date -s @{timestamp}
Remote-->>Local: time synced
Local->>Remote: wait for login prompt
Remote-->>Local: login prompt received
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
bennyz
approved these changes
Nov 11, 2025
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py(1 hunks)
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-11-05T13:31:39.304Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 735
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:100-101
Timestamp: 2025-11-05T13:31:39.304Z
Learning: In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py, the `flash()` method and its CLI command intentionally have different defaults for `fls_version`: the method defaults to `""` (empty string) while the CLI defaults to `"0.1.5"`. This is by design to provide CLI users with a convenient default while keeping the programmatic API ready for when FLS is included in the flasher images.
Applied to files:
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
📚 Learning: 2025-11-05T13:33:24.741Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 735
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:476-486
Timestamp: 2025-11-05T13:33:24.741Z
Learning: In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py, the FLS flasher download in _flash_with_fls() hardcodes "aarch64-linux" architecture because the flasher always targets ARM devices currently.
Applied to files:
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
📚 Learning: 2025-11-05T13:45:58.271Z
Learnt from: mangelajo
Repo: jumpstarter-dev/jumpstarter PR: 735
File: packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py:15-15
Timestamp: 2025-11-05T13:45:58.271Z
Learning: In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py, pexpect is intentionally used as a transitive dependency through the jumpstarter-driver-pyserial package. The flashers package does not declare pexpect as a direct dependency because the pyserial driver package is intended to control the pexpect version.
Applied to files:
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
🧬 Code graph analysis (1)
packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py (1)
examples/soc-pytest/jumpstarter_example_soc_pytest/test_on_rpi4.py (1)
console(20-24)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (11)
- GitHub Check: Redirect rules - jumpstarter-docs
- GitHub Check: Header rules - jumpstarter-docs
- GitHub Check: Pages changed - jumpstarter-docs
- GitHub Check: build
- GitHub Check: pytest-matrix (macos-15, 3.11)
- GitHub Check: pytest-matrix (macos-15, 3.13)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.13)
- GitHub Check: pytest-matrix (macos-15, 3.12)
- GitHub Check: pytest-matrix (ubuntu-24.04, 3.11)
- GitHub Check: e2e
Comment on lines
+332
to
+337
| # make sure that the remote system has the right time without using NTP | ||
| # otherwise SSL certificate verification will fail | ||
| self.logger.info("Setting the remote DUT time to match the local system time") | ||
| current_timestamp = int(time.time()) | ||
| console.sendline(f"date -s @{current_timestamp}") | ||
| console.expect(manifest.spec.login.prompt, timeout=EXPECT_TIMEOUT_DEFAULT) |
Contributor
There was a problem hiding this comment.
Add error handling to make time synchronization retryable.
The time synchronization logic correctly addresses the TLS certificate verification issue in labs without NTP. However, unlike the udhcpc block above (lines 322-330), this code lacks error handling. If console.expect() times out or another exception occurs, it won't be wrapped in FlashRetryableError, preventing the retry logic from working as intended.
Apply this diff to add consistent error handling:
except Exception as e:
self.logger.error(f"Error running udhcpc: {e}")
raise FlashRetryableError(f"Error running udhcpc: {e}") from e
- # make sure that the remote system has the right time without using NTP
- # otherwise SSL certificate verification will fail
- self.logger.info("Setting the remote DUT time to match the local system time")
- current_timestamp = int(time.time())
- console.sendline(f"date -s @{current_timestamp}")
- console.expect(manifest.spec.login.prompt, timeout=EXPECT_TIMEOUT_DEFAULT)
+ # make sure that the remote system has the right time without using NTP
+ # otherwise SSL certificate verification will fail
+ try:
+ self.logger.info("Setting the remote DUT time to match the local system time")
+ current_timestamp = int(time.time())
+ console.sendline(f"date -s @{current_timestamp}")
+ console.expect(manifest.spec.login.prompt, timeout=EXPECT_TIMEOUT_DEFAULT)
+ except pexpect.TIMEOUT as e:
+ self.logger.error(f"Timeout waiting for date command to complete: {e}")
+ raise FlashRetryableError("Timeout waiting for date command to complete") from e
+ except Exception as e:
+ self.logger.error(f"Error setting remote time: {e}")
+ raise FlashRetryableError(f"Error setting remote time: {e}") from e📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| # make sure that the remote system has the right time without using NTP | |
| # otherwise SSL certificate verification will fail | |
| self.logger.info("Setting the remote DUT time to match the local system time") | |
| current_timestamp = int(time.time()) | |
| console.sendline(f"date -s @{current_timestamp}") | |
| console.expect(manifest.spec.login.prompt, timeout=EXPECT_TIMEOUT_DEFAULT) | |
| # make sure that the remote system has the right time without using NTP | |
| # otherwise SSL certificate verification will fail | |
| try: | |
| self.logger.info("Setting the remote DUT time to match the local system time") | |
| current_timestamp = int(time.time()) | |
| console.sendline(f"date -s @{current_timestamp}") | |
| console.expect(manifest.spec.login.prompt, timeout=EXPECT_TIMEOUT_DEFAULT) | |
| except pexpect.TIMEOUT as e: | |
| self.logger.error(f"Timeout waiting for date command to complete: {e}") | |
| raise FlashRetryableError("Timeout waiting for date command to complete") from e | |
| except Exception as e: | |
| self.logger.error(f"Error setting remote time: {e}") | |
| raise FlashRetryableError(f"Error setting remote time: {e}") from e |
🤖 Prompt for AI Agents
In packages/jumpstarter-driver-flashers/jumpstarter_driver_flashers/client.py
around lines 332 to 337, the time sync block lacks error handling so exceptions
from console.expect() won't be treated as retryable; wrap the date command and
console.expect call in a try/except, catch Exception, log the exception with
context via self.logger.warning or self.logger.exception, and raise
FlashRetryableError(...) (same pattern and message style used in the udhcpc
block above) so the caller can retry the operation.
|
Successfully created backport PR for |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Otherwise, in labs where ntp is not available for the DUTs to use, time setting will be wrong and fls, or CA cert checking will fail.
Summary by CodeRabbit