Improve LimitRange documentation #46950

subhals · 2024-06-24T12:22:21Z

Improve limitRange documentation by creating example resources in specific namespaces.
fixes #46899

netlify · 2024-06-24T12:25:37Z

✅ Pull request preview available for checking

Built without sensitive environment variables

Name	Link
🔨 Latest commit	`70011c8`
🔍 Latest deploy log	https://app.netlify.com/sites/kubernetes-io-main-staging/deploys/669478ef52d95c0009d416e2
😎 Deploy Preview	https://deploy-preview-46950--kubernetes-io-main-staging.netlify.app
📱 Preview on mobile	Toggle QR Code... Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

content/en/docs/concepts/policy/limit-range.md

utkarsh-singh1 · 2024-06-26T11:22:21Z

content/en/examples/concepts/policy/limit-range/problematic-limit-range.yaml

@@ -2,6 +2,7 @@ apiVersion: v1
 kind: LimitRange
 metadata:
  name: cpu-resource-constraint
+  namespace: default


Can use other than default, like dev / prod

utkarsh-singh1 · 2024-06-26T11:22:53Z

content/en/examples/concepts/policy/limit-range/example-no-conflict-with-limitrange-cpu.yaml

@@ -2,6 +2,7 @@ apiVersion: v1
 kind: Pod
 metadata:
  name: example-no-conflict-with-limitrange-cpu
+  namespace: default


Can use other than default, like dev / prod

utkarsh-singh1 · 2024-06-26T11:23:08Z

content/en/examples/concepts/policy/limit-range/example-conflict-with-limitrange-cpu.yaml

@@ -2,6 +2,7 @@ apiVersion: v1
 kind: Pod
 metadata:
  name: example-conflict-with-limitrange-cpu
+  namespace: default


Can use other than default, like dev / prod

The problem with using other namespace than default is when user copies the manifest and executes it, they will potentially run into namespace not found error.
Although it might be a simple error which users can fix themselves, this will disrupt the user's workflow as the namespace creation is not mentioned anywhere before.

@utkarsh-singh1 wdyt? Do you still recommend specifying dev/prod namespace?
May be we can add a note to create the namespace before applying the yaml?

IMO

To keep the concept simple, I think it's okay to keep namespace as the default namespace and include a comment in the YAML manifest indicating that the namespace is implied.

@subhals, @utkarsh-singh1

tengqm · 2024-07-11T10:45:02Z

Explicitly specifying "namespace" property is not a good practice for manifest authors. It makes the manifest non-portable. Hardcoding the "namespace" to a fixed value, even if for demonstration's purpose, could be confusing.

I'd suggest we add a warning to that concept page rather than change the manifest.

T-Lakshmi · 2024-07-11T11:46:27Z

I'd suggest we add a warning to that concept page rather than change the manifest.

@tengqm,
I agree with your suggestion, that is also good idea!

k8s-ci-robot · 2024-07-15T01:18:52Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
Once this PR has been reviewed and has the lgtm label, please assign natalisucks for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these files:

content/en/docs/OWNERS

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

subhals · 2024-07-15T01:20:18Z

Explicitly specifying "namespace" property is not a good practice for manifest authors. It makes the manifest non-portable. Hardcoding the "namespace" to a fixed value, even if for demonstration's purpose, could be confusing.

I'd suggest we add a warning to that concept page rather than change the manifest.

Thank you @tengqm @T-Lakshmi for the suggestions. Updated the changes. PTAL!

T-Lakshmi · 2024-07-30T06:28:27Z

content/en/docs/concepts/policy/limit-range.md

@@ -55,16 +55,18 @@ The name of a LimitRange object must be a valid

 A `LimitRange` does **not** check the consistency of the default values it applies. This means that a default value for the _limit_ that is set by `LimitRange` may be less than the _request_ value specified for the container in the spec that a client submits to the API server. If that happens, the final Pod will not be schedulable.

+


Suggested change

I don't think this extra line required.

T-Lakshmi · 2024-07-30T06:31:52Z

content/en/docs/concepts/policy/limit-range.md

@@ -55,16 +55,18 @@ The name of a LimitRange object must be a valid

 A `LimitRange` does **not** check the consistency of the default values it applies. This means that a default value for the _limit_ that is set by `LimitRange` may be less than the _request_ value specified for the container in the spec that a client submits to the API server. If that happens, the final Pod will not be schedulable.


Suggested change

A `LimitRange` does **not** check the consistency of the default values it applies. This means that a default value for the _limit_ that is set by `LimitRange` may be less than the _request_ value specified for the container in the spec that a client submits to the API server. If that happens, the final Pod will not be schedulable.

A `LimitRange` does **not** check the consistency of the default values it applies. This means that

a default value for the _limit_ that is set by `LimitRange` may be less than the _request_ value

specified for the container in the spec that a client submits to the API server. If that happens,

the final Pod will not be schedulable.

This change doesn't align with the PR description, @T-Lakshmi.

It's OK to not make suggestions where those suggestions would leave the code changes out of step with the PR description.

Noted @sftim.

@subhals,
This suggestion is entirely optional as it is out of scope as per the PR description.
However, you are free to make changes so that it more closely matches with the PR recent title update.

T-Lakshmi · 2024-07-30T08:37:20Z

content/en/docs/concepts/policy/limit-range.md

 For example, you define a `LimitRange` with this manifest:

-{{% code_sample file="concepts/policy/limit-range/problematic-limit-range.yaml" %}}
+{{< warning >}}
+The following examples operate within the default namespace, as the namespace parameter is undefined. This implies that any references or operations within these examples will interact with elements within the default namespace unless otherwise specified.{{< /warning >}}


Suggested change

The following examples operate within the default namespace, as the namespace parameter is undefined. This implies that any references or operations within these examples will interact with elements within the default namespace unless otherwise specified.{{< /warning >}}

The following examples operate within the default namespace of your cluster, as the namespace

parameter is undefined and the `LimitRange` scope is limited to the namespace level.

This implies that any references or operations within these examples will interact with elements

within the default namespace of your cluster. You can override the operating namespace

by configuring namespace in the `metadata.namespace` field.

{{< /warning >}}

Do you agree with these?

OK, but we'd ideally we'd write LimitRange without backticks.

If you'd like to, @subhals, you can edit this file so that all the API kinds (eg LimitRange) are written without backticks. It's not mandatory!

sftim · 2024-07-30T12:41:53Z

/retitle Improve LimitRange documentation

sftim · 2024-07-30T12:44:14Z

content/en/docs/concepts/policy/limit-range.md

 For example, you define a `LimitRange` with this manifest:

-{{% code_sample file="concepts/policy/limit-range/problematic-limit-range.yaml" %}}
+{{< warning >}}


warning is much too strong. We use warning for: you must follow this advice; if you didn't, your cluster might be at risk.

OK, Agree with you @sftim.

In that case, replacing warning with a Note instruction would be more appropriate.
WDYT? @sftim @subhals @tengqm.

I'd use note

T-Lakshmi · 2024-08-06T09:22:57Z

@subhals,
Can you please respond to the reviewers feedback here when you get time.

sftim · 2024-09-30T14:40:12Z

@subhals, would you be willing to revise this PR further? With a few adjustments it would be good to merge.

Update limit-range.md

494ef6a

k8s-ci-robot added the language/en Issues or PRs related to English language label Jun 24, 2024

k8s-ci-robot requested review from divya-mohan0209 and reylejano June 24, 2024 12:22

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Jun 24, 2024