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

cleanup: create k8s.io/mount-utils Mounter only once #3247

Merged
merged 1 commit into from
Jul 21, 2022

Conversation

nixpanic
Copy link
Member

Recently the k8s.io/mount-utils package added more runtime dectection.
When creating a new Mounter, the detect is run every time. This is
unfortunate, as it logs a message like the following:

mount_linux.go:283] Detected umount with safe 'not mounted' behavior

This message might be useful, so it probably good to keep it.

In Ceph-CSI there are various locations where Mounter instances are
created. Moving that to the DefaultNodeServer type reduces it to a
single place. Some utility functions need to accept the additional
parameter too, so that has been modified as well.

See-also: kubernetes/kubernetes#109676
Updates: #3246


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

@mergify mergify bot added the cleanup label Jul 18, 2022
return &rbd.NodeServer{
DefaultNodeServer: csicommon.NewDefaultNodeServer(d, t, topology),
Mounter: mounter,
Copy link
Collaborator

Choose a reason for hiding this comment

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

we need this change for rbd right? or else it will panic when we try to access mounter

Copy link
Member Author

Choose a reason for hiding this comment

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

no, that should be set by the DefaultNodeServer... not sure why this isn't working as I expected 🤔

Having a look now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Dropping the overloading Mounter attribute from rbd.NodeServer should work 🤞

@nixpanic nixpanic force-pushed the cleanup/no-mounter-recreate branch from fdcfc05 to 2828ef0 Compare July 19, 2022 07:21
@nixpanic nixpanic requested a review from Madhu-1 July 19, 2022 07:22
@Rakshith-R
Copy link
Contributor

@Mergifyio rebase

@mergify
Copy link
Contributor

mergify bot commented Jul 20, 2022

rebase

✅ Branch has been successfully rebased

@nixpanic nixpanic force-pushed the cleanup/no-mounter-recreate branch from 2828ef0 to 51e9b43 Compare July 20, 2022 06:32
@Rakshith-R
Copy link
Contributor

@Mergifyio rebase

Recently the k8s.io/mount-utils package added more runtime dectection.
When creating a new Mounter, the detect is run every time. This is
unfortunate, as it logs a message like the following:

```
mount_linux.go:283] Detected umount with safe 'not mounted' behavior
```

This message might be useful, so it probably good to keep it.

In Ceph-CSI there are various locations where Mounter instances are
created. Moving that to the DefaultNodeServer type reduces it to a
single place. Some utility functions need to accept the additional
parameter too, so that has been modified as well.

See-also: kubernetes/kubernetes#109676
Signed-off-by: Niels de Vos <ndevos@redhat.com>
@mergify
Copy link
Contributor

mergify bot commented Jul 20, 2022

rebase

✅ Branch has been successfully rebased

@nixpanic nixpanic force-pushed the cleanup/no-mounter-recreate branch from 51e9b43 to f2d8a22 Compare July 20, 2022 09:44
@nixpanic nixpanic requested a review from a team July 20, 2022 14:49
@Rakshith-R Rakshith-R added the ci/retry/e2e Label to retry e2e retesting on approved PR's label Jul 21, 2022
@ceph-csi-bot
Copy link
Collaborator

/retest ci/centos/mini-e2e/k8s-1.22

@ceph-csi-bot
Copy link
Collaborator

@nixpanic "ci/centos/mini-e2e/k8s-1.22" test failed. Logs are available at location for debugging

@ceph-csi-bot
Copy link
Collaborator

/retest ci/centos/k8s-e2e-external-storage/1.23

@ceph-csi-bot
Copy link
Collaborator

@nixpanic "ci/centos/k8s-e2e-external-storage/1.23" test failed. Logs are available at location for debugging

@ceph-csi-bot
Copy link
Collaborator

/retest ci/centos/mini-e2e/k8s-1.23

@ceph-csi-bot
Copy link
Collaborator

@nixpanic "ci/centos/mini-e2e/k8s-1.23" test failed. Logs are available at location for debugging

@ceph-csi-bot
Copy link
Collaborator

/retest ci/centos/upgrade-tests-cephfs

@ceph-csi-bot
Copy link
Collaborator

@nixpanic "ci/centos/upgrade-tests-cephfs" test failed. Logs are available at location for debugging

@ceph-csi-bot
Copy link
Collaborator

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Jul 21, 2022

requeue

❌ This pull request head commit has not been previously disembarked from queue.

@ceph-csi-bot
Copy link
Collaborator

/retest ci/centos/k8s-e2e-external-storage/1.23

@ceph-csi-bot
Copy link
Collaborator

@nixpanic "ci/centos/k8s-e2e-external-storage/1.23" test failed. Logs are available at location for debugging

@ceph-csi-bot
Copy link
Collaborator

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Jul 21, 2022

requeue

❌ This pull request head commit has not been previously disembarked from queue.

@ceph-csi-bot
Copy link
Collaborator

/retest ci/centos/mini-e2e/k8s-1.23

@ceph-csi-bot
Copy link
Collaborator

@nixpanic "ci/centos/mini-e2e/k8s-1.23" test failed. Logs are available at location for debugging

@ceph-csi-bot
Copy link
Collaborator

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Jul 21, 2022

requeue

❌ This pull request head commit has not been previously disembarked from queue.

@ceph-csi-bot
Copy link
Collaborator

/retest ci/centos/mini-e2e/k8s-1.23

@ceph-csi-bot
Copy link
Collaborator

@nixpanic "ci/centos/mini-e2e/k8s-1.23" test failed. Logs are available at location for debugging

@ceph-csi-bot
Copy link
Collaborator

/retest ci/centos/k8s-e2e-external-storage/1.23

@ceph-csi-bot
Copy link
Collaborator

@nixpanic "ci/centos/k8s-e2e-external-storage/1.23" test failed. Logs are available at location for debugging

@ceph-csi-bot
Copy link
Collaborator

@Mergifyio requeue

@mergify
Copy link
Contributor

mergify bot commented Jul 21, 2022

requeue

❌ This pull request head commit has not been previously disembarked from queue.

@mergify mergify bot merged commit 011d4fc into ceph:devel Jul 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci/retry/e2e Label to retry e2e retesting on approved PR's cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants