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

feat: Add ability to enable Longhorn V2 Data Engine #55

Merged
merged 4 commits into from
Aug 28, 2024

Conversation

tserong
Copy link
Contributor

@tserong tserong commented Aug 22, 2024

This adds longhornConfigs.enableV2DataEngine to the node config CRD. When this value is set to true, we:

  • Add persistent config to /oem/99_settings.yaml to run modprobe and allocate hugepages on system boot
  • Run modprobe and allocate hugepages at runtime, to try to avoid having to reboot the system
  • Restart the kubelet so longhorn is able to verify the allocated hugepages

It's probably easier to review this commit-by-commit, because I added some tests and did some refactoring first, to make sure I wasn't going to break the ntp config persistence.

Related issue: harvester/harvester#5274

@tserong
Copy link
Contributor Author

tserong commented Aug 22, 2024

(Force push was to fix lint errors)

@tserong tserong force-pushed the wip-lhv2-enablement branch from 562f1ad to 2c3f8eb Compare August 22, 2024 11:27
tserong added a commit to tserong/node-manager that referenced this pull request Aug 23, 2024
Needed for harvester#55

Signed-off-by: Tim Serong <[email protected]>
@tserong tserong mentioned this pull request Aug 23, 2024
@tserong
Copy link
Contributor Author

tserong commented Aug 23, 2024

I've opened #56 separately for the bump of go-common necessary to pick up TryRestartService(). Will rebase this PR once that's in.

Vicente-Cheng pushed a commit that referenced this pull request Aug 23, 2024
Needed for #55

Signed-off-by: Tim Serong <[email protected]>
Copy link

mergify bot commented Aug 23, 2024

This pull request is now in conflict. Could you fix it @tserong? 🙏

@tserong tserong force-pushed the wip-lhv2-enablement branch from 2c3f8eb to 5a62092 Compare August 23, 2024 05:44
@tserong
Copy link
Contributor Author

tserong commented Aug 23, 2024

Rebased

pkg/apis/node.harvesterhci.io/v1beta1/nodeconfig.go Outdated Show resolved Hide resolved
pkg/controller/nodeconfig/controller.go Show resolved Hide resolved
Copy link
Member

@brandboat brandboat left a comment

Choose a reason for hiding this comment

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

LGTM, I didn't test this in my local env, only check the code.
The unit test part looks nice, thanks.

@tserong tserong force-pushed the wip-lhv2-enablement branch from 5a62092 to 49cf969 Compare August 27, 2024 02:50
Copy link
Contributor

@Vicente-Cheng Vicente-Cheng left a comment

Choose a reason for hiding this comment

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

overall LGTM, just would like to discuss some corner cases.

pkg/controller/nodeconfig/config/common.go Outdated Show resolved Hide resolved
pkg/controller/nodeconfig/config/common.go Outdated Show resolved Hide resolved
manifests/daemonset.yaml Show resolved Hide resolved
pkg/controller/nodeconfig/config/longhorn.go Show resolved Hide resolved
pkg/controller/nodeconfig/config/longhorn.go Show resolved Hide resolved
This allows adding extra stages to oem/99_settings.yaml,
independently of the NTP settings.

Signed-off-by: Tim Serong <[email protected]>
@tserong tserong force-pushed the wip-lhv2-enablement branch from 49cf969 to c2ce0f2 Compare August 28, 2024 09:00
Copy link
Contributor

@Vicente-Cheng Vicente-Cheng left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@Vicente-Cheng
Copy link
Contributor

@Mergifyio backport v0.3.x

Copy link

mergify bot commented Aug 28, 2024

backport v0.3.x

✅ Backports have been created

@Vicente-Cheng Vicente-Cheng merged commit 75ec929 into harvester:master Aug 28, 2024
4 checks passed
@tserong tserong deleted the wip-lhv2-enablement branch August 28, 2024 09:07
tserong added a commit that referenced this pull request Aug 28, 2024
Needed for #55

Signed-off-by: Tim Serong <[email protected]>
(cherry picked from commit 0d0cfa2)
bk201 pushed a commit that referenced this pull request Aug 28, 2024
Needed for #55

Signed-off-by: Tim Serong <[email protected]>
(cherry picked from commit 0d0cfa2)
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.

4 participants