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

Draft: chore(build): replace k8s.io/utils/mount with k8s.io/mount-utils #575

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

hensur
Copy link

@hensur hensur commented Aug 1, 2024

Currently waiting for openebs/lib-csi#21 to be merged. I'm replacing the upstream dependency with my fork, after lib-csi has been updated, I will remove that replace.

Pull Request template

Please, go through these steps before you submit a PR.

Why is this PR required? What issue does it fix?:
On Nodes with high mount activity, older versions of k8s mount-utils (or utils/mount) would call ConsistentRead to workaround a kernel bug which would skip mount entries on mount activity during a read.
See kubernetes/mount-utils@4ae857e

We noticed that the zfs provisioner would return an InconsistentRead error when the node was attaching a lot of volumes (mostly after reboots).

What this PR does?:

replaces the k8s.io/utils/mount dependency with k8s.io/mount-utils

the upstream package has been renamed: https://github.com/kubernetes/utils/tree/master/mount

The new version also includes the aforementioned fix.

Does this PR require any upgrade changes?:
No

If the changes in this PR are manually verified, list down the scenarios covered::

Any additional information for your reviewer? :
I'm not exactly sure why go mod tidy deems it necessary to also bump the module version (as well as some package versions). I assume some library requires an up to date go version, which is why this module now has to require that version as well.

Mention if this PR is part of any design or a continuation of previous PRs

Checklist:

  • Fixes #
  • PR Title follows the convention of <type>(<scope>): <subject>
  • Has the change log section been updated?
  • Commit has unit tests
  • Commit has integration tests
  • (Optional) Are upgrade changes included in this PR? If not, mention the issue/PR to track:
  • (Optional) If documentation changes are required, which issue on https://github.com/openebs/website is used to track them:

@Abhinandan-Purkait
Copy link
Member

I see the vendor directory being checked in. I don't think we should do that. cc @niladrih

@niladrih
Copy link
Member

niladrih commented Aug 7, 2024

@hensur We're not using a vendor directory. Moreover, if you rebase you branch with develop, you'd see that go version used is now 1.20 and I think mount-utils should be able to work with that. What do you think?

package has been renamed: https://github.com/kubernetes/utils/tree/master/mount

The new package also includes a change to drop consistentread calls on
newer kernels which don't require that workaround anymore:
kubernetes/mount-utils@4ae857e

Signed-off-by: Henning Surmeier <h.surmeier@mittwald.de>
@hensur
Copy link
Author

hensur commented Aug 8, 2024

Hi,
I rebased on latest develop. I also removed vendor, I based the PR on master at first, once I noticed that branch wasn't used anymore I rebased to develop. I forgot to remove vendor afterwards.

The newest mount-utils seem to require go 1.22:
https://github.com/kubernetes/mount-utils/blob/master/go.mod

@Abhinandan-Purkait
Copy link
Member

Abhinandan-Purkait commented Aug 9, 2024

This seems to be a flagged as an issue in the ci, can we use a version that works with 1.20? You probably need to bump the go versions in the github workflows.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants