HierarchicalResourceQuota documentation

zfrhv commented 1 year ago

solves #264

i have a feeling my charges are not that great, @adrianludwin what to you think of it for now?

linux-foundation-easycla[bot] commented 1 year ago

The committers listed above are authorized under a signed CLA.

:white_check_mark: login: zfrhv / name: Zahar (3aef487a1aabe96195e25577985ad50e31ce4716)

k8s-ci-robot commented 1 year ago

Welcome @zfrhv!

It looks like this is your first PR to kubernetes-sigs/hierarchical-namespaces 🎉. Please refer to our pull request process documentation to help your PR have a smooth ride to approval.

You will be prompted by a bot to use commands during the review process. Do not be afraid to follow the prompts! It is okay to experiment. Here is the bot commands documentation.

You can also check if kubernetes-sigs/hierarchical-namespaces has its own contribution guidelines.

You may want to refer to our testing guide if you run into trouble with your tests not passing.

If you are having difficulty getting your pull request seen, please follow the recommended escalation practices. Also, for tips and tricks in the contribution process you may want to read the Kubernetes contributor cheat sheet. We want to make sure your contribution gets all the attention it needs!

Thank you, and welcome to Kubernetes. :smiley:

k8s-ci-robot commented 1 year ago

Hi @zfrhv. Thanks for your PR.

I'm waiting for a kubernetes-sigs member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available [here](https://git.k8s.io/community/contributors/guide/pull-requests.md). If you have questions or suggestions related to my behavior, please file an issue against the [kubernetes/test-infra](https://github.com/kubernetes/test-infra/issues/new?title=Prow%20issue:) repository.

zfrhv commented 1 year ago

i knew i would have so mistakes and typos but i didnt thought i would be so bad :I sorry and thank you

adrianludwin commented 1 year ago

i knew i would have so mistakes and typos but i didnt thought i would be so bad :I sorry and thank you

Not at all, I really appreciate all the work you put into this and all the work I didn't have to do as a result! My only other language is French and I guarantee you I can't write anything this good in French 😁

zfrhv commented 1 year ago

@adrianludwin done! but i didnt checked test\e2e\quickstart_test.go, i just added few tests from head is there a pipeline that runs it? or do i need to install WSL and check it myself?

zfrhv commented 1 year ago

welp, i installed go (for some reason i thought its available only on linux), and idk how to run the test files sorry i tried both go test quickstart_test.go and go run quickstart_test.go

adrianludwin commented 1 year ago

You need to have a cluster with HNC installed, and your kubectl configured to point to it, before you try running the tests. Then you can say (see here):

HNC_FOCUS=Quickstart make test-e2e # Runs all tests in the "Quickstart" Describe block

If that doesn't work, no worries, lmk and I'll try running it for you.

zfrhv commented 1 year ago

sorry my clusters are airgapped, it will take me few days to test it

adrianludwin commented 1 year ago

I just tried this out myself and found the following issues:

The yaml files cannot have any leading whitespace (see the other embedded yaml files)
- The yaml files must be indented with two spaces, not tabs (not sure if you had tabs, or if my editor did that when I was fixing the formatting)
While you're limiting CPU and RAM, you're actually creating Services, which don't use any CPU or RAM - services are just load balancers to deployments. So you should try creating a Deployment instead - this is where you'll actually specify how much RAM and CPU you need.

I'd suggest creating the first Deployment in team-a and the second in team-b to demonstrate that they are sharing the same quota.

zfrhv commented 1 year ago

The yaml files cannot have any leading whitespace (see the other embedded yaml files) The yaml files must be indented with two spaces, not tabs (not sure if you had tabs, or if my editor did that when I was fixing the formatting)

yes sorry about that, i did copy pase of the yaml from quickstart.md and vscode just replaces my spaces with tabs

While you're limiting CPU and RAM, you're actually creating Services, which don't use any CPU or RAM - services are just load balancers to deployments. So you should try creating a Deployment instead - this is where you'll actually specify how much RAM and CPU you need.

yes, but the cpu and memory were just examples, the limitation was with amount of services. i can create a deployment but i wanted to keep the example as simple as possible, and creating services is a lot easier than deployments.

i removed the cpu and memory from hrq now, you can check again but if you think that demostrating with actual cpu/memory then i can make a deployment instead of service

zfrhv commented 1 year ago

oh no :0 i accidently squashed too many commits

zfrhv commented 1 year ago

well that was scary, all good now anyways, if you want deployments instead of services just let me know

adrianludwin commented 1 year ago

It might be even easier to create Secrets instead of Services or Deployments? But I'll try your new changes as-is.

zfrhv commented 1 year ago

It might be even easier to create Secrets instead of Services or Deployments? But I'll try your new changes as-is.

yes creating secrets/configmaps was my first idea, but kubernetes has few default cm/secrets that are being automatically creates when the namespace is created, and the amount of configmaps by default in openshift is different than in kubernetes (same with secrets) so i cant relay on how many secrets or configmaps already exist, unless i start counting them

zfrhv commented 1 year ago

i simplified the quickstart a little, let me know what you think and i will squash the commits

adrianludwin commented 1 year ago

Ok, I tried it and it worked with the following changes. Nice work!

diff --git a/test/e2e/quickstart_test.go b/test/e2e/quickstart_test.go
index 72cf25d8..7f6f9565 100644
--- a/test/e2e/quickstart_test.go
+++ b/test/e2e/quickstart_test.go
@@ -217,13 +217,13 @@ spec:
                MustApplyYAML(hrq)

                // create service in team-a
-               MustRun("kubectl create service clusterip", nsTeamA + "-svc", `--clusterip="None"`, "-n", nsTeamA)
+               MustRun("kubectl create service clusterip", nsTeamA + "-svc", `--clusterip=None`, "-n", nsTeamA)

                // show that you can't use resources more than the hrq
-               MustNotRun("kubectl create service clusterip", nsTeamB + "-svc", `--clusterip="None"`, "-n", nsTeamB)
+               MustNotRun("kubectl create service clusterip", nsTeamB + "-svc", `--clusterip=None`, "-n", nsTeamB)

                // show hrq usage
-               RunShouldContain("services 1/1", defTimeout, "kubectl hns hrq", "-n", nsOrg)
+               RunShouldContain("services: 1/1", defTimeout, "kubectl hns hrq", "-n", nsOrg)

                MustRun("kubectl delete hrq", nsOrg + "-hrq", "-n", nsOrg)

Note the changes: no double-quotes around the "None" in --clusterip=None (the double-quotes are removed by bash when you type it by hand, but there's no shell in this test) and an added colon in services: 1/1.

Please go ahead and squash the commits. I'm going to have to enable HRQ by default before these tests will pass though so I'll work on that next.

zfrhv commented 1 year ago

done should i create a new PR for #295 ? (so this doc wouldnt have to wait until that feature would be merge)

adrianludwin commented 1 year ago

Actually - let's leave HRQ off in the default version of v1.1, I'll turn it on along with all the tests next.

Can you please disable this test by default (add a "P" before the It function so it says PIt - "P" for "Pending") and I'll enable it on the master branch next. Then I'll merge. Thanks!

adrianludwin commented 1 year ago

Oh, and please add a mention that HRQ is off by default, and you need to use the hrq.yaml manifest to enable it? Thanks!

zfrhv commented 1 year ago

sure thing! i added it after the beta warning on each page, hope you like it (and for some reason the new commit is not added to this PR, even tho i can clearly see it in my repo)

rjbez17 commented 1 year ago

/ok-to-test /lgtm

@adrianludwin can approve when ready

adrianludwin commented 1 year ago

Thank you for all these changes! Can you please cherry-pick this commit to v1.1 as well? Thanks! :)

/lgtm /approve

k8s-ci-robot commented 1 year ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: adrianludwin, zfrhv

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

The pull request process is described here

Needs approval from an approver in each of these files: - ~~[OWNERS](https://github.com/kubernetes-sigs/hierarchical-namespaces/blob/master/OWNERS)~~ [adrianludwin] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

kubernetes-sigs / hierarchical-namespaces

HierarchicalResourceQuota documentation #289