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 suppport for cpuLimit, cpuRequest and memoryRequest #5252

Merged
merged 13 commits into from
Dec 8, 2021
Merged

Add suppport for cpuLimit, cpuRequest and memoryRequest #5252

merged 13 commits into from
Dec 8, 2021

Conversation

anandrkskd
Copy link
Contributor

@anandrkskd anandrkskd commented Nov 25, 2021 •
edited
Loading

What type of PR is this?
/kind feature

What does this PR do / why we need it:
This PR adds feature to add cpuLimit, cpuRequest and memoryRequest values using devfile for a container
Which issue(s) this PR fixes:

Fixes #5132

PR acceptance criteria:

How to test changes / Special notes to the reviewer:

 git clone https://github.com/openshift/nodejs-ex ; cd nodejs-ex
odo create nodejs --devfile ~/go/src/github.com/openshift/odo/tests/examples/source/devfiles/nodejs/devfile-with-MR-CL-CR.yaml
odo push
oc get pods podname -o jsonpath='{.spec.containers[0].resources}{"\n"}'

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Nov 25, 2021
@openshift-ci openshift-ci bot requested review from feloy and valaparthvi November 25, 2021 20:18
@netlify
Copy link

netlify bot commented Nov 25, 2021 •
edited
Loading

✔️ Deploy Preview for odo-docusaurus-preview canceled.

🔨 Explore the source changes: aa90334

🔍 Inspect the deploy log: https://app.netlify.com/sites/odo-docusaurus-preview/deploys/61b05f24124afd00077a6329

@anandrkskd anandrkskd force-pushed the dev/update-devfile-api-2.1.0 branch from 5411f00 to 15c8e2e Compare November 25, 2021 20:26
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Nov 25, 2021
@anandrkskd anandrkskd changed the title Dev/update devfile api 2.1.0 Add suppport for cpuLimit, cpuRequest and memoryRequest Nov 26, 2021
@feloy
Copy link
Contributor

feloy commented Nov 26, 2021

@anandrkskd during the integration tests, we get the following errors:

[odo]  ✗  failed to parse the devfile /tmp/133431546/devfile.yaml, with error: invalid devfile schema. errors :
[odo] - components.2.volume: Additional property ephemeral is not allowed

It seems a problem similar to devfile/api#675

@feloy feloy mentioned this pull request Nov 26, 2021
Closed
@feloy
Copy link
Contributor

feloy commented Nov 30, 2021

@anandrkskd , by downgrading to a previous commit of devfile/library, it seems that your PR will work.
You can try:

go get github.com/devfile/library@ab7bd9b8b615d75dbe99c414bd8d80f51894699d
go mod vendor

This is a commit just after the introduction of the new limits/requests, and before the change about the problematic ephemeral field. It is also before the changes of the GetRoute and GetIngress functions signatures, you will have to revert these changes.

@anandrkskd
Copy link
Contributor Author

Understood, will give it try ASAP.
Thanks, @feloy

@sonarqubecloud
Copy link

SonarCloud Quality Gate failed.    Quality Gate failed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
38.8% 38.8% Duplication

@anandrkskd
Copy link
Contributor Author

/test psi-kubernetes-integration-e2e

@valaparthvi
Copy link
Contributor

Can you also take a look at the Sonar Cloud duplication report?

@feloy
Copy link
Contributor

feloy commented Dec 1, 2021

Kubernetes Tests finished.
View logs: TXT HTML

@feloy
Copy link
Contributor

feloy commented Dec 1, 2021

Unit Tests finished.
View logs: TXT HTML

@feloy
Copy link
Contributor

feloy commented Dec 1, 2021

Kubernetes Tests finished.
View logs: TXT HTML

@feloy
Copy link
Contributor

feloy commented Dec 1, 2021

OpenShift Tests finished.
View logs: TXT HTML

@feloy
Copy link
Contributor

feloy commented Dec 1, 2021

Unit Tests finished.
View logs: TXT HTML

@anandrkskd anandrkskd requested a review from feloy December 1, 2021 18:28
@anandrkskd
Copy link
Contributor Author

@feloy @valaparthvi can we move forward with lgtm and approve?

valaparthvi
valaparthvi suggested changes Dec 2, 2021
View reviewed changes
Copy link
Contributor

@valaparthvi valaparthvi left a comment

Choose a reason for hiding this comment

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

Sorry about the nitty-gritty changes, but these are last few 🤞

tests/integration/devfile/cmd_devfile_push_test.go Outdated Show resolved Hide resolved
tests/integration/devfile/cmd_devfile_push_test.go Show resolved Hide resolved
tests/integration/devfile/cmd_devfile_push_test.go Show resolved Hide resolved
@valaparthvi
Copy link
Contributor

Thank you for the changes!
/approve

@feloy
Copy link
Contributor

feloy commented Dec 2, 2021

Kubernetes Tests finished.
View logs: TXT HTML

@feloy
Copy link
Contributor

feloy commented Dec 2, 2021

OpenShift Tests finished.
View logs: TXT HTML

@openshift-ci openshift-ci bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Dec 6, 2021
@anandrkskd
update Devfile library and api
8f2b04d 
Signed-off-by: anandrkskd <[email protected]>
@anandrkskd
resolve problem occured by update
5706524 
Signed-off-by: anandrkskd <[email protected]>
@anandrkskd
add test for cpuLimit,cpuRequest,memoryRequest
6619e0e 
Signed-off-by: anandrkskd <[email protected]>
@anandrkskd
resolve branch conflicts
f52671e 
Signed-off-by: anandrkskd <[email protected]>
@anandrkskd
update schema version to 2.2.0 for test devfiles
11fe55b 
Signed-off-by: anandrkskd <[email protected]>
@anandrkskd
revert - update schema version to 2.2.0 for test devfiles
3b3d784 
Signed-off-by: anandrkskd <[email protected]>
@anandrkskd
downgrade devfile library
8c170a8 
Signed-off-by: anandrkskd <[email protected]>
@anandrkskd
update test, remove duplication
eb6c29a 
Signed-off-by: anandrkskd <[email protected]>
@anandrkskd
replace oc with kubectl
e524467 
Signed-off-by: anandrkskd <[email protected]>
@anandrkskd
restructure when
6742204 
Signed-off-by: anandrkskd <[email protected]>
@anandrkskd
force push
e403f8c 
Signed-off-by: anandrkskd <[email protected]>
@anandrkskd
rebase and update the test to use cliRunner
e333156 
Signed-off-by: anandrkskd <[email protected]>
@anandrkskd anandrkskd force-pushed the dev/update-devfile-api-2.1.0 branch from 0dce90c to e333156 Compare December 8, 2021 06:53
@openshift-ci openshift-ci bot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. Required by Prow. label Dec 8, 2021
@feloy
Copy link
Contributor

feloy commented Dec 8, 2021

Kubernetes Tests finished.
View logs: TXT HTML

@feloy
Copy link
Contributor

feloy commented Dec 8, 2021

OpenShift Tests finished.
View logs: TXT HTML

@feloy
Copy link
Contributor

feloy commented Dec 8, 2021

Unit Tests finished.
View logs: TXT HTML

@anandrkskd
remove golint issues
aa90334 
Signed-off-by: anandrkskd <[email protected]>
@feloy
Copy link
Contributor

feloy commented Dec 8, 2021

Kubernetes Tests finished.
View logs: TXT HTML

@feloy
Copy link
Contributor

feloy commented Dec 8, 2021

Unit Tests finished.
View logs: TXT HTML

@feloy
Copy link
Contributor

feloy commented Dec 8, 2021

OpenShift Tests finished.
View logs: TXT HTML

@feloy
Copy link
Contributor

feloy commented Dec 8, 2021

/lgtm

Thanks for this work @anandrkskd

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. Required by Prow. label Dec 8, 2021
@openshift-merge-robot openshift-merge-robot merged commit 0e42e99 into redhat-developer:main Dec 8, 2021
@dharmit dharmit added the kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation label Dec 21, 2021
@anandrkskd anandrkskd deleted the dev/update-devfile-api-2.1.0 branch October 13, 2022 11:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@feloy feloy Awaiting requested review from feloy

@valaparthvi valaparthvi Awaiting requested review from valaparthvi

Assignees

@feloy feloy

@valaparthvi valaparthvi

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. Required by Prow. kind/feature Categorizes issue as a feature request. For PRs, that means that the PR is the implementation lgtm Indicates that a PR is ready to be merged. Required by Prow.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

support for cpuLimit, cpuRequest, memoryRequest
6 participants
@anandrkskd @feloy @valaparthvi @kadel @dharmit @openshift-merge-robot