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

♻️ Remove docker node constraints from simcore repo (⚠️ DEVOPS) #4571

Conversation

YuryHrytsuk
Copy link
Contributor

@YuryHrytsuk YuryHrytsuk commented Aug 3, 2023

What do these changes do?

It removes deprecated docker deploy placement constraints (node.platform.os == linux) since we run everything in containers and it doesn't make sense at the moment.

Related issue/s

ITISFoundation/osparc-ops-environments#278

Related PRs

How to test

Run deployment locally and check that everything is up and running

DevOps Checklist

Nothing to do

@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Merging #4571 (855ba44) into master (f8aa15d) will decrease coverage by 0.2%.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #4571     +/-   ##
========================================
- Coverage    86.7%   86.5%   -0.2%     
========================================
  Files        1023    1023             
  Lines       43772   43772             
  Branches     1013    1013             
========================================
- Hits        37953   37898     -55     
- Misses       5592    5647     +55     
  Partials      227     227             
Flag Coverage Δ
integrationtests 65.3% <ø> (-1.4%) ⬇️
unittests 84.2% <ø> (+<0.1%) ⬆️

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

see 10 files with indirect coverage changes

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.

I am not sure I understand this change.
the node.platform.os == linux I agree is useless and can be removed without any issue.
But, the node.role == manager is fully justified and defines the requirements of the services.
For instance the director(s) must be on a manager node as well as the autoscaling and that is explicitely defined in the docker-compose. Additionally if I deploy that stack on multiple machines this will now fail because of that.
I do not see/understand why this is now removed.

I'm happy to discuss that.

@YuryHrytsuk
Copy link
Contributor Author

I am not sure I understand this change. the node.platform.os == linux I agree is useless and can be removed without any issue. But, the node.role == manager is fully justified and defines the requirements of the services. For instance the director(s) must be on a manager node as well as the autoscaling and that is explicitely defined in the docker-compose. Additionally if I deploy that stack on multiple machines this will now fail because of that. I do not see/understand why this is now removed.

I'm happy to discuss that.

All the placement constraints have been moved to osparc-ops-environments. The idea is to keep all the constraints in one place to avoid duplication and to make it more transparent/clear. You can find the relevant changes in this PR. All the placement constraints in osparc-simcore have been moved to osparc-ops-environments.

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.

as discussed I wait for the next change. Thanks a lot!

@YuryHrytsuk YuryHrytsuk requested a review from sanderegg August 4, 2023 08:02
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.

As discussed. thanks a lot for this.

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.

looks good to me, thanks a lot!

@sonarqubecloud
Copy link

sonarqubecloud bot commented Aug 7, 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

@codeclimate
Copy link

codeclimate bot commented Aug 7, 2023

Code Climate has analyzed commit 855ba44 and detected 0 issues on this pull request.

View more on Code Climate.

@YuryHrytsuk YuryHrytsuk enabled auto-merge (squash) August 7, 2023 13:57
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.

4 participants