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.

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

Add Hardware resources and BMCRef #614

Merged
merged 1 commit into from
May 9, 2022

Conversation

micahhausler
Copy link
Contributor

Signed-off-by: Micah Hausler mhausler@amazon.com

Description

This PR adds two fields for external orchestrators (such as CAPT) to

  • Make scheduling (machine assignment) based on resources declared on a particular hardware
  • Manage a machine's lifecycle and maintain a relation to the hardware (such as Rufio)

How Has This Been Tested?

N/A. No

How are existing users impacted? What migration steps/scripts do we need?

No change to existing users.

Checklist:

I have:

  • updated the documentation and/or roadmap (if required)
  • added unit or e2e tests
  • provided instructions on how to upgrade

@codecov
Copy link

codecov bot commented May 3, 2022

Codecov Report

Merging #614 (5c8e524) into main (97aa134) will not change coverage.
The diff coverage is n/a.

❗ Current head 5c8e524 differs from pull request most recent head 30fe9e0. Consider uploading reports for the commit 30fe9e0 to get more accurate results

@@           Coverage Diff           @@
##             main     #614   +/-   ##
=======================================
  Coverage   44.65%   44.65%           
=======================================
  Files          61       61           
  Lines        3509     3509           
=======================================
  Hits         1567     1567           
  Misses       1860     1860           
  Partials       82       82           
Impacted Files Coverage Δ
pkg/apis/core/v1alpha1/hardware_types.go 100.00% <ø> (ø)
pkg/controllers/manager.go 40.00% <0.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 97aa134...30fe9e0. Read the comment docs.

@micahhausler micahhausler force-pushed the hardware/resources branch 2 times, most recently from 41c6afe to d2365b8 Compare May 6, 2022 13:27
chrisdoherty4
chrisdoherty4 previously approved these changes May 6, 2022
Copy link
Member

@chrisdoherty4 chrisdoherty4 left a comment

Choose a reason for hiding this comment

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

lgtm

resources:
additionalProperties:
anyOf:
- type: integer
Copy link
Member

Choose a reason for hiding this comment

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

Do we need clarification in the description on the assumed unit when specifying as an integer?

Copy link
Member

Choose a reason for hiding this comment

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

In-fact I don't think that makes sense because describing a CPUs is a single number. Something still seems odd though, how are users meant to know what value to use?

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 don't think so, its a Kubernetes type which has its own documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

CPUs can be fractional for K8s pods, or whole numbers. See the Format godoc example

Copy link
Member

@chrisdoherty4 chrisdoherty4 May 6, 2022

Choose a reason for hiding this comment

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

Right, but fractionality doesn't make sense in Tinkerbell. We only talk in terms of whole CPUs hence I'm wondering if we need to better define boundaries seeming as we're leveraging an existing type with behavior beyond our need.

I'd hate to see someone define a Hardware with .5m CPU for example and it seems like this is open to that sort of accidental usage.

Copy link
Member

@chrisdoherty4 chrisdoherty4 May 6, 2022

Choose a reason for hiding this comment

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

To clarify, I agree this doesn't need additional documentation from my OP about units assumed with integers. I've shifted to "are our bounds adequately described" because the resource.Quantity supports stuff beyond what we want.

chrisdoherty4
chrisdoherty4 previously approved these changes May 6, 2022
Signed-off-by: Micah Hausler <mhausler@amazon.com>
@jacobweinstock jacobweinstock added the ready-to-merge Signal to Mergify to merge the PR. label May 9, 2022
@jacobweinstock jacobweinstock removed the request for review from mmlb May 9, 2022 15:53
@mergify mergify bot merged commit 00d3337 into tinkerbell:main May 9, 2022
@displague displague added this to the 0.7.0 milestone Aug 15, 2022
mergify bot added a commit that referenced this pull request Aug 29, 2022
I'm a maintainer of several other services often related to the Kuberenetes back-end/Kubernetes controllers and I'm taking ownership for a lot of release synchronization making it both appropriate and necessary for me to maintain aspects of the Tink repository.

Requirements:

- I have reviewed the [community membership guidelines](https://github.com/tinkerbell/proposals/blob/main/proposals/0024/GOVERNANCE.md)
- I have [enabled 2FA on my GitHub account](https://github.com/settings/security)
- I have subscribed to the [tinkerbell-contributors e-mail list](https://groups.google.com/g/tinkerbell-contributors)
- I am actively contributing to 1 or more Tinkerbell subprojects

Here is a list of non-trival PRs I have been the primary reviewer on:

#596
#628
#614

I have also made a number of code contributions to this repository, here are a few of them:

#638
#631
#626
#622
#612

I have also raised various issues and am driving the releasing across Tinkerbell including in this repository.

Requesting consideration of expedited responsibilities: yes

Sponsors:

- @mmlb (Maintainer)
- @micahhausler (Maintainer)
- @jacobweinstock (Core contributor in other Tinkerbell repositories)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-to-merge Signal to Mergify to merge the PR.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants