-
Notifications
You must be signed in to change notification settings - Fork 60
fix/ota-3865/Log connectivity restored after an interruption #1463
Conversation
60b6823
to
64e1ee6
Compare
auto manifest = AssembleManifest(); | ||
if (custom != Json::nullValue) { | ||
manifest["custom"] = custom; | ||
} | ||
auto signed_manifest = uptane_manifest.signManifest(manifest); | ||
HttpResponse response = http->put(config.uptane.director_server + "/manifest", signed_manifest); | ||
if (response.isOk()) { | ||
if (!connect) { | ||
LOG_INFO << "Connectivity is restored."; | ||
sendEvent<event::LogConnectivityRestored>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think just logging is enough. This is probably easier than we were thinking. Sending an event isn't a bad idea but for now we can skip it.
EXPECT_NO_THROW(sota_client->initialize()); | ||
EXPECT_TRUE(sota_client->putManifestSimple()); | ||
EXPECT_EQ(num_events_LogConnectivityRestored, 1); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This general test format looks fine, although capturing logging output is slightly challenging, and I'd suggest putting this in uptane_network_test.cc
, although to be honest this isn't really Uptane-specific.
64e1ee6
to
573d771
Compare
Codecov Report
@@ Coverage Diff @@
## master #1463 +/- ##
==========================================
+ Coverage 80.51% 80.56% +0.05%
==========================================
Files 184 184
Lines 11083 11087 +4
==========================================
+ Hits 8923 8932 +9
+ Misses 2160 2155 -5
Continue to review full report at Codecov.
|
2ef60e1
to
bad609d
Compare
Any tips why my test failed at the first stage? I followed the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
../src/libaktualizr/uptane/uptane_network_test.cc:163: Failure
From what I see that points to https://github.com/advancedtelematic/aktualizr/pull/1463/files#diff-8b2accdd7accfa663ac3e253db791a7eR163 which means initialization failed. CaptureStdout
looks like it might be exactly what you want, but it's probably not the cause of the failure here.
*/ | ||
TEST(UptaneNetwork, LogConnectivityRestored) { | ||
// testing::internal::CaptureStdout(); | ||
RecordProperty("zephyr_key", "OTA-991,TST-158"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can leave out this RecordProperty
thing for now. Maybe someday that'll be important again, but for now, it isn't.
conf.provision.server = conf.provision.server.substr(conf.provision.server.size() - 2) + "11"; | ||
KeyManager keys(store, conf.keymanagerConfig()); | ||
Initializer initializer(conf.provision, store, http, keys, {}); | ||
result = initializer.isSuccessful(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this idea of feigning a broken connection by changing the server URL. However, does it require re-initialization every time? I was hoping we could just test this with calls to CheckUpdates
. At any rate, we almost certainly can avoid extra instantiations of KeyManager
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At any rate, we almost certainly can avoid extra instantiations of KeyManager.
But Initializer initializer(conf.provision, store, http, keys, {});
need keys
. And from doTestInit
it looks re-initialization is needed. I'm not sure though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fairly confident that you can reuse the keys if not the entire initialization. To simulate a more realistic scenario, we should only initialize once and merely call CheckUpdates
multiple times under different network conditions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we should only initialize once and merely call CheckUpdates multiple times under different network conditions.
How can I get under different network conditions if I don't use initialize? I didn't find the function I need.
1e035c1
to
d6c3ac2
Compare
auto sota_client = std_::make_unique<SotaUptaneClient>(conf, store, http); | ||
EXPECT_NO_THROW(sota_client->initialize()); | ||
EXPECT_NO_THROW(sota_client->fetchMeta()); | ||
EXPECT_NO_THROW(sota_client->checkUpdates()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're using the SotaUptaneClient functions directly, you probably want to just call initialize
once and then fetchMeta
each time. fetchMeta
is actually what Aktualizr::CheckUpdates
calls, and fetchMeta
calls SotaUptaneClient::checkUpdates
. It also is what calls putManifestSimple
, which is where your change is located, so just calling SotaUptaneClient::checkUpdates
won't trigger it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW if you make this test case a FRIEND_TEST
of SotaUptaneClient
(see sotauptaneclient.h
for other examples) you can directly manipulate the Config
object stored in SotaUptaneClient
and then you can bypass needing to re-instantiate or re-initialize SotaUptaneClient
.
ffd825e
to
ea74192
Compare
|
||
auto sota_client = std_::make_unique<SotaUptaneClient>(conf, store, http); | ||
EXPECT_NO_THROW(sota_client->initialize()); | ||
result::UpdateCheck result = sota_client->fetchMeta(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got this failure HTTP/1.0 501 Unsupported method ('PUT')
. Is auto http = std::make_shared<HttpClient>();
a susceptive target?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
HttpFake
seems to make no difference in local test.
ea74192
to
ebc0213
Compare
It looks like it's failing right now because the "hasupdates" HttpFake should be used with a secondary. Adding this line makes the fetchMeta call succeed. It would even be better to use
|
ebc0213
to
550577f
Compare
Thanks Laurent@lbonn ,yes, I tried your method, the fetchMeta call succeed, but when I try to simulate a broken connection, it seems still connected, and failed to directly manipulate the |
@Zee314159 Modifying the config object might work, or not if it has been copied before by an internal component. The most straightforward thing that comes to my mind is to subclass HttpFake and add a new method to toggle a switch that causes all the requests to fail. |
1b348cb
to
5cbc053
Compare
0f0dfad
to
bd82eb0
Compare
(void)data; | ||
return HttpResponse(url, 503, CURLE_OK, ""); | ||
} else { | ||
if (url.find("/devices") != std::string::npos || url.find("/director/ecus") != std::string::npos || url.empty()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there a reason why we can't call HttpFake::post()
here? Copying this logic here, would complicate maintenance.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure, but the test will fail if I use the original. I'll have a test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
file served: "/tmp/aktualizr-4d8c-c7cb-fcae-9383/619b-47da-dir/repo/targets_hasupdates.json"
4429
[2019-12-09 12:59:48.635639] [0x000000000404fcc0] [info] 2 new updates found in Uptane metadata.
4430
[2019-12-09 12:59:48.659888] [0x000000000404fcc0] [info] got UpdateCheckComplete event
4431
[2019-12-09 12:59:50.232787] [0x000000000404fcc0] [error] Failed to update director metadata:
4432
[2019-12-09 12:59:50.236768] [0x000000000404fcc0] [info] got UpdateCheckComplete event
4433
../src/libaktualizr/uptane/uptane_network_test.cc:217: Failure
4434
Expected: (std::string::npos) != (testing::internal::GetCapturedStdout().find("Connectivity is restored.")), actual: 18446744073709551615 vs 1844674407370955161
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, you still need to handle the failure case. In the last version you've pushed, put
and post
are the same as in HttpFake.
If you follow the same pattern that what you did for get()
, it should work.
e.g:
HttpResponse post(const std::string &url, const Json::Value &data) override {
if (!connectSwitch) {
(void)data;
return HttpResponse(url, 503, CURLE_OK, "");
} else {
return HttpFake::post(url, data);
}
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see, thank you!
(void)data; | ||
return HttpResponse(url, 504, CURLE_OK, ""); | ||
} else { | ||
last_manifest = data; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we also call HttpFake::put()
?
Looking good apart from the two small comments. Also, before we merge, could you edit the commit message so that it describes the changes better? |
bd82eb0
to
47f616c
Compare
Signed-off-by: Zee314159 <252806294@qq.com>
47f616c
to
d829078
Compare
It merged in master but the PR didn't get closed somehow 👐 |
fix/ota-3865/Log connectivity restored after an interruption
Draft code.