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 dedicated instances proposal #468

Merged

Conversation

alexander-demicev
Copy link
Contributor

No description provided.

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.

This is a good start! I think it would be good to flesh out with more code examples and would be good to understand what is required on the installer side to be able to create the resources required on GCP and Azure, do you think you could expand on this?

If we can, it would be good to include some User stories as well, and to expand on the motivation, goals, non-goals too

enhancements/machine-api/dedicated-instances.md Outdated Show resolved Hide resolved
enhancements/machine-api/dedicated-instances.md Outdated Show resolved Hide resolved
enhancements/machine-api/dedicated-instances.md Outdated Show resolved Hide resolved
}
```

That change will require adding `Tenancy` field to provider spec.

This comment was marked as resolved.

enhancements/machine-api/dedicated-instances.md Outdated Show resolved Hide resolved
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

most of this makes sense to me, i left some suggestions and a question

enhancements/machine-api/dedicated-instances.md Outdated Show resolved Hide resolved
enhancements/machine-api/dedicated-instances.md Outdated Show resolved Hide resolved
enhancements/machine-api/dedicated-instances.md Outdated Show resolved Hide resolved
enhancements/machine-api/dedicated-instances.md Outdated Show resolved Hide resolved
enhancements/machine-api/dedicated-instances.md Outdated Show resolved Hide resolved
@enxebre
Copy link
Member

enxebre commented Sep 7, 2020

Thanks for putting this together @alexander-demichev. I think this effort is pretty much about exposing a new field on the AWS providerSpec. Still I think is nice having this info into one single place so I'm good to merge the after addressing comments if you want. Regardless I'm tracking in Jira the work that we need to get done. I.e:

  • Expose tenancy dedicated field on aws providerSpec.
  • Include an e2e test which spin up a dedicated instance via machineSet.
  • Include webhook validation for the new field input.
  • Estimate and reasonable limit, cost and request to bump aws dedicated limits on our CI acccount.
  • Include the changes for the upstream aws cluster API actuator.

@itamarh
Copy link

itamarh commented Sep 10, 2020

Any thoughts how this could look like with KubeVirt (on bare metal instances) for creating virt nodes on demand?

enhancements/machine-api/dedicated-instances.md Outdated Show resolved Hide resolved
enhancements/machine-api/dedicated-instances.md Outdated Show resolved Hide resolved
enhancements/machine-api/dedicated-instances.md Outdated Show resolved Hide resolved
enhancements/machine-api/dedicated-instances.md Outdated Show resolved Hide resolved
enhancements/machine-api/dedicated-instances.md Outdated Show resolved Hide resolved
enhancements/machine-api/dedicated-instances.md Outdated Show resolved Hide resolved
@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci-robot openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Dec 22, 2020
@JoelSpeed
Copy link
Contributor

@alexander-demichev Given this feature was implemented and merged, we need to get this tidied up and merged. Can you take a look at the outstanding comments and clean this up please

/remove-lifecycle stale

@openshift-ci-robot openshift-ci-robot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Jan 4, 2021
@openshift-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by:
To complete the pull request process, please assign dustymabe after the PR has been reviewed.
You can assign the PR to them by writing /assign @dustymabe in a comment when ready.

The full list of commands accepted by this bot can be found 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

@alexander-demicev
Copy link
Contributor Author

@JoelSpeed All should be fixed now.

@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci-robot openshift-ci-robot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Apr 6, 2021
@JoelSpeed
Copy link
Contributor

@elmiko Could you give this another review?

Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

this is mostly making sense to me, and i think it reads pretty well. i have one small suggestion.


## Summary

Make it possible for users to create machines which run as dedicated instances.
Copy link
Contributor

Choose a reason for hiding this comment

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

i think it would be nice to define what a "dedicated instance" is here. maybe just add a second sentence that starts "A dedicated instance is a ...."

@openshift-bot
Copy link

Stale issues rot after 30d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle rotten.
Rotten issues close after an additional 30d of inactivity.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle rotten
/remove-lifecycle stale

@openshift-ci openshift-ci bot added lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. and removed lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. labels May 6, 2021
@JoelSpeed
Copy link
Contributor

/remove-lifecycle rotten

@openshift-ci openshift-ci bot removed the lifecycle/rotten Denotes an issue or PR that has aged beyond stale and will be auto-closed. label May 7, 2021
@openshift-bot
Copy link

Issues go stale after 90d of inactivity.

Mark the issue as fresh by commenting /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.
Exclude this issue from closing by commenting /lifecycle frozen.

If this issue is safe to close now please do so with /close.

/lifecycle stale

@openshift-ci openshift-ci bot added the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 5, 2021
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

looks mostly good to me Alex, just a couple questions


#### Autoscaling

Autoscaling dedicated instances can be a problem because dedicated hosts have quotas and limits on provider side. We should provide good documentation here.
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any more detail we could add here, what does the user need to be aware of?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

nothing more that I know about

Comment on lines 165 to 196
## Design Details

### Open Questions

### Test Plan

### Graduation Criteria

#### Examples

##### Dev Preview -> Tech Preview

##### Tech Preview -> GA

##### Removing a deprecated feature

### Upgrade / Downgrade Strategy

### Version Skew Strategy

## Implementation History

## Drawbacks

## Alternatives
Copy link
Contributor

Choose a reason for hiding this comment

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

is there any information to be added for these sections?

@elmiko
Copy link
Contributor

elmiko commented Aug 18, 2021

/remove-lifecycle stale

@openshift-ci openshift-ci bot removed the lifecycle/stale Denotes an issue or PR has remained open with no activity and has become stale. label Aug 18, 2021
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

the info looks good to me, +1

@alexander-demicev
Copy link
Contributor Author

@elmiko I added a link to implementation https://github.com/openshift/enhancements/pull/468/files#diff-d3eb6604aecfb02079e4440173764bd1fb31ac20a2e93ae75dee709854ebb9caR192

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.

Do we want to make a note that this is implemented for AWS?
Never mind I see we have that

- "@JoelSpeed"
- "@enxebre"
creation-date: 2020-09-01
last-updated: 2020-09-01
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
last-updated: 2020-09-01
last-updated: 2021-08-19

@JoelSpeed
Copy link
Contributor

/approve

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Aug 19, 2021

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: JoelSpeed

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

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Aug 19, 2021
Copy link
Contributor

@elmiko elmiko left a comment

Choose a reason for hiding this comment

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

thanks Alex
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Aug 19, 2021
@openshift-merge-robot openshift-merge-robot merged commit e45cb34 into openshift:master Aug 19, 2021
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. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants