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

*: support no worker nodes #1231

Merged
merged 2 commits into from
Feb 15, 2019
Merged

*: support no worker nodes #1231

merged 2 commits into from
Feb 15, 2019

Conversation

staebler
Copy link
Contributor

  • The user will be allowed to configure no compute machine pools. To do this, the user must explicitly specify the "master" machine pool. If the user does not specify any machine pools, then the installer will apply the default of a single "master" machine pool and a single "worker" machine pool.
  • The user will be allowed to configure a compute machine pool with 0 replicas. To do this, the user must explicitly specify a replica count of 0 for the machine pool. If the replicas field is omitted, then the installer will apply the default replica count, which depends upon the platform selected.
  • The user will be allowed to configure multiple compute machine pools. Every compute machine pool will use the same ignition config.
  • The installer will warn the user if there are no compute nodes configured. The installer will not block the user from using such a configuration.

Fix for https://jira.coreos.com/browse/CORS-988

@openshift-ci-robot openshift-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 11, 2019
@flaper87
Copy link
Contributor

Testing this patch with OpenStack. I'll report back asap.

@flaper87
Copy link
Contributor

I've tested this on OpenStack and it seems to work fine.

Thanks

@staebler
Copy link
Contributor Author

/retest

1 similar comment
@staebler
Copy link
Contributor Author

/retest

wking
wking reviewed Feb 14, 2019
View reviewed changes
pkg/asset/machines/worker.go
switch ic.Platform.Name() {
case awstypes.Name:
mpool := defaultAWSMachinePoolPlatform()
mpool.InstanceType = "m4.large"
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 care about prioritizing this PR vs. #1163? They will conflict on at least this line.

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 care about prioritizing. I can rebase this PR easily enough.

@wking
Copy link
Member

wking commented Feb 14, 2019

I think #1157 will make this easier. I'll bump the exception-request thread to see if we can unstick that.

@staebler
Copy link
Contributor Author

I think #1157 will make this easier. I'll bump the exception-request thread to see if we can unstick that.

Yes, this will be easier if #1157 lands. I created this PR mostly as a fallback in case the feature exception needed for #1157 is not approved. These are the minimal changes to fix the bug from https://jira.coreos.com/browse/CORS-988.

@chancez
Copy link
Contributor

chancez commented Feb 14, 2019

  • The user will be allowed to configure multiple compute machine pools. Every compute machine pool will use the same ignition config.

Is it going to be possible to change that in the future, or at least, does this change allow for that to happen in the future in a backwards compatible way? I imagine a primary use case for separate machine pools will be to customize the configuration the pool has. Eg, you may want to tune kernel parameters for the "DB machine pool" or similar.

@staebler
Copy link
Contributor Author

  • The user will be allowed to configure multiple compute machine pools. Every compute machine pool will use the same ignition config.

Is it going to be possible to change that in the future, or at least, does this change allow for that to happen in the future in a backwards compatible way? I imagine a primary use case for separate machine pools will be to customize the configuration the pool has. Eg, you may want to tune kernel parameters for the "DB machine pool" or similar.

We make the claim at https://github.com/openshift/installer/blame/master/docs/user/versioning.md#L7 that the installer will always generate a master.ign and worker.ign ignition config file. I considered changing it so that the ignition config files were named to match the name of the machine pools. However, I think that involves coordinated changes with the machine-config-operator (or at least its manifest) to serve the ignition config from endpoints with custom names.

@chancez
Copy link
Contributor

chancez commented Feb 14, 2019

I think it's fine if it uses that, my question is more about if this will allow (ie: doesn't prevent) for future additions where it might use a different ignition config.

@staebler
Copy link
Contributor Author

error: could not run steps: test "release-latest" failed: pod release-latest was already deleted

/retest

@openshift-ci-robot openshift-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 15, 2019
@staebler
Copy link
Contributor Author

Rebased onto #1157 in preparation for it merging.

@staebler
types: allow compute pools with names other than 'worker'
d8dc7a7 
Relax the restriction that there is exactly one compute machine pool and that
it is named "worker". Also, relax the restriction that a compute machine pool
must have a positive number of replicas.

Warn the user if the install-config.yaml does not specify any compute
nodes. The user will not be blocked from using no compute nodes.

Changes for https://jira.coreos.com/browse/CORS-988
@staebler
asset/machines: machineset for every compute pool
98dcc19 
Include machines sets for every compute machine pool in the
99_openshift-cluster-api_worker-machineset.yaml manifest. All of the
machine sets will use the same ignition config.

Changes for https://jira.coreos.com/browse/CORS-988
@openshift-ci-robot openshift-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels Feb 15, 2019
@abhinavdahiya
Copy link
Contributor

/approve

@wking
Copy link
Member

wking commented Feb 15, 2019

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Feb 15, 2019
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: abhinavdahiya, staebler, wking

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:
  • OWNERS [abhinavdahiya,staebler,wking]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot merged commit 1ae7d5c into openshift:master Feb 15, 2019
@crawford crawford mentioned this pull request Feb 16, 2019
Closed
@abhinavdahiya abhinavdahiya mentioned this pull request Feb 19, 2019
Merged
@dgoodwin dgoodwin mentioned this pull request Mar 5, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wking wking wking left review comments

@hardys hardys Awaiting requested review from hardys

@russellb russellb Awaiting requested review from russellb

Assignees

@wking wking

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. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@staebler @flaper87 @wking @chancez @abhinavdahiya @openshift-ci-robot @openshift-merge-robot