Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(muxpi): provisioning: skip user creation, boot check via URL #214

Merged
merged 1 commit into from
Mar 8, 2024

Conversation

thp-canonical
Copy link
Contributor

@thp-canonical thp-canonical commented Mar 1, 2024

Description

  1. Allow skipping of user creation when deploying an image (for pre-configured images).
  2. Allow a custom check URL to allow for testing services running in images.

Documentation

Documentation with the newly-added keys is added. As part of this, the recognized image types (for user creation) are also documented.

Tests

Untested proposal / request for comment.

Copy link
Collaborator

@plars plars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this looks pretty good and thanks a lot for also adding documentation around it! I have a couple of review comments below, but more generally, I was wondering what sort of command you would need to specify in order to determine that the device booted? especially since you can't easily template the IP of the dut as part of it.
I'm wondering if we should just handle this as a special image type and check whether it booted in a static way, because we don't normally allow injection of commands like this during the provision process, and I think this could be fraught with problems, and potential for allowing someone to inadvertently render the device unusable until someone fixes it.

docs/reference/device-connector-types.rst Outdated Show resolved Hide resolved
@thp-canonical
Copy link
Contributor Author

Thanks for your review. PR updated, responses to your questions inline below.

I was wondering what sort of command you would need to specify in order to determine that the device booted?

Any command that e.g. checks if a HTTP endpoint is available:

curl --connect-timeout 2 "http://$DEVICE_IP:1234/foo/bar"

especially since you can't easily template the IP of the dut as part of it.

That has now been fixed, I did have env.update() in there that exposes $DEVICE_IP, but I forgot shell=True in the subprocess.check_output() call, this should now work.

I'm wondering if we should just handle this as a special image type and check whether it booted in a static way, because we don't normally allow injection of commands like this during the provision process, and I think this could be fraught with problems, and potential for allowing someone to inadvertently render the device unusable until someone fixes it.

One thing that could work is instead of allowing a generic boot_check_cmd, we just allow a boot_check_tcp_port (say), and if set, it will consider the device booted once a TCP connection to the destination port can be established (or boot_check_http_port, which will consider a successful GET / HTTP/1.1 request on that port to mean the device is online). WDYT?

Copy link
Collaborator

@plars plars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like your idea of replacing boot_check_cmd with boot_check_tcp_port or boot_check_http_port, however I think for the latter, it would suffice to just call it boot_check_http and then give it the URL with the port if needed.

Also, you probably saw, but it also looks like the auto-spell-check got tripped up on "xz" of all things.

@thp-canonical
Copy link
Contributor Author

I like your idea of replacing boot_check_cmd with boot_check_tcp_port or boot_check_http_port, however I think for the latter, it would suffice to just call it boot_check_http and then give it the URL with the port if needed.

Went for boot_check_url (as it can be anything that's supported by urlib.request.urlopen()), but can change to boot_check_http if that sounds better to you. I reverted all the DEFAULT_BOOT_CHECK_CMD changes back to how it was initially to not unnecessarily touch existing code.

I opted to use $DEVICE_IP instead of any other kind of templating/replacement to be consistent with the usage of $DEVICE_IP in test_cmds, even though in test_cmds, it really is a proper environment variable, whereas here we just replace a literal $DEVICE_IP with the value (so e.g. ${DEVICE_IP} wouldn't work here, even though it would work fine in test_cmds), this is hopefully made clear enough in the documentation:

a literal $DEVICE_IP in the URL will be replaced with the IP address of the DUT.

Also, you probably saw, but it also looks like the auto-spell-check got tripped up on "xz" of all things.

Done.

@thp-canonical thp-canonical requested a review from plars March 7, 2024 14:20
@plars
Copy link
Collaborator

plars commented Mar 7, 2024

/canonical/self-hosted-runners/run-workflows 552e8b3

Copy link
Collaborator

@plars plars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like it's just failing the black formatting check. Would you mind re-running black on it and pushing it?

@thp-canonical
Copy link
Contributor Author

It looks like it's just failing the black formatting check. Would you mind re-running black on it and pushing it?

Done. I also noticed that my PR fixes weren't signed commits, so I squashed all into a single signed commit.

@thp-canonical thp-canonical changed the title feat(muxpi): provisioning: skip user creation, custom boot check cmd feat(muxpi): provisioning: skip user creation, boot check via URL Mar 8, 2024
@plars
Copy link
Collaborator

plars commented Mar 8, 2024

/canonical/self-hosted-runners/run-workflows b8a0e20'

Copy link
Collaborator

@plars plars left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 thanks!

@plars plars merged commit ce996d1 into canonical:main Mar 8, 2024
5 checks passed
@thp-canonical thp-canonical deleted the muxpi-custom branch April 24, 2024 06:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants