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

Jump to bottom

Remove python dep from handler and launcher containers #6593

Merged
merged 1 commit into from Oct 14, 2021
Merged

Remove python dep from handler and launcher containers #6593

merged 1 commit into from Oct 14, 2021

Conversation

rhrazdil
Copy link

Signed-off-by: Radim Hrazdil rhrazdil@redhat.com

What this PR does / why we need it:
This PR removed python packages from virt-launcher and handler, as it's not used anymore.

Depends on #6579 which removes the last python usage.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #6490

Special notes for your reviewer:

Release note:

Removed python dependencies from virt-launcher and virt-handler containers

@kubevirt-bot kubevirt-bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. size/XL labels Oct 13, 2021
rmohr
rmohr reviewed Oct 13, 2021
View reviewed changes
hack/rpm-deps.sh Outdated
@@ -163,6 +161,7 @@ bazel run \
--name launcherbase_x86_64 \
--basesystem centos-stream-release \
--force-ignore-with-dependencies '^mozjs60' \
--force-ignore-with-dependencies '^python' \
Copy link
Member

Choose a reason for hiding this comment

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

I think you see a compile error now because we try to still create some python binary simplinks on the tar files.

If you remove on the launcher and handler base images these lines:

        "/usr/bin/python": "/usr/bin/python3.6",
        "/usr/bin/python3": "/usr/bin/python3.6",

in the symlink section in rpm/BUILD.bazel it should build successfully again.

rpm/BUILD.bazel Outdated
@@ -477,26 +474,14 @@ rpmtree(
"@pcre2-0__10.32-2.el8.aarch64//rpm",
"@pixman-0__0.38.4-1.el8.aarch64//rpm",
"@platform-python-0__3.6.8-42.el8.aarch64//rpm",
Copy link
Member

Choose a reason for hiding this comment

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

It may even be possible to also force-remove platform-python*.

Copy link
Author

Choose a reason for hiding this comment

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

I changed ^python to python, would you suggest to stay more explicit and rather do this?

    --force-ignore-with-dependencies '^python' \
    --force-ignore-with-dependencies '^platform-python*' \
    --force-ignore-with-dependencies '^policycoreutils-python*' \

@kubevirt-bot kubevirt-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 14, 2021
@kubevirt-bot kubevirt-bot added size/L and removed needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL labels Oct 14, 2021
rmohr
rmohr reviewed Oct 14, 2021
View reviewed changes
rpm/BUILD.bazel Outdated
"@python3-pip-wheel-0__9.0.3-20.el8.x86_64//rpm",
"@python3-setuptools-0__39.2.0-6.el8.x86_64//rpm",
"@python3-setuptools-wheel-0__39.2.0-6.el8.x86_64//rpm",
"@python36-0__3.6.8-38.module_el8.5.0__plus__895__plus__a459eca8.x86_64//rpm",
Copy link
Member

Choose a reason for hiding this comment

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

This are the dependencies of the sandbox which bazel uses for building. Can it be that you added the force-remove line also to the sandbox rpmtree creation? This one needs python since some bazel dependencies need it.

Copy link
Author

Choose a reason for hiding this comment

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

Ah, thank you! Yes, I was on python removing spree. Looking sane now

rmohr
rmohr requested changes Oct 14, 2021
View reviewed changes
hack/rpm-deps.sh Outdated
@@ -107,7 +107,6 @@ sandbox_base="
findutils
gcc
glibc-static
python36
Copy link
Member

Choose a reason for hiding this comment

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

The sandbox needs python. We need to keep this one.

rpm/BUILD.bazel Outdated
"@zlib-0__1.2.11-17.el8.x86_64//rpm",
],
symlinks = {
"/var/run": "../run",
"/usr/bin/python": "/usr/bin/python3.6",
Copy link
Member

Choose a reason for hiding this comment

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

The sandbox still needs these links. Just handler and launcher base images don't need it anymore.

Copy link
Author

Choose a reason for hiding this comment

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

Right, thanks!

Remove python dep from handler and launcher containers
8e160f1 
Python is not used and thus can be removed.

Signed-off-by: Radim Hrazdil <rhrazdil@redhat.com>
@rhrazdil rhrazdil changed the title [WIP] Remove python dep from handler and launcher containers Remove python dep from handler and launcher containers Oct 14, 2021
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 14, 2021
@rhrazdil
Copy link
Author

/test pull-kubevirt-e2e-k8s-1.22-sig-storage

@kubevirt-bot
Copy link
Contributor

@rhrazdil: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name Commit Details Required Rerun command
pull-kubevirt-e2e-k8s-1.22-sig-storage 8e160f1 link false /test pull-kubevirt-e2e-k8s-1.22-sig-storage

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@rhrazdil
Copy link
Author

/test pull-kubevirt-e2e-k8s-1.22-sig-storage

@rmohr
Copy link
Member

rmohr commented Oct 14, 2021

/lgtm
/approve
@rhrazdil thanks!

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Oct 14, 2021
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rmohr

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kubevirt-bot kubevirt-bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 14, 2021
@kubevirt-bot kubevirt-bot merged commit 43cd2a3 into kubevirt:main Oct 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@rmohr rmohr rmohr requested changes

@enp0s3 enp0s3 Awaiting requested review from enp0s3

@vatsalparekh vatsalparekh Awaiting requested review from vatsalparekh

Assignees

@rmohr rmohr

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Removing dependencies on python inside virt-launcher and virt-handler
3 participants
@rhrazdil @kubevirt-bot @rmohr