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

use docker compose v2 #163

Open
wants to merge 22 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
22 commits
Select commit Hold shift + click to select a range
a361a02
use docker compose v2
fmcwalters-edited Feb 15, 2024
7cda8d3
linting
fmcwalters-edited Feb 15, 2024
f6c595e
use new form of `network` config
fmcwalters-edited Feb 15, 2024
a439c4c
remove version check and `print` statements
fmcwalters-edited Feb 15, 2024
03fee73
convert v1 container names to v2
fmcwalters-edited Feb 15, 2024
b2958d3
deal with cases where container name has more than one middle name
fmcwalters-edited Feb 15, 2024
70ed8ef
use v14.0.0 for Kubetools with Docker Compose v2, update README
fmcwalters-edited Feb 19, 2024
696efb3
readme formatting
fmcwalters-edited Feb 20, 2024
fc59bbf
add unit tests for a complex named app to check compatibility with co…
fmcwalters-edited Feb 20, 2024
3a4c3fe
add missing infra for test
fmcwalters-edited Feb 20, 2024
add0755
add correct complex-webserver
fmcwalters-edited Feb 20, 2024
f594288
use complex-named-app for cron
fmcwalters-edited Feb 20, 2024
eb53740
improve documentation
fmcwalters-edited Feb 21, 2024
9ff20c1
ensure dev release adheres to semver format
fmcwalters-edited Feb 27, 2024
8e7b32c
add reason in CHANGELOG why this is a dev release
fmcwalters-edited Feb 27, 2024
4da3de5
make workarounds more descriptive
fmcwalters-edited Feb 28, 2024
b9cc5a4
factor out code to convert container names, add unit tests
fmcwalters-edited Feb 28, 2024
7a8b920
improve documentation for buildx issue, make CHANGELOG more concise
fmcwalters-edited Feb 28, 2024
bddb7ea
Merge branch 'master' into PFM-827-docker-compose-v2-upgrade
fmcwalters-edited Apr 2, 2024
700658a
remove unused import
fmcwalters-edited Apr 2, 2024
2a39579
Merge branch 'master' into PFM-827-docker-compose-v2-upgrade
fmcwalters-edited Jul 11, 2024
0f3b1bf
use full version number
fmcwalters-edited Jul 11, 2024
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 7 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,13 @@

### Unreleased

# v14.0.0-dev
- before upgrading to version 14.0 or above, you _must_ run `ktd destroy` for all existing
local Kubetools projects.
- this is a `dev` release because of a bug in `docker/buildx` - there is a workaround for this but we don't want to use workarounds in production (see TROUBLESHOOTING in README)
- this **BREAKS COMPATIBILITY** with previous versions of `ktd` (see README)
- uses `docker compose` in place of `docker-compose`, meaning we use Docker compose v2 on the backend

# v13.14.0
- Fix docker-compose conflict when kubetools commands are called without activating their venv
- Add Python 3.12 to supported versions, albeit without Flake8 because of CPython bug
Expand Down
25 changes: 22 additions & 3 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ And you would like:

Kubetools provides the tooling required to achieve this, by way of two CLI tools:

+ **`ktd`**: generates _100% local_ development environments using Docker/docker-compose
+ **`ktd`**: generates _100% local_ development environments using docker compose
+ **`kubetools`**: deploys projects to Kubernetes, handling any changes/jobs as required

Both of these use a single configuration file, `kubetools.yml`, for example a basic `django` app:
Expand Down Expand Up @@ -80,15 +80,17 @@ cronjobs:
With this in your current directory, you can now:

```sh
# Bring up a local development environment using docker-compose
# Bring up a local development environment using docker compose
ktd up
```

```sh
# Deploy the project to a Kubernetes namespace
kubetools deploy my-namespace
```

## Installing

To install Kubetools run:
```sh
pip install kubetools
```
Expand Down Expand Up @@ -137,6 +139,23 @@ minikube delete
...
```


## Troubleshooting
There is a bug in `buildx` which is present in Docker Engine v25.0 and up, which is yet to be patched and causes `insecure-registries` to be ignored - [issue here](https://github.com/docker/buildx/issues/2226).
Versions of Docker Desktop which use versions of Docker Engine lower than 25.0 are unaffected.

If you try and run `ktd up` with reference to an unsecure registry, e.g. `http://docker-registry.example.net` and are affected by this bug, you will get an error message that is
similar to the following:
Comment on lines +151 to +152
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If you try and run `ktd up` with reference to an unsecure registry, e.g. `http://docker-registry.example.net` and are affected by this bug, you will get an error message that is
similar to the following:
If you try and run `ktd up` with reference to an unsecure registry, e.g. `http://docker-registry.example.net` and are affected by this bug, you will get an error message that is similar to the following:

```
ERROR: failed to do request: Head "https://docker-registry.example.net/3.6_alpine3.13_v0.1-multi": dialing docker-registry.example.net:443 with direct connection: connecting to 1.1.1.1:443: dial tcp 1.1.1.1:443: connect: connection refused
```

There are a few of workarounds:
* Migrate to secure registries (over HTTPS)
* Downgrade to Docker Desktop v4.26.1 or below and prefix `http://` to each of your `insecure_registries`' URLs.
* Alternatively, use the legacy builder by setting the environment variable `DOCKER_BUILDKIT=0` for `ktd` commands.


## Releasing (admins/maintainers only)
* Update [CHANGELOG](CHANGELOG.md) to add new version and document it
* In GitHub, create a new release
Expand Down
5 changes: 2 additions & 3 deletions kubetools/dev/backends/docker_compose/config.py
Original file line number Diff line number Diff line change
Expand Up @@ -248,9 +248,8 @@ def create_compose_config(kubetools_config):
if dev_network:
compose_config['networks'] = {
'default': {
'external': {
'name': 'dev',
},
'name': 'dev',
Copy link
Member

Choose a reason for hiding this comment

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

I'm wondering if we should take this opportunity to rename this "default" network used by ktd.

default here in the config is from the point of view of the project/container. This declares the default network for the project to be "external" (i.e. pre-existing outside this compose project) and named dev.

I think we should replace dev by something that clearly identifies it as used by ktd. Maybe ktd_dev could be an option.

Also, from a few tests I've run, it seems like other "environments" as created with --env option (or test in the case of ktd test), end up with a "temporary" network name (i.e. it's not "external" and IS deleted by the destroy operation) which looks like <appname>_<env>_default. I think it would be cool if that network name ended up being similarly, something like ktd_<env>.
This aspect probably requires a lot of work to implement, However it could be the base for a feature allowing to create "multi-app" environments, in a similar way as we can today do with the "default" environment.

'external': True,
},
}

Expand Down
19 changes: 13 additions & 6 deletions kubetools/dev/backends/docker_compose/docker_util.py
Original file line number Diff line number Diff line change
@@ -1,5 +1,3 @@
import sys

from functools import lru_cache

import docker
Expand Down Expand Up @@ -138,8 +136,7 @@ def get_containers_status(
if not env:
env = compose_project.replace(docker_name, '')

# Where the name is compose-name_container_N, get container
name = container.name.split('_')[1]
name = _get_container_name_from_full_name(container.name)

status = container.status == 'running'
ports = []
Expand Down Expand Up @@ -194,6 +191,17 @@ def get_containers_status(
return env_to_containers.get(kubetools_config['env'], {})


def _get_container_name_from_full_name(full_name):
# if the container was made in v1, it will be composename_container_N
converted_name = full_name.replace('_', '-')
Comment on lines +195 to +196
Copy link
Member

Choose a reason for hiding this comment

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

Just to check, are we trying to create logic that is backwards compatible with compose V2?

I think that, in line with the change to README and with the major version change, we don´t need to support any backwards compatible logic here.


# we need to keep the middle part of the name
# for example if we have a container called `composename-container-1-N`
# we want to keep `container-1`
middle_names = converted_name.split('-')[1:-1]
return '-'.join(middle_name for middle_name in middle_names)


def get_container_status(kubetools_config, name):
containers = get_containers_status(kubetools_config, container_name=name)
return containers.get(name)
Expand All @@ -204,8 +212,7 @@ def run_compose_process(kubetools_config, command_args, **kwargs):
create_compose_config(kubetools_config)

compose_command = [
# Use current interpreter to run the docker-compose module installed in the same venv
sys.executable, '-m', 'compose',
'docker', 'compose',
# Force us to look at the current directory, not relative to the compose
# filename (ie .kubetools/compose-name.yml).
'--project-directory', '.',
Expand Down
2 changes: 0 additions & 2 deletions setup.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,8 +59,6 @@ def get_readme_content():
# To support CronJob api versions 'batch/v1beta1' & 'batch/v1'
'kubernetes>=21.7.0,<25.0.0',
'tabulate<1',
# compose v2 has broken container naming
'docker-compose<2',
DilaraOflaz marked this conversation as resolved.
Show resolved Hide resolved
),
extras_require={
'dev': (
Expand Down
40 changes: 40 additions & 0 deletions tests/configs/complex_named_app/k8s_cronjobs.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,40 @@
kind: CronJob
metadata:
name: generic-cronjob
labels: {
kubetools/name: generic-cronjob,
kubetools/project_name: complex-named-app,
kubetools/role: cronjob
}
annotations: {
app.kubernetes.io/managed-by: kubetools,
description: 'Run: [''generic-command'']'
}
spec:
schedule: "*/1 * * * *"
startingDeadlineSeconds: 10
concurrencyPolicy: "Allow"
jobTemplate:
spec:
template:
metadata:
name: generic-cronjob
labels: {
kubetools/name: generic-cronjob,
kubetools/project_name: complex-named-app,
kubetools/role: cronjob
}
annotations: {
app.kubernetes.io/managed-by: kubetools,
description: 'Run: [''generic-command'']'
}
spec:
containers:
- command: [generic-command]
containerContext: generic-context
env:
- {name: KUBE, value: 'true'}
image: generic-image
imagePullPolicy: 'Always'
name: generic-container
restartPolicy: OnFailure
30 changes: 30 additions & 0 deletions tests/configs/complex_named_app/k8s_deployments.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
apiVersion: apps/v1
kind: Deployment
metadata:
annotations: {app.kubernetes.io/managed-by: kubetools}
labels: {kubetools/name: complex-named-app, kubetools/project_name: complex-named-app, kubetools/role: app}
name: complex-named-app
spec:
replicas: 1
revisionHistoryLimit: 5
selector:
matchLabels: {kubetools/name: complex-named-app, kubetools/project_name: complex-named-app,
kubetools/role: app}
template:
metadata:
labels: {kubetools/name: complex-named-app, kubetools/project_name: complex-named-app, kubetools/role: app}
spec:
containers:
- command: [generic-command]
containerContext: generic-context
env:
- {name: KUBE, value: 'true'}
image: generic-image
imagePullPolicy: Always
livenessProbe:
httpGet: {path: /ping, port: 80}
timeoutSeconds: 5
name: complex-webserver
readinessProbe:
httpGet: {path: /ping, port: 80}
timeoutSeconds: 5
30 changes: 30 additions & 0 deletions tests/configs/complex_named_app/k8s_jobs.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
apiVersion: batch/v1
kind: Job
metadata:
annotations: {app.kubernetes.io/managed-by: kubetools, description: 'Run: [''generic-command'']'}
labels: {job-id: UUID, kubetools/project_name: complex-named-app,
kubetools/role: job}
name: UUID
spec:
completions: 1
parallelism: 1
selector: {job-id: UUID, kubetools/project_name: complex-named-app,
kubetools/role: job}
template:
metadata:
labels: {job-id: UUID, kubetools/project_name: complex-named-app,
kubetools/role: job}
spec:
containers:
- chdir: /
command: [generic-command]
env:
- {name: KUBE, value: 'true'}
- {name: KUBE_JOB_ID, value: UUID}
image: generic-image
imagePullPolicy: Always
name: upgrade
resources:
requests:
memory: "1Gi"
restartPolicy: Never
11 changes: 11 additions & 0 deletions tests/configs/complex_named_app/k8s_services.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
apiVersion: v1
kind: Service
metadata:
annotations: {app.kubernetes.io/managed-by: kubetools}
labels: {kubetools/name: complex-named-app, kubetools/project_name: complex-named-app, kubetools/role: app}
name: complex-named-app
spec:
ports:
- {port: 80, targetPort: 80}
selector: {kubetools/name: complex-named-app, kubetools/project_name: complex-named-app, kubetools/role: app}
type: NodePort
38 changes: 38 additions & 0 deletions tests/configs/complex_named_app/kubetools.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
name: complex-named-app


containerContexts:
generic-context:
image: generic-image
command: [generic-command]
ports:
- 80

upgrades:
- name: Upgrade the database
containerContext: generic-context
command: [generic-command, generic-arg]
resources:
requests:
memory: "1Gi"


deployments:
complex-named-app:
containers:
complex-webserver:
command: [uwsgi, --ini, /etc/uwsgi.conf]
containerContext: generic-context
probes:
timeoutSeconds: 5
httpGet:
path: /ping


cronjobs:
generic-cronjob:
schedule: "*/1 * * * *"
concurrency_policy: "Allow"
containers:
generic-container:
containerContext: generic-context
3 changes: 3 additions & 0 deletions tests/test_config_generation.py
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,9 @@ class TestKubernetesConfigGeneration(TestCase):
def test_basic_app_configs(self):
_test_configs('basic_app')

def test_complex_named_app_configs(self):
_test_configs('complex_named_app')

def test_dependencies_configs(self):
_test_configs('dependencies')

Expand Down
37 changes: 37 additions & 0 deletions tests/test_docker_util.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
from string import ascii_lowercase
from unittest import TestCase

from kubetools.dev.backends.docker_compose.config import dockerise_label
from kubetools.dev.backends.docker_compose.docker_util import _get_container_name_from_full_name


def generate_long_names(number_of_separators, separator):
Copy link
Member

Choose a reason for hiding this comment

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

See comment below, this function probably will disappear. But if it had to stay, then it needs to be renamed because it's confusing::

  • It is generating ONE name, not (several) _names
  • Why do they have to be "long" ? Is it important for the tests? What would not be tested if the names were "short" ? Note that this aspect of making tests more understandable does not belong in this function's name only, but in how/why the result of it is used in tests.

return separator.join([
ch for ch in ascii_lowercase[:number_of_separators+1]])


class TestDockerComposeNameConversion(TestCase):
# we need to check this works for dashes and underscores
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# we need to check this works for dashes and underscores

def test_dockerise_label_dashes(self):
for i in range(5):
long_name = generate_long_names(i, '-')
dockerised_name = dockerise_label(long_name)
self.assertEqual(dockerised_name, ascii_lowercase[:i+1])
Copy link
Member

Choose a reason for hiding this comment

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

I'm not quite sure but I have a feeling that we're using ascii_lowercase here as a "long string", which "could look like an app name". If that the case, then in my opinion it's a bad idea. It's a case of "wrong abstraction". Instead here we want to use names that could really be what they stand for, so that it's obvious to the reader what we're talking about. Put them in a list and iterate over them. If you're worried about repeating that list too much, extract it in a constant and reuse it in each place.


def test_dockerise_label_underscores(self):
for i in range(5):
long_name = generate_long_names(i, '_')
dockerised_name = dockerise_label(long_name)
self.assertEqual(dockerised_name, ascii_lowercase[:i+1])

def test_container_name_from_full_name_dashes(self):
Copy link
Member

Choose a reason for hiding this comment

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

These test names don't explain the scenario that we're testing here.
What are the cases where this logic might be relevant?

for i in range(5):
container_name = generate_long_names(i, '-')
full_name = '-'.join([ascii_lowercase[:i+1], container_name, '1'])
self.assertEqual(_get_container_name_from_full_name(full_name), container_name)

def test_container_name_from_full_name_underscores(self):
for i in range(5):
container_name = generate_long_names(i, '-')
full_name = '_'.join([ascii_lowercase[:i+1], container_name, '1'])
self.assertEqual(_get_container_name_from_full_name(full_name), container_name)