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 devicemapper storage driver #8019

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

Remove devicemapper storage driver #8019

wants to merge 4 commits into from
+29 −62

Conversation

kolyshkin
Copy link
Collaborator

@kolyshkin kolyshkin commented Apr 15, 2024 •
edited

What type of PR is this?

/kind deprecation

What this PR does / why we need it:

Device mapper storage driver is hard to set up, slow, and obsoleted by overlayfs.
My best guess is nobody is using it nowadays.

Let's remove it.

Which issue(s) this PR fixes:

Fixes: #7002

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Remove device mapper storage driver.

@kolyshkin
Makefile: rm i386 hack
0e20dcd 
This was needed for build-386 job in .github/workflows/test.yml, but
it is apparently no longer necessary since commit 13d7b97 added
git to the list of preinstalled packages.

This commit reverts 13d7b97.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Makefile: rm $(PROJECT) use from paths
e1b983a 
We can use relative paths now.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin kolyshkin requested a review from mrunalp as a code owner April 15, 2024 22:00
@openshift-ci openshift-ci bot added release-note Denotes a PR that will be considered when it comes time to generate release notes. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. kind/deprecation labels Apr 15, 2024
@openshift-ci openshift-ci bot requested review from hasan4791 and klihub April 15, 2024 22:00
@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Apr 15, 2024
@kolyshkin kolyshkin changed the title Makefile Remove devicemapper storage driver Apr 15, 2024
@kolyshkin
Copy link
Collaborator Author

To add some context here, we were not able to remove DM earlier because it was used by Kata (see #7003 (comment)). This is no longer true (see containers/storage#1622 (comment)).

As for "let's deprecate it first, when remove it" concern (see #7002 (comment)) -- I think there are no existing users.

@kolyshkin
Copy link
Collaborator Author

/test ci-cgroupv2-e2e

@kolyshkin
hack/govulncheck.sh: nit
55857d8 
The comment being removed is wrong (gosec) and redundant.

Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin
Remove device mapper support
25d397f 
Signed-off-by: Kir Kolyshkin <kolyshkin@gmail.com>
@kolyshkin kolyshkin mentioned this pull request Apr 16, 2024
Closed
@kolyshkin
Copy link
Collaborator Author

/retest

@haircommander
Copy link
Member

I vote we mark as deprecated in 1.30 and remove in 1.31, just to be safe. WDYT?

@mrunalp
Copy link
Member

mrunalp commented Apr 19, 2024

Let's loop in kata folks. I think they were using it in some scenarios.
@littlejawa @fidencio ptal

@littlejawa
Copy link
Contributor

Let's loop in kata folks. I think they were using it in some scenarios. @littlejawa @fidencio ptal

Right, we had a dependency here, but it's been removed.
Checked with @fidencio, and we're ok to remove that.

@kwilczynski
Copy link
Member

/approve
/lgtm

Please remove the hold once everyone is OK to move forward.

/hold

@openshift-ci openshift-ci bot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. labels Apr 29, 2024
@saschagrunert
Copy link
Member

I vote we mark as deprecated in 1.30 and remove in 1.31, just to be safe. WDYT?

Looks like we have no consensus yet.

@kwilczynski
Copy link
Member

@cri-o/cri-o-maintainers, it's been a while. Any thoughts on moving forward here?

@haircommander
Copy link
Member

yeah if a user wants to use devicemapper, they can compile on their own still (until c/storage removes it)
/approve

WDYT @cri-o/cri-o-maintainers

@kwilczynski
Copy link
Member

@haircommander, sounds good!

/approve
/lgtm

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented May 23, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: haircommander, kolyshkin, kwilczynski

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:
  • OWNERS [haircommander,kolyshkin]

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mrunalp mrunalp Awaiting requested review from mrunalp mrunalp is a code owner

@hasan4791 hasan4791 Awaiting requested review from hasan4791

@klihub klihub Awaiting requested review from klihub

Assignees

@kwilczynski kwilczynski

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. do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. kind/deprecation 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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Deprecate and remove device mapper storage driver
6 participants
@kolyshkin @haircommander @mrunalp @littlejawa @kwilczynski @saschagrunert