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

CTRL+C not working anymore on current master #113

Closed
giffels opened this issue Jul 15, 2022 · 3 comments · Fixed by MatterMiners/tardis#259
Closed

CTRL+C not working anymore on current master #113

giffels opened this issue Jul 15, 2022 · 3 comments · Fixed by MatterMiners/tardis#259
Assignees
Labels
bug Something isn't working

Comments

@giffels
Copy link
Member

giffels commented Jul 15, 2022

It seems that neither CTRL+C nor kill -15 are working anymore on the current master.

@giffels giffels added the bug Something isn't working label Jul 15, 2022
@maxfischer2781 maxfischer2781 self-assigned this Jul 18, 2022
@maxfischer2781
Copy link
Member

maxfischer2781 commented Jul 27, 2022

Looks like a bad synergy between cobald and the tardis REST API/uvicorn. Running it without the API works.

Config that fails, needs cobald-tardis:

pipeline:
  - __type__: cobald.controller.linear.LinearController
    low_utilisation: 0.90
    high_allocation: 0.90
    rate: 1
  - __type__: cobald.decorator.limiter.Limiter
    minimum: 1
  - __type__: cobald.decorator.logger.Logger
    name: 'changes'
  - __type__: tardis.resources.poolfactory.create_composite_pool
logging:
  version: 1
  root:
    level: DEBUG
    handlers: [console, file]
  formatters:
    precise:
      format: '%(name)s: %(asctime)s %(message)s'
      datefmt: '%Y-%m-%d %H:%M:%S'
  handlers:
    console:
      class: logging.StreamHandler
      formatter: precise
      stream: ext://sys.stdout
    file:
      class: logging.handlers.RotatingFileHandler
      formatter: precise
      filename: tardis.log
      maxBytes: 10485760
      backupCount: 3
tardis:
  Services:
    restapi:
      !TardisRestApi
      host: 127.0.0.1
      port: 1234
      secret_key: 63328dc6b8524bf08b0ba151e287edb498852b77b97f837088de4d17247d032c
      algorithm: HS256
      users:
        - user_name: tardis
          hashed_password: $2b$12$c9SSllh1U6tOhIo37sDWF.kdRIU5RQAAOHL9bVYMs2.HluyFE43Uq
          scopes:
            - user:read
  Plugins:
    SqliteRegistry:
      db_file: drone_registry.db

  BatchSystem:
    adapter: FakeBatchSystem
    allocation: 1.0
    utilisation: !TardisPeriodicValue
                 period: 3600
                 amplitude: 0.5
                 offset: 0.5
                 phase: 0.
    machine_status: Available

  Sites:
    - name: Fake
      adapter: FakeSite
      quota: 8000 # CPU core quota

  Fake:
    api_response_delay: !TardisRandomGauss
                        mu: 0.1
                        sigma: 0.01
    resource_boot_time: !TardisRandomGauss
                        mu: 60
                        sigma: 10
    MachineTypes:
      - m1.infinity
    MachineTypeConfiguration:
      m1.infinity:
    MachineMetaData:
      m1.infinity:
        Cores: 8
        Memory: 16
        Disk: 160

@maxfischer2781
Copy link
Member

maxfischer2781 commented Jul 28, 2022

This is a uvicorn design issue: encode/uvicorn#1579

The TLDR is that uvicorn installs signal handlers for SIGINT and SIGTERM that only cause a graceful shutdown of the server. This suppresses KeyboardInterrupt forever since even after Server.serve finishes the signal handler is never restored.


The reason why this worked previously is that asyncio ran in its own thread before. uvicorn only overrides the signal handlers when running in the main thread.

@maxfischer2781
Copy link
Member

maxfischer2781 commented Jul 28, 2022

The core of the issue needs to be worked around on the TARDIS side: The REST server needs to raise KeyboardInterrupt inside run when the server is done. I think it is fine to just do that and not restore the signal handlers, since the loop will shutdown anyway and this reduces the code to adjust when uvicorn fixes things.

Testing this revealed that on the COBalD side, having KeyboardInterrupt directly in a service leads to a warning traceback – because it dies in an exceptional way and nothing bothered to acknowledge that. This will be suppressed with #114.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants