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

controller: should respect the StorageReserved setting of Longhorn #81

Closed

Conversation

Vicente-Cheng
Copy link
Collaborator

@Vicente-Cheng Vicente-Cheng commented Jan 29, 2024

Problem:
Should update the StorageReserved for extra disks.
before fixing (this is disk related info on nodes.longhorn.io)

spec:
  allowScheduling: true
  disks:
    a827440145f11f652f00f2a6ae656ed0:
      allowScheduling: true
      diskType: filesystem
      evictionRequested: false
      path: /var/lib/harvester/extra-disks/a827440145f11f652f00f2a6ae656ed0
      storageReserved: 0
      tags: []

After fixing

spec:
  allowScheduling: true
  disks:
    a827440145f11f652f00f2a6ae656ed0:
      allowScheduling: true
      diskType: filesystem
      evictionRequested: false
      path: /var/lib/harvester/extra-disks/a827440145f11f652f00f2a6ae656ed0
      storageReserved: 3221225472
      tags: []

Solution:
get the Longhorn storage reserved settings to calculate the correct value

Related Issue:
harvester/harvester#5042

Test plan:
make sure the storageReserved is the correct value. (default is 30%)

    - also update the rbac to get the longhorn settings

Signed-off-by: Vicente Cheng <[email protected]>
if err != nil {
return err
}
storageReservedRatio := 30
Copy link
Member

@votdev votdev Jan 29, 2024

Choose a reason for hiding this comment

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

Would it make sense to add a comment here to show where this default value comes from? E.g. this line.

storageReservedRatio := 30
storageReservedRatioObj, err := lhclient.LonghornV1beta2().Settings(longhornNS).Get(context.TODO(), string(longhornStorageReservedSettings), metav1.GetOptions{})
if err != nil {
logrus.Warnf("Failed to get longhorn setting %s: %v", longhornStorageReservedSettings, err)
Copy link
Member

Choose a reason for hiding this comment

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

Longhorn is the product name, so it should be in uppercase. The same applies to other occurrences.

Suggested change
logrus.Warnf("Failed to get longhorn setting %s: %v", longhornStorageReservedSettings, err)
logrus.Warnf("Failed to get Longhorn setting %s: %v", longhornStorageReservedSettings, err)

@Vicente-Cheng
Copy link
Collaborator Author

Thanks @votdev for reviewing it.

I thought maybe we did not need this PR. Confirming with LH team.

@Vicente-Cheng Vicente-Cheng marked this pull request as draft January 29, 2024 09:01
@Vicente-Cheng
Copy link
Collaborator Author

drop this because we do not need storage reserved as default for extra disks

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.

2 participants