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.

Sign up for GitHub

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

Jump to bottom

📖 Add Make install workaround #3543

Merged
merged 1 commit into from
Sep 15, 2023
Merged

📖 Add Make install workaround #3543

merged 1 commit into from
Sep 15, 2023

Conversation

Sajiyah-Salat
Copy link
Contributor

Description:

Fixes: #3460

Make install fails when crd description exceeds limit. Workaroud and why the error occur is added as suggested in this comment

@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 19, 2023
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Aug 21, 2023
camilamacedo86
camilamacedo86 reviewed Aug 21, 2023
View reviewed changes
docs/book/src/faq.md Outdated
# However, it has a size limit and if the CRD is too big with so many long descriptions as this one it will cause the failure.
$(CONTROLLER_GEN) rbac:roleName=manager-role crd:maxDescLen=0 webhook paths="./..." output:crd:artifacts:config=config/crd/bases

Instead of Kubectl create command use Kubectl apply and it will work because kubectl create will send the request at once and it will fail while kubect apply send the request in small chunks so the desc len will be divided.
Copy link
Member

Choose a reason for hiding this comment

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

It is not kubectl that generate the manifests.
By using the maxDescLen=0 controller gen (see: https://book.kubebuilder.io/reference/controller-gen.html) will generate the CRD without description which will consequently reduce its size which might sort out the issue

Could you please change this line and also ensure that we link the doc above?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I understood the maxdesclen=0 in wrong way. Now I got it. So I changed what maxdesclen do. Also It is not kubectl that generate the manifests. I am unable to understand the context of this line. Can you please explain

camilamacedo86
camilamacedo86 reviewed Aug 21, 2023
View reviewed changes
docs/book/src/faq.md
Instead of Kubectl create command use Kubectl apply and it will work because kubectl create will send the request at once and it will fail while kubect apply send the request in small chunks so the desc len will be divided.

Add maxDescLen=0 and it will ensure that the crd description size is utmost and hence it will go through it.

Copy link
Member

Choose a reason for hiding this comment

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

Note that in my comment, I added one more workaround option to ensure that the Single Responsibility principle is not violated. Can you please check that?

Also, we need to mention the ideal solution here, which would be to work with server-side, but it is not yet supported. In this case, would be either helpful ensure that we link the PR for controller-gen (please see my comment with the suggestions which has the PR as the issue)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added the link of controller-tools pr. But I am unsure about Single Responsibility principle In this comment You have talked about DDD principle. and in whole pr I didnt find anything about Single Responsibility principle. Can you please explain?

camilamacedo86
camilamacedo86 reviewed Aug 21, 2023
View reviewed changes
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

Great work 🥇
Thank you for your contribution

It seems close to get shape enough to get merged.
Please, see the comments and also ensure that we have the commits squashed so that we can move forward.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Aug 22, 2023
@k8s-ci-robot k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Aug 22, 2023
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 4, 2023
@Sajiyah-Salat
Copy link
Contributor Author

I have rebase the pr succesfully. Thank you @camilamacedo86 for helping me. It's great to learn from you. Sorry for the mistakes I have made. Also let me know if any further changes required.

@Sajiyah-Salat
Copy link
Contributor Author

I think the doc preview have some glitch. because it doesnt show the links in blue colour hyperlink. It shows it in markdown manner. Is there any issue or am I missing something?

@Sajiyah-Salat
Copy link
Contributor Author

/retest

camilamacedo86
camilamacedo86 reviewed Sep 4, 2023
View reviewed changes
docs/book/src/faq.md Outdated Show resolved Hide resolved
camilamacedo86
camilamacedo86 requested changes Sep 4, 2023
View reviewed changes
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hi @Sajiyah-Salat,

Thank you for your contribution. 🥇
I re-review this one:

varshaprasad96
varshaprasad96 previously requested changes Sep 5, 2023
View reviewed changes
docs/book/src/faq.md Show resolved Hide resolved
docs/book/src/faq.md Show resolved Hide resolved
docs/book/src/faq.md Show resolved Hide resolved
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Sep 11, 2023
Sajiyah-Salat
Sajiyah-Salat commented Sep 11, 2023
View reviewed changes
docs/book/src/faq.md Outdated Show resolved Hide resolved
Sajiyah-Salat
Sajiyah-Salat commented Sep 12, 2023
View reviewed changes
Copy link
Contributor Author

@Sajiyah-Salat Sajiyah-Salat left a comment

Choose a reason for hiding this comment

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

Dont we need to remove this @camilamacedo86

docs/book/src/faq.md Outdated
@@ -92,9 +92,39 @@ securityContext:
```
However, note that this problem is fixed and will not occur if you deploy the project in high versions (maybe >= 1.22).

## The error `Too long: must have at most 262144 bytes` is faced when I run `make install` to apply the CRD manifests. How to solve it? Why this error is faced?

When attempting to run `make install` to apply the CRD manifests, the error Too long: must have at most 262144 bytes may be encountered. This error arises due to a size limit enforced by the Kubernetes API. Note that the `make install` target will apply the CRD manifest under `config/crd` using `kubectl apply -f -`. Therefore, when the apply command is used, the API annotates the object with the `last-applied-configuration` which contains the entire previous configuration. If this configuration is too large, it will exceed the allowed byte size. ([More info][k8s-obj-creation]) and this error will be faced.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Suggested change
When attempting to run `make install` to apply the CRD manifests, the error Too long: must have at most 262144 bytes may be encountered. This error arises due to a size limit enforced by the Kubernetes API. Note that the `make install` target will apply the CRD manifest under `config/crd` using `kubectl apply -f -`. Therefore, when the apply command is used, the API annotates the object with the `last-applied-configuration` which contains the entire previous configuration. If this configuration is too large, it will exceed the allowed byte size. ([More info][k8s-obj-creation]) and this error will be faced.
When attempting to run `make install` to apply the CRD manifests, the error Too long: must have at most 262144 bytes may be encountered. This error arises due to a size limit enforced by the Kubernetes API. Note that the `make install` target will apply the CRD manifest under `config/crd` using `kubectl apply -f`. Therefore, when the apply command is used, the API annotates the object with the `last-applied-configuration` which contains the entire previous configuration. If this configuration is too large, it will exceed the allowed byte size. ([More info][k8s-obj-creation]) and this error will be faced.

camilamacedo86
camilamacedo86 approved these changes Sep 13, 2023
View reviewed changes
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added lgtm "Looks good to me", indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Sep 13, 2023
@camilamacedo86
Copy link
Member

/approved cancel
/lgtm cancel

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 13, 2023
@Sajiyah-Salat
Copy link
Contributor Author

Hello @camilamacedo86 is there anything else here remaining. if not can we merge this.

camilamacedo86
camilamacedo86 approved these changes Sep 15, 2023
View reviewed changes
Copy link
Member

@camilamacedo86 camilamacedo86 left a comment

Choose a reason for hiding this comment

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

/lgtm

@Sajiyah-Salat

It seems good to get merged.
All suggestions and recommendations made by @varshaprasad96 and I are addressed

So, let's merge this one.

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 15, 2023
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: camilamacedo86, Sajiyah-Salat

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:

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

@camilamacedo86 camilamacedo86 dismissed varshaprasad96’s stale review September 15, 2023 09:37

@varsha I am moving forward with this one but feel free to make comments if you see if we can always address in a follow up PR

@k8s-ci-robot k8s-ci-robot merged commit 6893605 into kubernetes-sigs:master Sep 15, 2023
9 checks passed
@Sajiyah-Salat
Copy link
Contributor Author

Thank you camilla for merging this one. Let me know if any changes required.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camilamacedo86 camilamacedo86 camilamacedo86 approved these changes

@varshaprasad96 varshaprasad96 varshaprasad96 left review comments

@Kavinjsir Kavinjsir Awaiting requested review from Kavinjsir

@rashmigottipati rashmigottipati Awaiting requested review from rashmigottipati

Assignees

@camilamacedo86 camilamacedo86

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

kubebuilder make install fails
4 participants
@Sajiyah-Salat @camilamacedo86 @k8s-ci-robot @varshaprasad96