Create new "aktualizr-lite" command #1107
Conversation
|
I see some code formatting failures. i'll get them fixed |
doanac
force-pushed
the
aktualizr-lite
branch
from
2dd700c
to
5c2e7d6
Compare
Codecov Report
@@ Coverage Diff @@
## master #1107 +/- ##
==========================================
+ Coverage 76.07% 76.43% +0.35%
==========================================
Files 158 159 +1
Lines 9544 9675 +131
==========================================
+ Hits 7261 7395 +134
+ Misses 2283 2280 -3
Continue to review full report at Codecov.
|
doanac
force-pushed
the
aktualizr-lite
branch
2 times, most recently
from
7548c3a
to
38f64bd
Compare
|
Hi Andy, the easiest way to fix the formatting errors is to run More importantly: what do you thing the CI strategy for this code might be? I would imagine adding |
|
It looks like CI is complaining: I think this is because we expect every .cc/.h file to get added to our source control checks, which normally is the case, but not if We were also wondering if there was a way we could test the code in CI. @OYTIS noticed https://github.com/doanac/ota-compose and we are wondering if you might be able to integrate some or all of that as a way to run a test. |
That seems to be doing nothing for me. I've got to poke around and see why. |
|
As per the ota-compose idea. I'm not sure that would work well because it requires docker compose. I'm not sure travis ci type stuff is going to want to do docker-in-docker stuff. However, you guys already have the aktualizr-repo stuff in your tests and all this code needs is the TUF repo it makes. I think there are 2 ways we could test this. I'm not sure which you prefer. Both ways start with populating some data using aktualizr-repo. Then we could:
|
I agree that generating metadata with aktualizr-repo is a good plan. We have some examples of scripts that do this dynamically for some of the advanced tests (particularly delegations). Either of the two options for evaluating the aktualizr-lite functions is fine by me; I don't have a strong preference. |
|
I'll start up on the unit tests today. Could you guys give a cursory glance at the code in the meantime? I'm worried it may have some fundamental issue you don't like and I'm going to wind up writing tests for something that will have to dramatically change. |
There was a problem hiding this comment.
Looks fine to me. I have just a couple comments but nothing big. Travis is still failing but I don't really understand why. Yesterday every PR was failing one of the cases, but that issue has resolved itself and this PR is failing two cases. We can try running this on our internal infrastructure and see if we get more information if that would be helpful for you.
|
|
||
| #include <openssl/ssl.h> | ||
| #include <boost/filesystem.hpp> | ||
| #include <boost/program_options.hpp> |
There was a problem hiding this comment.
Normally our linter complains about alphabetizing includes within a category, but sometimes it has unexpected preferences that deviate from that. However, I'd expect you'll need to put openssl after boost to make it happy.
| if (cmd == commands[i].name) { | ||
| return commands[i].main(config, commandline_map); | ||
| } | ||
| } |
There was a problem hiding this comment.
This seems like an obscure way to call the subcommands, and if you expect the number of commands to increase and the input parameters to vary between them, this could get more unwieldy. That said, I see no real problem with it as is, and we can change it in the future if we really want or need to.
| iresult.result_code.num_code != data::ResultCode::Numeric::kNeedCompletion) { | ||
| LOG_ERROR << "Unable to install update: " << iresult.description; | ||
| return 1; | ||
| } |
There was a problem hiding this comment.
Do you want to say anything about requiring a reboot if the result code is kNeedCompletion? We recently changed the code to fix a longstanding issue that an OSTree installation wasn't really complete until the system had restarted. We are now trying to make this much more clear, and you might want to do the same here.
|
Oh, Travis is still complaining about |
doanac
force-pushed
the
aktualizr-lite
branch
from
38f64bd
to
80f75bc
Compare
|
This has been rebased on the latest master. I've added a couple of changes suggested by @patrickvacek . I then attempted a commit to add unit testing. I'm pretty sure the cmake changes for the unit test might not be quite right, but you can run |
There was a problem hiding this comment.
Looks pretty good so far except that Travis is complaining about this:
/aktualizr/src/aktualizr_repo/main.cc: In function 'int main(int, char**)':
/aktualizr/src/aktualizr_repo/main.cc:115:39: error: variable 'std::ifstream custom_file' has initializer but incomplete type
std::ifstream custom_file(vm["targetcustom"].as<boost::filesystem::
But it looks like the new test is running and passing as expected, so that's cool! Thanks for putting that together. Testing the update might be a bit tedious, but it's worth doing sooner or later.
src/aktualizr_lite/CMakeLists.txt
Outdated
| add_dependencies(t_aktualizr-lite-list aktualizr-lite) | ||
|
|
||
| add_test(NAME t_aktualizr-lite-list | ||
| COMMAND ${PROJECT_SOURCE_DIR}/src/aktualizr_lite/test_list.sh ${PROJECT_SOURCE_DIR}) |
There was a problem hiding this comment.
This looks fine. We have a convenient add_aktualizr_test cmake function, but that probably won't work very smoothly for your needs here. The one thing you may want to add is add_dependencies(build_tests t_aktualizr-lite-list), and the other is to consider if you want to run the test under valgrind. We run almost all of the tests in almost all the environments under it just to be careful. We have a RUN_VALGRIND variable that includes the basic suppressions and parameters we normally need.
src/aktualizr_lite/test_list.sh
Outdated
| set -e | ||
|
|
||
| [ -z $DEBUG ] || exec >/tmp/tmp.txt 2>&1 | ||
| port="${PORT-9876}" |
There was a problem hiding this comment.
We have a script for finding an open port: tests/get_open_port.py.
src/aktualizr_lite/test_list.sh
Outdated
| repo_server = "http://localhost:$port/repo/image" | ||
|
|
||
| [provision] | ||
| primary_ecu_hardware_id = "raspberrypi3-64" |
There was a problem hiding this comment.
I might suggest using an obviously test-only hardware ID just to make it unambiguous that this isn't actually intended to run on an rpi.
I think this is fixed by adding and "#include ", but I'm not sure exactly how to re-create this build failure at home? |
|
I've got a few new commits to address some things @patrickvacek. Hopefully these make sense. The valgrind testing feels a bit different, but running valrind on test_list.sh seemed wrong. If these changes look okay, I'll squash the commits to make a clean PR. I'm also going to start looking at trying to make a test for the "update" subcommand. |
doanac
force-pushed
the
aktualizr-lite
branch
from
c04681a
to
3d8de71
Compare
|
Interesting failure here. Seems like a leak in ostree: Not sure I understand why this one is happening: |
|
@doanac Nice! This is looking really good. The valgrind bit is perhaps awkward, but it is useful. We do suppress some valgrind reports from libostree, but apparently this is new. @lbonn does that look familiar to you? Hard to say if it is legitimate or not, but it's worth investigating. For the update, is there anything else you can grep for or look for to verify that the expected version is being updated? Matching that seems worthwhile. What you've already got it quite good, though.
Does this need a more complete or at least relative path? The leading |
|
About the valgrind leak Ok, a huge part of the OstreeManager::Install() wasn't covered by our tests: everything from line 152, which is the nominal case... And we had a leak! The |
src/aktualizr_lite/CMakeLists.txt
Outdated
|
|
||
| set(TEST_SOURCES test_list.sh) | ||
|
|
||
| if((CMAKE_BUILD_TYPE MATCHES "Valgrind") AND (NOT AKTUALIZR_TEST_NO_VALGRIND)) |
There was a problem hiding this comment.
The NO_VALGRIND is an argument that can be given to add_aktualizr_test, for a given test.
It doesn't apply in this case, the condition should just be if(CMAKE_BUILD_TYPE MATCHES "Valgrind")
src/aktualizr_lite/CMakeLists.txt
Outdated
| add_dependencies(t_aktualizr-lite-list aktualizr-lite) | ||
| add_dependencies(build_tests t_aktualizr-lite-list) | ||
|
|
||
| add_test(NAME t_aktualizr-lite-list |
There was a problem hiding this comment.
Did you really intend to keep the t_aktualizr-lite-list custom target alongside the t_aktualizr-lite-list test run by ctest? The fact that they share the same name is a bit confusing and they run the same command with diferrent arguments.
I would expect that you'd only need two add_test, one with and without valgrind, instead of the two add_custom_target?
And we usually have the convention of calling the tests test_xxx, t_xxx is for their corresponding executables when applicable.
src/aktualizr_lite/test_list.sh
Outdated
| #!/usr/bin/env bash | ||
| set -e | ||
|
|
||
| build_dir=$(pwd) |
There was a problem hiding this comment.
This is a bit brittle. Usually ctest runs tests from the root project directory, so it doesn't look so true.
Because you only use build_dir to access aktualizr-lite, it could as well be directly given as an argument like akrepo_bin.
|
Thanks for making our test coverage better :). Here is a PR to fix the ostree leak: #1114. I've tested that it fixes it, on the top of your branch. So we could either cherry-pick it here (and I'll close the PR) or wait for a merge and rebase on top. |
I've got a few commits I'm going to be squashing to clean things up, so I'll just do a rebase once its merged and build up a clean commit history |
There's nothing really logged to stdout/stderr to grep for. However, I did discover that by simply setting pacman.os in the sota.toml, that the update actually exits successfully. So I think that's a nice improvement. I'll be pushing that change here later today. |
doanac
force-pushed
the
aktualizr-lite
branch
from
b9ba6b7
to
51dfd4c
Compare
doanac
force-pushed
the
aktualizr-lite
branch
from
51dfd4c
to
c5c924e
Compare
|
I think we also have a couple more memory leaks from valgrind now that the full update is completing. |
|
one other leak: Also - i'm still seeing the leak lbonn had fixed when i applied that patch. I'm wondering if running valgrind on ostree might produce the same leak. eg- its a bug there? |
|
The Also, while it looks like the issue with the CMake test vs target has been a bit cleared up, now the test gets run as a dependency of the |
I also can't reproduce the leak that Laurent fixed anymore. I'm still seeing other leaks in OstreeManager, but the issue with I'm still looking into the other issues. The |
|
@patrickvacek - thanks for the cmake point. I was actually thinking about this last night and was worried it was wrong. I think I've finally got it done correctly with: NOTE: I was originally copying this logic: https://github.com/advancedtelematic/aktualizr/blob/master/src/libaktualizr/package_manager/CMakeLists.txt#L33 which i now think might be wrong as well. I'm going to clean this branch up in a moment and rebase it on the current master. And then we can see the results. Disabling the valgrind logic would be simple, but i'll keep it for now. |
This adds the skeleton build and CLI arg-parsing logic for a new command "aktualizr-lite" that can run aktualizr in an anonymous mode without Uptane and only use TUF and OSTree. see: advancedtelematic#1056 Signed-off-by: Andy Doan <andy@foundries.io>
Just the bare-bones logic to display the current active image. Signed-off-by: Andy Doan <andy@foundries.io>
A simple command to list the updates available to a particular device. Signed-off-by: Andy Doan <andy@foundries.io>
This finds the latest update available to a device, downloads it, and applies it. Signed-off-by: Andy Doan <andy@foundries.io>
Signed-off-by: Andy Doan <andy@foundries.io>
This allows you to run something like:
cat >$custom_json <<EOF
{
"harwareIds": ["raspberrypi3-64"],
"targetFormat": "OSTREE"
}
EOF
akrepo --command image \
--targetname foo \
--targetsha256 d933f9e42163c73d6929e6333294312d50d8a6d2c489edaa53ba21da619d51ad \
--targetlength 0 \
--targetcustom $custom_json
akrepo --command signtargets
Signed-off-by: Andy Doan <andy@foundries.io>
Signed-off-by: Andy Doan <andy@foundries.io>
doanac
force-pushed
the
aktualizr-lite
branch
from
c5c924e
to
02d2b6d
Compare
There are some leaks in the OStreeManager that this new test case exposes. Disabling for now to make sure Travis CI passes. Signed-off-by: Andy Doan <andy@foundries.io>
There was a problem hiding this comment.
I was originally copying this logic: https://github.com/advancedtelematic/aktualizr/blob/master/src/libaktualizr/package_manager/CMakeLists.txt#L33 which i now think might be wrong as well.
The reason why that isn't wrong is because that is "just" a script for creating an OSTree sysroot, which then gets used by actual tests. Creating the sysroot itself is not part of the tests, just a prerequisite that isn't very interesting to test more carefully.
Anyway, it looks like you are on the right track now!
src/aktualizr_lite/CMakeLists.txt
Outdated
| add_custom_target(t_aktualizr-lite) | ||
| add_dependencies(t_aktualizr-lite aktualizr-repo) | ||
| add_dependencies(t_aktualizr-lite aktualizr-lite) | ||
| add_dependencies(build_tests t_aktualizr-lite) |
There was a problem hiding this comment.
This does appear to work now. However, I'm not sure that you need a custom target at all. You should just need add_dependencies(build_tests aktualizr-lite).
There was a problem hiding this comment.
Good point. I was trying to make it really easy to just run "make t_aktualizr-lite" and generate just what you need for unit testing. However, "make build_tests" is fine. Fixed now.
|
Travis is complaining about a lack of const: |
I don't see this in my setup. Hope Travis likes this. Signed-off-by: Andy Doan <andy@foundries.io>
We don't really need this custom target. This makes just running a unit test on aktualizr-lite a little slower, but nothing major. Signed-off-by: Andy Doan <andy@foundries.io>
|
Looks pretty good to me! @lbonn @eu-smirnov any other thoughts from your side? |
This PR is meant to address the functionality discussed in issue #1056. I'm not super happy about the lack of any new test coverage, but I was a little concerned it would require so much mocking that it might not provide much value. However, I'd be more than happy to work on that if you guys think its needed.
I've tested this against two different services:
Foundries.io's OTA Connect Deployment
https://api.foundries.io/lmp/repo/release/api/v1/user_repo/targets.json
An "aktualizr-lite" only docker-compose setup
https://github.com/doanac/ota-compose