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

Set up node and storage management for vsphere #212

Merged
merged 2 commits into from
May 25, 2018

Conversation

krousey
Copy link
Contributor

@krousey krousey commented May 24, 2018

What this PR does / why we need it:
This PR enables vsphere cloud provider management of node and storage objects. This ensures node objects are deleted when their VM is deleted. It also enables the vsphere-volume manager to dynamically provision and attach storage.

Special notes for your reviewer:
set up according to this guide

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 24, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: krousey

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels May 24, 2018
@krousey
Copy link
Contributor Author

krousey commented May 24, 2018

/cc @rsdcastro @roberthbailey @spew
/hold
so I can push the image after LGTM

@krousey
Copy link
Contributor Author

krousey commented May 24, 2018

/assign @spew

@krousey
Copy link
Contributor Author

krousey commented May 24, 2018

/hold

@k8s-ci-robot k8s-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 24, 2018
template = <<EOF
[Global]
datacenters = "$${datacenter}"
insecure-flag = "1" #set to 1 if the vCenter uses a self-signed cert
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we expect any usage of non-self-signed certs? If so, both here and allow_unverified_ssl = true above would need to be changed together, right? Probably not worth changing until we need to, but seems like a sharp edge we are leaving for the first person that wants to bring a real cert.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we might see that, but I don't see this as being incompatible with that. This just disables verification of the certificate chain and host name. This should still work with a proper cert, it just won't be checked.

The reason to change this is to harden against man in the middle attacks, which we'll probably want to do.

@spew
Copy link
Contributor

spew commented May 24, 2018

I can't find any mistakes. Looks good.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 24, 2018
@@ -53,19 +53,60 @@ items:
datacenter_id = "${data.vsphere_datacenter.dc.id}"
}

data "template_file" "cloud_provider_config" {
Copy link
Contributor

@karan karan May 25, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you point me to some documentation that can help me understand what's going on here?

EDIT: nvm I see the doc you linked in the PR description.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Linked in the PR description

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -213,7 +213,11 @@ sysctl net.bridge.bridge-nf-call-iptables=1
# kubeadm uses 10th IP as DNS server
CLUSTER_DNS_SERVER=$(prips ${SERVICE_CIDR} | head -n 11 | tail -n 1)

sed -i "s/KUBELET_DNS_ARGS=[^\"]*/KUBELET_DNS_ARGS=--cluster-dns=${CLUSTER_DNS_SERVER} --cluster-domain=${CLUSTER_DNS_DOMAIN}/" /etc/systemd/system/kubelet.service.d/10-kubeadm.conf
cat > /etc/systemd/system/kubelet.service.d/20-cloud.conf << EOF
[Service]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this needed? (it seems difficult to track down on google what exactly this block enables)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are part of the systemd configuration files for kubelet that are installed as part of the kubeadm package. We were sed editing an existing one, but it's cleaner to write a new file with a higher number to
override things.

These are ultimately kubelet flags. The gce startup scripts switched to this approach as well. https://github.com/kubernetes-sigs/cluster-api/blob/master/gcp-deployer/machine_setup_configs.yaml#L74-L79

@roberthbailey
Copy link
Contributor

/lgtm

@krousey
Copy link
Contributor Author

krousey commented May 25, 2018 via email

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label May 25, 2018
@k8s-ci-robot k8s-ci-robot merged commit 2461d32 into kubernetes-sigs:master May 25, 2018
@piosz
Copy link
Contributor

piosz commented May 30, 2018

cc @kawych @serathius

@serathius
Copy link

@krousey Will vsphere-volume-manager register storage class by it's own or it's need to be added to manifests?

chuckha pushed a commit to chuckha/cluster-api that referenced this pull request Oct 2, 2019
jayunit100 pushed a commit to jayunit100/cluster-api that referenced this pull request Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants