PolicyManager: Extend the EvaluationContext to handle IsTimeGreaterThan()
The EvaluationContext needs to know the point in time where the
current evaluation of a policy request started to be able to handle
time based values like IsTimeGreaterThan() efficiently and the
timeout for the current evaluation. This patch adds such method to
the EvaluationContext class and exposes a new ResetEvaluation()
method to allow restart an evaluation. The restart of an evaluation
is independent from the notification mechanism implemented by
RunOnValueChangeOrTimeout().
The documentation on the header file is updated and extended with an
example of the workflow.
BUG=chromium:358451
TEST=Unittests added.
Change-Id: I50b6ef9d582434d6ba0f466f7e650e4a2b442249
Reviewed-on: https://chromium-review.googlesource.com/195230
Reviewed-by: Alex Deymo <deymo@chromium.org>
Commit-Queue: Alex Deymo <deymo@chromium.org>
Tested-by: Alex Deymo <deymo@chromium.org>
diff --git a/fake_system_state.cc b/fake_system_state.cc
index a4fbbeb..40b98db 100644
--- a/fake_system_state.cc
+++ b/fake_system_state.cc
@@ -15,6 +15,7 @@
: mock_connection_manager_(this),
mock_update_attempter_(this, &dbus_),
default_request_params_(this),
+ fake_policy_manager_(&fake_clock_),
clock_(&fake_clock_),
connection_manager_(&mock_connection_manager_),
hardware_(&fake_hardware_),
diff --git a/policy_manager/evaluation_context.cc b/policy_manager/evaluation_context.cc
index 90855ad..03238cf 100644
--- a/policy_manager/evaluation_context.cc
+++ b/policy_manager/evaluation_context.cc
@@ -7,10 +7,18 @@
#include <base/bind.h>
using base::Closure;
+using base::Time;
using base::TimeDelta;
+using chromeos_update_engine::ClockInterface;
namespace chromeos_policy_manager {
+EvaluationContext::EvaluationContext(ClockInterface* clock)
+ : clock_(clock),
+ weak_ptr_factory_(this) {
+ ResetEvaluation();
+}
+
EvaluationContext::~EvaluationContext() {
RemoveObserversAndTimeout();
}
@@ -25,9 +33,7 @@
}
TimeDelta EvaluationContext::RemainingTime() const {
- // TODO(deymo): Return a timeout based on the elapsed time on the current
- // policy request evaluation.
- return TimeDelta::FromSeconds(1.);
+ return evaluation_monotonic_deadline_ - clock_->GetMonotonicTime();
}
void EvaluationContext::ValueChanged(BaseVariable* var) {
@@ -43,6 +49,32 @@
void EvaluationContext::OnValueChangedOrPollTimeout() {
RemoveObserversAndTimeout();
+
+ if (value_changed_callback_.get() != NULL) {
+ value_changed_callback_->Run();
+ value_changed_callback_.reset();
+ }
+}
+
+bool EvaluationContext::IsTimeGreaterThan(base::Time timestamp) {
+ if (evaluation_start_ > timestamp)
+ return true;
+ // We need to keep track of these calls to trigger a reevaluation.
+ if (reevaluation_time_ > timestamp)
+ reevaluation_time_ = timestamp;
+ return false;
+}
+
+void EvaluationContext::ResetEvaluation() {
+ // It is not important if these two values are not in sync. The first value is
+ // a reference in time when the evaluation started, to device time-based
+ // values for the current evaluation. The second is a deadline for the
+ // evaluation which required a monotonic source of time.
+ evaluation_start_ = clock_->GetWallclockTime();
+ evaluation_monotonic_deadline_ =
+ clock_->GetMonotonicTime() + evaluation_timeout_;
+ reevaluation_time_ = Time::Max();
+
// Remove the cached values of non-const variables
for (auto it = value_cache_.begin(); it != value_cache_.end(); ) {
if (it->first->GetMode() == kVariableModeConst) {
@@ -51,11 +83,6 @@
it = value_cache_.erase(it);
}
}
-
- if (value_changed_callback_.get() != NULL) {
- value_changed_callback_->Run();
- value_changed_callback_.reset();
- }
}
bool EvaluationContext::RunOnValueChangeOrTimeout(Closure callback) {
@@ -63,6 +90,13 @@
bool reeval_timeout_set = false;
bool waiting_for_value_change = false;
+ // Check if a reevaluation should be triggered due to a IsTimeGreaterThan()
+ // call.
+ if (reevaluation_time_ != Time::Max()) {
+ reeval_timeout = reevaluation_time_ - evaluation_start_;
+ reeval_timeout_set = true;
+ }
+
if (value_changed_callback_.get() != NULL) {
LOG(ERROR) << "RunOnValueChangeOrTimeout called more than once.";
return false;
diff --git a/policy_manager/evaluation_context.h b/policy_manager/evaluation_context.h
index dc7e53f..b08e296 100644
--- a/policy_manager/evaluation_context.h
+++ b/policy_manager/evaluation_context.h
@@ -12,6 +12,7 @@
#include <base/memory/weak_ptr.h>
#include <base/time/time.h>
+#include "update_engine/clock_interface.h"
#include "update_engine/policy_manager/boxed_value.h"
#include "update_engine/policy_manager/event_loop.h"
#include "update_engine/policy_manager/variable.h"
@@ -21,22 +22,59 @@
// The EvaluationContext class is the interface between a policy implementation
// and the state. The EvaluationContext tracks the variables used by a policy
// request and caches the returned values, owning those cached values.
+// The same EvaluationContext should be re-used for all the evaluations of the
+// same policy request (an AsyncPolicyRequest might involve several
+// re-evaluations). Each evaluation of the EvaluationContext is run at a given
+// point in time, which is used as a reference for the evaluation timeout and
+// the time based queries of the policy, such as IsTimeGreaterThan().
+//
+// Example:
+//
+// scoped_refptr<EvaluationContext> ec = new EvaluationContext;
+//
+// ...
+// // The following call to ResetEvaluation() is optional. Use it to reset the
+// // evaluation time if the EvaluationContext isn't used right after its
+// // construction.
+// ec->ResetEvaluation();
+// EvalStatus status = policy->SomeMethod(ec, state, &result, args...);
+//
+// ...
+// // Run a closure when any of the used async variables changes its value or
+// // the timeout for requery the values happens again.
+// ec->RunOnValueChangeOrTimeout(closure);
+// // If the provided |closure| wants to re-evaluate the policy, it should
+// // call ec->ResetEvaluation() to start a new evaluation.
+//
class EvaluationContext :
public base::RefCounted<EvaluationContext>,
private BaseVariable::ObserverInterface {
public:
- EvaluationContext() : weak_ptr_factory_(this) {}
+ explicit EvaluationContext(chromeos_update_engine::ClockInterface* clock);
~EvaluationContext();
// Returns a pointer to the value returned by the passed variable |var|. The
// EvaluationContext instance keeps the ownership of the returned object. The
- // returned object is valid during the life of the EvaluationContext, even if
- // the passed Variable changes it.
+ // returned object is valid during the life of the evaluation, even if the
+ // passed Variable changes it.
//
// In case of error, a NULL value is returned.
template<typename T>
const T* GetValue(Variable<T>* var);
+ // Returns whether the passed |timestamp| is greater than the evaluation
+ // time. The |timestamp| value should be in the same scale as the values
+ // returned by ClockInterface::GetWallclockTime().
+ bool IsTimeGreaterThan(base::Time timestamp);
+
+ // TODO(deymo): Move the following methods to an interface only visible by the
+ // PolicyManager class and not the policy implementations.
+
+ // Resets the EvaluationContext to its initial state removing all the
+ // non-const cached variables and re-setting the evaluation time. This should
+ // be called right before any new evaluation starts.
+ void ResetEvaluation();
+
// Schedules the passed |callback| closure to be called when a cached
// variable changes its value or a polling interval passes. If none of these
// events can happen, for example if there's no cached variable, this method
@@ -77,6 +115,31 @@
// Used to cancel the timeout callback.
EventId poll_timeout_event_ = kEventIdNull;
+ // Pointer to the mockable clock interface;
+ chromeos_update_engine::ClockInterface* clock_;
+
+ // The timestamp when the evaluation of this EvaluationContext started. This
+ // value is reset every time ResetEvaluation() is called. The time source
+ // used is the ClockInterface::GetWallclockTime().
+ base::Time evaluation_start_;
+
+ // The timestamp measured on the GetWallclockTime() scale, when a reevaluation
+ // should be triggered due to IsTimeGreaterThan() calls value changes. This
+ // timestamp is greater or equal to |evaluation_start_| since it is a
+ // timestamp in the future, but it can be lower than the current
+ // GetWallclockTime() at some point of the evaluation.
+ base::Time reevaluation_time_;
+
+ // The timeout of an evaluation, used to compute the RemainingTime() of an
+ // evaluation.
+ // TODO(deymo): Receive the timeout from the PolicyManager. crbug.com/363790
+ base::TimeDelta evaluation_timeout_ = base::TimeDelta::FromSeconds(5);
+
+ // The timestamp in the ClockInterface::GetMonotonicTime() scale at which the
+ // current evaluation should finish. This is used to compute the
+ // RemainingTime().
+ base::Time evaluation_monotonic_deadline_;
+
base::WeakPtrFactory<EvaluationContext> weak_ptr_factory_;
DISALLOW_COPY_AND_ASSIGN(EvaluationContext);
diff --git a/policy_manager/evaluation_context_unittest.cc b/policy_manager/evaluation_context_unittest.cc
index 6fad0ce..249a2d5 100644
--- a/policy_manager/evaluation_context_unittest.cc
+++ b/policy_manager/evaluation_context_unittest.cc
@@ -8,17 +8,24 @@
#include <base/memory/scoped_ptr.h>
#include <gtest/gtest.h>
+#include "update_engine/fake_clock.h"
#include "update_engine/policy_manager/evaluation_context.h"
#include "update_engine/policy_manager/fake_variable.h"
#include "update_engine/policy_manager/generic_variables.h"
+#include "update_engine/policy_manager/mock_variable.h"
#include "update_engine/policy_manager/pmtest_utils.h"
#include "update_engine/test_utils.h"
using base::Bind;
+using base::Time;
using base::TimeDelta;
+using chromeos_update_engine::FakeClock;
using chromeos_update_engine::RunGMainLoopMaxIterations;
using chromeos_update_engine::RunGMainLoopUntil;
using std::string;
+using testing::Return;
+using testing::StrictMock;
+using testing::_;
namespace {
@@ -38,12 +45,12 @@
namespace chromeos_policy_manager {
class PmEvaluationContextTest : public ::testing::Test {
- public:
- PmEvaluationContextTest() {}
-
protected:
virtual void SetUp() {
- eval_ctx_ = new EvaluationContext();
+ // Set the clock to a fixed values.
+ fake_clock_.SetMonotonicTime(Time::FromInternalValue(12345678L));
+ fake_clock_.SetWallclockTime(Time::FromInternalValue(12345678901234L));
+ eval_ctx_ = new EvaluationContext(&fake_clock_);
}
virtual void TearDown() {
@@ -55,6 +62,11 @@
EXPECT_TRUE(fake_poll_var_.observer_list_.empty());
}
+ // TODO(deymo): Update the default timeout to the one passed on construction.
+ // See crbug.com/363790
+ base::TimeDelta default_timeout_ = base::TimeDelta::FromSeconds(5);
+
+ FakeClock fake_clock_;
scoped_refptr<EvaluationContext> eval_ctx_;
// FakeVariables used for testing the EvaluationContext. These are required
@@ -65,6 +77,10 @@
FakeVariable<string> fake_const_var_ = {"fake_const", kVariableModeConst};
FakeVariable<string> fake_poll_var_ = {"fake_poll",
TimeDelta::FromSeconds(1)};
+ StrictMock<MockVariable<string>> mock_var_async_{"mock_var_async",
+ kVariableModeAsync};
+ StrictMock<MockVariable<string>> mock_var_poll_{"mock_var_poll",
+ kVariableModePoll};
};
TEST_F(PmEvaluationContextTest, GetValueFails) {
@@ -231,4 +247,63 @@
EXPECT_FALSE(value);
}
+TEST_F(PmEvaluationContextTest, DefaultTimeout) {
+ // Test that the RemainingTime() uses the default timeout on setup.
+ EXPECT_CALL(mock_var_async_, GetValue(default_timeout_, _))
+ .WillOnce(Return(nullptr));
+ PMTEST_EXPECT_NULL(eval_ctx_->GetValue(&mock_var_async_));
+}
+
+TEST_F(PmEvaluationContextTest, TimeoutUpdatesWithMonotonicTime) {
+ fake_clock_.SetMonotonicTime(
+ fake_clock_.GetMonotonicTime() + TimeDelta::FromSeconds(1));
+
+ TimeDelta timeout = default_timeout_ - TimeDelta::FromSeconds(1);
+
+ EXPECT_CALL(mock_var_async_, GetValue(timeout, _))
+ .WillOnce(Return(nullptr));
+ PMTEST_EXPECT_NULL(eval_ctx_->GetValue(&mock_var_async_));
+}
+
+TEST_F(PmEvaluationContextTest, ResetEvaluationResetsTimes) {
+ base::Time cur_time = fake_clock_.GetWallclockTime();
+ // Advance the time on the clock but don't call ResetEvaluation yet.
+ fake_clock_.SetWallclockTime(cur_time + TimeDelta::FromSeconds(4));
+
+ EXPECT_TRUE(eval_ctx_->IsTimeGreaterThan(cur_time -
+ TimeDelta::FromSeconds(1)));
+ EXPECT_FALSE(eval_ctx_->IsTimeGreaterThan(cur_time));
+ EXPECT_FALSE(eval_ctx_->IsTimeGreaterThan(cur_time +
+ TimeDelta::FromSeconds(1)));
+ // Call ResetEvaluation now, which should use the new evaluation time.
+ eval_ctx_->ResetEvaluation();
+
+ cur_time = fake_clock_.GetWallclockTime();
+ EXPECT_TRUE(eval_ctx_->IsTimeGreaterThan(cur_time -
+ TimeDelta::FromSeconds(1)));
+ EXPECT_FALSE(eval_ctx_->IsTimeGreaterThan(cur_time));
+ EXPECT_FALSE(eval_ctx_->IsTimeGreaterThan(cur_time +
+ TimeDelta::FromSeconds(1)));
+}
+
+TEST_F(PmEvaluationContextTest, IsTimeGreaterThanSignalsTriggerReevaluation) {
+ EXPECT_FALSE(eval_ctx_->IsTimeGreaterThan(
+ fake_clock_.GetWallclockTime() + TimeDelta::FromSeconds(1)));
+
+ // The "false" from IsTimeGreaterThan means that's not that timestamp yet, so
+ // this should schedule a callback for when that happens.
+ EXPECT_TRUE(eval_ctx_->RunOnValueChangeOrTimeout(Bind(&DoNothing)));
+}
+
+TEST_F(PmEvaluationContextTest, IsTimeGreaterThanDoesntRecordPastTimestamps) {
+ // IsTimeGreaterThan() should ignore timestamps on the past for reevaluation.
+ EXPECT_TRUE(eval_ctx_->IsTimeGreaterThan(
+ fake_clock_.GetWallclockTime() - TimeDelta::FromSeconds(20)));
+ EXPECT_TRUE(eval_ctx_->IsTimeGreaterThan(
+ fake_clock_.GetWallclockTime() - TimeDelta::FromSeconds(1)));
+
+ // Callback should not be scheduled.
+ EXPECT_FALSE(eval_ctx_->RunOnValueChangeOrTimeout(Bind(&DoNothing)));
+}
+
} // namespace chromeos_policy_manager
diff --git a/policy_manager/fake_policy_manager.h b/policy_manager/fake_policy_manager.h
index 6b126cb..2b6ea3a 100644
--- a/policy_manager/fake_policy_manager.h
+++ b/policy_manager/fake_policy_manager.h
@@ -13,7 +13,8 @@
class FakePolicyManager : public PolicyManager {
public:
- FakePolicyManager() {
+ explicit FakePolicyManager(chromeos_update_engine::ClockInterface* clock)
+ : PolicyManager(clock) {
// The FakePolicyManager uses a DefaultPolicy.
set_policy(new DefaultPolicy());
}
diff --git a/policy_manager/mock_variable.h b/policy_manager/mock_variable.h
new file mode 100644
index 0000000..73ae8d9
--- /dev/null
+++ b/policy_manager/mock_variable.h
@@ -0,0 +1,28 @@
+// Copyright 2014 The Chromium OS Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_MOCK_VARIABLE_H_
+#define CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_MOCK_VARIABLE_H_
+
+#include <gmock/gmock.h>
+
+#include "update_engine/policy_manager/variable.h"
+
+namespace chromeos_policy_manager {
+
+// This is a generic mock of the Variable class.
+template<typename T>
+class MockVariable : public Variable<T> {
+ public:
+ using Variable<T>::Variable;
+
+ MOCK_METHOD2_T(GetValue, const T*(base::TimeDelta, std::string*));
+
+ private:
+ DISALLOW_COPY_AND_ASSIGN(MockVariable);
+};
+
+} // namespace chromeos_policy_manager
+
+#endif // CHROMEOS_PLATFORM_UPDATE_ENGINE_POLICY_MANAGER_MOCK_VARIABLE_H_
diff --git a/policy_manager/policy_manager-inl.h b/policy_manager/policy_manager-inl.h
index f19df55..7ff6fa6 100644
--- a/policy_manager/policy_manager-inl.h
+++ b/policy_manager/policy_manager-inl.h
@@ -53,6 +53,7 @@
R* result,
Args... args) const,
Args... args) {
+ ec->ResetEvaluation();
R result;
EvalStatus status = EvaluatePolicy(ec, policy_method, &result, args...);
@@ -85,7 +86,7 @@
R* result,
Args... args) const,
R* result, Args... args) {
- scoped_refptr<EvaluationContext> ec(new EvaluationContext);
+ scoped_refptr<EvaluationContext> ec(new EvaluationContext(clock_));
// A PolicyRequest allways consists on a single evaluation on a new
// EvaluationContext.
return EvaluatePolicy(ec, policy_method, result, args...);
@@ -100,7 +101,7 @@
R* result,
Args... args) const,
Args... args) {
- scoped_refptr<EvaluationContext> ec = new EvaluationContext;
+ scoped_refptr<EvaluationContext> ec = new EvaluationContext(clock_);
base::Closure closure = base::Bind(
&PolicyManager::OnPolicyReadyToEvaluate<R, Args...>,
base::Unretained(this), ec, callback, policy_method, args...);
diff --git a/policy_manager/policy_manager.h b/policy_manager/policy_manager.h
index d874e9c..3be8437 100644
--- a/policy_manager/policy_manager.h
+++ b/policy_manager/policy_manager.h
@@ -9,6 +9,7 @@
#include <base/memory/ref_counted.h>
#include <base/memory/scoped_ptr.h>
+#include "update_engine/clock_interface.h"
#include "update_engine/policy_manager/default_policy.h"
#include "update_engine/policy_manager/policy.h"
#include "update_engine/policy_manager/state.h"
@@ -18,7 +19,8 @@
// The main Policy Manager singleton class.
class PolicyManager {
public:
- PolicyManager() {}
+ explicit PolicyManager(chromeos_update_engine::ClockInterface* clock)
+ : clock_(clock) {}
// Initializes the PolicyManager instance, assuming ownership on the provided
// |state|, which is assumed to be pre-initialized. Returns whether the
@@ -122,6 +124,9 @@
// State Providers.
scoped_ptr<State> state_;
+ // Pointer to the mockable clock interface;
+ chromeos_update_engine::ClockInterface* clock_;
+
DISALLOW_COPY_AND_ASSIGN(PolicyManager);
};
diff --git a/policy_manager/policy_manager_unittest.cc b/policy_manager/policy_manager_unittest.cc
index 1562727..9632609 100644
--- a/policy_manager/policy_manager_unittest.cc
+++ b/policy_manager/policy_manager_unittest.cc
@@ -12,6 +12,7 @@
#include <gtest/gtest.h>
#include <gmock/gmock.h>
+#include "update_engine/fake_clock.h"
#include "update_engine/policy_manager/default_policy.h"
#include "update_engine/policy_manager/fake_state.h"
#include "update_engine/policy_manager/mock_policy.h"
@@ -21,6 +22,7 @@
using base::Bind;
using base::Callback;
+using chromeos_update_engine::FakeClock;
using std::pair;
using std::string;
using std::vector;
@@ -31,6 +33,9 @@
namespace chromeos_policy_manager {
class PmPolicyManagerTest : public ::testing::Test {
+ public:
+ PmPolicyManagerTest() : pmut_(&fake_clock_) {}
+
protected:
virtual void SetUp() {
fake_state_ = FakeState::Construct();
@@ -39,6 +44,7 @@
}
FakeState* fake_state_;
+ FakeClock fake_clock_;
PolicyManager pmut_;
};
diff --git a/real_system_state.cc b/real_system_state.cc
index b06830f..9964155 100644
--- a/real_system_state.cc
+++ b/real_system_state.cc
@@ -17,6 +17,7 @@
connection_manager_(this),
update_attempter_(this, &dbus_),
request_params_(this),
+ policy_manager_(&clock_),
system_rebooted_(false) {}
bool RealSystemState::Initialize(bool enable_gpio) {