Add services to study-light profile, add depends on#618
Open
antoinebhs wants to merge 22 commits into
Open
Conversation
Signed-off-by: BOUHOURS Antoine <[email protected]>
Signed-off-by: BOUHOURS Antoine <[email protected]>
Signed-off-by: LE SAULNIER Kevin <[email protected]>
Signed-off-by: BOUHOURS Antoine <[email protected]>
Signed-off-by: BOUHOURS Antoine <[email protected]>
Signed-off-by: BOUHOURS Antoine <[email protected]>
Signed-off-by: BOUHOURS Antoine <[email protected]>
Signed-off-by: BOUHOURS Antoine <[email protected]>
Signed-off-by: BOUHOURS Antoine <[email protected]>
Signed-off-by: BOUHOURS Antoine <[email protected]>
Signed-off-by: BOUHOURS Antoine <[email protected]>
Signed-off-by: BOUHOURS Antoine <[email protected]>
Signed-off-by: BOUHOURS Antoine <[email protected]>
Signed-off-by: BOUHOURS Antoine <[email protected]>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughThis PR adds healthchecks for infrastructure services, gates many application services on healthy upstreams, reduces JVM/container memory settings across compose files, and introduces a study-light profile with a .env and include-based compose entry. ChangesHealth Checks and Service Dependencies
Study-Light Profile Composition
Documentation Updates
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docker-compose/study/docker-compose.study.yml (1)
280-302:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winVerify that UI gracefully handles missing backends or add services to
study-light.
gridstudy-appis now in thestudy-lightprofile, but several analysis and calculation servers remain outside it:geo-data-server,cgmes-gl-server,odre-server,security-analysis-server,sensitivity-analysis-server,shortcircuit-server,voltage-init-server, andtimeseries-server. If the UI exposes features that call these services, users will encounter errors rather than disabled menu items. Either confirm the light variant intentionally omits these features, or perform a functional smoke test to verify graceful degradation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose/study/docker-compose.study.yml` around lines 280 - 302, The UI container gridstudy-app was added to the study-light profile while backend services (geo-data-server, cgmes-gl-server, odre-server, security-analysis-server, sensitivity-analysis-server, shortcircuit-server, voltage-init-server, timeseries-server) remain outside that profile, which can cause runtime UI errors; either add the missing services to the study-light profile in docker-compose.study.yml so the light profile includes those backend services, or modify the UI (gridstudy-app) to detect and gracefully disable/hide features that call those specific services (check client-side code that references those service endpoints) and implement fallbacks/error-handling for absent backends. Ensure you update service profiles for each listed service or add feature-gating in the UI code paths that call endpoints for geo-data-server, cgmes-gl-server, odre-server, security-analysis-server, sensitivity-analysis-server, shortcircuit-server, voltage-init-server, and timeseries-server.
🧹 Nitpick comments (4)
docker-compose/technical/docker-compose.technical.yml (2)
9-13: ⚡ Quick winConsider adding
start_periodto healthchecks.The healthchecks have
retries: 10withinterval: 10s(≈100s tolerance), but nostart_period. During slow boots (e.g., Postgres runninginit-databases.shto bootstrap geo data / lines catalog, or Elasticsearch cluster bootstrap), early probe failures will count towardretries, which can flap dependent services that rely oncondition: service_healthy. Astart_period(e.g.,30s–60s) prevents early failures from being counted.🛠️ Example for postgres
healthcheck: test: ["CMD-SHELL", "pg_isready -U $$POSTGRES_USER -d $$POSTGRES_DEFAULT_DB"] interval: 10s timeout: 5s retries: 10 + start_period: 30sAlso applies to: 51-55, 88-92, 192-196
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose/technical/docker-compose.technical.yml` around lines 9 - 13, The RabbitMQ healthcheck block (healthcheck with test: ["CMD-SHELL", "rabbitmq-diagnostics ping"]) currently uses interval and retries but no start_period, so add a start_period (e.g., start_period: 30s–60s) to that healthcheck so early boot failures aren’t counted toward retries; apply the same change to the other healthcheck blocks referenced (the ones using healthcheck, interval, timeout, retries) to ensure dependent services using condition: service_healthy don’t flap during slow initialization.
192-196: ⚡ Quick winUse
mc ready localfor the healthcheck probe.The current
curl-based healthcheck works (curl is present in the image), but MinIO's official guidance recommends["CMD", "mc", "ready", "local"]instead. This is more reliable as it directly checks local node readiness without depending on HTTP endpoints.Consider updating line 193 to:
test: ["CMD", "mc", "ready", "local"]🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose/technical/docker-compose.technical.yml` around lines 192 - 196, The healthcheck currently uses a curl-based probe in the healthcheck.test block which relies on HTTP and is less reliable; replace the test command to use MinIO's CLI readiness check by switching the healthcheck.test from the curl invocation to use mc ready local (i.e., update the healthcheck.test entry to call ["CMD", "mc", "ready", "local"]); ensure the healthcheck section (healthcheck.test, interval, timeout, retries) remains intact and that the container image includes the mc binary so mc ready can run successfully.README.md (1)
96-113: 💤 Low valueProfile/service matrix is hard to scan — consider stable row ordering.
The table mixes per-row multi-service grouping with single-service rows, and the grouping criterion is the matching profile set. Two small polish items:
- The rows aren’t alphabetized within each grouping (e.g., line 108 lists
directory-notification-server<br>explore-server<br>gridexplore-app<br>...which is correct, but other rows like line 112 start withapps-metadata-server<br>case-server<br>...ordering — make sure ordering is consistent across rows so reviewers can diff quickly).- A
study-lightcolumn was added; consider adding a leading legend row (e.g., what(none)means vs blank cells in newly added profiles) so the asymmetry between(none)getting✅only on the infra row (line 113) and other rows being blank is explicit.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@README.md` around lines 96 - 113, The profile/service matrix is hard to scan because grouped cell lists are inconsistently ordered and the meaning of the “_(none)_” column vs blank cells is unclear; fix by alphabetizing service names within each multi-service row (e.g., rows containing directory-notification-server, explore-server, gridexplore-app, gridstudy-app, network-map-server, single-line-diagram-server, study-notification-server, study-server) to a stable lexical order and ensure all grouped rows follow the same ordering rule, and add a leading legend row above the table that explains the “_(none)_” column vs empty cells (and confirm the infra row with elasticsearch/postgres/rabbitmq/s3-storage retains the ✅ under “_(none)_” if that denotes required infra).docker-compose/docker-compose.base.yml (1)
317-321: 💤 Low valueQuote unit-suffixed memory limits to avoid YAML/Compose ambiguity.
memswap_limit: 1g,memory: 1g, and the new1280mvalues are written unquoted. Docker Compose accepts these, but mixing quoted ("512m") and unquoted forms across the same file is inconsistent and historically has bitten teams when a value like1g(with a trailing space, as on line 317/321) is parsed by older toolchains. Quoting them as strings ("1g","1280m") is the documented form and removes the trailing-space risk visible here.Docker Compose memswap_limit and deploy.resources.limits.memory accepted formats and trailing whitespace handlingAlso applies to: 354-358, 279-283
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docker-compose/docker-compose.base.yml` around lines 317 - 321, Update the YAML to quote all unit-suffixed memory values to avoid parsing ambiguity: change occurrences of memswap_limit and deploy.resources.limits.memory (and any other memory keys like those at the other noted blocks) from unquoted forms such as 1g, 1280m or 512m to quoted strings like "1g" and "1280m", ensuring no trailing spaces remain inside the quotes; apply this to the memswap_limit and deploy.resources.limits.memory entries referenced in the diff and the other blocks called out.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docker-compose/docker-compose.base.yml`:
- Around line 377-381: The docker-compose.services for config-server and
config-notification-server unintentionally lost the optional logspout ordering
dependency; restore a depends_on entry mirroring other services by adding a
logspout: { condition: service_started, required: false } under the existing
depends_on block for both config-server and config-notification-server (look for
the depends_on: postgres/rabbitmq blocks in docker-compose.base.yml and add the
logspout entry similarly, and apply the same fix at the other occurrence
referenced around lines ~409-411).
- Around line 18-29: The depends_on entry for rabbitmq under the case-server
service is too strict; remove or relax it so case-server startup isn't blocked
by transient RabbitMQ issues: locate the depends_on block for the case-server
service (the rabbitmq sub-entry) and either remove the rabbitmq dependency
entirely, or change its condition from service_healthy to "service_started" and
add required: false; alternatively verify in case-server application config
(case-server-application.yml) and code that it actually binds to Spring Cloud
Stream at boot—if it does, keep the dependency but document why it's needed.
In `@docker-compose/study-light/.env`:
- Line 1: The env flag SHOULD_INIT_GEO_DATA is enabling geo-data initialization
even though the geo-data-server service is excluded from the study-light
profile; either set SHOULD_INIT_GEO_DATA=false in the study-light .env to
prevent Postgres from loading unused geo data, or add the geo-data-server
service to the study-light profile in your docker-compose study configuration so
the data loading is actually served; locate the SHOULD_INIT_GEO_DATA entry in
the study-light .env and the geo-data-server service definition/profile key
(profile "study-light") in the docker-compose study compose file to apply one of
these two fixes.
In `@docker-compose/study/docker-compose.study.yml`:
- Around line 350-356: The three services dynamic-simulation-server,
dynamic-security-analysis-server, and dynamic-margin-calculation-server dropped
the optional logspout ordering hint; restore a depends_on entry for logspout
with condition: service_started and required: false for each of these services
(i.e., add the same "logspout: { condition: service_started, required: false }"
block alongside the existing postgres/s3-storage/rabbitmq depends_on entries) so
their startup ordering matches the other study services.
In `@README.md`:
- Line 60: The in-page anchor in the sentence containing "profiles as explained
in the [next section](`#docker-compose-profiles`)" doesn't match the header text
"### Docker-compose with profiles"; update the link target to the correct anchor
`#docker-compose-with-profiles` (or alternatively rename the header to
"Docker-compose with profiles" -> "Docker-compose profiles" to match
`#docker-compose-profiles`) so the link points to the actual "Docker-compose
with profiles" section; locate the link text in README.md and adjust the anchor
or the header text accordingly.
---
Outside diff comments:
In `@docker-compose/study/docker-compose.study.yml`:
- Around line 280-302: The UI container gridstudy-app was added to the
study-light profile while backend services (geo-data-server, cgmes-gl-server,
odre-server, security-analysis-server, sensitivity-analysis-server,
shortcircuit-server, voltage-init-server, timeseries-server) remain outside that
profile, which can cause runtime UI errors; either add the missing services to
the study-light profile in docker-compose.study.yml so the light profile
includes those backend services, or modify the UI (gridstudy-app) to detect and
gracefully disable/hide features that call those specific services (check
client-side code that references those service endpoints) and implement
fallbacks/error-handling for absent backends. Ensure you update service profiles
for each listed service or add feature-gating in the UI code paths that call
endpoints for geo-data-server, cgmes-gl-server, odre-server,
security-analysis-server, sensitivity-analysis-server, shortcircuit-server,
voltage-init-server, and timeseries-server.
---
Nitpick comments:
In `@docker-compose/docker-compose.base.yml`:
- Around line 317-321: Update the YAML to quote all unit-suffixed memory values
to avoid parsing ambiguity: change occurrences of memswap_limit and
deploy.resources.limits.memory (and any other memory keys like those at the
other noted blocks) from unquoted forms such as 1g, 1280m or 512m to quoted
strings like "1g" and "1280m", ensuring no trailing spaces remain inside the
quotes; apply this to the memswap_limit and deploy.resources.limits.memory
entries referenced in the diff and the other blocks called out.
In `@docker-compose/technical/docker-compose.technical.yml`:
- Around line 9-13: The RabbitMQ healthcheck block (healthcheck with test:
["CMD-SHELL", "rabbitmq-diagnostics ping"]) currently uses interval and retries
but no start_period, so add a start_period (e.g., start_period: 30s–60s) to that
healthcheck so early boot failures aren’t counted toward retries; apply the same
change to the other healthcheck blocks referenced (the ones using healthcheck,
interval, timeout, retries) to ensure dependent services using condition:
service_healthy don’t flap during slow initialization.
- Around line 192-196: The healthcheck currently uses a curl-based probe in the
healthcheck.test block which relies on HTTP and is less reliable; replace the
test command to use MinIO's CLI readiness check by switching the
healthcheck.test from the curl invocation to use mc ready local (i.e., update
the healthcheck.test entry to call ["CMD", "mc", "ready", "local"]); ensure the
healthcheck section (healthcheck.test, interval, timeout, retries) remains
intact and that the container image includes the mc binary so mc ready can run
successfully.
In `@README.md`:
- Around line 96-113: The profile/service matrix is hard to scan because grouped
cell lists are inconsistently ordered and the meaning of the “_(none)_” column
vs blank cells is unclear; fix by alphabetizing service names within each
multi-service row (e.g., rows containing directory-notification-server,
explore-server, gridexplore-app, gridstudy-app, network-map-server,
single-line-diagram-server, study-notification-server, study-server) to a stable
lexical order and ensure all grouped rows follow the same ordering rule, and add
a leading legend row above the table that explains the “_(none)_” column vs
empty cells (and confirm the infra row with
elasticsearch/postgres/rabbitmq/s3-storage retains the ✅ under “_(none)_” if
that denotes required infra).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7a6c336-64ae-4296-bc3d-30b31869cd6e
📒 Files selected for processing (8)
README.mddocker-compose/docker-compose.base.ymldocker-compose/dynamic-mapping/docker-compose.dynamic-mapping.ymldocker-compose/monitor/docker-compose.monitor.ymldocker-compose/study-light/.envdocker-compose/study-light/docker-compose.ymldocker-compose/study/docker-compose.study.ymldocker-compose/technical/docker-compose.technical.yml
Signed-off-by: BOUHOURS Antoine <[email protected]>
antoinebhs
force-pushed
the
add-to-profiles
branch
from
2d3d31e to
28b124a
Compare
Signed-off-by: BOUHOURS Antoine <[email protected]>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
PR Summary
This PR introduces a dedicated
study-lightprofile and makes that profile usable with the set of services GridStudy/GridExplore actually need.It also improve docker startup order and lower the memory footprint of the local deployment.