Revert "Revert "Move apex sessions directory to /metadata""
This reverts commit 7645e253a16b7874cc7d4736dfecec24e411abb1.
Reason for revert: Could not repro error locally, will run Forrest tests to see if error persists
Change-Id: Ic328d3987d9ada1f5e96db33a131f376047b0f99
diff --git a/apexd/aidl/android/apex/ApexSessionInfo.aidl b/apexd/aidl/android/apex/ApexSessionInfo.aidl
index d8be1b5..a7a6daf 100644
--- a/apexd/aidl/android/apex/ApexSessionInfo.aidl
+++ b/apexd/aidl/android/apex/ApexSessionInfo.aidl
@@ -28,4 +28,5 @@
boolean isSuccess;
boolean isReverted;
boolean isRevertFailed;
+ @utf8InCpp String crashingNativeProcess;
}
diff --git a/apexd/apexd.cpp b/apexd/apexd.cpp
index 3a4fcac..f81c016 100644
--- a/apexd/apexd.cpp
+++ b/apexd/apexd.cpp
@@ -827,12 +827,6 @@
return {};
}
-Result<void> RevertStagedSession(ApexSession& session) {
- // If the session is staged, it hasn't been activated yet, and we just need
- // to update its state to prevent it from being activated later.
- return session.UpdateStateAndCommit(SessionState::REVERTED);
-}
-
Result<void> UnmountPackage(const ApexFile& apex, bool allow_latest) {
LOG(VERBOSE) << "Unmounting " << GetPackageId(apex.GetManifest());
@@ -1236,6 +1230,35 @@
}
}
+// Migrates sessions directory from /data/apex/sessions to
+// /metadata/apex/sessions, if necessary.
+Result<void> migrateSessionsDirIfNeeded() {
+ namespace fs = std::filesystem;
+ auto from_path = std::string(kApexDataDir) + "/sessions";
+ auto exists = PathExists(from_path);
+ if (!exists) {
+ return Error() << "Failed to access " << from_path << ": "
+ << exists.error();
+ }
+ if (!*exists) {
+ LOG(DEBUG) << from_path << " does not exist. Nothing to migrate.";
+ return {};
+ }
+ auto to_path = kApexSessionsDir;
+ std::error_code error_code;
+ fs::copy(from_path, to_path, fs::copy_options::recursive, error_code);
+ if (error_code) {
+ return Error() << "Failed to copy old sessions directory"
+ << error_code.message();
+ }
+ fs::remove_all(from_path, error_code);
+ if (error_code) {
+ return Error() << "Failed to delete old sessions directory "
+ << error_code.message();
+ }
+ return {};
+}
+
void scanStagedSessionsDirAndStage() {
LOG(INFO) << "Scanning " << kApexSessionsDir
<< " looking for sessions to be activated.";
@@ -1466,23 +1489,6 @@
return {};
}
-void revertAllStagedSessions() {
- auto sessions = ApexSession::GetSessionsInState(SessionState::STAGED);
- if (sessions.empty()) {
- LOG(WARNING) << "No session to revert";
- return;
- }
-
- for (auto& session : sessions) {
- LOG(INFO) << "Reverting back session " << session;
- auto st = RevertStagedSession(session);
- if (!st) {
- LOG(ERROR) << "Failed to revert staged session " << session << ": "
- << st.error();
- }
- }
-}
-
/**
* During apex installation, staged sessions located in /data/apex/sessions
* mutate the active sessions in /data/apex/active. If some error occurs during
@@ -1501,14 +1507,6 @@
return Error() << "Revert requested, when there are no active sessions.";
}
- if (gInFsCheckpointMode) {
- LOG(DEBUG) << "Checkpoint mode is enabled";
- // On checkpointing devices, our modifications on /data will be
- // automatically reverted when we abort changes. Updating the session
- // state is pointless here, as it will be reverted as well.
- return {};
- }
-
for (auto& session : activeSessions) {
if (!crashing_native_process.empty()) {
session.SetCrashingNativeProcess(crashing_native_process);
@@ -1522,17 +1520,21 @@
}
}
- auto restoreStatus = RestoreActivePackages();
- if (!restoreStatus) {
- for (auto& session : activeSessions) {
- auto st = session.UpdateStateAndCommit(SessionState::REVERT_FAILED);
- LOG(DEBUG) << "Marking " << session << " as failed to revert";
- if (!st) {
- LOG(WARNING) << "Failed to mark session " << session
- << " as failed to revert : " << st.error();
+ if (!gInFsCheckpointMode) {
+ auto restoreStatus = RestoreActivePackages();
+ if (!restoreStatus) {
+ for (auto& session : activeSessions) {
+ auto st = session.UpdateStateAndCommit(SessionState::REVERT_FAILED);
+ LOG(DEBUG) << "Marking " << session << " as failed to revert";
+ if (!st) {
+ LOG(WARNING) << "Failed to mark session " << session
+ << " as failed to revert : " << st.error();
+ }
}
+ return restoreStatus;
}
- return restoreStatus;
+ } else {
+ LOG(INFO) << "Not restoring active packages in checkpoint mode.";
}
for (auto& session : activeSessions) {
@@ -1682,7 +1684,7 @@
}
}
- // Ask whether we should revert any staged sessions; this can happen if
+ // Ask whether we should revert any active sessions; this can happen if
// we've exceeded the retry count on a device that supports filesystem
// checkpointing.
if (gSupportsFsCheckpoints) {
@@ -1694,7 +1696,7 @@
LOG(INFO) << "Exceeded number of session retries ("
<< kNumRetriesWhenCheckpointingEnabled
<< "). Starting a revert";
- revertAllStagedSessions();
+ revertActiveSessions("");
}
}
diff --git a/apexd/apexd.h b/apexd/apexd.h
index c05a53c..0a85728 100644
--- a/apexd/apexd.h
+++ b/apexd/apexd.h
@@ -36,6 +36,7 @@
android::base::Result<void> scanPackagesDirAndActivate(
const char* apex_package_dir);
void scanStagedSessionsDirAndStage();
+android::base::Result<void> migrateSessionsDirIfNeeded();
android::base::Result<void> preinstallPackages(
const std::vector<std::string>& paths) WARN_UNUSED;
diff --git a/apexd/apexd_main.cpp b/apexd/apexd_main.cpp
index 7fc2f5f..b119931 100644
--- a/apexd/apexd_main.cpp
+++ b/apexd/apexd_main.cpp
@@ -88,6 +88,7 @@
vold_service = &*vold_service_st;
}
+ android::apex::migrateSessionsDirIfNeeded();
android::apex::onStart(vold_service);
android::apex::binder::CreateAndRegisterService();
android::apex::binder::StartThreadPool();
diff --git a/apexd/apexd_session.h b/apexd/apexd_session.h
index 1da2084..b2b1c2f 100644
--- a/apexd/apexd_session.h
+++ b/apexd/apexd_session.h
@@ -28,8 +28,7 @@
namespace android {
namespace apex {
-static const std::string kApexSessionsDir =
- std::string(kApexDataDir) + "/sessions";
+static const std::string kApexSessionsDir = "/metadata/apex/sessions";
class ApexSession {
public:
diff --git a/apexd/apexservice.cpp b/apexd/apexservice.cpp
index 8c3f3c6..19c9c4d 100644
--- a/apexd/apexservice.cpp
+++ b/apexd/apexservice.cpp
@@ -210,6 +210,7 @@
ClearSessionInfo(session_info);
session_info->sessionId = session.GetId();
+ session_info->crashingNativeProcess = session.GetCrashingNativeProcess();
switch (session.GetState()) {
case SessionState::VERIFIED:
@@ -520,10 +521,15 @@
child_ids_str += " " + std::to_string(childSessionId);
}
}
+ std::string revert_reason = "";
+ std::string crashing_native_process = session.GetCrashingNativeProcess();
+ if (!crashing_native_process.empty()) {
+ revert_reason = " Revert Reason: " + crashing_native_process;
+ }
std::string msg =
StringLog() << "Session ID: " << session.GetId() << child_ids_str
<< " State: " << SessionState_State_Name(session.GetState())
- << std::endl;
+ << revert_reason << std::endl;
dprintf(fd, "%s", msg.c_str());
}
@@ -705,6 +711,11 @@
ApexSessionInfo session_info;
BinderStatus status = getStagedSessionInfo(session_id, &session_info);
if (status.isOk()) {
+ std::string revert_reason = "";
+ std::string crashing_native_process = session_info.crashingNativeProcess;
+ if (!crashing_native_process.empty()) {
+ revert_reason = " revertReason: " + crashing_native_process;
+ }
std::string msg = StringLog()
<< "session_info: "
<< " isUnknown: " << session_info.isUnknown
@@ -712,7 +723,8 @@
<< " isStaged: " << session_info.isStaged
<< " isActivated: " << session_info.isActivated
<< " isActivationFailed: "
- << session_info.isActivationFailed << std::endl;
+ << session_info.isActivationFailed << revert_reason
+ << std::endl;
dprintf(out, "%s", msg.c_str());
return OK;
}
diff --git a/apexd/apexservice_test.cpp b/apexd/apexservice_test.cpp
index ca628f9..6760b6c 100644
--- a/apexd/apexservice_test.cpp
+++ b/apexd/apexservice_test.cpp
@@ -433,6 +433,7 @@
ASSERT_FALSE(ec) << "Failed to delete " << p.path() << " : "
<< ec.message();
});
+ fs::remove_all(kApexSessionsDir);
ASSERT_TRUE(IsOk(status));
}
};
@@ -2206,10 +2207,6 @@
}
TEST_F(ApexServiceRevertTest, RevertStoresCrashingNativeProcess) {
- if (supports_fs_checkpointing_) {
- GTEST_SKIP() << "Can't run if filesystem checkpointing is enabled";
- }
-
PrepareTestApexForInstall installer(GetTestFile("apex.apexd_test_v2.apex"));
if (!installer.Prepare()) {
return;
diff --git a/tests/Android.bp b/tests/Android.bp
index b798c26..9b0a41b 100644
--- a/tests/Android.bp
+++ b/tests/Android.bp
@@ -268,6 +268,7 @@
data: [
"testdata/trigger_watchdog.rc",
"testdata/trigger_watchdog.sh",
+ "testdata/trigger_reboot.sh",
":com.android.apex.cts.shim.v2_prebuilt",
],
}
diff --git a/tests/apex-rollback-tests.xml b/tests/apex-rollback-tests.xml
index 211176a..e5e82bd 100644
--- a/tests/apex-rollback-tests.xml
+++ b/tests/apex-rollback-tests.xml
@@ -22,6 +22,7 @@
<option name="remount-system" value="true" />
<option name="push" value="trigger_watchdog.rc->/system/etc/init/trigger_watchdog.rc" />
<option name="push" value="trigger_watchdog.sh->/system/bin/trigger_watchdog.sh" />
+ <option name="push" value="trigger_reboot.sh->/system/bin/trigger_reboot.sh"/>
</target_preparer>
<test class="com.android.tradefed.testtype.HostTest" >
<option name="jar" value="apex_rollback_tests.jar" />
diff --git a/tests/src/com/android/tests/apex/ApexRollbackTests.java b/tests/src/com/android/tests/apex/ApexRollbackTests.java
index b42f9b6..c2d3bee 100644
--- a/tests/src/com/android/tests/apex/ApexRollbackTests.java
+++ b/tests/src/com/android/tests/apex/ApexRollbackTests.java
@@ -51,6 +51,7 @@
mUtils.uninstallShimApexIfNecessary();
resetProperty("persist.debug.trigger_watchdog.apex");
resetProperty("persist.debug.trigger_updatable_crashing_for_testing");
+ resetProperty("persist.debug.trigger_reboot_after_activation");
}
/**
@@ -63,6 +64,7 @@
mUtils.uninstallShimApexIfNecessary();
resetProperty("persist.debug.trigger_watchdog.apex");
resetProperty("persist.debug.trigger_updatable_crashing_for_testing");
+ resetProperty("persist.debug.trigger_reboot_after_activation");
}
private void resetProperty(String propertyName) throws Exception {
@@ -92,6 +94,10 @@
String error = device.installPackage(apexFile, false, "--wait");
assertThat(error).isNull();
+ String sessionIdToCheck = device.executeShellCommand("pm get-stagedsessions --only-ready "
+ + "--only-parent --only-sessionid").trim();
+ assertThat(sessionIdToCheck).isNotEmpty();
+
// After we reboot the device, we expect the device to go into boot
// loop from trigger_watchdog.sh. Native watchdog should detect and
// report the boot loop, causing apexd to roll back to the previous
@@ -106,6 +112,40 @@
Set<ApexInfo> activatedApexes = device.getActiveApexes();
assertThat(activatedApexes).contains(ctsShimV1);
assertThat(activatedApexes).doesNotContain(ctsShimV2);
+
+ // Assert that a session has failed with the expected reason
+ String sessionInfo = device.executeShellCommand("cmd apexservice getStagedSessionInfo "
+ + sessionIdToCheck);
+ assertThat(sessionInfo).contains("revertReason: zygote");
+ }
+
+ @Test
+ public void testCheckpointingRevertsSession() throws Exception {
+ assumeTrue("Device does not support updating APEX", mUtils.isApexUpdateSupported());
+ assumeTrue("Device doesn't support fs checkpointing", supportsFsCheckpointing());
+
+ File apexFile = mUtils.getTestFile("com.android.apex.cts.shim.v2.apex");
+
+ ITestDevice device = getDevice();
+ assertThat(device.setProperty("persist.debug.trigger_reboot_after_activation",
+ "com.android.apex.cts.shim@2.apex")).isTrue();
+ String error = device.installPackage(apexFile, false, "--wait");
+ assertThat(error).isNull();
+
+ String sessionIdToCheck = device.executeShellCommand("pm get-stagedsessions --only-ready "
+ + "--only-parent --only-sessionid").trim();
+ assertThat(sessionIdToCheck).isNotEmpty();
+
+ // After we reboot the device, the apexd session should be activated as normal. After this,
+ // trigger_reboot.sh will reboot the device before the system server boots. Checkpointing
+ // will kick in, and at the next boot any non-finalized sessions will be reverted.
+ device.reboot();
+
+ ApexInfo ctsShimV1 = new ApexInfo("com.android.apex.cts.shim", 1L);
+ ApexInfo ctsShimV2 = new ApexInfo("com.android.apex.cts.shim", 2L);
+ Set<ApexInfo> activatedApexes = device.getActiveApexes();
+ assertThat(activatedApexes).contains(ctsShimV1);
+ assertThat(activatedApexes).doesNotContain(ctsShimV2);
}
// TODO(ioffe): check that we recover from the boot loop in of userspace reboot.
diff --git a/tests/testdata/trigger_reboot.sh b/tests/testdata/trigger_reboot.sh
new file mode 100644
index 0000000..d6606fc
--- /dev/null
+++ b/tests/testdata/trigger_reboot.sh
@@ -0,0 +1,15 @@
+# To trigger a reboot on install of version 300000000 of
+# the com.android.tzdata apex:
+# $ adb shell setprop persist.debug.trigger_reboot_after_activation com.android.tzdata@300000000.apex
+active_apex=/data/apex/active/`/system/bin/getprop persist.debug.trigger_reboot_after_activation`
+if [[ $active_apex == *.apex ]]
+then
+ while :
+ do
+ if [ -a $active_apex ]
+ then
+ /system/bin/log -t TriggerWatchdog "Detected presence of $active_apex"
+ /system/bin/setprop sys.powerctl reboot
+ fi
+ done
+fi
\ No newline at end of file
diff --git a/tests/testdata/trigger_watchdog.rc b/tests/testdata/trigger_watchdog.rc
index 300cca5..47c6f41 100644
--- a/tests/testdata/trigger_watchdog.rc
+++ b/tests/testdata/trigger_watchdog.rc
@@ -4,6 +4,12 @@
exec -- /system/bin/log -t TriggerWatchdog "Starting trigger_watchdog"
exec_background u:r:su:s0 -- /system/bin/sh /system/bin/trigger_watchdog.sh
+# Only trigger if persist.debug.trigger_reboot_after_activation is non-empty.
+on boot && property:persist.debug.trigger_reboot_after_activation=*
+ setprop debug.trigger_watchdog.status start
+ exec -- /system/bin/log -t TriggerWatchdog "Checking checkpointing revert"
+ exec_background u:r:su:s0 -- /system/bin/sh /system/bin/trigger_reboot.sh
+
on boot && property:persist.debug.trigger_updatable_crashing_for_testing=1
# Mock the fact that a service was crashing.
setprop sys.init.updatable_crashing_process_name test_process
