Skip to content
This repository was archived by the owner on Sep 4, 2020. It is now read-only.

Feature: add application scopes framework and imp health scope#367

Merged
wonderflow merged 5 commits intooam-dev:masterfrom
wonderflow:health-scop
Oct 25, 2019
Merged

Feature: add application scopes framework and imp health scope#367
wonderflow merged 5 commits intooam-dev:masterfrom
wonderflow:health-scop

Conversation

@wonderflow
Copy link
Copy Markdown
Member

@wonderflow wonderflow commented Oct 16, 2019

This PR is mainly to add application scope framework and health scope

This PR lasts several weeks mostly because the spec is changing.

https://github.com/microsoft/hydra-spec/issues/130
https://github.com/microsoft/hydra-spec/issues/144
https://github.com/microsoft/hydra-spec/pull/133
https://github.com/microsoft/hydra-spec/pull/127
https://github.com/microsoft/hydra-spec/pull/177
https://github.com/microsoft/hydra-spec/pull/196

add application scopes framework

  • 1. add framework for network/health
  • 2. create health scope instance
  • 3. check component scope overlap
  • 4. add/remove Component to scope

add health scope

  • 1. create a sub project to implement healthscope
  • 2. periodically aggregate status of health scope
  • 3. create a service to make health scope available for others
  • 4. docs for how to use it.

This is same with #160 as we have changed repo, I have to make a new PR.

fixes #9

@wonderflow
Copy link
Copy Markdown
Member Author

It seems the version of openssl installed by base image is 1.1.0 which didn't match the requirement of the updated lib.

I'm trying to figure out whether I should use a new base image

@wonderflow wonderflow force-pushed the health-scop branch 3 times, most recently from 6004118 to dfc655e Compare October 18, 2019 05:34
@suhuruli suhuruli requested a review from technosophos October 18, 2019 17:03
@wonderflow wonderflow force-pushed the health-scop branch 2 times, most recently from 41310b6 to 330bac1 Compare October 20, 2019 10:03
@wonderflow
Copy link
Copy Markdown
Member Author

wonderflow commented Oct 20, 2019

@technosophos @hongchaodeng I think this PR is ready for review now.

I have reset all the commits and split them into four big commits. Each commit is a separated part of code so you can review them one by one.

@wonderflow wonderflow added Status: Available This is available to be picked up by someone Status: Review Needed This is in PR and needs a review labels Oct 20, 2019
@wonderflow wonderflow self-assigned this Oct 20, 2019
Comment thread Dockerfile
| cut -d: -f1 \
| sort -u \
| xargs -r apt-mark manual \
&& apt-get purge -y --auto-remove -o APT::AutoRemove::RecommendsImportant=false
Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

kube-rs 0.6.1 need openssl 1.1.1 while using apt-get in debian:stretch-slim didn't work, so I have to compile from source here.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe add the comments in the Dockerfile?

Copy link
Copy Markdown
Member

@hongchaodeng hongchaodeng left a comment

Choose a reason for hiding this comment

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

LGTM after nit.

The code is a great start to use Scope

Comment thread src/schematic/scopes/network.rs Outdated
@@ -0,0 +1,117 @@
use crate::schematic::configuration::ComponentConfiguration;
/// Network scope is defined as https://github.com/oam-dev/spec/blob/master/4.application_scopes.md#network-scope
/// Now we don't really implement network scope, this is just a framework as the spec describe.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

add TODO

Comment thread Dockerfile
| cut -d: -f1 \
| sort -u \
| xargs -r apt-mark manual \
&& apt-get purge -y --auto-remove -o APT::AutoRemove::RecommendsImportant=false
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maybe add the comments in the Dockerfile?

Comment thread healthscope/README.md Outdated
@@ -0,0 +1,197 @@
# Health Scope Controller

Health Scope Controller used for periodically check health scope crd and check the health of all the components related.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
Health Scope Controller used for periodically check health scope crd and check the health of all the components related.
HealthScope controller is used to periodically check the health of all the components and update health scope custom resources.

Comment thread healthscope/src/main.rs Outdated
};
}
}
Err(e) => log::error!("get health scope list err {}", e),
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
Err(e) => log::error!("get health scope list err {}", e),
Err(e) => error!("get health scope list err {}", e),

Comment thread healthscope/README.md Outdated

## What will health scope controller do?

1. periodically check all component health and update the CR status.
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Suggested change
1. periodically check all component health and update the CR status.
1. periodically check health status of components and update the HealthScope resource status.

Comment thread healthscope/src/main.rs Outdated
debug!("health scope aggregate loop running...");
}
//FIXME: we could change this to use an informer if we have a runtime controller queue
std::thread::sleep(std::time::Duration::from_secs(1));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

1s as default might be too frequent. Maybe 5s?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ok

Comment thread healthscope/src/main.rs Outdated
health_scope_watch.join().unwrap()
}

use std::{
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Put this in the head of file?

Comment thread healthscope/src/main.rs
}

type BoxFut = Box<dyn Future<Item = Response<Body>, Error = hyper::Error> + Send>;
fn serve_health(req: Request<Body>) -> BoxFut {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

add some comments?

Comment thread healthscope/src/main.rs
Box::new(future::ok(response))
}

fn request_health(instance_name: String) -> Result<String, Error> {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

add some comments?

Comment thread healthscope/src/main.rs Outdated
Ok(health.to_string())
}

fn aggregate_health(
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

aggregate_health -> aggregate_components_health

@hongchaodeng
Copy link
Copy Markdown
Member

ref oam-dev/spec#156

@wonderflow
Copy link
Copy Markdown
Member Author

@hongchaodeng thanks very much, all comments are fixed in the last commit

Copy link
Copy Markdown
Member

@hongchaodeng hongchaodeng left a comment

Choose a reason for hiding this comment

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

lgtm

@fibonacci1729
Copy link
Copy Markdown
Collaborator

@wonderflow i'll review this today.

Copy link
Copy Markdown
Contributor

@technosophos technosophos left a comment

Choose a reason for hiding this comment

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

LGTM.

Comment thread healthscope/src/main.rs
}
}

// FIXME kube-rs client doesn't support async call so we have to wrap it in HealthFuture here.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This looks like a solid solution while we wait for the Kube upstream.

@technosophos
Copy link
Copy Markdown
Contributor

The rebase that needs to be done is really simple, so feel free to merge as soon as you have rebased. I am excited to see this new addition!

@technosophos technosophos added Status: Needs rebase PR is awaiting a rebase and removed Status: Review Needed This is in PR and needs a review labels Oct 24, 2019
@wonderflow
Copy link
Copy Markdown
Member Author

Thanks @technosophos , I've rebased.

I noticed changes from #375 has been overrided by #366. I rebased it back here.

@wonderflow
Copy link
Copy Markdown
Member Author

CI has passed, merging

@wonderflow wonderflow merged commit 2298a35 into oam-dev:master Oct 25, 2019
@wonderflow wonderflow deleted the health-scop branch October 25, 2019 01:44
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Status: Available This is available to be picked up by someone Status: Needs rebase PR is awaiting a rebase

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support scopes

4 participants