Skip to content
This repository has been archived by the owner on Dec 4, 2023. It is now read-only.

Check update and trigger update #121

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from

Conversation

nsosio
Copy link
Contributor

@nsosio nsosio commented Aug 2, 2023

Check available update

  1. Get last premd tag by parsing premd
  2. Get sha of the last tag
  3. Compare with the sha of available local images

Trigger update

  1. Creating a new container of premd (on a different host port)
  2. Stop and remove previous container
  3. Create a new container with the default host port (on startup event of FastAPI only if the running container is running in a different port than default)

fixes #92

@nsosio nsosio requested a review from casperdcl August 2, 2023 18:27
@nsosio
Copy link
Contributor Author

nsosio commented Aug 2, 2023

@casperdcl the solution is super tricky, would it be worth it to have a look at watchtower for automatic update (in this case no endpoint is exposed but watchtower update it.

Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

nice; just a few comments

app/core/utils.py Outdated Show resolved Hide resolved
app/core/utils.py Outdated Show resolved Hide resolved
app/core/utils.py Outdated Show resolved Hide resolved
app/core/utils.py Outdated Show resolved Hide resolved
app/core/utils.py Outdated Show resolved Hide resolved
app/core/utils.py Outdated Show resolved Hide resolved
app/core/utils.py Outdated Show resolved Hide resolved
@casperdcl casperdcl added enhancement New feature or request python Pull requests that update Python code labels Aug 2, 2023
Copy link
Contributor

@casperdcl casperdcl left a comment

Choose a reason for hiding this comment

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

deeper review:

  1. Creating a new container of premd (on a different host port)
  2. Stop and remove previous container
  3. Create a new container with the default host port (on startup event of FastAPI only if the running container is running in a different port than default)

Not a fan of any of the above steps (changing ports, changing container names). Instead:

  • to avoid port conflict:
    • create (don't run) the new container
    • remove the old container
    • start the new container
  • to avoid name suffixes:
    • create a new container with an automatic (random) name
    • remove the old premd container
    • new_container.rename("premd")

Also missing:

  • actually pulling the new image
  • pruning the old image
  • retagging latest

app/core/utils.py Show resolved Hide resolved
app/core/utils.py Show resolved Hide resolved
app/core/utils.py Outdated Show resolved Hide resolved
app/core/utils.py Outdated Show resolved Hide resolved
app/routes.py Outdated Show resolved Hide resolved
app/core/utils.py Outdated Show resolved Hide resolved

def update_container(host_port):
container_name, new_container_name = generate_container_name("premd")
create_new_container(
Copy link
Contributor

Choose a reason for hiding this comment

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

aren't we missing here:

  • pull get_premd_last_tag()
  • update latest tag to point to above
  • untag old tag
  • after removing old container, prune old image tag

@@ -9,6 +9,23 @@

def create_start_app_handler(app: FastAPI):
def start_app() -> None:
container_name, new_container_name = utils.generate_container_name("premd")
client = utils.get_docker_client()
if utils.container_exists(container_name):
Copy link
Contributor

Choose a reason for hiding this comment

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

should there be an else statement following this if? Don't we need to create a new container if it doesn't already exist?

"HostPort"
]
if host_port != f"{utils.DEFAULT_PORT}":
utils.check_host_port_availability(utils.DEFAULT_PORT)
Copy link
Contributor

Choose a reason for hiding this comment

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

What if this returns False? If you ignore the return value of this function, why call it at all?

app/core/utils.py Outdated Show resolved Hide resolved
@nsosio
Copy link
Contributor Author

nsosio commented Sep 3, 2023

@casperdcl I've reviewed your proposed changes and at first sight I agreed but then I've realised that the problem with

to avoid port conflict:

  • create (don't run) the new container
  • remove the old container
  • start the new container

is that we're starting the new container running the new version of premd from the running premd container itself. Therefore when we stop premd the new container will never be started.

new_container = create_new_container(
    PREMD_IMAGE, "latest", "new_container", "premd"
)
update_and_remove_old_container("premd")
new_container.start() # <-- code never reached
new_container.rename("premd")

That's the main problem of exposing an endpoint to update the container itself.

@casperdcl
Copy link
Contributor

Yep makes sense, so looks like watchtower auto-updates (plus option to disable watchtower) might be a better way to go.


def get_premd_last_tag(owner, repository, package):
response = requests.get(
f"https://github.com/{owner}/{repository}/pkgs/container/{package}"
Copy link
Contributor

@Janaka-Steph Janaka-Steph Sep 11, 2023

Choose a reason for hiding this comment

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

Why not calling the API instead?
Something like this, with an authentication token if necessary:

def get_premd_last_tag():
    try:
        url = f'https://api.github.com/orgs/premai-io/packages/container/premd/versions'
        headers = {
            'Accept': 'application/vnd.github+json'
        }
        response = requests.get(url, headers)
        if response.status_code == 200:
            data = response.json()
            if data:
                # Find the latest version by sorting by created_at in descending order
                latest_version = max(data, key=lambda x: x['created_at'])
                return latest_version['name']
            else:
                print("No versions found for the image.")
        else:
            logger.error(f"Failed to retrieve data from GitHub API. Status code: {response.status_code}")
            return None
    except Exception as e:
        logger.error(f"An error occurred: {e}")
        return None

Copy link
Contributor

Choose a reason for hiding this comment

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

because we'd have to hardcode an auth token (this particular API requires authentication even to access public info)

@tiero
Copy link
Contributor

tiero commented Nov 1, 2023

AFAICT is an internal updater functionality for premd itself, not the entire stack of all services (app, dnsd, controllerd, authd etc..)

Main difference with this service is mainly that will be dedicated and can arbitrarily restart any other service without going offline in the first place (ie. the app can monitor the update and don't disrupt serving updates to clients)

That being said not urgent to come to a consensus in this sprint.

@casperdcl
Copy link
Contributor

PoC: bit-gpt/app#458

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request python Pull requests that update Python code
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Update premd if new version available
4 participants