Commit ad0262bc authored by Francis Dupont's avatar Francis Dupont
Browse files

[#1247] Addressed comments

parent f7a9b5d0
......@@ -240,7 +240,7 @@ public:
/// @param modification_time Timestamp being a lower limit for the returned
/// result set, i.e. entries later than specified time are returned.
/// @param modification_id Identifier being a lower limit for the returned
/// result set, used when two (or more) entries have the same
/// result set, used when two (or more) revisions have the same
/// modification_time.
/// @param [out] audit_entries Reference to the container where fetched audit
/// entries will be inserted.
......
......@@ -464,7 +464,7 @@ public:
/// specified by the caller.
///
/// @param exp_object_type Expected object type.
/// @param exp_modification_time Expected modification time.
/// @param exp_modification_type Expected modification type.
/// @param exp_log_message Expected log message.
/// @param server_selector Server selector to be used for next query.
/// @param new_entries_num Number of the new entries expected to be inserted.
......
......@@ -512,7 +512,7 @@ public:
/// specified by the caller.
///
/// @param exp_object_type Expected object type.
/// @param exp_modification_time Expected modification time.
/// @param exp_modification_type Expected modification type.
/// @param exp_log_message Expected log message.
/// @param server_selector Server selector to be used for next query.
/// @param new_entries_num Number of the new entries expected to be inserted.
......
......@@ -54,7 +54,7 @@ typedef boost::shared_ptr<AuditEntry> AuditEntryPtr;
/// - object_id: 123
/// - modification_type: 3 (DELETE)
/// - modification_time: "2019-01-15 15:45:23"
/// - id: 234
/// - revision id: 234
/// - log_message: "DHCPv4 subnet 123 deleted"
///
/// The subnet is instantly removed from the dhcp4_subnet table. When
......@@ -82,7 +82,7 @@ public:
/// @param object_id identifier of the modified record in this table.
/// @param modification_type type of the modification, e.g. DELETE.
/// @param modification_time time of modification for that record.
/// @param id identifier of the entry itself.
/// @param id identifier of the revision.
/// @param log_message optional log message associated with the
/// modification.
AuditEntry(const std::string& object_type,
......@@ -97,7 +97,7 @@ public:
/// @param object_type name of the table where data was modified.
/// @param object_id identifier of the modified record in this table.
/// @param modification_type type of the modification, e.g. DELETE.
/// @param id identifier of the entry itself.
/// @param id identifier of the revision.
/// @param log_message optional log message associated with the
/// modification.
AuditEntry(const std::string& object_type,
......@@ -117,7 +117,7 @@ public:
/// @param object_id identifier of the modified record in this table.
/// @param modification_type type of the modification, e.g. DELETE.
/// @param modification_time time of modification for that record.
/// @param id identifier of the entry itself.
/// @param id identifier of the revision.
/// @param log_message optional log message associated with the
/// modification.
///
......@@ -139,7 +139,7 @@ public:
/// @param object_type name of the table where data was modified.
/// @param object_id identifier of the modified record in this table.
/// @param modification_type type of the modification, e.g. DELETE.
/// @param id identifier of the entry itself.
/// @param id identifier of the revision.
/// @param log_message optional log message associated with the
/// modification.
///
......@@ -178,9 +178,9 @@ public:
return (modification_time_);
}
/// @brief Returns entry id.
/// @brief Returns revision id aka modification id.
///
/// @return Identifier of the entry.
/// @return Identifier of the revision.
uint64_t getModificationId() const {
return (id_);
}
......@@ -212,7 +212,7 @@ private:
/// @brief Modification time.
boost::posix_time::ptime modification_time_;
/// @brief Entry id.
/// @brief Revision id aka modification id.
uint64_t id_;
/// @brief Log message.
......
......@@ -227,7 +227,7 @@ AuditEntryCollectionPtr CBControlDHCPTest::callback_audit_entries_;
/// @brief Naked @c CBControlDHCPv4 class exposing protected methods.
class TestCBControlDHCPv4 : public CBControlDHCPv4 {
public:
using CBControlDHCPv4::getInitialAuditEntryTime;
using CBControlDHCPv4::getInitialAuditRevisionTime;
using CBControlDHCPv4::databaseConfigApply;
};
......@@ -460,7 +460,7 @@ public:
auto srv_cfg = CfgMgr::instance().getCurrentCfg();
// If there is an audit entry for global parameter and the parameter
// modification time is later than last audit entry time it should
// modification time is later than last audit revision time it should
// be merged.
if (hasConfigElement("dhcp4_global_parameter") &&
(getTimestamp("dhcp4_global_parameter") > lb_modification_time)) {
......@@ -472,7 +472,7 @@ public:
}
// If there is an audit entry for option definition and the definition
// modification time is later than last audit entry time it should
// modification time is later than last audit revision time it should
// be merged.
auto found_def = srv_cfg->getCfgOptionDef()->get("isc", "one");
if (hasConfigElement("dhcp4_option_def") &&
......@@ -486,7 +486,7 @@ public:
}
// If there is an audit entry for an option and the option
// modification time is later than last audit entry time it should
// modification time is later than last audit revision time it should
// be merged.
auto options = srv_cfg->getCfgOption();
auto found_opt = options->get("dhcp4", DHO_HOST_NAME);
......@@ -500,7 +500,7 @@ public:
}
// If there is an audit entry for a shared network and the network
// modification time is later than last audit entry time it should
// modification time is later than last audit revision time it should
// be merged.
auto networks = srv_cfg->getCfgSharedNetworks4();
auto found_network = networks->getByName("one");
......@@ -514,7 +514,7 @@ public:
}
// If there is an audit entry for a subnet and the subnet modification
// time is later than last audit entry time it should be merged.
// time is later than last audit revision time it should be merged.
auto subnets = srv_cfg->getCfgSubnets4();
auto found_subnet = subnets->getSubnet(1);
if (hasConfigElement("dhcp4_subnet") &&
......@@ -555,7 +555,7 @@ public:
// as if the server is starting up and fetches the entire
// configuration from the database.
ctl_.databaseConfigApply(BackendSelector::UNSPEC(), ServerSelector::ALL(),
ctl_.getInitialAuditEntryTime(),
ctl_.getInitialAuditRevisionTime(),
AuditEntryCollection());
// Commit the configuration so as it is moved from the staging
// to current.
......@@ -873,7 +873,7 @@ public:
CfgMgr::instance().setFamily(AF_INET6);
}
using CBControlDHCPv6::getInitialAuditEntryTime;
using CBControlDHCPv6::getInitialAuditRevisionTime;
using CBControlDHCPv6::databaseConfigApply;
};
......@@ -1106,7 +1106,7 @@ public:
auto srv_cfg = CfgMgr::instance().getCurrentCfg();
// If there is an audit entry for global parameter and the parameter
// modification time is later than last audit entry time it should
// modification time is later than last audit revision time it should
// be merged.
if (hasConfigElement("dhcp6_global_parameter") &&
(getTimestamp("dhcp6_global_parameter") > lb_modification_time)) {
......@@ -1118,7 +1118,7 @@ public:
}
// If there is an audit entry for option definition and the definition
// modification time is later than last audit entry time it should
// modification time is later than last audit revision time it should
// be merged.
auto found_def = srv_cfg->getCfgOptionDef()->get("isc", "one");
if (hasConfigElement("dhcp6_option_def") &&
......@@ -1132,7 +1132,7 @@ public:
}
// If there is an audit entry for an option and the option
// modification time is later than last audit entry time it should
// modification time is later than last audit revision time it should
// be merged.
auto options = srv_cfg->getCfgOption();
auto found_opt = options->get("dhcp6", D6O_BOOTFILE_URL);
......@@ -1146,7 +1146,7 @@ public:
}
// If there is an audit entry for a shared network and the network
// modification time is later than last audit entry time it should
// modification time is later than last audit revision time it should
// be merged.
auto networks = srv_cfg->getCfgSharedNetworks6();
auto found_network = networks->getByName("one");
......@@ -1160,7 +1160,7 @@ public:
}
// If there is an audit entry for a subnet and the subnet modification
// time is later than last audit entry time it should be merged.
// time is later than last audit revision time it should be merged.
auto subnets = srv_cfg->getCfgSubnets6();
auto found_subnet = subnets->getSubnet(1);
if (hasConfigElement("dhcp6_subnet") &&
......@@ -1201,7 +1201,7 @@ public:
// as if the server is starting up and fetches the entire
// configuration from the database.
ctl_.databaseConfigApply(BackendSelector::UNSPEC(), ServerSelector::ALL(),
ctl_.getInitialAuditEntryTime(),
ctl_.getInitialAuditRevisionTime(),
AuditEntryCollection());
// Commit the configuration so as it is moved from the staging
// to current.
......
......@@ -95,8 +95,8 @@ public:
/// Sets the time of the last fetched audit entry to Jan 1st, 1970,
/// with id 0.
CBControlBase()
: last_audit_entry_time_(getInitialAuditEntryTime()),
last_audit_entry_id_(0) {
: last_audit_revision_time_(getInitialAuditRevisionTime()),
last_audit_revision_id_(0) {
}
/// @brief Virtual destructor.
......@@ -109,11 +109,11 @@ public:
/// @brief Resets the state of this object.
///
/// Disconnects the configuration backends resets the recorded last
/// audit entry time and id.
/// audit revision time and id.
void reset() {
databaseConfigDisconnect();
last_audit_entry_time_ = getInitialAuditEntryTime();
last_audit_entry_id_ = 0;
last_audit_revision_time_ = getInitialAuditRevisionTime();
last_audit_revision_id_ = 0;
}
/// @brief (Re)connects to the specified configuration backends.
......@@ -195,24 +195,24 @@ public:
// If we're fetching updates we need to retrieve audit entries to see
// which objects have to be updated. If we're performing full reconfiguration
// we also need audit entries to set the last_audit_entry_time_ to the
// we also need audit entries to set the last_audit_revision_time_ to the
// time of the most recent audit entry.
/// @todo We need a separate API call for the latter case to only
/// fetch the last audit entry rather than all of them.
// Save the timestamp indicating last audit entry time.
auto lb_modification_time = last_audit_entry_time_;
// Save the identifier indicating last audit entry id.
auto lb_modification_id = last_audit_entry_id_;
// Save the timestamp indicating last audit revision time.
auto lb_modification_time = last_audit_revision_time_;
// Save the identifier indicating last audit revision id.
auto lb_modification_id = last_audit_revision_id_;
audit_entries = getMgr().getPool()->getRecentAuditEntries(backend_selector,
server_selector,
lb_modification_time,
lb_modification_id);
// Store the last audit entry time. It should be set to the most recent
// Store the last audit revision time. It should be set to the most recent
// audit entry fetched. If returned audit is empty we don't update.
updateLastAuditEntryTimeId(audit_entries);
updateLastAuditRevisionTimeId(audit_entries);
// If this is full reconfiguration we don't need the audit entries anymore.
// Let's remove them and proceed as if they don't exist.
......@@ -229,12 +229,12 @@ public:
databaseConfigApply(backend_selector, server_selector,
lb_modification_time, audit_entries);
} catch (...) {
// Revert last audit entry time and id so as we can retry
// Revert last audit revision time and id so as we can retry
// from the last successful attempt.
/// @todo Consider reverting to the initial value to reload
/// the entire configuration if the update failed.
last_audit_entry_time_ = lb_modification_time;
last_audit_entry_id_ = lb_modification_id;
last_audit_revision_time_ = lb_modification_time;
last_audit_revision_id_ = lb_modification_id;
throw;
}
}
......@@ -314,10 +314,10 @@ protected:
}
/// @brief Convenience method returning initial timestamp to set the
/// @c last_audit_entry_time_ to.
/// @c last_audit_revision_time_ to.
///
/// @return Returns 1970-Jan-01 00:00:00 in local time.
static boost::posix_time::ptime getInitialAuditEntryTime() {
static boost::posix_time::ptime getInitialAuditRevisionTime() {
static boost::posix_time::ptime
initial_time(boost::gregorian::date(1970, boost::gregorian::Jan, 1));
return (initial_time);
......@@ -330,30 +330,30 @@ protected:
/// returns without updating the timestamp.
///
/// @param audit_entries reference to the collection of the fetched audit entries.
void updateLastAuditEntryTimeId(const db::AuditEntryCollection& audit_entries) {
void updateLastAuditRevisionTimeId(const db::AuditEntryCollection& audit_entries) {
// Do nothing if there are no audit entries. It is the case if
// there were no updates to the configuration.
if (audit_entries.empty()) {
return;
}
// Get the audit entries sorted by modification time and pick the
// latest entry.
// Get the audit entries sorted by modification time and id,
// and pick the latest entry.
const auto& index = audit_entries.get<db::AuditEntryModificationTimeIdTag>();
last_audit_entry_time_ = (*index.rbegin())->getModificationTime();
last_audit_entry_id_ = (*index.rbegin())->getModificationId();
last_audit_revision_time_ = (*index.rbegin())->getModificationTime();
last_audit_revision_id_ = (*index.rbegin())->getModificationId();
}
/// @brief Stores the most recent audit entry timestamp.
boost::posix_time::ptime last_audit_entry_time_;
/// @brief Stores the most recent audit revision timestamp.
boost::posix_time::ptime last_audit_revision_time_;
/// @brief Stores the most recent audit entry identifier.
/// @brief Stores the most recent audit revision identifier.
///
/// The identifier is used when two (or more) audit entries have
/// the same timestamp. It is not used by itself because timestamps
/// are more user friendly. Unfortunately old versions of MySQL do not
/// support millisecond timestamps.
uint64_t last_audit_entry_id_;
uint64_t last_audit_revision_id_;
};
/// @brief Checks if an object is in a collection od audit entries.
......
......@@ -236,7 +236,7 @@ public:
using CBControlBase<CBControlBackendMgr>::fetchConfigElement;
using CBControlBase<CBControlBackendMgr>::getMgr;
using CBControlBase<CBControlBackendMgr>::getInitialAuditEntryTime;
using CBControlBase<CBControlBackendMgr>::getInitialAuditRevisionTime;
/// @brief Constructor.
CBControl()
......@@ -295,27 +295,27 @@ public:
}
/// @brief Returns the recorded time of last audit entry.
boost::posix_time::ptime getLastAuditEntryTime() const {
return (last_audit_entry_time_);
boost::posix_time::ptime getLastAuditRevisionTime() const {
return (last_audit_revision_time_);
}
/// @brief Returns the recorded id of last audit entry.
uint64_t getLastAuditEntryId() const {
return (last_audit_entry_id_);
uint64_t getLastAuditRevisionId() const {
return (last_audit_revision_id_);
}
/// @brief Overwrites the last audit entry time.
///
/// @param last_audit_entry_time New time to be set.
void setLastAuditEntryTime(const boost::posix_time::ptime& last_audit_entry_time) {
last_audit_entry_time_ = last_audit_entry_time;
/// @param last_audit_revision_time New time to be set.
void setLastAuditRevisionTime(const boost::posix_time::ptime& last_audit_revision_time) {
last_audit_revision_time_ = last_audit_revision_time;
}
/// @brief Overwrites the last audit entry id.
/// @brief Overwrites the last audit revision id.
///
/// @param last_audit_entry_id New id to be set.
void setLastAuditEntryId(const uint64_t& last_audit_entry_id) {
last_audit_entry_id_ = last_audit_entry_id;
/// @param last_audit_revision_id New id to be set.
void setLastAuditRevisionId(const uint64_t& last_audit_revision_id) {
last_audit_revision_id_ = last_audit_revision_id;
}
/// @brief Enables the @c databaseConfigApply function to throw.
......@@ -422,10 +422,10 @@ TEST_F(CBControlBaseTest, getMgr) {
// This test verifies that last audit entry time is reset upon the
// call to CBControlBase::reset().
TEST_F(CBControlBaseTest, reset) {
cb_ctl_.setLastAuditEntryTime(timestamps_["tomorrow"]);
cb_ctl_.setLastAuditRevisionTime(timestamps_["tomorrow"]);
cb_ctl_.reset();
EXPECT_EQ(cb_ctl_.getInitialAuditEntryTime(), cb_ctl_.getLastAuditEntryTime());
EXPECT_EQ(0, cb_ctl_.getLastAuditEntryId());
EXPECT_EQ(cb_ctl_.getInitialAuditRevisionTime(), cb_ctl_.getLastAuditRevisionTime());
EXPECT_EQ(0, cb_ctl_.getLastAuditRevisionId());
}
// This test verifies that it is correctly determined what entries the
......@@ -538,8 +538,8 @@ TEST_F(CBControlBaseTest, fetchAll) {
ASSERT_EQ(BackendSelector::Type::MYSQL, cb_ctl_.getBackendSelector().getBackendType());
ASSERT_EQ(ServerSelector::Type::UNASSIGNED, cb_ctl_.getServerSelector().getType());
ASSERT_EQ(-1, cb_ctl_.getAuditEntriesNum());
EXPECT_EQ(cb_ctl_.getInitialAuditEntryTime(), cb_ctl_.getLastAuditEntryTime());
EXPECT_EQ(0, cb_ctl_.getLastAuditEntryId());
EXPECT_EQ(cb_ctl_.getInitialAuditRevisionTime(), cb_ctl_.getLastAuditRevisionTime());
EXPECT_EQ(0, cb_ctl_.getLastAuditRevisionId());
// Connect to the database and fetch the configuration.
ASSERT_NO_THROW(cb_ctl_.databaseConfigFetch(config_base));
......@@ -554,8 +554,8 @@ TEST_F(CBControlBaseTest, fetchAll) {
// Make sure that the internal timestamp is set to the most recent
// audit entry, so as the server will only later fetch config
// updates after this timestamp.
EXPECT_EQ(timestamps_["today"], cb_ctl_.getLastAuditEntryTime());
EXPECT_EQ(4567, cb_ctl_.getLastAuditEntryId());
EXPECT_EQ(timestamps_["today"], cb_ctl_.getLastAuditRevisionTime());
EXPECT_EQ(4567, cb_ctl_.getLastAuditRevisionId());
}
// This test verifies that the configuration can be fetched for a
......@@ -570,8 +570,8 @@ TEST_F(CBControlBaseTest, fetchFromServer) {
ASSERT_EQ(BackendSelector::Type::MYSQL, cb_ctl_.getBackendSelector().getBackendType());
ASSERT_EQ(ServerSelector::Type::UNASSIGNED, cb_ctl_.getServerSelector().getType());
ASSERT_EQ(-1, cb_ctl_.getAuditEntriesNum());
EXPECT_EQ(cb_ctl_.getInitialAuditEntryTime(), cb_ctl_.getLastAuditEntryTime());
EXPECT_EQ(0, cb_ctl_.getLastAuditEntryId());
EXPECT_EQ(cb_ctl_.getInitialAuditRevisionTime(), cb_ctl_.getLastAuditRevisionTime());
EXPECT_EQ(0, cb_ctl_.getLastAuditRevisionId());
ASSERT_NO_THROW(cb_ctl_.databaseConfigFetch(config_base));
......@@ -580,7 +580,7 @@ TEST_F(CBControlBaseTest, fetchFromServer) {
EXPECT_EQ(BackendSelector::Type::UNSPEC, cb_ctl_.getBackendSelector().getBackendType());
// An explicit server selector should have been used this time.
ASSERT_EQ(ServerSelector::Type::SUBSET, cb_ctl_.getServerSelector().getType());
EXPECT_EQ(cb_ctl_.getInitialAuditEntryTime(), cb_ctl_.getLastAuditEntryTime());
EXPECT_EQ(cb_ctl_.getInitialAuditRevisionTime(), cb_ctl_.getLastAuditRevisionTime());
// Make sure that the server selector used in databaseConfigFetch is
// correct.
......@@ -610,8 +610,8 @@ TEST_F(CBControlBaseTest, fetchUpdates) {
ASSERT_EQ(BackendSelector::Type::MYSQL, cb_ctl_.getBackendSelector().getBackendType());
ASSERT_EQ(ServerSelector::Type::UNASSIGNED, cb_ctl_.getServerSelector().getType());
ASSERT_EQ(-1, cb_ctl_.getAuditEntriesNum());
EXPECT_EQ(cb_ctl_.getInitialAuditEntryTime(), cb_ctl_.getLastAuditEntryTime());
EXPECT_EQ(0, cb_ctl_.getLastAuditEntryId());
EXPECT_EQ(cb_ctl_.getInitialAuditRevisionTime(), cb_ctl_.getLastAuditRevisionTime());
EXPECT_EQ(0, cb_ctl_.getLastAuditRevisionId());
ASSERT_NO_THROW(cb_ctl_.databaseConfigFetch(config_base,
CBControl::FetchMode::FETCH_UPDATE));
......@@ -623,8 +623,8 @@ TEST_F(CBControlBaseTest, fetchUpdates) {
EXPECT_EQ(BackendSelector::Type::UNSPEC, cb_ctl_.getBackendSelector().getBackendType());
EXPECT_EQ(ServerSelector::Type::ALL, cb_ctl_.getServerSelector().getType());
// The last audit entry time should be set to the latest audit entry.
EXPECT_EQ(timestamps_["today"], cb_ctl_.getLastAuditEntryTime());
EXPECT_EQ(4567, cb_ctl_.getLastAuditEntryId());
EXPECT_EQ(timestamps_["today"], cb_ctl_.getLastAuditRevisionTime());
EXPECT_EQ(4567, cb_ctl_.getLastAuditRevisionId());
}
// Check that the databaseConfigApply function is not called when there
......@@ -635,8 +635,8 @@ TEST_F(CBControlBaseTest, fetchNoUpdates) {
// Set last audit entry time to the timestamp of the audit
// entry we are going to add. That means that there will be
// no new audit entries to fetch.
cb_ctl_.setLastAuditEntryTime(timestamps_["yesterday"]);
cb_ctl_.setLastAuditEntryId(4567);
cb_ctl_.setLastAuditRevisionTime(timestamps_["yesterday"]);
cb_ctl_.setLastAuditRevisionId(4567);
ASSERT_TRUE(cb_ctl_.databaseConfigConnect(config_base));
......@@ -682,8 +682,8 @@ TEST_F(CBControlBaseTest, fetchFailure) {
ASSERT_EQ(BackendSelector::Type::MYSQL, cb_ctl_.getBackendSelector().getBackendType());
ASSERT_EQ(ServerSelector::Type::UNASSIGNED, cb_ctl_.getServerSelector().getType());
ASSERT_EQ(-1, cb_ctl_.getAuditEntriesNum());
EXPECT_EQ(cb_ctl_.getInitialAuditEntryTime(), cb_ctl_.getLastAuditEntryTime());
EXPECT_EQ(0, cb_ctl_.getLastAuditEntryId());
EXPECT_EQ(cb_ctl_.getInitialAuditRevisionTime(), cb_ctl_.getLastAuditRevisionTime());
EXPECT_EQ(0, cb_ctl_.getLastAuditRevisionId());
ASSERT_THROW(cb_ctl_.databaseConfigFetch(config_base, CBControl::FetchMode::FETCH_UPDATE),
isc::Unexpected);
......@@ -696,8 +696,8 @@ TEST_F(CBControlBaseTest, fetchFailure) {
EXPECT_EQ(ServerSelector::Type::ALL, cb_ctl_.getServerSelector().getType());
// The last audit entry time should not be modified because there was a merge
// error.
EXPECT_EQ(cb_ctl_.getInitialAuditEntryTime(), cb_ctl_.getLastAuditEntryTime());
EXPECT_EQ(0, cb_ctl_.getLastAuditEntryId());
EXPECT_EQ(cb_ctl_.getInitialAuditRevisionTime(), cb_ctl_.getLastAuditRevisionTime());
EXPECT_EQ(0, cb_ctl_.getLastAuditRevisionId());
}
}
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment