Run storage provisioner as pod #2137

priyawadhwa · 2017-10-30T18:55:14Z

No description provided.

k8s-ci-robot · 2017-10-30T18:55:17Z

Thanks for your pull request. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please follow instructions at https://github.com/kubernetes/kubernetes/wiki/CLA-FAQ to sign the CLA.

It may take a couple minutes for the CLA signature to be fully registered; after that, please reply here with a new comment and we'll verify. Thanks.

If you've already signed a CLA, it's possible we don't have your GitHub username or you're using a different email address. Check your existing CLA data and verify that your email is set on your git commits.
If you signed the CLA as a corporation, please sign in with your organization's credentials at https://identity.linuxfoundation.org/projects/cncf to be authorized.
If you have done the above and are still having issues with the CLA being reported as unsigned, please email the CNCF helpdesk: helpdesk@rt.linuxfoundation.org

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. I understand the commands that are listed here.

r2d4

Code looks good so far! One final check needed

Remove the conditional here, since the test is actually being skipped on kubeadm right now.
https://github.com/kubernetes/minikube/blob/master/test/integration/functional_test.go#L39-L42

r2d4 · 2017-10-30T20:06:49Z

deploy/storage-provisioner/Dockerfile

+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+FROM ubuntu:16.04


A slimmer image would be nice. Maybe scratch?

r2d4 · 2017-10-30T20:08:38Z

pkg/minikube/constants/constants.go

@@ -206,6 +209,9 @@ func GetKubeadmCachedImages(version string) []string {
 		"gcr.io/google_containers/kube-scheduler-amd64:" + version,
 		"gcr.io/google_containers/kube-controller-manager-amd64:" + version,
 		"gcr.io/google_containers/kube-apiserver-amd64:" + version,
+
+		//Storage provisioner
+		"gcr.io/k8s-minikube/storage-provisioner:" + version,


I think you can just leave this as whatever is above. The version here is actually from the --kubernetes-version parameter, so if you use --kubernetes-version v1.8.1 it will try to cache the 1.8.1 tag of the storage provisioner.

We probably won't publish one of these for every version of kubernetes, so make this the hardcoded
"gcr.io/k8s-minikube/storage-provisioner:v1.8.0"

r2d4 · 2017-10-30T20:09:12Z

deploy/addons/storage-provisioner/storage-provisioner.yaml

+  namespace: kube-system
+  labels:
+    integration-test: storage-provisioner
+    addonmanager.kubernetes.io/mode: Reconcile


I think you want this to be addonmanager.kubernetes.io/mode: EnsureExists, so we can disable it.

r2d4 · 2017-10-30T20:09:28Z

cmd/storage-provisioner/main.go

+	wg.Add(2)
+	go func() {
+		defer wg.Done()
+		storageProvisionerServer.Start()


You can just call this directly, no need to put it in a waitgroup

So I tried that initially, but it didn't work. I looked it up and I think the main program ends before the server has a chance to start?

I think thats due to a few things i mentioned above.
Once you do that, then you can see that its actually just exiting since there are errors

$ ./main --v 10 --logtostderr F1030 14:08:28.887110 152274 main.go:30] unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined

r2d4 · 2017-10-30T20:21:09Z

ref #2134

r2d4 · 2017-10-30T20:38:23Z

The other thing we'll need to think about is how we're going to test changes to the storage provisioner addon in minikube CI tests. I don't think this is super important now, but we should start thinking about it.

Currently:
No integration tests on initial code changes to the storage provisioner, tests are ran when version is bumped

Ideally:
Minikube CI builds both minikube and the storage provisioner image, and subsequently uses that image in that CI run.

Challenges:
How to template the addon? We can pass through version information through a CLI flag or buildtime ldflag to minikube, but the addon is in plain YAML, which makes it difficult to template the version.

At a higher level, should addons be:

Pulled from a remote location
Templated within the minikube binary itself.

One thing we've talked about for kube-dns (which also needs templating) is to remove it from go-bin-data and template it in the minikube binary itself.

r2d4 · 2017-10-30T21:03:11Z

Makefile

@@ -300,6 +300,14 @@ $(ISO_BUILD_IMAGE): deploy/iso/minikube-iso/Dockerfile
 	@echo ""
 	@echo "$(@) successfully built"

+storage-provisioner-main: cmd/storage-provisioner/main.go
+	go build cmd/storage-provisioner/main.go


rename to storage-provisioner and can you make this output to our build dir? go build ... -o $(BUILD_DIR)/storage-provisioner

r2d4 · 2017-10-30T21:04:11Z

Makefile

@@ -300,6 +300,14 @@ $(ISO_BUILD_IMAGE): deploy/iso/minikube-iso/Dockerfile
 	@echo ""
 	@echo "$(@) successfully built"

+storage-provisioner-main: cmd/storage-provisioner/main.go


This should technically depend on all the storage-provisioner files.

This is not great, but you'll need do add something that computes the dependencies,

STORAGE_PROVISIONER_FILES := GOPATH=$(GOPATH) go list -f '{{join .Deps "\n"}}' k8s.io/minikube/cmd/storage-provisioner | grep k8s.io | GOPATH=$(GOPATH) xargs go list -f '{{ range $$file := .GoFiles }} {{$$.Dir}}/{{$$file}}{{"\n"}}{{end}}'

r2d4 · 2017-10-30T21:11:43Z

cmd/storage-provisioner/main.go

+
+func main() {
+	localkubeServer := cmd.NewLocalkubeServer()
+	storageProvisionerServer := localkubeServer.NewStorageProvisionerServer()


main should look something like

flag.Parse() if err := localkube.StartStorageProvisioner(); err != nil { glog.Exit(err) }

Here, instead of creating a new localkube server, change the signature to be func StartStorageProvisioner() error and call that directly.

minikube/pkg/localkube/storage_provisioner.go

Line 117 in 6a53c0c

func StartStorageProvisioner(lk LocalkubeServer) func() error {

SGTM, but we don't need the flag.Parse() in this case, right?

glog (the logging library) adds some flags to set verbosity, which would be nice to have on this binary (you might want to run the container command as storage-provisioner --v 10 --logtostderr)

Unfortunately, some of the localkube dependencies also register their flags, so theres some extra crap that gets added, but thats fine.

r2d4 · 2017-10-30T21:12:19Z

cmd/storage-provisioner/main.go

+	wg.Add(2)
+	go func() {
+		defer wg.Done()
+		storageProvisionerServer.Start()


I think thats due to a few things i mentioned above.
Once you do that, then you can see that its actually just exiting since there are errors

$ ./main --v 10 --logtostderr F1030 14:08:28.887110 152274 main.go:30] unable to load in-cluster configuration, KUBERNETES_SERVICE_HOST and KUBERNETES_SERVICE_PORT must be defined

codecov-io · 2017-10-30T23:45:22Z

Codecov Report

Merging #2137 into master will increase coverage by 0.01%.
The diff coverage is 0%.

@@            Coverage Diff            @@
##           master   #2137      +/-   ##
=========================================
+ Coverage   28.59%   28.6%   +0.01%     
=========================================
  Files          82      82              
  Lines        5372    5369       -3     
=========================================
  Hits         1536    1536              
+ Misses       3640    3637       -3     
  Partials      196     196

Impacted Files	Coverage Δ
pkg/localkube/storage_provisioner.go	`0% <0%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 38e719a...c342ed4. Read the comment docs.

r2d4 · 2017-11-01T17:49:41Z

pkg/localkube/storage_provisioner.go

-}
-
-func StartStorageProvisioner(lk LocalkubeServer) func() error {
+func StartStorageProvisioner() func() error {


nit: "unwrap" this, and just return an error instead of a func() error

r2d4

LGTM, nice!

k8s-ci-robot added cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Oct 30, 2017

Create main file for storage provisioner

a8fedc8

r2d4 reviewed Oct 30, 2017

View reviewed changes

priyawadhwa force-pushed the storage-provisioner-as-pod branch from c8ff298 to e372b27 Compare October 30, 2017 20:38

Tests passing with storage-provisioner in addons

71b38d1

priyawadhwa force-pushed the storage-provisioner-as-pod branch from e372b27 to 71b38d1 Compare October 30, 2017 20:46

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. and removed cncf-cla: no Indicates the PR's author has not signed the CNCF CLA. labels Oct 30, 2017

Small changes

7098431

priyawadhwa force-pushed the storage-provisioner-as-pod branch from 6ef4f44 to 7098431 Compare October 30, 2017 20:53

r2d4 reviewed Oct 30, 2017

View reviewed changes

r2d4 reviewed Nov 1, 2017

View reviewed changes

Added storage prov deps to Makefile and rewrote main file

c342ed4

priyawadhwa force-pushed the storage-provisioner-as-pod branch from dd60a95 to c342ed4 Compare November 1, 2017 17:56

r2d4 approved these changes Nov 1, 2017

View reviewed changes

r2d4 merged commit f368ac4 into kubernetes:master Nov 1, 2017

r2d4 mentioned this pull request Jan 11, 2018

Dynamic provisioning doesn't work with kubeadm bootstrapper #2059

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Run storage provisioner as pod #2137

Run storage provisioner as pod #2137

priyawadhwa commented Oct 30, 2017

k8s-ci-robot commented Oct 30, 2017

r2d4 left a comment

r2d4 Oct 30, 2017

r2d4 Oct 30, 2017

r2d4 Oct 30, 2017 •

edited

Loading

r2d4 Oct 30, 2017

priyawadhwa Oct 30, 2017

r2d4 Oct 30, 2017

r2d4 commented Oct 30, 2017

r2d4 commented Oct 30, 2017

r2d4 Oct 30, 2017

r2d4 Oct 30, 2017

r2d4 Oct 30, 2017

r2d4 Oct 30, 2017

priyawadhwa Oct 30, 2017

r2d4 Oct 31, 2017

r2d4 Oct 30, 2017

codecov-io commented Oct 30, 2017 •

edited

Loading

r2d4 Nov 1, 2017

r2d4 left a comment

Run storage provisioner as pod #2137

Run storage provisioner as pod #2137

Conversation

priyawadhwa commented Oct 30, 2017

k8s-ci-robot commented Oct 30, 2017

r2d4 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r2d4 Oct 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r2d4 commented Oct 30, 2017

r2d4 commented Oct 30, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Oct 30, 2017 • edited Loading

Codecov Report

Choose a reason for hiding this comment

r2d4 left a comment

Choose a reason for hiding this comment

r2d4 Oct 30, 2017 •

edited

Loading

codecov-io commented Oct 30, 2017 •

edited

Loading