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.

Sign up for GitHub

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

Revert "Makefile: scripts: Add build args for proxy when using docker… #387

Merged
merged 4 commits into from
Jun 21, 2024
Merged

Revert "Makefile: scripts: Add build args for proxy when using docker… #387

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
3f8441f
Revert "Makefile: scripts: Add build args for proxy when using docker…
fidencio Jun 20, 2024
915d8d8
ci: Handle docker installation for Ubuntu 24.04
fidencio Jun 20, 2024
114c410
ci: Prefer distro packaged containerd for Ubuntu >20.04
fidencio Jun 20, 2024
c52d54b
ci: Install python3 packages from the distro
fidencio Jun 20, 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
9 changes: 3 additions & 6 deletions Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,9 +52,6 @@ IMG ?= controller:latest
# ENVTEST_K8S_VERSION refers to the version of kubebuilder assets to be downloaded by envtest binary.
ENVTEST_K8S_VERSION = 1.24.2

http_proxy := ""
https_proxy := ""

# Get the currently used golang install path (in GOPATH/bin, unless GOBIN is set)
ifeq (,$(shell go env GOBIN))
GOBIN=$(shell go env GOPATH)/bin
Expand Down Expand Up @@ -146,9 +143,9 @@ run: manifests generate fmt vet ## Run a controller from your host.
docker-build: test ## Build docker image with the manager.
ifneq (, $(PEERPODS))
@echo PEERPODS is enabled
docker build --build-arg http_proxy=$(http_proxy) --build-arg https_proxy=$(https_proxy) -t ${IMG} -f Dockerfile.peerpods .
docker build -t ${IMG} -f Dockerfile.peerpods .
else
docker build --build-arg http_proxy=$(http_proxy) --build-arg https_proxy=$(https_proxy) -t ${IMG} .
docker build -t ${IMG} .
endif

.PHONY: docker-push
Expand Down Expand Up @@ -273,7 +270,7 @@ bundle: manifests kustomize operator-sdk## Generate bundle manifests and metadat

.PHONY: bundle-build
bundle-build: ## Build the bundle image.
docker build --build-arg http_proxy=$(http_proxy) --build-arg https_proxy=$(https_proxy) -f bundle.Dockerfile -t $(BUNDLE_IMG) .
docker build -f bundle.Dockerfile -t $(BUNDLE_IMG) .

.PHONY: bundle-push
bundle-push: ## Push the bundle image.
Expand Down
4 changes: 0 additions & 4 deletions install/pre-install-payload/payload.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,8 +12,6 @@ vfio_gpu_containerd_repo=${vfio_gpu_containerd_repo:-"https://github.com/confide
nydus_snapshotter_repo=${nydus_snapshotter_repo:-"https://github.com/containerd/nydus-snapshotter"}
containerd_dir="$(mktemp -d -t containerd-XXXXXXXXXX)/containerd"
extra_docker_manifest_flags="${extra_docker_manifest_flags:-}"
http_proxy="${http_proxy:-}"
https_proxy="${https_proxy:-}"

registry="${registry:-quay.io/confidential-containers/reqs-payload}"

Expand Down Expand Up @@ -61,8 +59,6 @@ function build_payload() {

echo "Building containerd payload image for ${arch}"
docker buildx build \
--build-arg HTTP_PROXY="${http_proxy}" \
--build-arg HTTPS_PROXY="${https_proxy}" \
--build-arg ARCH="${golang_arch}" \
--build-arg COCO_CONTAINERD_VERSION="${coco_containerd_version}" \
--build-arg COCO_CONTAINERD_REPO="${coco_containerd_repo}" \
Expand Down
2 changes: 1 addition & 1 deletion tests/e2e/ansible/install_containerd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
tasks:
- name: Install containerd from distro
package:
name: "{{ 'containerd.io' if ansible_distribution_version != '22.04' else 'containerd' }}"
name: "{{ 'containerd.io' if ansible_distribution_version == '20.04' else 'containerd' }}"
state: present
# The docker package overwrite the /etc/containerd/config.toml installed
# by the containerd.io package. As a result we are hit by the following
Expand Down
4 changes: 2 additions & 2 deletions tests/e2e/ansible/install_docker.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,7 @@
state: present
# TODO: add regular non-root users to docker group
when: docker_exist.rc != 0 and ansible_distribution == "Ubuntu" and ansible_distribution_version == "20.04"
- name: Handle docker installation on Ubuntu 22.04
- name: Handle docker installation on Ubuntu 22.04 and 24.04
block:
- name: Install docker packages
package:
Expand All @@ -70,7 +70,7 @@
state: present
retries: 3
delay: 10
when: docker_exist.rc != 0 and ansible_distribution == "Ubuntu" and ansible_distribution_version == "22.04"
when: docker_exist.rc != 0 and ansible_distribution == "Ubuntu" and ansible_distribution_version in ("22.04", "24.04")
- name: Handle docker installation on CentOS.
block:
- name: Install yum-utils
Expand Down
18 changes: 5 additions & 13 deletions tests/e2e/ansible/start_docker_registry.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,24 +12,16 @@
local_registry_port: 5000
local_registry_name: local-registry
tasks:
- name: Install pip3
- name: Install python3-docker and python3-requests
package:
# TODO: this is ubuntu specific...
name: python3-pip
state: present
retries: 3
delay: 10
# The docker and requests pip packages are required by the docker_container ansible module itself.
# The requests package is pinned to a version less than 2.32 to avoid a bug
# at https://github.com/docker/docker-py/issues/3256
- name: Install docker and requests pip packages
pip:
name:
- docker
- requests<2.32
# This is Ubuntu specific
- python3-docker
- python3-requests
Copy link
Member

Choose a reason for hiding this comment

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

My concern here is that due to a breaking change in requests 2.32, we had to have a version less than that (but great than 2.28 IIRC?

What version is python3-requests installing on the 3 different versions of Ubuntu we are supporting and are we at risk of falling into incompatibility again if there are distro updates, or do we think it is safe to rely on canonical to ensure that doesn't happen?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is a very good question and I'd like to challenge that it doesn't work with something that's not greater than 2.28.

Let's take a look of what we have in our CIs:

  • azure -> Ubuntu 20.04 | python3-requests 2.22.0 -> CI passes with this PR
  • azure -> Ubuntu 22.04 | python3-requests 2.25.1 -> CI passes with this PR
  • TDX -> Ubuntu 24.04 | python3-request 2.31.0 -> CI passes with this PR
  • s390x -> ? -> CI passes with this PR

When using PIP, the first thing that I've faced was:

fatal: [localhost]: FAILED! => {"attempts": 3, "changed": false, "cmd": ["/usr/bin/python3", "-m", "pip.__main__", "install", "--user", "docker", "requests<2.32"], "msg": "\n:stderr: error: externally-managed-environment\n\n× This environment is externally managed\n╰─> To install Python packages system-wide, try apt install\n    python3-xyz, where xyz is the package you are trying to\n    install.\n    \n    If you wish to install a non-Debian-packaged Python package,\n    create a virtual environment using python3 -m venv path/to/venv.\n    Then use path/to/venv/bin/python and path/to/venv/bin/pip. Make\n    sure you have python3-full installed.\n    \n    If you wish to install a non-Debian packaged Python application,\n    it may be easiest to use pipx install xyz, which will manage a\n    virtual environment for you. Make sure you have pipx installed.\n    \n    See /usr/share/doc/python3.12/README.venv for more information.\n\nnote: If you believe this is a mistake, please contact your Python installation or OS distribution provider. You can override this, at the risk of breaking your Python installation or OS, by passing --break-system-packages.\nhint: See PEP 668 for the detailed specification.\n"}

So, being totally honest, I'd rather deal with the decision later on on whether using pip and facing the risk to break the system packages, or dealing with an update that may break ubuntu (and then, it seems TDX would be the first one receiving such update and it'd fall on me / Intel to solve it).

Let me know if you're comfortable enough with this, Steve.

Copy link
Member

@stevenhorsman stevenhorsman Jun 21, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Ok, it might be that pre-2.28 doesn't work the latest python-docker and the distros have old docker package that works, so if the tests pass I'm okay to carry this for now.

For reference - this was the error that Choi was seeing with it:

The pinned version of requests (2.28.1) is no longer compatible with the current
docker pip, resulting in errors like:

"Error connecting: Error while fetching server API version: Not supported URL scheme http+docker"

state: present
retries: 3
delay: 10
when: ansible_distribution == "Ubuntu"
fidencio marked this conversation as resolved.
Show resolved Hide resolved
- name: Start a docker registry
docker_container:
name: "{{ local_registry_name }}"
Expand Down
4 changes: 2 additions & 2 deletions tests/e2e/run-local.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ undo_changes() {

if [ $step_bootstrap_env -eq 1 ]; then
echo "::info:: Undo the bootstrap"
run 5m sudo -E ansible-playbook -i localhost, -c local --tags undo ansible/main.yaml || true
run 5m ansible-playbook -i localhost, -c local --tags undo ansible/main.yaml || true
fi
popd >/dev/null
}
Expand Down Expand Up @@ -102,7 +102,7 @@ main() {
pushd "$script_dir" >/dev/null
echo "::info:: Bootstrap the local machine"
step_bootstrap_env=1
run 10m sudo -E ansible-playbook -i localhost, -c local --tags untagged ansible/main.yaml
run 10m ansible-playbook -i localhost, -c local --tags untagged ansible/main.yaml

echo "::info:: Bring up the test cluster"
step_start_cluster=1
Expand Down
Loading