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

✨ Computational backend: set container limits as labels ⚠️ (devops checks on grafana dashboards!) 🚨 #4453

Merged

Conversation

sanderegg
Copy link
Member

@sanderegg sanderegg commented Jul 3, 2023

🚨 check when releasing to staging and prod

  • legacy dynamic services need to be shutdown
  • ask ANE to have a look after the release and point him to this PR

What do these changes do?

  • add container cpu limits and memory limits on container (both for dynamic-sidecar services and computational services)
  • uniformize labels for dynamic-sidecar services + computational services
  • all labels used now are prefixed with io.simcore.runtime. (as in io.simcore.runtime.node-id or io.simcore.runtime.project-id) to properly differentiate from simcore image labels which are static
  • backwards compatible with old nomenclature (which should be removed once this PR reaches production + 1)
  • simplified dynamic-sidecar labels by cleaning up unused or misused ones
  • ⚠️: possibly some grafana dashboards might need to be adapted. (i.e. simcore_user_agent changed to io.simcore.runtime.simcore-user-agent)
  • ⚠️: any running legacy dynamic service (i.e. running via director-v0) are NOT backwards compatible. I did not take care of them as almost no one is using them for production work.

NOTE that underscores are not considered as valid docker label keys!

legacy services now:

image

computational service now:

image

dynamic-services now:

show the same labels in service and container labels.
NOTE: io.simcore.scheduler-data is still the same. I did not dare touch this now.
NOTE2: docker network/volumes labels are not changed. I will let @GitHK take care of this or when I get again an incentive to touch this.

Related issue/s

How to test

DevOps Checklist

@sanderegg sanderegg added a:webserver issue related to the webserver service a:director-v2 issue related with the director-v2 service a:models-library labels Jul 3, 2023
@sanderegg sanderegg added this to the Watermelon milestone Jul 3, 2023
@sanderegg sanderegg self-assigned this Jul 3, 2023
@sanderegg sanderegg changed the title Comp backend/change label names Computational backend: set container limits as labels Jul 4, 2023
@sanderegg sanderegg changed the title Computational backend: set container limits as labels ✨ Computational backend: set container limits as labels Jul 4, 2023
@sanderegg sanderegg force-pushed the comp_backend/change-label-names branch from 68a20fc to 9f60948 Compare July 4, 2023 13:04
@codecov
Copy link

codecov bot commented Jul 4, 2023

Codecov Report

Merging #4453 (1396d73) into master (bef80bb) will decrease coverage by 0.1%.
The diff coverage is 91.3%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #4453     +/-   ##
========================================
- Coverage    86.7%   86.6%   -0.1%     
========================================
  Files         867     685    -182     
  Lines       39204   34142   -5062     
  Branches      745     467    -278     
========================================
- Hits        34006   29583   -4423     
+ Misses       5022    4448    -574     
+ Partials      176     111     -65     
Flag Coverage Δ
integrationtests 78.1% <84.5%> (+10.0%) ⬆️
unittests 84.9% <87.0%> (+0.8%) ⬆️

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

Impacted Files Coverage Δ
..._v2/modules/dynamic_sidecar/docker_api/__init__.py 100.0% <ø> (ø)
...modules/dynamic_sidecar/scheduler/_core/_events.py 96.7% <50.0%> (-0.1%) ⬇️
...s/dynamic_sidecar/docker_service_specs/settings.py 81.8% <78.5%> (+0.5%) ⬆️
...tor_v2/modules/dynamic_sidecar/docker_api/_core.py 95.6% <81.3%> (-0.5%) ⬇️
.../director/src/simcore_service_director/producer.py 67.4% <87.5%> (+0.2%) ⬆️
...ckages/models-library/src/models_library/docker.py 90.7% <97.2%> (+4.5%) ⬆️
...rary/src/models_library/service_settings_labels.py 97.6% <100.0%> (+0.1%) ⬆️
.../src/simcore_service_autoscaling/utils/rabbitmq.py 100.0% <100.0%> (ø)
...ctor_v2/models/schemas/dynamic_services/service.py 100.0% <100.0%> (ø)
...simcore_service_director_v2/modules/dask_client.py 92.9% <100.0%> (ø)
... and 5 more

... and 285 files with indirect coverage changes

@sanderegg sanderegg marked this pull request as ready for review July 5, 2023 20:25
@sanderegg sanderegg requested review from mrnicegyu11 and mguidon July 5, 2023 20:25
@sanderegg sanderegg changed the title ✨ Computational backend: set container limits as labels ✨ Computational backend: set container limits as labels ⚠️ Jul 5, 2023
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.

👍

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.

very promissing, and very good for cleanup pruposes on the labels! thanks a lot!

I have only added minors and some questions for my learndings.

Please add a big verbose devops warning to the title of the PR etc. , for sure we have to modify some PromQl in the grafan dashboards, and I will prepare an ops-commit once this is in master (and I see what breaks) that shall propagate with this PR

@sanderegg sanderegg changed the title ✨ Computational backend: set container limits as labels ⚠️ ✨ Computational backend: set container limits as labels ⚠️ (devops checks on grafana dashboards!) Jul 6, 2023
@sanderegg sanderegg force-pushed the comp_backend/change-label-names branch from 1c57f2a to d315476 Compare July 6, 2023 07:24
Copy link
Contributor

@GitHK GitHK left a comment

Choose a reason for hiding this comment

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

So it's looking good. I have expressed some concerns below.
Also, did you test that service composed of multiple containers like s4l work as expected?

@codeclimate
Copy link

codeclimate bot commented Jul 6, 2023

Code Climate has analyzed commit 1396d73 and detected 0 issues on this pull request.

View more on Code Climate.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 6, 2023

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.0% 0.0% Duplication

@sanderegg
Copy link
Member Author

So it's looking good. I have expressed some concerns below. Also, did you test that service composed of multiple containers like s4l work as expected?

I tried iSeg

@sanderegg sanderegg merged commit 5f8fe65 into ITISFoundation:master Jul 6, 2023
@sanderegg sanderegg deleted the comp_backend/change-label-names branch July 6, 2023 08:55
@GitHK GitHK changed the title ✨ Computational backend: set container limits as labels ⚠️ (devops checks on grafana dashboards!) ✨ Computational backend: set container limits as labels ⚠️ (devops checks on grafana dashboards!) 🚨 Jul 6, 2023
mrnicegyu11 pushed a commit to mrnicegyu11/osparc-ops-environments that referenced this pull request Aug 22, 2023
mrnicegyu11 added a commit to ITISFoundation/osparc-ops-environments that referenced this pull request Aug 22, 2023
…bels (#322)

* Remove ceph scrape (was broken) on osparc-public

* Adress DevOps Changes ITISFoundation/osparc-simcore#4453

---------

Co-authored-by: kaiser <[email protected]>
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 a:models-library a:webserver issue related to the webserver service
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Maintenance: adjust the labels set on all services to follow inversed url notation
4 participants