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

OADP-535 allow for nullable resource allocations #711

Merged
merged 3 commits into from
May 27, 2022
Merged

OADP-535 allow for nullable resource allocations #711

merged 3 commits into from
May 27, 2022

Conversation

dymurray
Copy link
Member

@dymurray dymurray commented May 25, 2022 •
edited by openshift-ci bot

This change allows for null resource allocations fields.

NOTE this includes a manual update to the DPA CRD to allow for null requests and allocations fields. This means that make manifests will no longer give us exactly what we need and will require folks to constantly add it back in. We need to figure out a better path forward for this.
OADP-535

@kaovilai
Copy link
Member

/cherrypick oadp-1.0

@openshift-cherrypick-robot
Copy link
Contributor

@kaovilai: once the present PR merges, I will cherry-pick it on top of oadp-1.0 in a new PR and assign it to you.

In response to this:

/cherrypick oadp-1.0

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.

shubham-pampattiwar
shubham-pampattiwar reviewed May 26, 2022
View reviewed changes
bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml
@@ -216,6 +216,7 @@ spec:
x-kubernetes-int-or-string: true
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need nullable: true on line number 209 as well as on line number 353.

shubham-pampattiwar
shubham-pampattiwar reviewed May 26, 2022
View reviewed changes
config/crd/bases/oadp.openshift.io_dataprotectionapplications.yaml
@@ -208,6 +208,7 @@ spec:
resourceAllocations:
description: ResourceAllocations defines the CPU and Memory
resource allocations for the restic Pod
nullable: true
properties:
limits:
additionalProperties:
Copy link
Member

Choose a reason for hiding this comment

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

Don't we need nullable: true specifically for limits as well as requests ?

@dymurray
Copy link
Member Author

/cherrypick oadp-1.0

@openshift-cherrypick-robot
Copy link
Contributor

@dymurray: once the present PR merges, I will cherry-pick it on top of oadp-1.0 in a new PR and assign it to you.

In response to this:

/cherrypick oadp-1.0

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.

pranavgaikwad
pranavgaikwad requested changes May 27, 2022
View reviewed changes
bundle/manifests/oadp.openshift.io_dataprotectionapplications.yaml
@@ -216,6 +217,7 @@ spec:
x-kubernetes-int-or-string: true
description: 'Limits describes the maximum amount
of compute resources allowed. More info: https://kubernetes.io/docs/concepts/configuration/manage-resources-containers/'
nullable: true
type: object
requests:
additionalProperties:
Copy link

Choose a reason for hiding this comment

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

needs nullable for the additionalProperties.

pranavgaikwad
pranavgaikwad approved these changes May 27, 2022
View reviewed changes
Copy link

@pranavgaikwad pranavgaikwad left a comment

Choose a reason for hiding this comment

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

ACK, thank you for the fix!

@jmontleon
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label May 27, 2022
@dymurray dymurray merged commit ab393ae into openshift:master May 27, 2022
@openshift-ci
Copy link

openshift-ci bot commented May 27, 2022

@dymurray: all tests passed!

Full PR test history. Your PR dashboard.

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. I understand the commands that are listed here.

@dymurray
Copy link
Member Author

/cherrypick oadp-1.0

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request May 27, 2022
Merged
@openshift-cherrypick-robot
Copy link
Contributor

@kaovilai: new pull request created: #715

In response to this:

/cherrypick oadp-1.0

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
Copy link
Contributor

@dymurray: new pull request could not be created: failed to create pull request against openshift/oadp-operator#oadp-1.0 from head openshift-cherrypick-robot:cherry-pick-711-to-oadp-1.0: status code 422 not one of [201], body: {"message":"Validation Failed","errors":[{"resource":"PullRequest","code":"custom","message":"A pull request already exists for openshift-cherrypick-robot:cherry-pick-711-to-oadp-1.0."}],"documentation_url":"https://docs.github.com/rest/reference/pulls#create-a-pull-request"}

In response to this:

/cherrypick oadp-1.0

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.

kaovilai pushed a commit to kaovilai/oadp-operator that referenced this pull request Jun 1, 2022
@dymurray @kaovilai
OADP-535 allow for nullable resource allocations (openshift#711)
771df34 
* OADP-535 allow for nullable resource allocations

* Add missing nullables

* Include nullable on additionalProperties
kaovilai added a commit that referenced this pull request Jun 3, 2022
@kaovilai @shubham-pampattiwar
@savitharaghunathan @jmontleon @dymurray
Remove NoDefaultBackupLocation e2e entry + registry probe timeout bum…
09024d8 
…p (fix azure flakes) (#717)

* revert to b64e96e

* bump probe to 10s

* mimic velero cli loglevel options (#692)

use logrus ParseLevel directly

* Fix OADP 526 (#704)

* fix OADP 526

* add ignore err comments

* fix docs link for build from src (#706)

* Datamover CRD design (#597)

* Adding initial datamover design

* Typo in reviewers

* Add summary & user stories

* Fixing nits

* Adding intial feedback

* Adding feedback

* nit: changing the selector field name

* Adding review comments

* Adding plugin clarification

* updating image to reflect the latest design

* Reflect recent velero csi plugin changes

* use a full vm for arm builds in travis (#713)

* OADP-535 allow for nullable resource allocations (#711)

* OADP-535 allow for nullable resource allocations

* Add missing nullables

* Include nullable on additionalProperties

Co-authored-by: Shubham Pampattiwar <shubhampampattiwar7@gmail.com>
Co-authored-by: Savitha Raghunathan <sraghuna@redhat.com>
Co-authored-by: Jason Montleon <jmontleo@redhat.com>
Co-authored-by: Dylan Murray <dymurray@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@shubham-pampattiwar shubham-pampattiwar shubham-pampattiwar left review comments

@pranavgaikwad pranavgaikwad pranavgaikwad approved these changes

@jwmatthews jwmatthews Awaiting requested review from jwmatthews

@rayfordj rayfordj Awaiting requested review from rayfordj

Assignees

@jmontleon jmontleon

Labels
lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@dymurray @kaovilai @openshift-cherrypick-robot @jmontleon @pranavgaikwad @shubham-pampattiwar