Skip to content
This repository has been archived by the owner on Jul 25, 2022. It is now read-only.

agent: Add start/stop resource to rust agent #2146

Merged
merged 7 commits into from Aug 28, 2020
Merged

agent: Add start/stop resource to rust agent #2146

merged 7 commits into from Aug 28, 2020

Conversation

utopiabound
Copy link
Contributor

@utopiabound utopiabound commented Aug 10, 2020

This is to suppliment python agent start_target. It does not do any movement if the resources is started, nor does it try to force a start in a particular place.

Related #1464

Signed-off-by: Nathaniel Clark nclark@whamcloud.com


This change is Reviewable

@utopiabound utopiabound requested a review from a team August 10, 2020 23:10
@utopiabound utopiabound self-assigned this Aug 10, 2020
Copy link
Contributor

@mkpankov mkpankov left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @utopiabound)


iml-agent/src/action_plugins/high_availability/mod.rs, line 82 at r1 (raw file):

fn resource_status(tree: Element) -> Result<HashMap<String, String>, ImlAgentError> {
    let resources = tree.find("resources").unwrap();
    Ok(resources

Wrapping tens of lines in the returned value reads weird


iml-agent/src/action_plugins/high_availability/mod.rs, line 90 at r1 (raw file):

                        "resource" => e.get_attr("role").map(|r| vec![(id, r)]),
                        "clone" => {
                            // "Started" -> "Starting" -> "Stopping" -> "Stopped"

Typo?

I expected Starting to be first


iml-agent/src/action_plugins/high_availability/mod.rs, line 95 at r1 (raw file):

                                .max_by(|x, y| {
                                    if x == y {
                                        return Ordering::Equal;

This is too much nesting IMO...


iml-agent/src/action_plugins/high_availability/mod.rs, line 98 at r1 (raw file):

                                    }
                                    match (*x, *y) {
                                        (_, "Stopped") => Ordering::Greater,

This should go to impl Ord I think. Complement with impl From<&str> for State for ease of use.

ip1981
ip1981 previously approved these changes Aug 11, 2020
iml-agent/src/action_plugins/action_plugin.rs Show resolved Hide resolved
Copy link
Member

@jgrund jgrund left a comment

Choose a reason for hiding this comment

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

Format check is failing

@jgrund
Copy link
Member

jgrund commented Aug 11, 2020

Why does this supplement the python start_target instead of replacing it?

@utopiabound
Copy link
Contributor Author

Why does this supplement the python start_target instead of replacing it?

Because of all the other work that start_target does (looking for paired ZFS resources, and movement of resources)

Signed-off-by: Nathaniel Clark <nclark@whamcloud.com>
johnsonw
johnsonw previously approved these changes Aug 12, 2020
iml-agent/src/cli.rs Outdated Show resolved Hide resolved
iml-agent/src/cli.rs Outdated Show resolved Hide resolved
Signed-off-by: Nathaniel Clark <nclark@whamcloud.com>
ip1981
ip1981 previously approved these changes Aug 14, 2020
Copy link
Member

@jgrund jgrund left a comment

Choose a reason for hiding this comment

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

A few comments about Result

johnsonw
johnsonw previously approved these changes Aug 19, 2020
Signed-off-by: Nathaniel Clark <nclark@whamcloud.com>
jgrund
jgrund previously approved these changes Aug 19, 2020
johnsonw
johnsonw previously approved these changes Aug 19, 2020
Signed-off-by: Nathaniel Clark <nclark@whamcloud.com>
@utopiabound utopiabound dismissed stale reviews from johnsonw and jgrund via 41221eb August 20, 2020 13:06
Signed-off-by: Nathaniel Clark <nclark@whamcloud.com>
@johnsonw
Copy link
Contributor

Looks like there is a failure in the integration tests.

}

fn resource_status(tree: Element) -> HashMap<String, String> {
let resources = tree.find("resources").unwrap();
Copy link
Member

Choose a reason for hiding this comment

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

Nit: this is similar to what I'm doing in terms of reading crm_mon resources. We should consolidate our changes.

@jgrund jgrund merged commit 9062c8f into master Aug 28, 2020
@jgrund jgrund deleted the rust-ha branch August 28, 2020 21:00
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
5 participants