-
Notifications
You must be signed in to change notification settings - Fork 1.5k
Add machines and machinesets for VSphere #2889
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 machines and machinesets for VSphere #2889
Conversation
|
cc: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Template should be the name of the virtual machine template - config.Platform.VSphere.Template
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patrickdillon I am wrong about this. We will download the ova and add it to vSphere. That's still in the backlog.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patrickdillon I missed network from types/vsphere/platform.go
pkg/asset/machines/worker.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Based on reviewing other providers it seems MachinePool should contain:
https://github.com/openshift/machine-api-operator/blob/master/pkg/apis/vsphereprovider/v1alpha1/vsphereproviderconfig_types.go#L32-L54
Though I am not sure about Network NetworkSpec.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm maybe not NetworkSpec since that seems to environmental specific.
pkg/types/vsphere/platform.go
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jcpowermac I can add Network to the Platform. omitempty means it is optional, but it looks like the provider requires a NetworkName: https://github.com/openshift/machine-api-operator/blob/master/pkg/apis/vsphereprovider/v1alpha1/vsphereproviderconfig_types.go#L32 WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patrickdillon UPI doesn't need it and if we don't include omitempty then I am assuming it would become required parameter for UPI?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am trying to gain some background on this, but have to run to another meeting in 5 mins.
UPI doesn't need it
But we do need to pass a value to terraform, right? https://github.com/openshift/installer/pull/2841/files#diff-779e91f6248cdf8645f11e13f8849d28R47
I think we just need to make sure those values match here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@patrickdillon yeah for IPI terraform and the machine-api-operator:
https://github.com/openshift/machine-api-operator/blob/master/pkg/controller/vsphere/reconciler.go#L324-L325
|
/woof |
DetailsIn response to this:
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. |
0fe9fa5 to
9c5d8cf
Compare
9c5d8cf to
3837fcc
Compare
91b6692 to
fcf7f64
Compare
fcf7f64 to
180ec05
Compare
180ec05 to
12d7e49
Compare
|
/cc @jstuever |
jstuever
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
I didn't see any obvious issues; we'll see when we get all the pieces together.
|
/assign @jcpowermac |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
5 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/test images |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/skip |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/skip |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
2 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
…odes. Vendors additional vsphere-provider api code using dep ensure.
656723c to
903d13c
Compare
|
/lgtm |
|
/test all |
|
/hold |
|
/hold cancel |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
3 similar comments
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
/retest Please review the full test history for this PR and help us cut down flakes. |
|
@patrickdillon: The following tests failed, say
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. DetailsInstructions 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. |

Adds machines and machinesets for VSphere workers and control plane. Plumbs values through to tfvars.
/cc @jcpowermac