-
Notifications
You must be signed in to change notification settings - Fork 69
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(setting): add upgrade-config setting #626
Conversation
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For customer convenience, I have following suggestions, thanks.
:::note | ||
|
||
The `concurrency` option only takes effect when the image-preload strategy `type` is set to `parallel`. | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add another:
Harvester deploys a cluster-repo
service on the cluster to serve as a http server for nodes to preload the image. When concurrency
is set, certain number of nodes will download the Harvester ISO from this cluster-repo
in parallel, following factors need to be considered:
(1) The Harvester management network speed.
(2) The writing speed of the default disk for Longhorn.
When your cluster has 20+ nodes, suggest to set the concurrency
as 5
.
note: Before we have tested this on a at least medium sized cluster, it is better to suggest a conservative number, rather than 0.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we don't have data from experiments about the appropriate concurrency value, IMO we should not publish any concrete number in the documentation. Maybe just add a "this is an experimental function" warning?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
experimental
is a good label
@starbops I'll start my review after you check Jian's comments. Also, I noticed that you used separate headings for the options. We now use an unordered list for such information (within the section dedicated to the setting). Is there any reason you decided to structure the content this way? Also, are all of the options required? |
Hi @w13915984028 thank you for the suggestion. I've updated the content, PTAL. Hi @jillian-maroket because each option under the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks.
@starbops Here is the recommended wording and structure:
|
Thank you @jillian-maroket. I slightly changed the wording in the last "note" section:
Please take a look. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@starbops The changes work for me so I'll approve now. Just please correct the grammatical error that I flagged ("a" > "an"). Thanks!
Signed-off-by: Zespre Chang <zespre.chang@suse.com> Co-authored-by: Jian Wang <jian.wang@suse.com> Co-authored-by: Jillian <67180770+jillian-maroket@users.noreply.github.com>
Add description for the newly introduced
upgrade-config
setting and its sub-options.Related issues:
harvester/harvester#3059
harvester/harvester#6028