libexperiments: properly refresh experiment states
An experiment is enabled if the file /config/experiment/<exp>.active
exists. The old code assumed that Catawampus would create/delete this
file whenever the state of an experiment changed (as pushed by ACS).
It turns out that it does not do that and instead just creates a new
file, either <exp>.requested or <exp>.unrequested, and then expects
the user process to check for that and create/delete the <exp>.active
accordingly. The new code does this now.
I also dumped all my understanding of the experiments framework into
go/gfiber-experiments-for-the-rest-of-us. Please read there for more
details.
Change-Id: Ibdd4fcce447905ce6f63694357e9f335d630872f
diff --git a/libexperiments/experiments.cc b/libexperiments/experiments.cc
index 89fec64..daf3e20 100644
--- a/libexperiments/experiments.cc
+++ b/libexperiments/experiments.cc
@@ -70,9 +70,6 @@
if (!names_to_register.empty()) {
if (!Register_Locked(names_to_register))
return false;
-
- // initial read of registered experiments states
- Refresh();
}
return true;
@@ -104,6 +101,13 @@
registered_experiments_.insert(name);
log("experiments:Registered '%s'", name.c_str());
+
+ // check if experiment is active
+ std::string file_active = config_dir_ + "/" + name + ".active";
+ if (file_exists(file_active.c_str())) {
+ enabled_experiments_.insert(name);
+ log("experiments:'%s' is now enabled", name.c_str());
+ }
}
return true;
}
@@ -138,15 +142,29 @@
return;
}
- std::string file_path = config_dir_ + "/" + name + ".active";
- bool was_enabled = IsInEnabledList(name);
- bool is_enabled = file_exists(file_path.c_str());
- if (is_enabled && !was_enabled) {
- log("experiments:'%s' is now enabled", name.c_str());
- enabled_experiments_.insert(name);
- } else if (!is_enabled && was_enabled) {
- log("experiments:'%s' is now disabled", name.c_str());
- enabled_experiments_.erase(name);
+ std::string experiments_path = config_dir_ + "/" + name;
+
+ if (IsInEnabledList(name)) {
+ // currently enabled, check if it was set to ".unrequested"
+ std::string file_unrequested = experiments_path + ".unrequested";
+ if (file_exists(file_unrequested.c_str())) {
+ // deactivate experiment
+ std::string file_active = experiments_path + ".active";
+ rm_file(file_active.c_str());
+ rm_file(file_unrequested.c_str());
+ enabled_experiments_.erase(name);
+ log("experiments:'%s' is now disabled", name.c_str());
+ }
+ } else {
+ // currently not enabled, check if it is requested
+ std::string file_requested = experiments_path + ".requested";
+ if (file_exists(file_requested.c_str())) {
+ // activate experiment
+ std::string file_active = experiments_path + ".active";
+ mv_file(file_requested.c_str(), file_active.c_str());
+ enabled_experiments_.insert(name);
+ log("experiments:'%s' is now enabled", name.c_str());
+ }
}
}
diff --git a/libexperiments/experiments_test.cc b/libexperiments/experiments_test.cc
index 5f0178d..659b430 100644
--- a/libexperiments/experiments_test.cc
+++ b/libexperiments/experiments_test.cc
@@ -3,7 +3,6 @@
#include "experiments.h"
#include "experiments_c_api_test.h"
-#include <fcntl.h>
#include <linux/limits.h>
#include <sys/types.h>
#include <sys/stat.h>
@@ -35,68 +34,20 @@
ASSERT_EQ(0, system(cmd.c_str()));
}
- bool CreateFile(const std::string &name) {
- int fd = open(name.c_str(), O_CREAT | O_TRUNC,
- S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
- if (fd < 0) {
- log_perror(errno, "Cannot create file '%s':", name.c_str());
- return false;
- } else {
- close(fd);
- }
- return true;
+ bool SetRequested(const std::string &exp_name) {
+ remove((exp_name + ".unrequested").c_str());
+ return touch_file((exp_name + ".requested").c_str());
}
- bool RenameFile(const std::string &from_name, const std::string &to_name) {
- if (rename(from_name.c_str(), to_name.c_str()) < 0) {
- log_perror(errno, "Cannot rename file '%s' to '%s':", from_name.c_str(),
- to_name.c_str());
- return false;
- }
- return true;
+ bool SetUnrequested(const std::string &exp_name) {
+ remove((exp_name + ".requested").c_str());
+ return touch_file((exp_name + ".unrequested").c_str());
}
- bool DeleteFile(const std::string &name) {
- if (remove(name.c_str()) < 0) {
- log_perror(errno, "Cannot delete file '%s':", name.c_str());
- return false;
- }
- return true;
- }
-
- bool SwitchFromTo(Experiments *e, const std::string &name,
- const std::string &from_ext, const std::string &to_ext) {
- std::string from_file = name + from_ext;
- std::string to_file = name + to_ext;
- if (file_exists(from_file.c_str())) {
- return RenameFile(from_file, to_file);
- } else {
- return CreateFile(to_file);
- }
- }
-
- bool SetActive(Experiments *e, const std::string &name) {
- return SwitchFromTo(e, name, ".inactive", ".active");
- }
-
- bool SetInactive(Experiments *e, const std::string &name) {
- return SwitchFromTo(e, name, ".active", ".inactive");
- }
-
- bool Remove(Experiments *e, const std::string &name) {
- std::string active_file = name + ".active";
- if (file_exists(active_file.c_str())) {
- if (!DeleteFile(active_file)) {
- return false;
- }
- }
- std::string inactive_file = name + ".inactive";
- if (file_exists(inactive_file.c_str())) {
- if (!DeleteFile(inactive_file)) {
- return false;
- }
- }
- return true;
+ void Remove(const std::string &exp_name) {
+ remove((exp_name + ".unrequested").c_str());
+ remove((exp_name + ".requested").c_str());
+ remove((exp_name + ".active").c_str());
}
static char root_path_[PATH_MAX];
@@ -163,17 +114,20 @@
EXPECT_FALSE(e.IsEnabled("exp1"));
EXPECT_EQ(1, e.GetNumOfRegisteredExperiments());
- EXPECT_TRUE(SetActive(&e, "exp1"));
+ EXPECT_TRUE(SetRequested("exp1"));
EXPECT_TRUE(e.IsEnabled("exp1"));
- EXPECT_TRUE(SetInactive(&e, "exp1"));
+ EXPECT_TRUE(SetUnrequested("exp1"));
EXPECT_FALSE(e.IsEnabled("exp1"));
- EXPECT_TRUE(SetActive(&e, "exp1"));
+ EXPECT_TRUE(SetRequested("exp1"));
EXPECT_TRUE(e.IsEnabled("exp1"));
- EXPECT_TRUE(Remove(&e, "exp1"));
+ EXPECT_TRUE(SetUnrequested("exp1"));
EXPECT_FALSE(e.IsEnabled("exp1"));
+
+ // clean up
+ Remove("exp1");
}
TEST_F(ExperimentsTest, Multiple) {
@@ -186,47 +140,52 @@
EXPECT_FALSE(e.IsEnabled("exp3"));
// activate exp1 - AII
- EXPECT_TRUE(SetActive(&e, "exp1"));
+ EXPECT_TRUE(SetRequested("exp1"));
EXPECT_TRUE(e.IsEnabled("exp1"));
EXPECT_FALSE(e.IsEnabled("exp2"));
EXPECT_FALSE(e.IsEnabled("exp3"));
// activate exp2 - AAI
- EXPECT_TRUE(SetActive(&e, "exp2"));
+ EXPECT_TRUE(SetRequested("exp2"));
EXPECT_TRUE(e.IsEnabled("exp1"));
EXPECT_TRUE(e.IsEnabled("exp2"));
EXPECT_FALSE(e.IsEnabled("exp3"));
// active exp3 - AAA
- EXPECT_TRUE(SetActive(&e, "exp3"));
+ EXPECT_TRUE(SetRequested("exp3"));
EXPECT_TRUE(e.IsEnabled("exp1"));
EXPECT_TRUE(e.IsEnabled("exp2"));
EXPECT_TRUE(e.IsEnabled("exp3"));
// inactivate exp2 - AIA
- EXPECT_TRUE(SetInactive(&e, "exp2"));
+ EXPECT_TRUE(SetUnrequested("exp2"));
EXPECT_TRUE(e.IsEnabled("exp1"));
EXPECT_FALSE(e.IsEnabled("exp2"));
EXPECT_TRUE(e.IsEnabled("exp3"));
- // remove exp1 file - IIA
- EXPECT_TRUE(Remove(&e, "exp1"));
+ // inactivate exp1 file - IIA
+ EXPECT_TRUE(SetUnrequested("exp1"));
EXPECT_FALSE(e.IsEnabled("exp1"));
EXPECT_FALSE(e.IsEnabled("exp2"));
EXPECT_TRUE(e.IsEnabled("exp3"));
// re-activate exp2 - IAA
- EXPECT_TRUE(SetActive(&e, "exp2"));
+ EXPECT_TRUE(SetRequested("exp2"));
EXPECT_FALSE(e.IsEnabled("exp1"));
EXPECT_TRUE(e.IsEnabled("exp2"));
EXPECT_TRUE(e.IsEnabled("exp3"));
// inactivate exp1 (re-create file) - IAA
- EXPECT_TRUE(SetInactive(&e, "exp1"));
+ EXPECT_TRUE(SetUnrequested("exp1"));
EXPECT_FALSE(e.IsEnabled("exp1"));
EXPECT_TRUE(e.IsEnabled("exp2"));
EXPECT_TRUE(e.IsEnabled("exp3"));
- // remove all - III
- EXPECT_TRUE(Remove(&e, "exp1"));
- EXPECT_TRUE(Remove(&e, "exp2"));
- EXPECT_TRUE(Remove(&e, "exp3"));
+ // inactivate all - III
+ EXPECT_TRUE(SetUnrequested("exp1"));
+ EXPECT_TRUE(SetUnrequested("exp2"));
+ EXPECT_TRUE(SetUnrequested("exp3"));
EXPECT_FALSE(e.IsEnabled("exp1"));
EXPECT_FALSE(e.IsEnabled("exp2"));
EXPECT_FALSE(e.IsEnabled("exp3"));
+
+ // clean up
+ Remove("exp1");
+ Remove("exp2");
+ Remove("exp3");
}
TEST_F(ExperimentsTest, TimeBetweenRefresh) {
@@ -238,7 +197,7 @@
&DummyExperimentsRegisterFunc, {"exp1"}));
EXPECT_EQ(1, e.GetNumOfRegisteredExperiments());
EXPECT_FALSE(e.IsEnabled("exp1"));
- EXPECT_TRUE(SetActive(&e, "exp1"));
+ EXPECT_TRUE(SetRequested("exp1"));
// measure time until we see "exp1" active
uint64_t duration = us_elapse(start_time);
@@ -251,7 +210,7 @@
EXPECT_LT(duration, kTimeout) << "time:" << duration;
// clean up
- EXPECT_TRUE(Remove(&e, "exp1"));
+ Remove("exp1");
}
TEST_F(ExperimentsTest, C_API_Test) {
@@ -260,9 +219,9 @@
EXPECT_FALSE(test_experiments_register("exp1"));
EXPECT_FALSE(test_experiments_is_registered("exp1"));
EXPECT_FALSE(test_experiments_is_enabled("exp1"));
- EXPECT_TRUE(SetActive(experiments, "exp1"));
+ EXPECT_TRUE(SetRequested("exp1"));
EXPECT_FALSE(test_experiments_is_enabled("exp1"));
- EXPECT_TRUE(Remove(experiments, "exp1"));
+ EXPECT_TRUE(SetUnrequested("exp1"));
// initialize
EXPECT_TRUE(test_experiments_initialize(test_folder_path_));
@@ -274,13 +233,13 @@
EXPECT_EQ(1, experiments_get_num_of_registered_experiments());
EXPECT_FALSE(test_experiments_is_enabled("exp1"));
- EXPECT_TRUE(SetActive(experiments, "exp1"));
+ EXPECT_TRUE(SetRequested("exp1"));
EXPECT_TRUE(test_experiments_is_enabled("exp1"));
EXPECT_FALSE(test_experiments_is_enabled("exp2"));
- EXPECT_TRUE(SetInactive(experiments, "exp1"));
+ EXPECT_TRUE(SetUnrequested("exp1"));
EXPECT_FALSE(test_experiments_is_enabled("exp1"));
// clean up
- EXPECT_TRUE(Remove(experiments, "exp1"));
+ EXPECT_TRUE(SetUnrequested("exp1"));
}
diff --git a/libexperiments/utils.h b/libexperiments/utils.h
index 3c1adb4..2bb8ddf 100644
--- a/libexperiments/utils.h
+++ b/libexperiments/utils.h
@@ -1,6 +1,7 @@
#ifndef _LIBEXPERIMENTS_UTILS_H_
#define _LIBEXPERIMENTS_UTILS_H_
+#include <fcntl.h>
#include <stdint.h>
#include <sys/time.h>
#include <sys/types.h>
@@ -53,6 +54,42 @@
return S_ISDIR(dir_stat.st_mode);
}
+static inline bool touch_file(const char *name) {
+ if (!name)
+ return false;
+
+ int fd = open(name, O_CREAT | O_TRUNC, S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
+ if (fd < 0) {
+ log_perror(errno, "Cannot create file '%s':", name);
+ return false;
+ }
+ close(fd);
+ return true;
+}
+
+static inline bool rm_file(const char *name) {
+ if (!name)
+ return false;
+
+ if (remove(name) < 0) {
+ log_perror(errno, "Cannot rm file '%s':", name);
+ return false;
+ }
+ return true;
+}
+
+static inline bool mv_file(const char *from_name, const char *to_name) {
+ if (!from_name || !to_name)
+ return false;
+
+ // Note this uses rename() and hence only works on the same filesystem.
+ if (rename(from_name, to_name) < 0) {
+ log_perror(errno, "Cannot mv file '%s' to '%s':", from_name, to_name);
+ return false;
+ }
+ return true;
+}
+
// This function runs the command cmd (but not in a shell), providing the
// return code, stdout, and stderr. It also allows to specify the stdin,
// and a command timeout value (timeout_usec, use <0 to block indefinitely).
