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

InstallConfig machine pool typing and validation #264

Closed
wking opened this issue Sep 17, 2018 · 6 comments
Closed

InstallConfig machine pool typing and validation #264

wking opened this issue Sep 17, 2018 · 6 comments

Comments

@wking
Copy link
Member

wking commented Sep 17, 2018

Spun off from this thread, @dgoodwin expressed concerns about Machines corner cases. What if no pool named master is configured? What if multiple pools named master is configured? What if a pool named somethingTheInstallerDoesNotUnderstand is configured? I'd floated:

Master MachinePool `json:"master"

Worker MachinePool `json:"worker"

but @dgoodwin pointed out compat issues between that approach and ClusterDeployment information.

@abhinavdahiya suggested we leave Machines alone, but we don't currently have any validation for Machines, and I expect we'll want to have some at some point.

@dgoodwin suggested leaving Machines as it stands but adding a typed variable for the name with constants for the master and worker names (he suggested control-plane and compute for the names) with verification checking for exactly one control-plane and at least one compute. He didn't give an opinion on the somethingTheInstallerDoesNotUnderstand case.

I'm not clear enough on the trade-offs to have much of an opinion here, but think we should sort out how we want verification to work here before InstallConfig gets too popular and migration becomes difficult ;).

@dgoodwin
Copy link
Contributor

One thing I didn't think of at the time was:

ControlPlane MachinePool
Compute MachinePool[]

I would be ok with this as it's flexible enough for the long term definition of what should exist in the cluster. You could enforce compute only contains one at install time if you wanted.

I'd suggest avoiding "Master" and going "ControlPlane" as that's been a consistent trend as far as I can tell lately.

But yes if leaving as is I think explicit type identifiers would be better than assumptions about names, and let the user choose the names.

@wking
Copy link
Member Author

wking commented Sep 18, 2018

@abhinavdahiya, what do you think about:

ControlPlane MachinePool
Compute MachinePool[]

?

@crawford
Copy link
Contributor

crawford commented Jan 4, 2019

Closing due to inactivity.

@openshift-ci-robot
Copy link
Contributor

@crawford: Closing this issue.

In response to this:

Closing due to inactivity.

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.

@wking
Copy link
Member Author

wking commented Jan 4, 2019

I'm still in favor of this change, but was waiting for feedback from other maintainers ;). Mind if I just go ahead and work up a PR?

@dgoodwin
Copy link
Contributor

dgoodwin commented Jan 7, 2019

I think @abutcher has one in flight for #264

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants