-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Working vsphere clusterctl example #263
Working vsphere clusterctl example #263
Conversation
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: jessicaochen 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:
Approvers can indicate their approval by writing |
/assign @k4leung4 @mkjelland |
@@ -0,0 +1,22 @@ | |||
# Vsphere Example Files | |||
## Contents | |||
*.yaml files - concrete example files that can be used as is. |
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.
did you want this in a list? this and the next line shows up together
might help to check out how the md file is parsed [View] button
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.
Done
## Generation | ||
For convenience, a generation script which populates templates where possible. | ||
|
||
1. Run the generation script. |
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.
can you mention which files are generated by the generation script
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.
Done
clusterctl/cmd/create_cluster.go
Outdated
// Adapt the terraform methods calls since gcp/terraform are not on the same page. | ||
// Long term, these providers should converge or the need for a provider will go away. | ||
// Whichever comes first. | ||
type terraformAdapter struct { |
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 was just thinking about this. Now that all providers are using the same tool it's more important for the current providers to be in sync. The machine.Actuator already forces them to be somewhat in sync, do you think we need something similar the GetIP and GetKubeConfig methods?
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.
The clusterdeployer.ProviderDeployer can do that in the future. We could get vsphere caught up with gcp and then remove this adapter. I do want to do such work not in this PR though.
PROVIDERCOMPONENT_GENERATED_FILE=provider-components.yaml | ||
|
||
MACHINE_CONTROLLER_SSH_PUBLIC_FILE=vsphere_tmp.pub | ||
MACHINE_CONTROLLER_SSH_PUBLIC= |
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.
Is this supposed to be empty?
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.
Yep. It is the contents of the file. Will be populated after generation.
while test $# -gt 0; do | ||
case "$1" in | ||
-h|--help) | ||
echo "$SCRIPT - generates input yaml files for Cluster API on Google Cloud Platform" |
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.
Replace Google Cloud Platform with vSphere
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.
Done.
clusterctl/cmd/create_cluster.go
Outdated
@@ -144,9 +144,27 @@ func getProvider(provider string) (clusterdeployer.ProviderDeployer, error) { | |||
case "google": | |||
return google.NewMachineActuator(google.MachineActuatorParams{}) | |||
case "terraform": |
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.
Do you plan to have a separate option for the vsphere provider, or do you think this will eventually be replaced by that one? #233
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 swapped for vsphere one. We can add terraform back should we having a case for that.
a9b57b6
to
a0f1302
Compare
clusterctl/cmd/create_cluster.go
Outdated
default: | ||
return nil, fmt.Errorf("Unrecognized provider %v", provider) | ||
} | ||
} | ||
|
||
// Adapt the terraform methods calls since gcp/vsphere are not on the same page. |
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.
should it be vsphere methods instead of terraform methods?
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.
Fixed.
"virtual_machine_domain = \"\"", | ||
] | ||
versions: | ||
kubelet: 1.9.7 |
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.
is it intentional to have the master run at 1.10.1 while the nodes run at 1.9.7
if so, a comment would be nice.
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.
Fixed.
- apiVersion: "cluster.k8s.io/v1alpha1" | ||
kind: Machine | ||
metadata: | ||
generateName: jesschen-tf-test- |
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.
did you want to keep your username in the example?
its ok if you did, just checking
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.
Fixed.
limits: | ||
cpu: 100m | ||
memory: 30Mi | ||
- name: terraform-machine-controller |
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.
should this be renamed the vsphere-machine-controller?
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.
Fixed.
cpu: 100m | ||
memory: 30Mi | ||
- name: terraform-machine-controller | ||
image: gcr.io/k8s-cluster-api/terraform-machine-controller:0.0.6 |
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.
the image should point to the vsphere image since you are using the vsphere provider now.
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.
Fixed.
@@ -0,0 +1,62 @@ | |||
items: |
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.
Now that you are using the vsphere client, this may need to updated based on PR #257. The apiVersion will be vsphereproviderconfig and the kind is either VsphereClusterProviderConfig or VsphereMachineProviderConfig. The username/password/server fields moved from the machine to the cluster providerConfig.
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.
Done.
@@ -95,7 +95,7 @@ func init() { | |||
createClusterCmd.Flags().StringVarP(&co.Machine, "machines", "m", "", "A yaml file containing machine object definition(s)") | |||
createClusterCmd.Flags().StringVarP(&co.ProviderComponents, "provider-components", "p", "", "A yaml file containing cluster api provider controllers and supporting objects") | |||
// TODO: Remove as soon as code allows https://github.com/kubernetes-sigs/cluster-api/issues/157 | |||
createClusterCmd.Flags().StringVarP(&co.Provider, "provider", "", "", "Which provider deployment logic to use (google/terraform)") | |||
createClusterCmd.Flags().StringVarP(&co.Provider, "provider", "", "", "Which provider deployment logic to use (google/vsphere)") |
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.
Can we mark the required flags using things like:
createClusterCmd.MarkFlagRequired("provider")
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 think that is a good idea for a different PR.
a0f1302
to
c82c5a7
Compare
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, leaving for @k4leung4 to add lgtm label
/lgtm |
…n-certs 🐛 Separate certificate logic for joins
Add the v0 vsphere configuration examples.
Pod specs are up to date as of commit cf9bcf6 (#219)
Special notes for your reviewer:
Release note:
@kubernetes/kube-deploy-reviewers