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.

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

🔨 Adding monitor to director-v2 #2411

Merged
merged 92 commits into from
Jul 9, 2021

Conversation

GitHK
Copy link
Contributor

@GitHK GitHK commented Jun 30, 2021

What do these changes do?

Brings in the monitor, a component used by the director-v2 to keep track of the status of the dynamic-sidecars and trigger events in reaction to status changes. Changes coming from #1887

Related issue/s

How to test

Checklist

  • Unit tests for the changes exist
  • Runs in the swarm

@GitHK GitHK self-assigned this Jun 30, 2021
@GitHK GitHK added the a:director-v2 issue related with the director-v2 service label Jun 30, 2021
@GitHK GitHK added this to the Marmoset milestone Jun 30, 2021
@codecov
Copy link

codecov bot commented Jun 30, 2021

Codecov Report

Merging #2411 (bd7a355) into master (dd74034) will increase coverage by 0.5%.
The diff coverage is 92.9%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #2411     +/-   ##
========================================
+ Coverage    75.5%   76.0%   +0.5%     
========================================
  Files         558     571     +13     
  Lines       20780   21498    +718     
  Branches     2010    2066     +56     
========================================
+ Hits        15691   16356    +665     
- Misses       4562    4592     +30     
- Partials      527     550     +23     
Flag Coverage Δ
integrationtests 65.7% <58.4%> (-0.6%) ⬇️
unittests 70.0% <92.4%> (+0.8%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...ls-library/src/models_library/projects_nodes_io.py 96.1% <ø> (ø)
...ls-library/src/models_library/projects_nodes_ui.py 100.0% <ø> (ø)
...y/src/models_library/settings/application_bases.py 0.0% <0.0%> (ø)
...ary/src/models_library/settings/services_common.py 0.0% <0.0%> (ø)
...irector-v2/src/simcore_service_director_v2/main.py 0.0% <ø> (ø)
...irector-v2/src/simcore_service_director_v2/meta.py 75.0% <0.0%> (ø)
...simcore_service_dynamic_sidecar/core/validation.py 66.6% <ø> (ø)
...es/sidecar/src/simcore_service_sidecar/executor.py 78.4% <75.0%> (ø)
..._director_v2/modules/dynamic_sidecar/docker_api.py 85.6% <85.6%> (ø)
..._director_v2/modules/dynamic_sidecar/client_api.py 89.5% <89.5%> (ø)
... and 43 more

@GitHK GitHK changed the title Adding monitor to director-v2 🔨 Adding monitor to director-v2 Jun 30, 2021
@GitHK GitHK requested review from sanderegg and pcrespov July 8, 2021 11:45
Copy link
Member

@sanderegg sanderegg left a comment

Choose a reason for hiding this comment

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

let's be dynamic!!

return cls(**params)

class Config:
extra = Extra.allow
Copy link
Member

Choose a reason for hiding this comment

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

why allow here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because it is inheriting from DynamicSidecarServiceLabels which is configured as Extra.forid and is not able to merge extra arguments coming from other models via inheritance and additional properties.

@GitHK GitHK merged commit 58ee4cd into ITISFoundation:master Jul 9, 2021
@GitHK GitHK deleted the adding-dv2-monitor branch July 9, 2021 09:35
pcrespov added a commit that referenced this pull request Jul 14, 2021
Settings introduced in PR #2411 fail to initialize when deployed in master environment

* Disables dynamic sidecar settings

* Fixes dynamic-services vs dynamic-sidecar settings

* Adds test with envdevel

* disables scheduler in client fixture
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:director-v2 issue related with the director-v2 service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants