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

connecting new ai-lab container to existing docker-db network #92

Merged
merged 8 commits into from
Mar 25, 2024
1 change: 1 addition & 0 deletions doc/changes/changes_0.2.9.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,4 @@ Post-release fixes.
* AI-Lab#230: Connection via SQLAlchemy fails
- Enables fingerprints in the host name.
- Handles correctly special characters in the password.
* #89: Connecting a new AI-Lab container to the Docker DB network when the latter container restarts.
64 changes: 48 additions & 16 deletions exasol/nb_connector/itde_manager.py
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,7 @@ def bring_itde_up(conf: Secrets) -> None:
db_info = env_info.database_info
container_info = db_info.container_info

_add_current_container_to_db_network(container_info)
_add_current_container_to_db_network(container_info.network_info.network_name)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I like narrow interfaces: 👍


conf.save(AILabConfig.itde_container, container_info.container_name)
conf.save(AILabConfig.itde_volume, container_info.volume_name)
Expand All @@ -85,18 +85,37 @@ def bring_itde_up(conf: Secrets) -> None:
conf.save(AILabConfig.cert_vld, "False")


def _add_current_container_to_db_network(container_info: ContainerInfo):
network_name = container_info.network_info.network_name
def _get_current_container(docker_client: docker.DockerClient):
ip_addresses = _get_ipv4_ddresses()
ahsimb marked this conversation as resolved.
Show resolved Hide resolved
return ContainerByIp(docker_client).find(ip_addresses)


def _add_current_container_to_db_network(network_name: str) -> None:
with ContextDockerClient() as docker_client:
ip_addresses = _get_ipv4_ddresses()
container = ContainerByIp(docker_client).find(ip_addresses)
container = _get_current_container(docker_client)
if not container:
return
network = _get_docker_network(docker_client, network_name)
if network:
if network and (container not in network.containers):
network.connect(container.id)


def _is_current_container_not_in_db_network(network_name: str) -> bool:
Copy link
Collaborator

Choose a reason for hiding this comment

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

How does this function relate to self._add_current_container_to_db_network()?
I have the feeling that part of self._add_current_container_to_db_network() can be replaced by using this function?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

One is checking if the AI-Lab container needs to be connected to the Docker-DB network. The other one connects the container to the network if the container exists and is not connected to this network yet.

"""
For the Docker Edition returns True if the current (AI-Lab) container
is NOT connected to the network with the specified name. Otherwise,
including the cases of other editions, returns False.
"""
with ContextDockerClient() as docker_client:
container = _get_current_container(docker_client)
if not container:
return False
network = _get_docker_network(docker_client, network_name)
if not network:
return True
return container not in network.containers


def _get_docker_network(docker_client: docker.DockerClient, network_name: str) -> Optional[Network]:
networks = docker_client.networks.list(names=[network_name])
if len(networks) == 1:
Expand Down Expand Up @@ -127,47 +146,60 @@ def _get_ipv4_ddresses():

def is_itde_running(conf: Secrets) -> Tuple[bool, bool]:
"""
Checks if the ITDE container exists and if it is running. Returns the two boolean
flags - (exists, running).
Checks if the ITDE container exists and ready to be used. In the Docker Edition that
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it be possible to replace the tuple by an enum?

class ItdeContainerStatus(Enum):
    EXISTS = 0
    RUNNING = 1

This would also enable to handle the case not container_name or not network_name more explicitly by returning None or having an explicit enum value UNKNOWN or INVALID.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree, but this would also require changes in the upstream code of the the AI-Lab.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I moved the out-of-scope-findings to #93

means the ITDE is running and the AI-Lab container is connected to its network. In
other editions it will just check that the ITDE is running.

Returns two boolean flags - (exists, running).

The name of the container is taken from the provided secret store.
If the name cannot be found in the secret store the function returns False, False.
"""

# Try to get the name of the container from the secret store.
# Try to get the names of the Docker-DB container and its network from the secret store.
container_name = conf.get(AILabConfig.itde_container)
if not container_name:
network_name = conf.get(AILabConfig.itde_network)
if not container_name or not network_name:
return False, False

# Check the existence and the status of the container using the Docker API.
with ContextDockerClient() as docker_client:
if docker_client.containers.list(all=True, filters={"name": container_name}):
container = docker_client.containers.get(container_name)
return True, container.status == 'running'
is_ready = (container.status == 'running' and not
_is_current_container_not_in_db_network(network_name))
return True, is_ready
ahsimb marked this conversation as resolved.
Show resolved Hide resolved
return False, False


def start_itde(conf: Secrets) -> None:
"""
Starts an existing ITDE container. If the container is already running the function
takes no effect. For this function to work the container must exist. If it doesn't
Starts an existing ITDE container if it's not already running. In the Docker Edition
ckunki marked this conversation as resolved.
Show resolved Hide resolved
connects the AI-Lab container to the Docker-DB network, unless it's already connected
to it.

For this function to work the container must exist. If it doesn't
the docker.errors.NotFound exception will be raised. Use the is_itde_running
function to check if the container exists.

The name of the container is taken from the provided secret store, where it must
exist. Otherwise, a RuntimeError will be raised.
"""

# The name of the container should be in the secret store.
# The names of the Docker-DB container and its network should be in the secret store.
container_name = conf.get(AILabConfig.itde_container)
if not container_name:
raise RuntimeError('Unable to find the name of the Docker container.')
network_name = conf.get(AILabConfig.itde_network)
if not container_name or not network_name:
raise RuntimeError('Unable to find the name of the Docker container or its network.')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
raise RuntimeError('Unable to find the name of the Docker container or its network.')
raise RuntimeError('Unable to find the name of the Docker DB container or its network.')


# Start the container using the Docker API, unless it's already running.
with ContextDockerClient() as docker_client:
container = docker_client.containers.get(container_name)
if container.status != 'running':
container.start()

_add_current_container_to_db_network(network_name)


def take_itde_down(conf: Secrets) -> None:
"""
Expand Down
Loading