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

🐛 adds exception rules for legacy services (⚠️ devops) #2813

Merged

Conversation

pcrespov
Copy link
Member

@pcrespov pcrespov commented Feb 9, 2022

What do these changes do?

This fix follows up from #2803

After observing master, we noticed that the GC was failing to stop old services because they would not save their state. It happens that some old services do not add that functionality and threfore the new rule introduced in #2803 needs to introduce some exceptions to support this legacy.

NOTE1: that all this functionality will be taken over by the new side-car.
NOTE2 (⚠️ devops): #2803 must not be released alone but together with this PR. Otherwise, the GC will fail to close old services (mostly used in the e2e) and will collapse resources in the cluster (as observed in master)

Related issue/s

#2803

How to test

Checklist

  • Openapi changes? make openapi-specs, git commit ... and then make version-*)
  • Database migration script? cd packages/postgres-database, make setup-commit, sc-pg review -m "my changes"
  • Unit tests for the changes exist
  • Runs in the swarm
  • Documentation reflects the changes
  • New module? Add your github username to .github/CODEOWNERS

@pcrespov pcrespov changed the title 🐛 adds exception rules for legacy services 🐛 adds exception rules for legacy services (⚠️ devops) Feb 9, 2022
@codecov
Copy link

codecov bot commented Feb 9, 2022

Codecov Report

Merging #2813 (c390b3c) into master (89c2673) will increase coverage by 0.0%.
The diff coverage is 55.5%.

Impacted file tree graph

@@          Coverage Diff           @@
##           master   #2813   +/-   ##
======================================
  Coverage    78.9%   78.9%           
======================================
  Files         670     670           
  Lines       27230   27236    +6     
  Branches     2677    2678    +1     
======================================
+ Hits        21486   21492    +6     
- Misses       4988    4989    +1     
+ Partials      756     755    -1     
Flag Coverage Δ
integrationtests 65.9% <ø> (-0.1%) ⬇️
unittests 74.4% <55.5%> (+<0.1%) ⬆️

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

Impacted Files Coverage Δ
.../director/src/simcore_service_director/producer.py 62.1% <55.5%> (-0.4%) ⬇️
...car/src/simcore_service_dask_sidecar/dask_utils.py 91.7% <0.0%> (+1.0%) ⬆️
...ce_webserver/resource_manager/websocket_manager.py 97.3% <0.0%> (+4.0%) ⬆️

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.

👍

services/director/src/simcore_service_director/producer.py Outdated Show resolved Hide resolved
@pcrespov pcrespov self-assigned this Feb 9, 2022
@pcrespov pcrespov added this to the R.Schumann milestone Feb 9, 2022
@pcrespov pcrespov force-pushed the fix/override-for-service-legacy branch from 8981fe9 to c390b3c Compare February 9, 2022 16:11
@pcrespov pcrespov merged commit 441542e into ITISFoundation:master Feb 9, 2022
@pcrespov pcrespov deleted the fix/override-for-service-legacy branch February 9, 2022 16:51
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.

5 participants