sablierapp / sablier

Start your containers on demand, shut them down automatically when there's no activity. Docker, Docker Swarm Mode and Kubernetes compatible.
https://sablierapp.dev/
GNU Affero General Public License v3.0
1.36k stars 46 forks source link

feat(api) Added Status API #167

Closed ab623 closed 11 months ago

ab623 commented 1 year ago

I have recently picked up learning Go and trying to learn to contribute back into open source. And thought that Sablier would be a good project to get involved in.

I have been putting in the early stages of a status page, which intends to show the status of the services Sablier manages, along with useful information about those services, mainly, the status, expiration time, and remaining time.

I had to make quite a few updates across the solution. I have documented the rationale below

Output: Calling /api/status :

{
  "managed_services": [
    {
      "name": "whoami-secondary",
      "currentReplicas": 1,
      "desiredReplicas": 1,
      "status": "ready",
      "expires_on": "0001-01-01T00:00:00Z",
      "expires_in": 0
    },
    {
      "name": "whoami-primary",
      "currentReplicas": 0,
      "desiredReplicas": 1,
      "status": "unrecoverable",
      "message": "ready"
      "expires_on": "2023-08-08T11:57:59.3412899Z",
      "expires_in": 260
    }
  ]
}

Potential non-blocking issues:

I haven't used Go before, nor any of the libraries such as Gin, so some good feedback and advice would be helpful. I have tired to stick close to the format in the project.

acouvreur commented 1 year ago

Hey @ab623, thanks for your contribution, I will be looking it it

ab623 commented 1 year ago

Thanks @acouvreur. Additionally I just pushed a commit to fix https://github.com/acouvreur/sablier/issues/153. I did it on this branch as it requires the functionality in the initial commits.

This works by checking all managed containers on startup, and if the containers are running already, it just applies the default timeout to them. If a containers is already stopped it just skips it.

This way the containers can start first, and then Sabiler can be started after and the containers that it manages will be brought down after the default timeout.

acouvreur commented 1 year ago

So for the status page we have a few things to consider:

If it is started by name, we will only see the status when it's up, because as soon as it's down, the instance will be out of the KV store.

For instances started with the group name, then you can already use the group values to build the status page. So I think you do not need to add anything to the providers.

What do you think about that?

ab623 commented 1 year ago

My implementation already accounts for that and is independent of the calling method.

It looks for the enable label and uses that to track all monitored containers.

Then if it is up, it exists in the KV and pulls the expiration data, if not then the service is down, but the container name is still shown in the output, just with a exited status.

ab623 commented 1 year ago

Overall I agree with your changes, but I think we need to generify the usage to instance instead of container.

Also, I believe the startup feature is not as expected. We should review it again. And it should also be enabled through a configuration flag.

Thanks for the feedback. Glad my first foray into the Go world and this project was somewhat successful.

I will refactor this to use the instances and not containers as per your suggestions and review the startup feature. I have left a few comments on both. Would be good to get your thoughts on them.

acouvreur commented 1 year ago

Overall I agree with your changes, but I think we need to generify the usage to instance instead of container. Also, I believe the startup feature is not as expected. We should review it again. And it should also be enabled through a configuration flag.

Thanks for the feedback. Glad my first foray into the Go world and this project was somewhat successful.

I will refactor this to use the instances and not containers as per your suggestions and review the startup feature. I have left a few comments on both. Would be good to get your thoughts on them.

Take your time for the refactor by the way, if you need some help feel free to ask me for it.

Thanks again for your contribution by the way :)

ab623 commented 1 year ago

Thanks. I'm travelling for a few weeks. So won't have time to work on this, but will pick it up when I'm back.

Appreciate the support.

I left a few comments on the review can you take a look and let me know your thoughts.

ab623 commented 11 months ago

Hey @acouvreur. I'm back from some holidays. Should I continue to work on this, or wait until the rewrite? Or are you building this in as part of the rewrite 😄

acouvreur commented 11 months ago

Hey @acouvreur. I'm back from some holidays. Should I continue to work on this, or wait until the rewrite? Or are you building this in as part of the rewrite 😄

Hey! Welcome back!

I'll add this to the rewrite! No need to try to merge conflict.

Feel free to try and play with the rewrite branch, help is very appreciated!

ab623 commented 11 months ago

Alright. Good to know! I'll close this PR. I'll give the refactor branch a go also.