whole-tale / girder_wholetale

Girder plugin providing basic Whole Tale functionality
BSD 3-Clause "New" or "Revised" License
3 stars 5 forks source link

Expose instance stdout/err via GET /instance/:id/logs #557

Closed Xarthisius closed 1 year ago

Xarthisius commented 1 year ago

Allow to fetch instance logs via exposing service.logs. Custom http server to do that is available from here: https://github.com/whole-tale/instance_logger

How to test?

  1. Modify your local deploy-dev:

    diff --git a/docker-stack.yml b/docker-stack.yml
    index 9960f28..5dd2425 100644
    --- a/docker-stack.yml
    +++ b/docker-stack.yml
    @@ -72,6 +72,17 @@ services:
             - "traefik.http.middlewares.girder.forwardauth.address=http://girder:8080/api/v1/instance/authorize/"
             - "traefik.http.middlewares.girder.forwardauth.trustforwardheader=true"
    
    +  logger:
    +    image: wholetale/instance_logger:latest
    +    networks:
    +      - celery
    +    volumes:
    +      - /var/run/docker.sock:/var/run/docker.sock:ro
    +    deploy:
    +      replicas: 1
    +      labels:
    +        - "traefik.enable=false"
    +
       redis:
         image: redis:4-stretch
         networks:
  2. Deploy, create a Tale, run instance.
  3. Using https://girder.local.wholetale.org/api/v1#!/instance/instance_getInstanceLog try accessing instance's logs.

Note On production deployment logger needs to be bound to manager node.

codecov[bot] commented 1 year ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (master@fc1322e). Click here to learn what that means. Patch coverage: 93.10% of modified lines in pull request are covered.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #557 +/- ## ========================================= Coverage ? 92.81% ========================================= Files ? 60 Lines ? 4814 Branches ? 0 ========================================= Hits ? 4468 Misses ? 346 Partials ? 0 ``` | [Impacted Files](https://codecov.io/gh/whole-tale/girder_wholetale/pull/557?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=whole-tale) | Coverage Δ | | |---|---|---| | [server/\_\_init\_\_.py](https://codecov.io/gh/whole-tale/girder_wholetale/pull/557/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=whole-tale#diff-c2VydmVyL19faW5pdF9fLnB5) | `90.58% <88.88%> (ø)` | | | [server/rest/instance.py](https://codecov.io/gh/whole-tale/girder_wholetale/pull/557/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=whole-tale#diff-c2VydmVyL3Jlc3QvaW5zdGFuY2UucHk=) | `91.30% <90.00%> (ø)` | | | [server/constants.py](https://codecov.io/gh/whole-tale/girder_wholetale/pull/557/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=whole-tale#diff-c2VydmVyL2NvbnN0YW50cy5weQ==) | `89.36% <100.00%> (ø)` | | | [server/models/instance.py](https://codecov.io/gh/whole-tale/girder_wholetale/pull/557/diff?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=whole-tale#diff-c2VydmVyL21vZGVscy9pbnN0YW5jZS5weQ==) | `87.20% <100.00%> (ø)` | | Help us with your feedback. Take ten seconds to tell us [how you rate us](https://about.codecov.io/nps?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=whole-tale). Have a feature suggestion? [Share it here.](https://app.codecov.io/gh/feedback/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=whole-tale)

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

Xarthisius commented 1 year ago

Aimed at fixing https://github.com/whole-tale/whole-tale/issues/98

Xarthisius commented 1 year ago

LGTM +1 this works as advertised!

I noticed that it returns a 500 error when the instance is still building/starting up.. I wonder if could this possibly be changed to a 503 in this case? that way, we could tell between when there is an error fetching the logs and when they will be available soon.

If this would be a really large change, then feel free to disregard.

I'll do that, but that belongs to another repo.