Allow 0 replicas #958

michaelgugino · 2018-12-20T15:57:19Z

This is needed for BYO team to have 0 worker replicas.

staebler · 2018-12-20T16:04:57Z

Wouldn't it be better to get rid of the validation that requires the worker machine pool?

staebler

Improve the commit message. See https://github.com/openshift/installer/blob/master/CONTRIBUTING.md#commit-message-format.

pkg/types/validation/machinepools.go

michaelgugino · 2018-12-20T16:10:07Z

pkg/types/validation/machinepools.go

I'm not sure why this is necessary of all a sudden. It worked fine in v0.6.0. Does not work in v0.7.0-master or master.

Validation was added in v0.7.0.

staebler · 2018-12-20T16:20:30Z

This will make it valid to set a replica size of 0 for the master machine pool as well. I don't think we want that.

michaelgugino · 2018-12-20T16:22:29Z

This will make it valid to set a replica size of 0 for the master machine pool as well. I don't think we want that.

@staebler I can make it check just masters if you prefer.

staebler · 2018-12-20T16:24:05Z

This will make it valid to set a replica size of 0 for the master machine pool as well. I don't think we want that.

@staebler I can make it check just masters if you prefer.

@michaelgugino My preference is to remove the requirement for the "worker" machine pool. Is there a reason to keep that machine pool with a replica size of 0 instead of omitting that machine pool?

michaelgugino · 2018-12-20T16:59:06Z

This will make it valid to set a replica size of 0 for the master machine pool as well. I don't think we want that.

@staebler I can make it check just masters if you prefer.

@michaelgugino My preference is to remove the requirement for the "worker" machine pool. Is there a reason to keep that machine pool with a replica size of 0 instead of omitting that machine pool?

@staebler

Aside from the fact that there are other validations in place expect that to be present? No, I don't think we need it to be present at all, but that would be a bigger patch set. Keeping it also would result in the machineset being created, which might be desired for scaling later in other usecases, of which I don't have a compelling example ATM.

While we could exclude workers, I think it makes more sense to specifically include masters. I think in all likelihood there will be an expectation to have arbitrary machinesets in the future, but it makes no difference to me.

Let me know what you prefer, I think the simplest fix is the best here, I just need it to not fail when I want to create ignition configs with 0 workers.

staebler · 2018-12-20T17:04:30Z

This will make it valid to set a replica size of 0 for the master machine pool as well. I don't think we want that.

@staebler I can make it check just masters if you prefer.

@michaelgugino My preference is to remove the requirement for the "worker" machine pool. Is there a reason to keep that machine pool with a replica size of 0 instead of omitting that machine pool?

@staebler

Aside from the fact that there are other validations in place expect that to be present? No, I don't think we need it to be present at all, but that would be a bigger patch set. Keeping it also would result in the machineset being created, which might be desired for scaling later in other usecases, of which I don't have a compelling example ATM.

While we could exclude workers, I think it makes more sense to specifically include masters. I think in all likelihood there will be an expectation to have arbitrary machinesets in the future, but it makes no difference to me.

Let me know what you prefer, I think the simplest fix is the best here, I just need it to not fail when I want to create ignition configs with 0 workers.

We can do either, especially for the short term. I think changing the validation to remove the "worker" requirement is easy, too. Delete these 3 lines.

installer/pkg/types/validation/installconfig.go

Lines 103 to 105 in ac006ae

    
           if !poolNames["worker"] { 
        
           	allErrs = append(allErrs, field.Required(fldPath, "must specify a machine pool with a name of 'worker'")) 
        
           }

staebler · 2018-12-20T17:05:09Z

@michaelgugino Whichever you prefer to do in the short term, I will lgtm to get BYO unblocked on this.

This commit is needed to allow 0 replicas for workers which is necessary for the BYO usecase.

michaelgugino · 2019-01-04T16:17:56Z

@staebler ptal

openshift-ci-robot · 2019-01-04T17:09:59Z

@michaelgugino: The following test failed, say /retest to rerun them all:

Test name	Commit	Details	Rerun command
ci/prow/e2e-aws	`36bc20a`	link	`/test e2e-aws`

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

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.

sdodson · 2019-01-04T18:38:50Z

/cc @wking @abhinavdahiya
/approve
PTAL

lgtm

openshift-ci-robot · 2019-01-04T18:40:39Z

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: michaelgugino, sdodson
To fully approve this pull request, please assign additional approvers.
We suggest the following additional approver: wking

If they are not already assigned, you can assign the PR to them by writing /assign @wking in a comment when ready.

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:

OWNERS

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

crawford · 2019-01-04T20:01:32Z

@michaelgugino I ran into this recently as well. I think I'd prefer @staebler's suggestion of removing the need for the worker machine pool. Do you have time to take another pass at this? If not, I'll eventually pick it up.

michaelgugino · 2019-01-04T21:11:56Z

@michaelgugino I ran into this recently as well. I think I'd prefer @staebler's suggestion of removing the need for the worker machine pool. Do you have time to take another pass at this? If not, I'll eventually pick it up.

@crawford I think that would still require this patch set. While the workers could indeed not be defined, if they are defined and set to zero, we'd still hit the same error. IMO, allowing for zero makes more sense as it's easier to template.

We don't currently support configuring zero workers [1], largely because some key operators still do not tolerate masters. Still, some users are attempting to work around our checks by leaving 'replicas' unset (which ends up as nil in Go) [2]. This commit adjusts our install-config defaulting to fill in the default replica counts when the user provides machine-pool entries but leaves replicas unset. [1]: openshift#958 [2]: https://bugzilla.redhat.com/show_bug.cgi?id=1670005#c1

wking · 2019-01-30T20:48:45Z

On the operator-support front, I've filed Bugzilla issues for the three remaining pods which do not tolerate masters:

Ingress: https://bugzilla.redhat.com/show_bug.cgi?id=1671136
Prometheus: https://bugzilla.redhat.com/show_bug.cgi?id=1671137
OLM: https://bugzilla.redhat.com/show_bug.cgi?id=1671140

crawford · 2019-02-16T01:20:50Z

Superseded by #1231.

openshift-ci-robot · 2019-02-16T01:20:51Z

@crawford: Closed this PR.

Details

In response to this:

Superseded by #1231.

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-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Dec 20, 2018

openshift-ci-robot requested review from aaronlevy and staebler December 20, 2018 15:57

staebler reviewed Dec 20, 2018

View reviewed changes

pkg/types/validation/machinepools.go Outdated Show resolved Hide resolved

michaelgugino commented Dec 20, 2018

View reviewed changes

michaelgugino force-pushed the min-replicas-0 branch from c102986 to 15b8dd4 Compare December 20, 2018 16:17

michaelgugino force-pushed the min-replicas-0 branch 2 times, most recently from 8666e74 to eb55931 Compare January 4, 2019 14:57

validation/machinepools: Allow 0 replicas for non-masters

36bc20a

This commit is needed to allow 0 replicas for workers which is necessary for the BYO usecase.

michaelgugino force-pushed the min-replicas-0 branch from eb55931 to 36bc20a Compare January 4, 2019 15:47

openshift-ci-robot requested review from abhinavdahiya and wking January 4, 2019 18:39

wking mentioned this pull request Jan 29, 2019

pkg/types/defaults/installconfig: Set defaults for null replicas #1146

Merged

openshift-ci-robot closed this Feb 16, 2019

Allow 0 replicas #958

Allow 0 replicas #958

Uh oh!

Conversation

michaelgugino commented Dec 20, 2018

Uh oh!

staebler commented Dec 20, 2018

Uh oh!

staebler left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

michaelgugino Dec 20, 2018

Choose a reason for hiding this comment

Uh oh!

staebler Dec 20, 2018

Choose a reason for hiding this comment

Uh oh!

staebler commented Dec 20, 2018

Uh oh!

michaelgugino commented Dec 20, 2018

Uh oh!

staebler commented Dec 20, 2018

Uh oh!

michaelgugino commented Dec 20, 2018

Uh oh!

staebler commented Dec 20, 2018

Uh oh!

staebler commented Dec 20, 2018

Uh oh!

michaelgugino commented Jan 4, 2019

Uh oh!

openshift-ci-robot commented Jan 4, 2019

Uh oh!

sdodson commented Jan 4, 2019

Uh oh!

openshift-ci-robot commented Jan 4, 2019

Uh oh!

crawford commented Jan 4, 2019

Uh oh!

michaelgugino commented Jan 4, 2019

Uh oh!

wking commented Jan 30, 2019

Uh oh!

crawford commented Feb 16, 2019

Uh oh!

openshift-ci-robot commented Feb 16, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants