CNV-19689: Add note about resource quotas #48252

bgaydosrh · 2022-07-25T19:03:49Z

For 4.8, 4.9, 4.10, 4.11, and 4.12.

Jira: https://issues.redhat.com/browse/CNV-19689
BZ: https://bugzilla.redhat.com/show_bug.cgi?id=2104430

Direct doc preview link: https://48252--docspreview.netlify.app/openshift-enterprise/latest/virt/virtual_machines/advanced_vm_management/virt-working-with-resource-quotas-for-vms.html

Tagging @fabiand and @stu-gott for Code Review
Tagging Oded Ramraz to assign a QE in Jira.

Thakns
Bob

stu-gott

One small suggestion, otherwise this looks good. thanks!

modules/virt-about-resource-quota-limits.adoc

stu-gott · 2022-08-05T17:12:26Z

/cc @dshchedr would you have bandwidth to review this?

stu-gott

LGTM for content

stu-gott · 2022-08-05T17:22:26Z

I've never seen that before. I reviewed so fast that github got the sequence out of order. My LGTM applies to this point in time.

modules/virt-about-resource-quota-limits.adoc

dshchedr · 2022-08-05T18:29:30Z

Thanks,
/LGTM

openshift-ci · 2022-08-05T18:29:55Z

@dshchedr: changing LGTM is restricted to collaborators

In response to this:

Thanks,
/LGTM

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

ousleyp

I think this would benefit from a bit of rework. Please let me know if you have any questions about my comments. Thanks!

ousleyp · 2022-08-05T18:41:03Z

virt/virtual_machines/advanced_vm_management/virt-specifying-nodes-for-vms.adoc

@@ -8,6 +8,8 @@ toc::[]

 You can place virtual machines (VMs) on specific nodes by using node placement rules.

+include::modules/virt-about-resource-quota-limits.adoc[leveloffset=+1]


I'm confused about this placement; does it apply to this assembly specifically? I checked the BZ and it looks like @fabiand linked to the node placement assembly because you can't link directly to the section on docs.openshift.com.

Section Number and Name: "Advanced Virtual Machine management"

Should this have its own simple (one module and an intro sentence) assembly instead? Or is there an existing assembly on resource quotas?

After revisiting the 4.10 docs, I'd suggest that inside the "Advanced Virtual Machine management" section, we add a new section called "Working with ResourceQuotas".

Thanks @ousleyp and @fabiand and apologies for taking so long to wrap this up :-)

I only have one other question which I have tagged @ousleyp on below.

Pan, if you think this makes more sense as just an assembly with no module, let me know.

Also, from conversations with @stu-gott, he had stated that the resource quota implementation was the same in both OCP and CNV, except in CNV you only work with quotas for VMs. Let me know what you think about that and possible rewording. Thanks.

I'd definitely suggest keeping the procedure in its own module; assemblies without any modules are rare edge cases and usually created out of necessity/as a workaround for something like the xref-in-modules ban

ousleyp · 2022-08-05T18:43:42Z

modules/virt-about-resource-quota-limits.adoc

+  template:
+    spec:
+      domain:
+        …


I think we usually left-justify the "..." (also, this looks like a special character that should be swapped to ...)

The docs guidelines also state "When truncating YAML, always precede an ellipsis with a pound so that the YAML is valid."

Whatever is needed :)

Done, thanks I was not aware of the # but that makes sense.

ousleyp · 2022-08-05T19:12:06Z

modules/virt-about-resource-quota-limits.adoc

+          limits:
+            memory: 256Mi  <1>
+----
+<1> `resources/limits` is twice the size of `resource/requests`, meeting the `100Mi` requirement.


This callout is somewhat confusing when taken out of context, and I think it might not be localization-friendly. (We also use ., not / to delineate YAML fields.) Here's a suggestion for you to consider:

<1> This configuration is supported because the `limits.memory` value is at least `100Mi` larger than the `requests.memory` value.

+1 on Pans wording

ousleyp · 2022-08-05T19:13:00Z

modules/virt-about-resource-quota-limits.adoc

+
+:_content-type: CONCEPT
+[id="virt-about-resource-quota-limits_{context}"]
+= About resource limits


The ID/filename/title don't quite match. Should this be titled "About resource quota limits" or should the filename/ID be changed to remove "quota"? Edit: I think this should be converted into a procedure module, in which case you'd need to retitle it to something that starts with a gerund.

However it is worded, but the title should refer to working with ResourceQuotas

I changed the ID and title to "Setting resource quota limits" and made this a PROCEDURE module called by the "Working with resource quotas" assembly.

@ousleyp - Do you think I should retain "in {VirtProductName}" in the assembly and module titles to differentiate from OCP or do you think that's just redundant?

I don't think we need "in {VirtProductName}" but I wouldn't be opposed to you adding something like "...for virtual machines" to a title. I'm kind of ambivalent, though, since it's in the CNV docs

I'll probably include "for VMs" just for findability.

ousleyp · 2022-08-05T20:11:08Z

modules/virt-about-resource-quota-limits.adoc

+[id="virt-about-resource-quota-limits_{context}"]
+= About resource limits
+
+You can only set `ResourceQuotas` for virtual machines if the resource quota has a set of requests. If the resource quota contains both requests and limits, the resource limits on the virtual machine must be set.


I am confused by this intro, as well as its location in this assembly (which doesn't mention resource quotas).

I think this should be a procedure rather than a concept.

After reviewing the bug, I would reword the intro to something like this:

Resource quotas that only use requests automatically work with virtual machines (VMs). If your resource quota uses limits, you must manually set resource limits on VMs. Resource limits must be at least 100 MiB larger than resource requests.

Re: s/megabytes/MiB: the Mi in the YAML makes me think that it is supposed to be MiB (mebibytes). The ISG also seems to recommend just writing MiB instead of spelling it out: https://www.ibm.com/docs/en/ibm-style?topic=measurement-units

I think there should be a procedure step or two; something like this:

. Set limits for a VM by editing the `VirtualMachine` manifest. For example: + <yaml example> . <step to apply changes>

Pan, +1 on your suggested wording.

Thanks, Pan. Much clearer.

bergerhoffer · 2022-08-10T12:59:16Z

The enterprise-4.12 label has been added to this PR.

This is because your PR targets the main branch and is labeled for enterprise-4.11. And any PR going into main must also target the latest version branch (enterprise-4.12).

If the update in your PR does NOT apply to version 4.12 onward, please re-target this PR to go directly into the appropriate version branch or branches (enterprise-4.x) instead of main.

ousleyp · 2022-09-22T16:52:39Z

modules/virt-about-resource-quota-limits.adoc

+// * virt/virtual_machines/advanced_vm_management/virt-specifying-nodes-for-vms.adoc
+
+:_content-type: PROCEDURE
+[id="virt-setting-resource-quota-limits_{context}"]


Please rename the module file itself to match the new title/ID (and of course, update it in the assembly when you do). Thanks!

ousleyp · 2022-09-22T16:58:16Z

modules/virt-about-resource-quota-limits.adoc

@@ -0,0 +1,35 @@
+// Module included in the following assemblies:
+//
+// * virt/virtual_machines/advanced_vm_management/virt-specifying-nodes-for-vms.adoc


Please update this to the new assembly path that you created

Doh! Thanks.

ousleyp · 2022-09-22T16:59:20Z

virt/virtual_machines/advanced_vm_management/virt-working-with-resource-quotas.adoc

+
+toc::[]
+
+Resource quotas are implemented in {VirtProductName} similarly to how they are implemented in {product-title}. However, in (VirtProductName}, you are creating and managing resource quotas only for virtual machines.


I might reword this intro to something simpler, since the 'meat' of this doc change is in the procedure module. You can have an assembly intro that is as simple as "Create and manage resource quotas for virtual machines."

Suggestion: we could also include an xref to the OCP resource quota docs somewhere, if that would be useful.

Went with the simple recommendation. I thought about mentioning limits for a more comprehensive abstract sentence but I also don't want to limit the scope of this assembly.

ousleyp

Just one nit re: additional resources formatting; otherwise this LGTM

ousleyp · 2022-09-26T16:31:02Z

virt/virtual_machines/advanced_vm_management/virt-working-with-resource-quotas-for-vms.adoc

+
+include::modules/virt-setting-resource-quota-limits-for-vms.adoc[leveloffset=+1]
+
+.Additional resources


Please use the add'l resources formatting for assemblies:

[role="_additional-resources"] [id="additional-resources_virt-working-with-resource-quotas-for-vms"] == Additional resources

Done. Thanks.

Thanks for adding the role and id lines! One last thing, though: you still have .Additional resources in line 15. That should be changed to == Additional resources.

Sorry about that. Should be good now :-)

ousleyp

I left one comment regarding the additional resources section. Please let me know when you've fixed that last thing and I'll go ahead with merging.

ousleyp · 2022-09-29T21:24:52Z

virt/virtual_machines/advanced_vm_management/virt-working-with-resource-quotas-for-vms.adoc

+
+include::modules/virt-setting-resource-quota-limits-for-vms.adoc[leveloffset=+1]
+
+.Additional resources


Thanks for adding the role and id lines! One last thing, though: you still have .Additional resources in line 15. That should be changed to == Additional resources.

ousleyp · 2022-10-05T17:29:52Z

/cherrypick enterprise-4.12

ousleyp · 2022-10-05T17:29:58Z

/cherrypick enterprise-4.11

ousleyp · 2022-10-05T17:30:04Z

/cherrypick enterprise-4.10

ousleyp · 2022-10-05T17:30:11Z

/cherrypick enterprise-4.9

ousleyp · 2022-10-05T17:30:16Z

/cherrypick enterprise-4.8

openshift-cherrypick-robot · 2022-10-05T17:30:36Z

@ousleyp: new pull request created: #51296

In response to this:

/cherrypick enterprise-4.12

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-cherrypick-robot · 2022-10-05T17:30:42Z

@ousleyp: new pull request created: #51297

In response to this:

/cherrypick enterprise-4.11

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-cherrypick-robot · 2022-10-05T17:30:48Z

@ousleyp: new pull request created: #51298

In response to this:

/cherrypick enterprise-4.10

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-cherrypick-robot · 2022-10-05T17:30:57Z

@ousleyp: new pull request created: #51299

In response to this:

/cherrypick enterprise-4.9

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-cherrypick-robot · 2022-10-05T17:31:02Z

@ousleyp: new pull request created: #51300

In response to this:

/cherrypick enterprise-4.8

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

openshift-ci bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Jul 25, 2022

bgaydosrh force-pushed the CNV-19689 branch from a3746c9 to 4d27262 Compare August 5, 2022 16:00

openshift-ci bot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 5, 2022

bgaydosrh force-pushed the CNV-19689 branch 3 times, most recently from c1209b4 to ed9b518 Compare August 5, 2022 16:23

stu-gott reviewed Aug 5, 2022

View reviewed changes

modules/virt-about-resource-quota-limits.adoc Outdated Show resolved Hide resolved

bgaydosrh force-pushed the CNV-19689 branch from ac9a913 to 2199d25 Compare August 5, 2022 17:10

stu-gott reviewed Aug 5, 2022

View reviewed changes

bgaydosrh force-pushed the CNV-19689 branch from 311e745 to 3fcc410 Compare August 5, 2022 17:18

dshchedr reviewed Aug 5, 2022

View reviewed changes

modules/virt-about-resource-quota-limits.adoc Outdated Show resolved Hide resolved

bgaydosrh force-pushed the CNV-19689 branch 2 times, most recently from 31090e2 to c51a13f Compare August 5, 2022 18:13

ousleyp added branch/enterprise-4.8 branch/enterprise-4.9 branch/enterprise-4.10 branch/enterprise-4.11 CNV Label for all CNV PRs labels Aug 5, 2022

ousleyp added this to the Next Release milestone Aug 5, 2022

ousleyp reviewed Aug 5, 2022

View reviewed changes

ousleyp added the peer-review-done Signifies that the peer review team has reviewed this PR label Aug 5, 2022

bergerhoffer added the branch/enterprise-4.12 label Aug 10, 2022

kalexand-rh modified the milestones: Next Release, Continuous Release Aug 19, 2022

bgaydosrh force-pushed the CNV-19689 branch from d1f7161 to 4120dd1 Compare September 21, 2022 23:18

ousleyp reviewed Sep 22, 2022

View reviewed changes

bgaydosrh force-pushed the CNV-19689 branch 3 times, most recently from 9343f3c to d5ce393 Compare September 22, 2022 20:39

ousleyp reviewed Sep 26, 2022

View reviewed changes

bgaydosrh force-pushed the CNV-19689 branch 2 times, most recently from 30ffc5a to eaf99de Compare September 29, 2022 19:07

ousleyp reviewed Sep 29, 2022

View reviewed changes

bgaydosrh force-pushed the CNV-19689 branch from eaf99de to 38878de Compare October 5, 2022 03:38

Code Review

9595583

bgaydosrh force-pushed the CNV-19689 branch from 38878de to 9595583 Compare October 5, 2022 03:39

ousleyp merged commit 88203b0 into openshift:main Oct 5, 2022

openshift-cherrypick-robot mentioned this pull request Oct 5, 2022

[enterprise-4.12] CNV-19689: Add note about resource quotas #51296

Merged

openshift-cherrypick-robot mentioned this pull request Oct 5, 2022

[enterprise-4.11] CNV-19689: Add note about resource quotas #51297

Merged

openshift-cherrypick-robot mentioned this pull request Oct 5, 2022

[enterprise-4.10] CNV-19689: Add note about resource quotas #51298

Merged

openshift-cherrypick-robot mentioned this pull request Oct 5, 2022

[enterprise-4.9] CNV-19689: Add note about resource quotas #51299

Merged

openshift-cherrypick-robot mentioned this pull request Oct 5, 2022

[enterprise-4.8] CNV-19689: Add note about resource quotas #51300

Merged

		@@ -8,6 +8,8 @@ toc::[]

		You can place virtual machines (VMs) on specific nodes by using node placement rules.

		include::modules/virt-about-resource-quota-limits.adoc[leveloffset=+1]


		toc::[]

		Resource quotas are implemented in {VirtProductName} similarly to how they are implemented in {product-title}. However, in (VirtProductName}, you are creating and managing resource quotas only for virtual machines.


		include::modules/virt-setting-resource-quota-limits-for-vms.adoc[leveloffset=+1]

		.Additional resources

CNV-19689: Add note about resource quotas #48252

CNV-19689: Add note about resource quotas #48252

Conversation

bgaydosrh commented Jul 25, 2022 • edited

stu-gott left a comment

Choose a reason for hiding this comment

stu-gott commented Aug 5, 2022

stu-gott left a comment

Choose a reason for hiding this comment

stu-gott commented Aug 5, 2022

dshchedr commented Aug 5, 2022

openshift-ci bot commented Aug 5, 2022

ousleyp left a comment

Choose a reason for hiding this comment

ousleyp Aug 5, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bgaydosrh Sep 15, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bergerhoffer commented Aug 10, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ousleyp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ousleyp left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ousleyp commented Oct 5, 2022

ousleyp commented Oct 5, 2022

ousleyp commented Oct 5, 2022

ousleyp commented Oct 5, 2022

ousleyp commented Oct 5, 2022

openshift-cherrypick-robot commented Oct 5, 2022

openshift-cherrypick-robot commented Oct 5, 2022

openshift-cherrypick-robot commented Oct 5, 2022

openshift-cherrypick-robot commented Oct 5, 2022

openshift-cherrypick-robot commented Oct 5, 2022

bgaydosrh commented Jul 25, 2022 •

edited

ousleyp Aug 5, 2022 •

edited

bgaydosrh Sep 15, 2022 •

edited