[vsphere] create machineset spec based on machineset spec created at install time#669
Conversation
|
Skipping CI for Draft Pull Request. |
rvanderp3
changed the title
openshift-ci
Bot
added
the
do-not-merge/work-in-progress
rvanderp3
force-pushed
the
determine-workspace-from-cluster
branch
3 times, most recently
from
0a70bc4 to
44844d7
Compare
|
Confirmed in both VMC and the IBM Cloud that the workspace properly populates based on the existing compute node workspace and that a windows machine is created. Also confirmed that the |
rvanderp3
changed the title
openshift-ci
Bot
removed
the
do-not-merge/work-in-progress
rvanderp3
force-pushed
the
determine-workspace-from-cluster
branch
2 times, most recently
from
c4e0c9c to
a61eb83
Compare
sebsoto
left a comment
sebsoto
left a comment
There was a problem hiding this comment.
Thanks for the PR! Left a few requests
| templateSpec := machineSet.Spec.Template.Spec | ||
| providerSpecRaw := templateSpec.ProviderSpec.Value |
There was a problem hiding this comment.
This can be simplified to providerSpecRaw := machineSet.Spec.Template.Spec.ProviderSpec.Value
Having the extra variable templateSpec is confusing as we are only interested in the ProviderSpec's value.
| providerSpec, err := newVSphereMachineProviderSpec(clusterID) | ||
| providerSpec, err := p.newVSphereMachineProviderSpec() |
There was a problem hiding this comment.
I'm not sure about making this a method and no longer passing in clusterID, seems like this is going to result in an extra API call.
There was a problem hiding this comment.
My rationale for that was that the cluster ID is no longer required in that method. Since we are importing the workspace from an existing machineset, we no longer need the cluster ID to produce a valid workspace as it's already embedded in the workspace we are importing.
There was a problem hiding this comment.
That is what I understood too. Are we missing something, @sebsoto? Where is the extra API call coming from?
There was a problem hiding this comment.
I think @sebsoto is referring to this added call in getWorkspaceFromExistingMachineSet() despite already getting the cluster ID here.
There was a problem hiding this comment.
@saifshaikh48 @sebsoto got it, I updated the PR to prevent the additional call.
rvanderp3
force-pushed
the
determine-workspace-from-cluster
branch
from
a61eb83 to
47a9b77
Compare
The intent of this PR is to derive the Workspace for machinesets created during testing based on the compute machineset provisioned by the cluster at installation. The reason for this change is that a vCenter in IBM Cloud is being also used to run CI jobs from a shared lease pool to increase test coverage in some key configurations. Additionally, the vmTemplate can now be optionally defined as an environment variable in the e2e test script.
rvanderp3
force-pushed
the
determine-workspace-from-cluster
branch
from
47a9b77 to
c1bc14b
Compare
|
/hold |
openshift-ci
Bot
added
the
do-not-merge/hold
|
/cancel hold |
|
/hold cancel |
openshift-ci
Bot
removed
the
do-not-merge/hold
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rvanderp3, sebsoto The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-ci
Bot
added
the
approved
saifshaikh48
left a comment
saifshaikh48
left a comment
There was a problem hiding this comment.
/lgtm
Thanks for working on this @rvanderp3
|
/cherry-pick release-4.8 |
|
@sebsoto: new pull request created: #693 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. |
|
/cherry-pick release-4.9 |
|
/cherry-pick community-4.8 |
|
/cherry-pick release-4.7 |
|
@aravindhp: new pull request created: #694 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. |
|
@aravindhp: new pull request created: #695 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. |
|
@aravindhp: #669 failed to apply on top of branch "release-4.7": 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. |
6 participants
The intent of this PR is to derive the Workspace for machinesets created during testing based on the compute machineset provisioned by the cluster at installation. The reason for this change is that a vCenter in IBM Cloud is being also used to run CI jobs from a shared lease pool to increase test coverage in some key configurations. Additionally, the vmTemplate can now be optionally defined as an environment variable in the e2e test script.