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

fix: remove the logic that sets datalocality to disable when v2 volume is created #3133

Merged
merged 1 commit into from
Sep 23, 2024

Conversation

hookak
Copy link
Contributor

@hookak hookak commented Sep 2, 2024

Which issue(s) this PR fixes:

longhorn/longhorn#9280
longhorn/longhorn#9371

What this PR does / why we need it:

when a Longhorn engine v2 volume is created, the dataLocality field is always set to disable by the volume mutator in the webhook. To allow the use of different dataLocality settings for engine v2 volumes, this behavior should be removed.

Special notes for your reviewer:

Additional documentation or context

@hookak hookak changed the title fix: The datalocality of engine v2 volumes is always set to disable by the mutator. fix: remove the logic that sets datalocality to disable when v2 volume is created Sep 2, 2024
@hookak hookak force-pushed the remove-enginev2-volume-mutated branch from e26e386 to d3c77ab Compare September 2, 2024 12:22
@derekbit
Copy link
Member

derekbit commented Sep 3, 2024

@hookak
Nice catch! Thanks for your contribution.
We just implemented the online replica rebuilding feature in v1.7.0 and haven't verified the data locality feature. It is time to verify and support it in v1.8.0.

BTW, are you using v2 volumes in your environment?

@hookak
Copy link
Contributor Author

hookak commented Sep 3, 2024

Thanks, the data locality feature is important for our purposes.

We are looking for high-performance block storage, and the v2 volume is one of the options we're considering. We are testing its functionality and performance. The most important thing is whether we can operate this system reliably at a production level, but we haven't found a good example of that yet.

We are still not very familiar with Longhorn, especially the v2 volumes, so if there are any considerations we should be aware of, it would be great if you could let us know. @derekbit

@derekbit derekbit force-pushed the remove-enginev2-volume-mutated branch 2 times, most recently from cf68998 to 109cd48 Compare September 10, 2024 01:45
@derekbit
Copy link
Member

Hello @hookak

Need to remove https://github.com/longhorn/longhorn-manager/blob/master/webhook/resources/volume/validator.go#L269-L273 as well. After the removal, we can merge the PR. Thank you.

@derekbit derekbit force-pushed the remove-enginev2-volume-mutated branch from 109cd48 to fe8bea7 Compare September 23, 2024 06:32
…e is created

Signed-off-by: jinhong.kim0 <jinhong.kim0@navercorp.com>
@hookak hookak force-pushed the remove-enginev2-volume-mutated branch from fe8bea7 to 4bc31e2 Compare September 23, 2024 06:53
@hookak
Copy link
Contributor Author

hookak commented Sep 23, 2024

Hello, @derekbit

I have also removed the logic from the volume validator(https://github.com/longhorn/longhorn-manager/blob/master/webhook/resources/volume/validator.go#L269-L273)

Copy link
Member

@derekbit derekbit left a comment

Choose a reason for hiding this comment

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

LGTM

@derekbit derekbit merged commit 2edf8ec into longhorn:master Sep 23, 2024
7 checks passed
@derekbit
Copy link
Member

Merged. Thanks for your contribution @hookak. ;)

@innobead
Copy link
Member

@hookak thanks for the contribution!

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