Skip to content
Snippets Groups Projects
Commit 259ff4ac authored by Georgy Moshkin's avatar Georgy Moshkin :speech_balloon:
Browse files

refactor: don't use `storage` in `update_instance`

parent a3f3ad35
No related branches found
No related tags found
1 merge request!1323fix: don't use handle_update_instance_request_and_wait in governor
Hide whitespace changes
Inline Side-by-side
src/instance.rs
+ 14
− 11
View file @ 259ff4ac
......@@ -337,6 +337,7 @@ mod tests {
let storage = Clusterwide::for_tests();
add_tier(&storage, DEFAULT_TIER, 1, true).unwrap();
let instance = add_instance(&storage, 1, "i1", "r1", &State::new(Online, 1)).unwrap();
let existing_fds = HashSet::new();
//
// Current state incarnation is allowed to go down,
......@@ -344,7 +345,7 @@ mod tests {
//
let req = rpc::update_instance::Request::new(instance.instance_id.clone(), "".into())
.with_current_state(State::new(Offline, 0));
let (dml, do_bump) = update_instance(&instance, &req, &storage).unwrap().unwrap();
let (dml, do_bump) = update_instance(&instance, &req, &existing_fds).unwrap().unwrap();
let mut ops = UpdateOps::new();
ops.assign("current_state", State::new(Offline, 0)).unwrap();
......@@ -359,7 +360,7 @@ mod tests {
//
let req = rpc::update_instance::Request::new(instance.instance_id.clone(), "".into())
.with_current_state(State::new(Offline, 0));
let dml = update_instance(&instance, &req, &storage).unwrap();
let dml = update_instance(&instance, &req, &existing_fds).unwrap();
assert_eq!(dml, None);
//
......@@ -367,7 +368,7 @@ mod tests {
//
let req = rpc::update_instance::Request::new(instance.instance_id.clone(), "".into())
.with_target_state(Offline);
let (dml, do_bump) = update_instance(&instance, &req, &storage).unwrap().unwrap();
let (dml, do_bump) = update_instance(&instance, &req, &existing_fds).unwrap().unwrap();
let mut ops = UpdateOps::new();
ops.assign("target_state", State::new(Offline, 0)).unwrap();
......@@ -382,7 +383,7 @@ mod tests {
//
let req = rpc::update_instance::Request::new(instance.instance_id.clone(), "".into())
.with_target_state(Online);
let (dml, do_bump) = update_instance(&instance, &req, &storage).unwrap().unwrap();
let (dml, do_bump) = update_instance(&instance, &req, &existing_fds).unwrap().unwrap();
let mut ops = UpdateOps::new();
ops.assign("target_state", State::new(Online, 1)).unwrap();
......@@ -397,7 +398,7 @@ mod tests {
//
let req = rpc::update_instance::Request::new(instance.instance_id.clone(), "".into())
.with_target_state(Online);
let (dml, do_bump) = update_instance(&instance, &req, &storage).unwrap().unwrap();
let (dml, do_bump) = update_instance(&instance, &req, &existing_fds).unwrap().unwrap();
let mut ops = UpdateOps::new();
ops.assign("target_state", State::new(Online, 2)).unwrap();
......@@ -412,7 +413,7 @@ mod tests {
//
let req = rpc::update_instance::Request::new(instance.instance_id.clone(), "".into())
.with_target_state(Expelled);
let (dml, do_bump) = update_instance(&instance, &req, &storage).unwrap().unwrap();
let (dml, do_bump) = update_instance(&instance, &req, &existing_fds).unwrap().unwrap();
let mut ops = UpdateOps::new();
ops.assign("target_state", State::new(Expelled, 0)).unwrap();
......@@ -427,7 +428,7 @@ mod tests {
//
let req = rpc::update_instance::Request::new(instance.instance_id.clone(), "".into())
.with_current_state(State::new(Expelled, 69));
let (dml, do_bump) = update_instance(&instance, &req, &storage).unwrap().unwrap();
let (dml, do_bump) = update_instance(&instance, &req, &existing_fds).unwrap().unwrap();
let mut ops = UpdateOps::new();
ops.assign("current_state", State::new(Expelled, 69)).unwrap();
......@@ -442,7 +443,7 @@ mod tests {
//
let req = rpc::update_instance::Request::new(instance.instance_id.clone(), "".into())
.with_target_state(Online);
let e = update_instance(&instance, &req, &storage).unwrap_err();
let e = update_instance(&instance, &req, &existing_fds).unwrap_err();
assert_eq!(e.to_string(), "cannot update expelled instance \"i1\"");
}
......@@ -516,6 +517,7 @@ mod tests {
//
let instance1 = build_instance(Some(&InstanceId::from("i1")), None, &faildoms! {planet: Earth}, &storage, DEFAULT_TIER).unwrap();
storage.instances.put(&instance1).unwrap();
let existing_fds = storage.instances.failure_domain_names().unwrap();
assert_eq!(instance1.failure_domain, faildoms! {planet: Earth});
assert_eq!(instance1.replicaset_id, "r1");
......@@ -524,7 +526,7 @@ mod tests {
//
let req = rpc::update_instance::Request::new(instance1.instance_id.clone(), "".into())
.with_failure_domain(faildoms! {owner: Ivan});
let e = update_instance(&instance1, &req, &storage).unwrap_err();
let e = update_instance(&instance1, &req, &existing_fds).unwrap_err();
assert_eq!(e.to_string(), "missing failure domain names: PLANET");
//
......@@ -533,7 +535,7 @@ mod tests {
let fd = faildoms! {planet: Mars, owner: Ivan};
let req = rpc::update_instance::Request::new(instance1.instance_id.clone(), "".into())
.with_failure_domain(fd.clone());
let (dml, do_bump) = update_instance(&instance1, &req, &storage).unwrap().unwrap();
let (dml, do_bump) = update_instance(&instance1, &req, &existing_fds).unwrap().unwrap();
let mut ops = UpdateOps::new();
ops.assign("failure_domain", fd).unwrap();
......@@ -556,6 +558,7 @@ mod tests {
#[rustfmt::skip]
let instance2 = build_instance(Some(&InstanceId::from("i2")), None, &fd, &storage, DEFAULT_TIER).unwrap();
storage.instances.put(&instance2).unwrap();
let existing_fds = storage.instances.failure_domain_names().unwrap();
assert_eq!(instance2.failure_domain, fd);
// doesn't fit into r1
assert_eq!(instance2.replicaset_id, "r2");
......@@ -566,7 +569,7 @@ mod tests {
let fd = faildoms! {planet: Earth, owner: Mike};
let req = rpc::update_instance::Request::new(instance2.instance_id.clone(), "".into())
.with_failure_domain(fd.clone());
let (dml, do_bump) = update_instance(&instance2, &req, &storage).unwrap().unwrap();
let (dml, do_bump) = update_instance(&instance2, &req, &existing_fds).unwrap().unwrap();
let mut ops = UpdateOps::new();
ops.assign("failure_domain", fd).unwrap();
......
src/rpc/update_instance.rs
+ 8
− 10
View file @ 259ff4ac
use std::time::Duration;
use crate::cas;
use crate::column_name;
use crate::failure_domain::FailureDomain;
......@@ -9,12 +7,14 @@ use crate::instance::StateVariant::*;
use crate::instance::{Instance, InstanceId};
use crate::replicaset::Replicaset;
use crate::schema::ADMIN_ID;
use crate::storage::Clusterwide;
use crate::storage::ClusterwideTable;
use crate::tier::Tier;
use crate::traft::op::{Dml, Op};
use crate::traft::Result;
use crate::traft::{error::Error, node};
use crate::util::Uppercase;
use std::collections::HashSet;
use std::time::Duration;
use tarantool::fiber;
use tarantool::space::UpdateOps;
......@@ -125,7 +125,9 @@ pub fn handle_update_instance_request_and_wait(req: Request, timeout: Duration)
return Err(Error::NoSuchReplicaset { id: replicaset_id.to_string(), id_is_uuid: false });
};
let dml = update_instance(&instance, &req, storage)?;
let existing_fds = storage.instances.failure_domain_names()?;
let dml = update_instance(&instance, &req, &existing_fds)?;
let Some((dml, version_bump_needed)) = dml else {
// No point in proposing an operation which doesn't change anything.
// Note: if the request tried setting target state Online while it
......@@ -196,7 +198,7 @@ pub fn handle_update_instance_request_and_wait(req: Request, timeout: Duration)
pub fn update_instance(
instance: &Instance,
req: &Request,
storage: &Clusterwide,
existing_fds: &HashSet<Uppercase>,
) -> Result<Option<(Dml, bool)>> {
if instance.current_state.variant == Expelled
&& !matches!(
......@@ -215,11 +217,7 @@ pub fn update_instance(
let mut ops = UpdateOps::new();
if let Some(fd) = req.failure_domain.as_ref() {
let existing_fds = storage
.instances
.failure_domain_names()
.expect("storage should not fail");
fd.check(&existing_fds)?;
fd.check(existing_fds)?;
if fd != &instance.failure_domain {
ops.assign(column_name!(Instance, failure_domain), fd)?;
}
......
src/util.rs
+ 4
− 0
View file @ 259ff4ac
......@@ -171,6 +171,10 @@ macro_rules! define_string_newtype {
////////////////////////////////////////////////////////////////////////////////
/// A wrapper around `String` that garantees the string is uppercase by
/// converting it to uppercase (if needed) on construction.
// TODO: this is too much boilerplate for such a simple feature. We should
// simplify and get rid of this struct, just use `String`, make sure it's
// uppercase after we get it from the user (arguments, config, env, etc.)
// and sprinkle `debug_assert` in places where it matters (when comparing values)
#[derive(Default, Debug, PartialEq, Eq, PartialOrd, Ord, Hash, Clone, serde::Serialize)]
pub struct Uppercase(String);
......
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