Skip to content

Conversation

@eggfoobar
Copy link
Contributor

@eggfoobar eggfoobar commented Sep 7, 2022

This PR adds a new Infrastructure status for identifying when a cluster is desired to have workload partitioning turned on.

openshift/enhancements#1213

Signed-off-by: ehila ehila@redhat.com

@openshift-ci openshift-ci bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 7, 2022
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 7, 2022

Hello @eggfoobar! Some important instructions when contributing to openshift/api:
API design plays an important part in the user experience of OpenShift and as such API PRs are subject to a high level of scrutiny to ensure they follow our best practices. If you haven't already done so, please review the OpenShift API Conventions and ensure that your proposed changes are compliant. Following these conventions will help expedite the api review process for your PR.

For merging purposes, this repository follows the no-Feature-Freeze process which means that in addition to the standard lgtm and approved labels this repository requires either:

bugzilla/valid-bug - applied if your PR references a valid bugzilla bug

OR

qe-approved, docs-approved, and px-approved - these labels can be applied by anyone in the openshift org via the /label <labelname> command.

Who should apply these qe/docs/px labels?

  • For a no-Feature-Freeze team who is merging a feature before code freeze, they need to get those labels applied to their api repo PR by the appropriate teams (i.e. qe, docs, px)
  • For a Feature Freeze (traditional) team who is merging a feature before FF, they can self-apply the labels (via /label commands), they are basically irrelevant for those teams
  • For a Feature Freeze team who is merging a feature after FF, the PR should be rejected barring an exception

@eggfoobar eggfoobar changed the title WIP: feat: added infra status for cpu partitioning feature CNF-6165: feat: added infra status for cpu partitioning feature Sep 13, 2022
@openshift-ci openshift-ci bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 13, 2022
@eggfoobar eggfoobar force-pushed the cpu_partitioning_config branch from 4be26ae to dabd6ec Compare September 13, 2022 16:57
@jerpeter1
Copy link
Member

/assign @deads2k

@eggfoobar eggfoobar force-pushed the cpu_partitioning_config branch from a738c53 to 8796e00 Compare September 27, 2022 16:51
@deads2k
Copy link
Contributor

deads2k commented Sep 27, 2022

With the addition of an admission plugin that makes this field authoritative, I'm ok with this approach.

/approve

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 27, 2022
@JoelSpeed
Copy link
Contributor

/label tide/merge-method-squash

@openshift-ci openshift-ci bot added the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Sep 30, 2022
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2022
@eggfoobar eggfoobar force-pushed the cpu_partitioning_config branch from f56683a to a3cb2e5 Compare September 30, 2022 18:27
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 30, 2022
@JoelSpeed
Copy link
Contributor

/remove-label tide/merge-method-squash

@openshift-ci openshift-ci bot removed the tide/merge-method-squash Denotes a PR that should be squashed by tide when it merges. label Oct 3, 2022
@eggfoobar eggfoobar force-pushed the cpu_partitioning_config branch from a3cb2e5 to e4208f3 Compare October 3, 2022 12:21
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 8, 2022
@rphillips
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Dec 8, 2022
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

We have some integration testing to test YAML based interactions with our APIs, I think it would be good to get a test in for this new field to make sure the combination of defaulting and validation works as expected (docs)

Signed-off-by: ehila <ehila@redhat.com>

doc: updated comment docs

Signed-off-by: ehila <ehila@redhat.com>
@eggfoobar eggfoobar force-pushed the cpu_partitioning_config branch from 9445915 to eb16a13 Compare January 27, 2023 07:46
@openshift-ci openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Jan 27, 2023
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 27, 2023
@eggfoobar
Copy link
Contributor Author

Thanks @JoelSpeed! The integration test was a great addition, please give it another look when you have some time.

@eggfoobar eggfoobar force-pushed the cpu_partitioning_config branch from eb16a13 to 33b0032 Compare January 27, 2023 08:14
Copy link
Contributor

@JoelSpeed JoelSpeed left a comment

Choose a reason for hiding this comment

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

One nit, but otherwise API changes LGTM

@eggfoobar eggfoobar force-pushed the cpu_partitioning_config branch from 33b0032 to 5cfca35 Compare January 27, 2023 13:28
updated unit test to use techpreview file
updated integration tests to test default and validation for cpuPartitioning status

Signed-off-by: ehila <ehila@redhat.com>
@eggfoobar eggfoobar force-pushed the cpu_partitioning_config branch from 5cfca35 to 712a0ca Compare January 27, 2023 13:46
@JoelSpeed
Copy link
Contributor

With the addition of an admission plugin that makes this field authoritative, I'm ok with this approach.

Was this done? Is there a WIP PR for it at least?

@eggfoobar
Copy link
Contributor Author

With the addition of an admission plugin that makes this field authoritative, I'm ok with this approach.

Was this done? Is there a WIP PR for it at least?

That hasn't been added yet, there is a WIP PR openshift/kubernetes#1312 waiting for this to go in.

That effort is currently being tracked as part of CNF-5901 and API-1508

@JoelSpeed
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 30, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Jan 30, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: deads2k, eggfoobar, JoelSpeed, rphillips

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

The pull request process is described here

Details 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

@CFields651
Copy link

/label px-approved

@openshift-ci openshift-ci bot added the px-approved Signifies that Product Support has signed off on this PR label Feb 6, 2023
@sunilcio
Copy link

sunilcio commented Feb 7, 2023

/label qe-approved

@openshift-ci openshift-ci bot added the qe-approved Signifies that QE has signed off on this PR label Feb 7, 2023
@eggfoobar
Copy link
Contributor Author

/label docs-approved

@openshift-ci openshift-ci bot added the docs-approved Signifies that Docs has signed off on this PR label Feb 8, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented Feb 8, 2023

@eggfoobar: all tests passed!

Full PR test history. Your PR dashboard.

Details

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.

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

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. docs-approved Signifies that Docs has signed off on this PR lgtm Indicates that a PR is ready to be merged. px-approved Signifies that Product Support has signed off on this PR qe-approved Signifies that QE has signed off on this PR

Projects

None yet

Development

Successfully merging this pull request may close these issues.

8 participants