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

♻️ rabbitmq queue names ⚠️🚨 #5931

Merged
merged 8 commits into from
Jun 11, 2024

Conversation

GitHK
Copy link
Contributor

@GitHK GitHK commented Jun 10, 2024

devops ⚠️

Should have no impact when releasing.
TL:DR;: the names of the exclusive queues where changed, not the other queues.

What do these changes do?

NOTE: the side effect of this is that the web socket does not connect. Users see the red cloud and they cannot open projects.

  • ♻️ fixed get_rabbitmq_client_unique_name which now actually returns a unique queue name that is stable for the service slot in which is running. This means that if a container restarts, even with multiple replaces, the same name will be given.
  • ♻️ since the docker webserver image is used to run multiple services, there is a collision in the uniqueness of the "client_unique_name". All services that are run from the same base docker image get a slightly different hostname.
  • 🐛 webserver fails on boot if it cannot subscribe to a queue, no longer continues to start

Related issue/s

How to test

Dev-ops checklist

@GitHK GitHK self-assigned this Jun 10, 2024
Copy link

codecov bot commented Jun 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 79.8%. Comparing base (cafbf96) to head (23b21bf).
Report is 262 commits behind head on master.

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #5931      +/-   ##
=========================================
- Coverage    84.5%   79.8%    -4.8%     
=========================================
  Files          10     500     +490     
  Lines         214   25580   +25366     
  Branches       25       0      -25     
=========================================
+ Hits          181   20418   +20237     
- Misses         23    5162    +5139     
+ Partials       10       0      -10     
Flag Coverage Δ
integrationtests 51.0% <ø> (?)
unittests 81.5% <ø> (-3.1%) ⬇️

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

Files Coverage Δ
...server/notifications/_rabbitmq_consumers_common.py 100.0% <ø> (ø)

... and 509 files with indirect coverage changes

@GitHK GitHK marked this pull request as ready for review June 10, 2024 13:37
@GitHK GitHK added this to the South Island Iced Tea milestone Jun 10, 2024
@GitHK GitHK added a:webserver issue related to the webserver service a:services-library issues on packages/service-libs labels Jun 10, 2024
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.

thanks!

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 a lot, I see no warning signals or exclamation marks etc., so I presume it is fine if this change does not go into the next prod release (this week) but only in the next one, likely in 4-6 weeks. Is that correct?

Copy link

sonarcloud bot commented Jun 10, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.1% Duplication on New Code

See analysis details on SonarCloud

@GitHK
Copy link
Contributor Author

GitHK commented Jun 10, 2024

Thanks a lot, I see no warning signals or exclamation marks etc., so I presume it is fine if this change does not go into the next prod release (this week) but only in the next one, likely in 4-6 weeks. Is that correct?

@mrnicegyu11 after double checking with @sanderegg we do not expect any side effects when deploying. Maybe do you still want to flag it with a warning to just keep an eye on it? @matusdrobuliak66

@GitHK GitHK changed the title ♻️ rabbitmq queue names ♻️ rabbitmq queue names ⚠️ Jun 10, 2024
Copy link
Contributor

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

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

thanks

@matusdrobuliak66 matusdrobuliak66 changed the title ♻️ rabbitmq queue names ⚠️ ♻️ rabbitmq queue names ⚠️🚨 Jun 10, 2024
@GitHK GitHK added the t:maintenance Some planned maintenance work label Jun 10, 2024
@GitHK GitHK enabled auto-merge (squash) June 10, 2024 13:58
@sanderegg sanderegg disabled auto-merge June 11, 2024 06:31
@sanderegg sanderegg merged commit 9e48b49 into ITISFoundation:master Jun 11, 2024
57 checks passed
@GitHK GitHK deleted the pr-osparc-webserver-rabbit branch June 11, 2024 06:32
@odeimaiz
Copy link
Member

thanks

GitHK pushed a commit that referenced this pull request Jun 20, 2024
GitHK pushed a commit that referenced this pull request Jun 20, 2024
@GitHK GitHK mentioned this pull request Jun 21, 2024
26 tasks
@YuryHrytsuk YuryHrytsuk mentioned this pull request Jun 25, 2024
23 tasks
@@ -528,7 +528,7 @@ services:
webserver:
image: ${DOCKER_REGISTRY:-itisfoundation}/webserver:${DOCKER_IMAGE_TAG:-latest}
init: true
hostname: "{{.Node.Hostname}}-{{.Task.Slot}}"
hostname: "wb-{{.Node.Hostname}}-{{.Task.Slot}}" # the hostname is used in conjonction with other services and must be unique see https://github.com/ITISFoundation/osparc-simcore/pull/5931
Copy link
Member

@pcrespov pcrespov Jun 25, 2024

Choose a reason for hiding this comment

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

Good catch! Thx

NOTE: perhaps we can add a simple test to guarantee this along services that use the same images?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
a:services-library issues on packages/service-libs a:webserver issue related to the webserver service t:maintenance Some planned maintenance work
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants