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

Remove losetup workaround in minikube VM #1840

Merged
merged 1 commit into from
Apr 19, 2021

Conversation

nixpanic
Copy link
Member

@nixpanic nixpanic commented Jan 22, 2021

Describe what this PR does

Currently the losetup executable is copied into the minikube VM because the version of losetup inside the VM does not support the -j option which is required for BlockMode volumes.

Related issues

See-also: kubernetes/minikube#8284 and kubernetes/minikube#10255


Show available bot commands

These commands are normally not required, but in case of issues, leave any of
the following bot commands in an otherwise empty comment in this PR:

  • /retest ci/centos/<job-name>: retest the <job-name> after unrelated
    failure (please report the failure too!)
  • /retest all: run this in case the CentOS CI failed to start/report any test
    progress or results

@nixpanic nixpanic added DNM DO NOT MERGE dependency/k8s depends on Kubernetes features component/testing Additional test cases or CI work labels Jan 22, 2021
@nixpanic
Copy link
Member Author

FWIW, the minikube.iso that is used for testing has been built with this change.

@nixpanic nixpanic force-pushed the testing/remove-losetup-workaround branch 2 times, most recently from e29d1c2 to 0c8c6c1 Compare January 25, 2021 10:15
@nixpanic
Copy link
Member Author

kubernetes/minikube#10255 has been merged, we'll have to wait for a minikube release that includes it before this PR can get moved out of draft.

@nixpanic
Copy link
Member Author

nixpanic commented Feb 1, 2021

https://github.com/kubernetes/minikube/tree/v1.17.1 has been released and adds losetup from util-linux. The workaround is not needed anymore. This PR can get merged once #1811 is included.

@nixpanic nixpanic force-pushed the testing/remove-losetup-workaround branch from 0c8c6c1 to 335c15a Compare February 10, 2021 15:51
@nixpanic nixpanic marked this pull request as ready for review February 10, 2021 15:51
@nixpanic nixpanic removed the DNM DO NOT MERGE label Feb 10, 2021
@nixpanic nixpanic requested a review from Madhu-1 February 10, 2021 15:52
@@ -130,17 +130,6 @@ function disable_storage_addons() {
${minikube} addons disable storage-provisioner 2>/dev/null || true
}

# minikube has the Busybox losetup, and that does not work with raw-block PVCs.
Copy link
Collaborator

Choose a reason for hiding this comment

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

removing this one will cause any issue for the one who is using an older minikube version?

@nixpanic
Copy link
Member Author

nixpanic commented Feb 10, 2021 via email

@Madhu-1
Copy link
Collaborator

Madhu-1 commented Feb 10, 2021

On Wed, Feb 10, 2021 at 07:54:50AM -0800, Madhu Rajanna wrote: @Madhu-1 commented on this pull request. > @@ -130,17 +130,6 @@ function disable_storage_addons() { ${minikube} addons disable storage-provisioner 2>/dev/null || true } -# minikube has the Busybox losetup, and that does not work with raw-block PVCs. removing this one will cause any issue for the one who is using an older minikube version?
Yes. But unfortunately we do not keep track of versions of dependencies we have for testing, so I think that is ok.

if we keep the current workaround for losetup it will it causes any issues for people who are using 1.17.1+?

@nixpanic
Copy link
Member Author

if we keep the current workaround for losetup it will it causes any issues for people who are using 1.17.1+?

Possibly. There is no guarantee that the binary from the host works on the guest. We are just lucky it does. If anyone runs the setup script on a different Linux distribution, or on an other architecture, there is a big chance things will break.

@nixpanic
Copy link
Member Author

nixpanic commented Feb 11, 2021

It seems the minikube iso has not been updated? kubernetes/minikube#10444 has been filed to track it.

@nixpanic
Copy link
Member Author

nixpanic commented Feb 11, 2021

It seems the minikube iso has not been updated? kubernetes/minikube#10444 has been filed to track it.

Minor minikube versions mostly do not provide a new iso image. We'll have to wait until minikube 1.18.0 to get this merged.

@Madhu-1 Madhu-1 added the DNM DO NOT MERGE label Feb 11, 2021
@Madhu-1
Copy link
Collaborator

Madhu-1 commented Feb 11, 2021

It seems the minikube iso has not been updated? kubernetes/minikube#10444 has been filed to track it.

Minor minikube versions mostly do not provide a new iso image. We'll have to wait until minikube 1.18.0 to get this merged.

adding DNM till we get minikube major release

Base automatically changed from master to devel March 1, 2021 05:22
nixpanic added a commit to nixpanic/ceph-csi that referenced this pull request Mar 2, 2021
Minikube v1.18 has been released. This conains a fix for our `losetup`
workaround, so that can be removed now.

Updates: ceph#1840
Signed-off-by: Niels de Vos <ndevos@redhat.com>
ceph-csi-bot pushed a commit to nixpanic/ceph-csi that referenced this pull request Mar 3, 2021
Minikube v1.18 has been released. This conains a fix for our `losetup`
workaround, so that can be removed now.

Updates: ceph#1840
Signed-off-by: Niels de Vos <ndevos@redhat.com>
mergify bot pushed a commit that referenced this pull request Mar 3, 2021
Minikube v1.18 has been released. This conains a fix for our `losetup`
workaround, so that can be removed now.

Updates: #1840
Signed-off-by: Niels de Vos <ndevos@redhat.com>
@nixpanic nixpanic removed the DNM DO NOT MERGE label Mar 3, 2021
@nixpanic
Copy link
Member Author

nixpanic commented Mar 3, 2021

Deploying Rook failed with:

  Normal   BackOff    24s (x2 over 48s)  kubelet            Back-off pulling image "docker.io/ceph/ceph:v14.2.12"
  Warning  Failed     24s (x2 over 48s)  kubelet            Error: ImagePullBackOff
  Normal   Pulling    9s (x3 over 49s)   kubelet            Pulling image "docker.io/ceph/ceph:v14.2.12"
  Warning  Failed     9s (x3 over 49s)   kubelet            Failed to pull image "docker.io/ceph/ceph:v14.2.12": rpc error: code = Unknown desc = Error response from daemon: toomanyrequests: You have reached your pull rate limit. You may increase the limit by authenticating and upgrading: https://www.docker.com/increase-rate-limit

Need to pull the ceph image from the local CI mirror into the minikube VM.

@nixpanic
Copy link
Member Author

nixpanic commented Mar 3, 2021

Tests can be re-run after #1903 had been merged

@nixpanic
Copy link
Member Author

nixpanic commented Mar 3, 2021

/retest all

@nixpanic
Copy link
Member Author

nixpanic commented Mar 3, 2021

Even with minikube v1.18.0 the CI jobs fail with

Mar  3 12:45:09.791: INFO: At 2021-03-03 12:35:13 +0000 GMT - event for pod-with-raw-block-volume: {kubelet minikube} FailedMapVolume: MapVolume.MapBlockVolume failed for volume "pvc-c253a4c4-9fea-4de1-99cc-787d28f0c6c4" : blkUtil.AttachFileDevice failed. globalMapPath:/var/lib/kubelet/plugins/kubernetes.io/csi/volumeDevices/pvc-c253a4c4-9fea-4de1-99cc-787d28f0c6c4/dev, podUID: 5671604c-d8e5-4cc9-897f-a8012c635340: GetLoopDevice failed for path /var/lib/kubelet/plugins/kubernetes.io/csi/volumeDevices/pvc-c253a4c4-9fea-4de1-99cc-787d28f0c6c4/dev/5671604c-d8e5-4cc9-897f-a8012c635340: losetup -j /var/lib/kubelet/plugins/kubernetes.io/csi/volumeDevices/pvc-c253a4c4-9fea-4de1-99cc-787d28f0c6c4/dev/5671604c-d8e5-4cc9-897f-a8012c635340 failed: exit status 1

This suggests that the losetup command in the minikube iso is still the Busybox one, and not from util-linux. Weird.

Looking into it, kubernetes/minikube#10286 reverted the change... Trying to find out what the problem would be (as building the test iso failed with the revert too).

@nixpanic
Copy link
Member Author

nixpanic commented Mar 3, 2021

kubernetes/minikube#10704 adds losetup from util-linux to the ISO image again... sigh

@nixpanic
Copy link
Member Author

@Mergifyio rebase

Signed-off-by: Niels de Vos <ndevos@redhat.com>
@mergify
Copy link
Contributor

mergify bot commented Apr 19, 2021

Command rebase: success

Branch has been successfully rebased

@ceph-csi-bot ceph-csi-bot force-pushed the testing/remove-losetup-workaround branch from 335c15a to 2c9696f Compare April 19, 2021 11:16
@mergify mergify bot merged commit 27247d1 into ceph:devel Apr 19, 2021
@mergify
Copy link
Contributor

mergify bot commented Apr 19, 2021

This pull request has been merged with the
unsupported configuration option bot_account.

This option will be ignored starting May 1st, 2021, and removed
on June 1st, 2021.

This option can be replaced by update_bot_account, merge_bot_account or both
depending on your use-case (https://docs.mergify.io/actions/merge/).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component/testing Additional test cases or CI work dependency/k8s depends on Kubernetes features
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants