iotedge check update
#4153
iotedge check update
#4153
Conversation
in the upcoming commits, i'll add the code to shell-out to `aziot check`, which will populate the `iothub_hostname` variable (which is currently set to None).
it works, but it could really do with some cleanup. notably, it currently duplicates all the wire-protocol types from `aziot`, whereas it really aught to use a shared library to keep things consistent.
the aziot PR has to land before updating the submodule.
Adds a new `aziot check` subcommand to `aziot`, modeled after `iotedge check`. Should be reviewed in conjunction with the upstream PR at Azure/iotedge#4153 Please note that the `aziot check` implementation is _not_ a straight copy-paste from `iotedge check`, and while the two implementations have a lot in common, `aziot check`'s underlying check framework has been substantially refactored, simplified, and modernized. Notable changes include the use of `async_trait` on the `Check` trait, the use of `serde_erased` to simplify checker serialization, and removing all the Windows console specific text formatting code. While the internals of `aziot check` are quite different from `iotedge check`, I've made sure that the user-facing `text` and `json` output formats have remained exactly the same between the two tools. One notable addition to `aziot check` is the new `json-stream` output format, which will enable `iotedge check` to invoke `iotedge check` as a sub-process. Aside from streamlining check flow (users won't have to invoke both `check` commands separately), the `additional_info` payload will be used to share certain bits of provisioning-specific information with `iotedge` that it wouldn't otherwise have access to (e.g: the `iothub_hostname` configuration option used in the host connectivity checks in `iotedge check` is part of the `identityd` config, which `iotedge` should _not_ be able to access directly). * * * This PR should be reviewed along two axes: 1. The `aziot check` subcommand, framework, and implementation quality 2. The actual checks themselves The checks are divided into two categories: 1. Checks ported over from `iotedge check` (due to certain functionality being moved down the stack into `aziot`) - `host_connect_dps_endpoint` - `host_local_time` - `hostname` - `identity_certificate_expiry` - `well_formed_configs` 2. New checks specific to the new `aziot` architecture - `certs_preloaded` - `dameons_running` While the primary goal of this PR is to get `aziot check` "up and running" in an effort to fix `iotedge check`, if there are any checks that I may have missed that are "critical" to merge as part of this initial PR, please let me know. * * * This PR will be un-drafted once the corresponding `iotedge check` PR has been submitted, as there's a non-zero chance the `json-streaming` output format will require changes as part of the integration work.
edgelet/iotedge/src/check/mod.rs
Outdated
| { | ||
| let aziot_check_out = std::process::Command::new(aziot_bin) | ||
| .arg("check-list") | ||
| .arg("-j") |
There was a problem hiding this comment.
check-list ought to be changed to be consistent with check and use --output json instead of --json
There was a problem hiding this comment.
Fair enough, though given that this isn't a flag that users will ever be expected to use directly, I don't think this bit of inconsistency is too big of a deal. Plus, it'll be a bit of a PITA to update aziot check now that the first PR has already been merged 😉
There was a problem hiding this comment.
Sooooo I should make the change then 😄
|
|
|
Hold off on any submodule update before merge because I might have exposed a bug in the module runtime with the latest aziot version. I'll post a fix for it shortly. If you aren't updating the |
|
#4240 fixes the above issue. If you don't update the submodule, I can do so in my PR. |
I still need to fix #4153 (comment), so I'll end up bumping the submodule regardless. |
daprilik
force-pushed
the
iotedge-check-update
branch
from
3bc24e2 to
bb2e0a7
Compare
daprilik
force-pushed
the
iotedge-check-update
branch
from
61c1abb to
5ea1255
Compare
daprilik
force-pushed
the
iotedge-check-update
branch
from
5ea1255 to
8868430
Compare
Yes indeed, I realized that and force pushed a fix. |
Updates
iotedge checkto work with the new underlyingaziotarchitecture.This PR builds on-top of the functionality introduced in the downstream
aziotPR at Azure/iot-identity-service#83In a nutshell, many of the checks that used to be in
iotedge checkhave been ported over into the newaziot checktool. Please see the associated downstream PR for more details. That being said, we don't want to make users invoke multiplecheckcommands, so this PR also includes functionality wherebyiotedgewill shell-out toaziotwhen runningcheckandcheck-list, transparently displayingaziot's check results alongsideiotedge-specific checks.As with the downstream PR, this PR should be reviewed along two axes:
iotedge check/check-listsubcommandsI would really appreciate it if the reviewers actually pulled these changes locally and tested them out, as subtle things such as check output formatting and/or checking check pass/failure cases aren't easy to spot by looking at the code alone.