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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

(鈿狅笍 devops) 馃悰 Adding missing custom constraints #3017

Merged
merged 16 commits into from
May 4, 2022

Conversation

GitHK
Copy link
Contributor

@GitHK GitHK commented May 3, 2022

(鈿狅笍 devops)

On each deployment a new env var DIRECTOR_V2_SERVICES_CUSTOM_CONSTRAINTS should be defined on director-v2. The value should be taken from the DIRECTOR_SERVICES_CUSTOM_CONSTRAINTS in director and formatted to a json list. Example: from one==yes -> ["one==yes"]

What do these changes do?

  • dynamic-sidecar new takes into consideration custom placement constraints
  • added image name to dynamic-sidecar labels to make searching for services simpler
  • if director-v2 cannot contact the dynamic-sidecar to save the state, it will no longer remove the dynamic-sidecar, giving the user an opportunity to save the state
  • added utility scripts to dynamic-sidecar for saving states and pushing output ports when ssh into the container

Related issue/s

How to test

Checklist

@codecov
Copy link

codecov bot commented May 3, 2022

Codecov Report

Merging #3017 (4fa1364) into master (d851b99) will decrease coverage by 0.0%.
The diff coverage is 80.0%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #3017     +/-   ##
========================================
- Coverage    79.7%   79.7%   -0.1%     
========================================
  Files         693     693             
  Lines       29049   29052      +3     
  Branches     3745    3745             
========================================
+ Hits        23154   23155      +1     
  Misses       5062    5062             
- Partials      833     835      +2     
Flag Coverage 螖
integrationtests 65.8% <80.0%> (-0.1%) 猬囷笍
unittests 75.5% <80.0%> (+<0.1%) 猬嗭笍

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

Impacted Files Coverage 螖
...es/dynamic_sidecar/docker_service_specs/sidecar.py 78.5% <酶> (酶)
...tor_v2/modules/dynamic_sidecar/scheduler/events.py 92.2% <0.0%> (-1.7%) 猬囷笍
...2/src/simcore_service_director_v2/core/settings.py 98.7% <100.0%> (+<0.1%) 猬嗭笍
...mcore_service_webserver/garbage_collector_utils.py 79.4% <0.0%> (-3.0%) 猬囷笍
..._director_v2/modules/dynamic_sidecar/client_api.py 77.3% <0.0%> (-1.1%) 猬囷笍
.../director/src/simcore_service_director/producer.py 62.3% <0.0%> (+0.6%) 猬嗭笍
...c/simcore_service_catalog/core/background_tasks.py 68.4% <0.0%> (+2.1%) 猬嗭笍

@GitHK GitHK self-assigned this May 3, 2022
@GitHK GitHK added changelog:馃悰bugfix t:enhancement Improvement or request on an existing feature labels May 3, 2022
@GitHK GitHK requested review from mrnicegyu11 and Surfict May 3, 2022 10:36
@GitHK GitHK added this to the Macarons +1 milestone May 3, 2022
@GitHK GitHK changed the title 馃悰 Adding missing custom constraints (鈿狅笍 devops) 馃悰 Adding missing custom constraints May 3, 2022
@GitHK GitHK requested a review from pcrespov May 3, 2022 12:33
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.

great! thanks!

DIRECTOR_V2_SERVICES_CUSTOM_CONSTRAINTS: str = Field(
"",
description="added to service Placement constraint in dynamic-sidecars",
example="node.labels.region==east",
Copy link
Member

Choose a reason for hiding this comment

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

Q: did you try putting more than one constraint?

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've added a regex and made it singular, only one constraint is accepted.

Copy link
Member

Choose a reason for hiding this comment

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

well actually it might cool to put several ones...

Copy link
Member

@pcrespov pcrespov May 3, 2022

Choose a reason for hiding this comment

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

Here is an example on how it could be done combining regex and and list of strings

https://github.com/pcrespov/sandbox-python/blob/a65378079615d259832fdfec436d8a8f7ed08460/pydantic-lib/test_settings.py

and this is the associated env-file

image

so something like

DIRECTOR_V2_SERVICES_CUSTOM_CONSTRAINT: list[constr(strip_whitespace=True, regex=PATTERN)] = []

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the nice suggestion!

Copy link
Member

@pcrespov pcrespov left a comment

Choose a reason for hiding this comment

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

Please check the suggesion on the lists above. I would also add in that case a default_factory=list as default and remove the Optional so you always get a list

Copy link
Member

@mrnicegyu11 mrnicegyu11 left a comment

Choose a reason for hiding this comment

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

thanks, looks good to me

@GitHK GitHK added changelog:馃帹enhancement and removed t:enhancement Improvement or request on an existing feature labels May 4, 2022
@sonarcloud
Copy link

sonarcloud bot commented May 4, 2022

Kudos, SonarCloud Quality Gate passed!聽 聽 Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.5% 0.5% Duplication

@GitHK GitHK merged commit eec1f9e into ITISFoundation:master May 4, 2022
@GitHK GitHK deleted the add-missing-labels branch May 4, 2022 13:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants