Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

graph-builder: add plugin to parse openshift/cincinnati-graph-data #231

Merged
merged 6 commits into from
Feb 26, 2020
Merged

graph-builder: add plugin to parse openshift/cincinnati-graph-data #231

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f8ac699
dist/prow_yaml_lint: ignore the test fixtures
steveej Feb 24, 2020
592f1af
commons/testing: use log::trace instead of println
steveej Feb 24, 2020
ac2cef9
cincinnati/plugins/{node_add_remove,edge_add_remove}: make DEFAULT_KE…
steveej Feb 24, 2020
1ca63e0
cincinnati/plugins/edge_add_remove: add edge removal by regex and met…
steveej Feb 17, 2020
e661734
graph-builder/Cargo: stream feature for tokio; pretty_assertions
steveej Feb 24, 2020
09fcff9
graph-builder: add plugin to parse openshift/cincinnati-graph-data
steveej Feb 24, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
48 changes: 40 additions & 8 deletions Cargo.lock
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

116 changes: 108 additions & 8 deletions cincinnati/src/plugins/internal/edge_add_remove.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,10 @@ use crate::plugins::BoxedPlugin;
use crate::plugins::{InternalIO, InternalPlugin, InternalPluginWrapper, PluginSettings};
use crate::ReleaseId;
use async_trait::async_trait;
use failure::Fallible;
use failure::{Fallible, ResultExt};
use prometheus::Registry;

static DEFAULT_KEY_FILTER: &str = "io.openshift.upgrades.graph";
pub static DEFAULT_KEY_FILTER: &str = "io.openshift.upgrades.graph";
pub static DEFAULT_REMOVE_ALL_EDGES_VALUE: &str = "*";

#[derive(Clone, Debug, Deserialize, SmartDefault)]
Expand All @@ -19,6 +19,10 @@ pub struct EdgeAddRemovePlugin {

#[default(DEFAULT_REMOVE_ALL_EDGES_VALUE.to_string())]
pub remove_all_edges_value: String,

/// If true causes the removal of all processed metadata from the releases.
#[default(false)]
pub remove_consumed_metadata: bool,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to make this tuneable? Can we just always do it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It can be helpful for debugging to leave them in, so see what annotations are on the release. Also for now it's still off by default, which is compatible with the existing deployments.

Copy link
Member

@LalatenduMohanty LalatenduMohanty Feb 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this be used to force the plugin to discard previous processed metadata and start with fresh metadata scanning ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This controls whether the metadata which is used by this plugin is removed after processing or not. As an example, if there is a previous.remove with the value 1.2.3 metadata on a release, this plugin will remove the edge, in the graph. If remove_consumed_metadata is true in the plugin configuration, it will also remove the metadata entry with the key previous.remove.

}

#[async_trait]
Expand Down Expand Up @@ -53,7 +57,8 @@ impl PluginSettings for EdgeAddRemovePlugin {
/// 2. next
/// 2. *.remove
/// 1. previous
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems both previous and previous_regex data would be applied. Shouldn't we prefer previous_regex instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the existing syntax can coexist with the additional one, so I don't see why we should remove the existing one. What makes you think we should?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd expect we use either previous or previous_regex in blocked-edges, but current implementation allows both. This may lead to a few odd situations - lets throw error when both are specified?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't see why it would be an error. They could be used in conjunction as well. Why do you want to restrict it?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Its not an error really, just feels that it might have undesirable consequences. Maybe I'm overreacting really as I can't come up with a potential issue on this one

Copy link
Contributor Author

@steveej steveej Feb 26, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it helps, I don't see a potential issue in this part of the code either, and I've thought about it quite a bit 🙂

/// 2. next
/// 2. previous_regex
/// 3. next
///
/// This ordering has implications on the result of semantical contradictions, so that the `*.remove` labels take precedence over `*.add`.
///
Expand Down Expand Up @@ -94,11 +99,18 @@ impl EdgeAddRemovePlugin {
};
}

let previous_remove_key = format!("{}.{}", self.key_prefix, "previous.remove");
graph
.find_by_metadata_key(&format!("{}.{}", self.key_prefix, "previous.remove"))
.find_by_metadata_key(&previous_remove_key)
.into_iter()
.try_for_each(
|(to, to_version, from_csv): (ReleaseId, String, String)| -> Fallible<()> {
if self.remove_consumed_metadata {
graph
.get_metadata_as_ref_mut(&to)
.map(|metadata| metadata.remove(&previous_remove_key))?;
}

if from_csv.trim() == self.remove_all_edges_value {
let parents: Vec<daggy::EdgeIndex> = graph
.previous_releases(&to)
Expand All @@ -114,7 +126,7 @@ impl EdgeAddRemovePlugin {
try_annotate_semver_build(&mut graph, from_version, &to)?;

if let Some(from) = graph.find_by_version(&from_version) {
info!("[{}]: removing previous {}", from_version, to_version,);
info!("[{}]: removing previous {}", to_version, from_version);
handle_remove_edge!(from, to)
} else {
warn!(
Expand All @@ -127,11 +139,73 @@ impl EdgeAddRemovePlugin {
},
)?;

// Remove edges instructed by "previous.remove_regex"
let previous_remove_regex_key = format!("{}.{}", self.key_prefix, "previous.remove_regex");
graph
.find_by_metadata_key(&previous_remove_regex_key)
.into_iter()
.try_for_each(
|(to, to_version, from_regex_string): (ReleaseId, String, String)| -> Fallible<()> {
if self.remove_consumed_metadata {
graph
.get_metadata_as_ref_mut(&to)
.map(|metadata| metadata.remove(&previous_remove_regex_key))?;
}

let from_regex = regex::Regex::new(&from_regex_string)
.context(format!("Parsing {} as Regex", &from_regex_string))?;

if from_regex_string == ".*" {
let parents: Vec<daggy::EdgeIndex> = graph
.previous_releases(&to)
.map(|(edge_index, _, _)| edge_index)
.collect();

trace!(
"removing parents by regex for '{}': {:?}",
to_version,
parents
);
return graph.remove_edges_by_index(&parents);
};

let froms = graph.find_by_fn_mut(|release| {
if from_regex.is_match(release.version()) {
debug!(
"Regex '{}' matches version '{}'",
&from_regex,
release.version(),
);
true
} else {
false
}
});

for (from, from_version) in froms {
info!(
"[{}]: removing previous {} by regex",
to_version, from_version
);
handle_remove_edge!(from, to);
}

Ok(())
},
)?;

let next_remove_key = format!("{}.{}", self.key_prefix, "next.remove");
graph
.find_by_metadata_key(&format!("{}.{}", self.key_prefix, "next.remove"))
.find_by_metadata_key(&next_remove_key)
.into_iter()
.try_for_each(
|(from, from_version, to_csv): (ReleaseId, String, String)| -> Fallible<()> {
if self.remove_consumed_metadata {
graph
.get_metadata_as_ref_mut(&from)
.map(|metadata| metadata.remove(&next_remove_key))?;
}

for to_version in to_csv.split(',').map(str::trim) {
let to_version = try_annotate_semver_build(&mut graph, to_version, &from)?;
if let Some(to) = graph.find_by_version(&to_version) {
Expand Down Expand Up @@ -167,10 +241,17 @@ impl EdgeAddRemovePlugin {
};
}

let previous_add_key = format!("{}.{}", self.key_prefix, "previous.add");
graph
.find_by_metadata_key(&format!("{}.{}", self.key_prefix, "previous.add"))
.find_by_metadata_key(&previous_add_key)
.into_iter()
.try_for_each(|(to, to_version, from_csv)| -> Fallible<()> {
if self.remove_consumed_metadata {
graph
.get_metadata_as_ref_mut(&to)
.map(|metadata| metadata.remove(&previous_add_key))?;
}

for from_version in from_csv.split(',').map(str::trim) {
let from_version_annotated =
try_annotate_semver_build(&mut graph, from_version, &to)?;
Expand All @@ -191,10 +272,17 @@ impl EdgeAddRemovePlugin {
Ok(())
})?;

let next_add_key = format!("{}.{}", self.key_prefix, "next.add");
graph
.find_by_metadata_key(&format!("{}.{}", self.key_prefix, "next.add"))
.find_by_metadata_key(&next_add_key)
.into_iter()
.try_for_each(|(from, from_version, to_csv)| -> Fallible<()> {
if self.remove_consumed_metadata {
graph
.get_metadata_as_ref_mut(&from)
.map(|metadata| metadata.remove(&next_add_key))?;
}

for to_version in to_csv.split(',').map(str::trim) {
let to_version_annotated =
try_annotate_semver_build(&mut graph, to_version, &from)?;
Expand Down Expand Up @@ -290,6 +378,8 @@ mod tests {
let plugin = Box::new(EdgeAddRemovePlugin {
key_prefix,
remove_all_edges_value: DEFAULT_REMOVE_ALL_EDGES_VALUE.to_string(),

..Default::default()
});
let future_processed_graph = plugin.run_internal(InternalIO {
graph: input_graph.clone(),
Expand Down Expand Up @@ -343,6 +433,8 @@ mod tests {
let plugin = Box::new(EdgeAddRemovePlugin {
key_prefix,
remove_all_edges_value: DEFAULT_REMOVE_ALL_EDGES_VALUE.to_string(),

..Default::default()
});
let future_processed_graph = plugin.run_internal(InternalIO {
graph: input_graph,
Expand Down Expand Up @@ -405,6 +497,8 @@ mod tests {
let plugin = Box::new(EdgeAddRemovePlugin {
key_prefix,
remove_all_edges_value: DEFAULT_REMOVE_ALL_EDGES_VALUE.to_string(),

..Default::default()
});
let future_processed_graph = plugin.run_internal(InternalIO {
graph: input_graph,
Expand Down Expand Up @@ -455,6 +549,8 @@ mod tests {
let plugin = Box::new(EdgeAddRemovePlugin {
key_prefix,
remove_all_edges_value: DEFAULT_REMOVE_ALL_EDGES_VALUE.to_string(),

..Default::default()
});
let future_processed_graph = plugin.run_internal(InternalIO {
graph: input_graph,
Expand Down Expand Up @@ -508,6 +604,8 @@ mod tests {
let plugin = Box::new(EdgeAddRemovePlugin {
key_prefix,
remove_all_edges_value: DEFAULT_REMOVE_ALL_EDGES_VALUE.to_string(),

..Default::default()
});

let future_processed_graph = plugin.run_internal(InternalIO {
Expand Down Expand Up @@ -558,6 +656,8 @@ mod tests {
let plugin = Box::new(EdgeAddRemovePlugin {
key_prefix: KEY_PREFIX.to_string(),
remove_all_edges_value: DEFAULT_REMOVE_ALL_EDGES_VALUE.to_string(),

..Default::default()
});
let future_processed_graph = plugin.run_internal(InternalIO {
graph: input_graph.clone(),
Expand Down
3 changes: 2 additions & 1 deletion cincinnati/src/plugins/internal/node_remove.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,8 @@ use async_trait::async_trait;
use failure::Fallible;
use prometheus::Registry;

static DEFAULT_KEY_FILTER: &str = "io.openshift.upgrades.graph";
/// Prefix for the metadata key operations.
pub static DEFAULT_KEY_FILTER: &str = "io.openshift.upgrades.graph";

#[derive(Clone, Debug, Deserialize, SmartDefault)]
#[serde(default)]
Expand Down
Loading