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

doc: add a page explaining storage class parameters #762

Merged
merged 10 commits into from
Sep 11, 2023

Conversation

james-munson
Copy link
Contributor

@james-munson james-munson commented Aug 1, 2023

@james-munson james-munson requested a review from a team as a code owner August 1, 2023 22:13
Copy link
Contributor

@shuo-wu shuo-wu left a comment

Choose a reason for hiding this comment

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

  1. It's better to add one example at the beginning.
  2. I think we can explain how Longhorn creates the default SC. And why we use a ConfigMap to create that default SC (since some fields are immutable.)

shuo-wu
shuo-wu previously approved these changes Aug 8, 2023
Copy link
Contributor

@shuo-wu shuo-wu left a comment

Choose a reason for hiding this comment

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

In general LGTM

Copy link
Collaborator

@ejweber ejweber left a comment

Choose a reason for hiding this comment

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

Much improved. A couple of questions still. One big thing we need to do is "backport" this to other versions of the website. Probably we don't need to go all the way back to the beginning of time, but to the versions that aren't EOL. That will be annoying, as some of these options are new.

Copy link
Member

@innobead innobead left a comment

Choose a reason for hiding this comment

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

In general, LGTM. Just some minor suggestions.

Good job, James.

content/docs/1.6.0/references/storage-class-parameters.md Outdated Show resolved Hide resolved
content/docs/1.6.0/references/storage-class-parameters.md Outdated Show resolved Hide resolved
content/docs/1.6.0/references/storage-class-parameters.md Outdated Show resolved Hide resolved
content/docs/1.6.0/references/storage-class-parameters.md Outdated Show resolved Hide resolved
content/docs/1.6.0/references/storage-class-parameters.md Outdated Show resolved Hide resolved
@PhanLe1010
Copy link
Contributor

The PR LGTM once all the above review comments are resolved.

 & incorporate review feedback

Signed-off-by: James Munson <[email protected]>
Signed-off-by: James Munson <[email protected]>
Signed-off-by: James Munson <[email protected]>
Signed-off-by: James Munson <[email protected]>
Copy link
Collaborator

@ejweber ejweber left a comment

Choose a reason for hiding this comment

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

It looks like you addressed every comment of mine but one. This is looking really good. Still need to add a version of this (minus newly introduced parameters) to other versions IMO. Let me know if you disagree.

@innobead innobead self-requested a review August 30, 2023 06:39
Copy link
Member

@innobead innobead left a comment

Choose a reason for hiding this comment

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

LGTM. The only remaining from my side is to have this page for the latest existing patch or upcoming patch release doc as well, such as 1.3.3. 1.4.3 & 1.4.4, 1.5.1 & 1.5.2.

P.S. No need to have that in 1.3.4, since it's EOL and we will not release it.

@james-munson
Copy link
Contributor Author

james-munson commented Aug 30, 2023

I added the 1.4.x and 1.5.x versions.
1.4.x are all identical.
1.5.x are all identical to each other, and differ from 1.4.x in that the backendStoreDriver (v2 engine) parameter is added.
1.6.0 differs from 1.5.x in that the replica disk soft anti-affinity is added. (Looking ahead to when Eric's PR is merged! It will need one tiny tweak when that happens, to point to the global setting, which doesn't exist yet, and gives an invalid reference in the build.)

I did not add a 1.3.x version. If we do that, it will also require adjusting filesystem trim and mkfs parameters. WDYT? Worth going back that far?

@james-munson
Copy link
Contributor Author

Also added documentation for "replica disk soft anti-affinity" to global settings and replica scheduling section.

ejweber
ejweber previously approved these changes Aug 31, 2023
Copy link
Collaborator

@ejweber ejweber left a comment

Choose a reason for hiding this comment

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

Thanks for the Replica Disk Soft Anti Affinity addition. Looks good!

I am personally fine with not including this page on the v1.3.x branch given its EOL status, so approving. Not sure where others will fall.

@PhanLe1010
Copy link
Contributor

I am fine with not having it in 1.3.x too

Copy link
Member

@innobead innobead left a comment

Choose a reason for hiding this comment

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

LGTM, but some comments need to resolve first.

> Global setting: [Replica Zone Level Soft Anti-Affinity](../settings#replica-zone-level-soft-anti-affinity).
> More details in [Scheduling](../../volumes-and-nodes/scheduling).

#### Backend Store Driver *(field: `parameters.backendStoreDriver`)*
Copy link
Member

Choose a reason for hiding this comment

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

The value should be v1 or v2. If it's empty, it's just v1.

/k8s/pkg/apis/longhorn/v1beta2/volume.go#L162-L165

const (
	BackendStoreDriverTypeV1 = BackendStoreDriverType("v1")
	BackendStoreDriverTypeV2 = BackendStoreDriverType("v2")
)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Fixed, thanks.


When this setting is un-checked, Longhorn Manager will not allow scheduling new replicas of a volume to the same disks as existing healthy replicas.

> **Note:**
Copy link
Member

@innobead innobead Sep 8, 2023

Choose a reason for hiding this comment

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

Why is it not false by default? A disk with two replicas of a volume seems not a good practice.

BTW, Node Soft Anti Affinity will supersede this setting if it's false. It's better to mention this.

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added, thanks.

@innobead innobead merged commit 0b22c15 into longhorn:master Sep 11, 2023
6 checks passed
@james-munson james-munson deleted the 4776-storage-class-parms branch September 11, 2023 21:29
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.

5 participants