GCE: install containerized mounter on COS #3482

justinsb · 2017-09-30T06:09:37Z

The containerized mounter is a little tricky to install, with lots of
bind mounts. This code path is only hit on GCE though.

k8s-github-robot · 2017-10-01T08:03:52Z

@justinsb PR needs rebase

chrislovecnm · 2017-10-05T06:45:50Z

@justinsb we need bazel files updated please :)

chrislovecnm

Lot of questions for yah. Looks good. Please rebase and bazelize ... aka update the bazel files.

chrislovecnm · 2017-09-30T19:31:38Z

nodeup/pkg/model/kubelet.go

@@ -105,6 +105,10 @@ func (b *KubeletBuilder) Build(c *fi.ModelBuilderContext) error {
 		return err
 	}

+	if err := b.addContainerizedMounter(c); err != nil {
+		return err


should we fmt?

I'd say no: it's one of our functions, we have nothing to add really.

chrislovecnm · 2017-09-30T19:32:59Z

nodeup/pkg/model/kubelet.go

+		Options:    []string{"ro"},
+	})
+
+	//		cp "${KUBE_HOME}/kube-manifests/kubernetes/gci-trusty/gci-mounter" "${CONTAINERIZED_MOUNTER_HOME}/mounter"


remove comments or add to func docs?

chrislovecnm · 2017-09-30T19:34:00Z

upup/pkg/fi/cloudup/apply_cluster.go

+// needsKubernetesManifests checks if we need kubernetes manifests
+// This is only needed currently on ContainerOS i.e. GCE, but we don't have a nice way to detect it yet
+func needsKubernetesManifests(c *kops.Cluster, instanceGroups []*kops.InstanceGroup) bool {
+	// TODO: Do real detection of ContainerOS (but this has to work with AMI names, and maybe even forked AMIs)


AMI? Don't think gce has AMIs :)

Changed to "image"

chrislovecnm · 2017-09-30T19:34:35Z

upup/pkg/fi/nodeup/nodetasks/archive.go

+)
+
+// Archive task downloads and extracts a tar file
+type Archive struct {


we going to use this for the CNI tarball now?

We use assets today. Assets let us pluck individual files out of a tarball. mounter.tar is a big tar file that is expanded in its entirety into a directory, which is a little different - we would have to list every file in the tar.

I think we want to keep the fine-grained control over CNI. This tar is really a container image, and we just want to expand whatever is in there.

I would like to tie this into Assets though, but this is an odd one - it's really more similar to other system docker images, like kube-apiserver, than it is to CNI.

chrislovecnm · 2017-09-30T19:35:20Z

upup/pkg/fi/nodeup/nodetasks/archive.go

+	}
+
+	if stateBytes == nil {
+		return nil, nil


nil, nil? What is going on here? Logging? err?

Please at least make a code comment it is not obvious to me why we are not returning err.

If Find function on a Task doesn't find an existing item, it returns nil, nil.

chrislovecnm · 2017-10-05T06:44:24Z

upup/pkg/fi/nodeup/nodetasks/archive.go

+			return fmt.Errorf("error creating directories %q: %v", targetDir, err)
+		}
+
+		args := []string{"tar", "xf", localFile, "-C", targetDir}


Does RHEL support not using the tick with tar now? https://access.redhat.com/documentation/en-US/Red_Hat_Enterprise_Linux/6/html/Security-Enhanced_Linux/sect-Security-Enhanced_Linux-Maintaining_SELinux_Labels_-Archiving_Files_with_tar.html they are still using - with there args. But I would need to login to a client system and check for certain.

Yeah, I think RHEL tar is standard GNU tar. Let's fix if we find it isn't. Hopefully we can get kops under e2e on RHEL

chrislovecnm · 2017-10-05T06:46:58Z

upup/pkg/fi/nodeup/nodetasks/archive.go

+		}
+
+		args := []string{"tar", "xf", localFile, "-C", targetDir}
+		glog.Infof("running command %s", args)


shoud we glog.V(3).Infof("running command %s", args)?

It looks like in nodeup we glog.Infof when we're making changes and glog.V(2).Infof when we're just looking for stuff. So I think glog.Infof here

chrislovecnm · 2017-10-05T06:48:54Z

upup/pkg/fi/nodeup/nodetasks/bindmount.go

+package nodetasks
+
+import (
+	"fmt"


Should we start formatting our imports? We have such differences across the project :( I know nit pick :P

My votes is for goimports style. Not sure what k/k uses.

Will fix the new files in this PR, but let's discuss that in another PR (I think we have discussion of it already in another issue - the shortcoming is that goimports doesn't remove blocks that have been added, IIRC. If there's a better tool available that would be great...)

chrislovecnm · 2017-10-05T06:51:42Z

upup/pkg/fi/nodeup/nodetasks/bindmount_test.go

+}
+
+// MockExecutor is a mock implementation of Executor
+type MockExecutor struct {


we should start a mock under /pkg :) We should be using this in other places.

RIght, I was going to move it when the second use arose :-)

chrislovecnm · 2017-10-05T06:53:19Z

upup/pkg/fi/nodeup/nodetasks/mount_disk.go

+	return fmt.Sprintf("MountDisk: %s %s->%s", s.Name, s.Device, s.Mountpoint)
+}
+
+var _ CreatesDir = &MountDiskTask{}


We are doing this in protokube as well. Should we file an issue to refactor this into pkg and have both protokube and nodeup use this code?

It's a nice idea - we could start to use tasks in protokube. But send a PR instead of opening an issue :-)

The containerized mounter is a little tricky to install, with lots of bind mounts. This code path is only hit on GCE though.

justinsb · 2017-10-08T15:42:20Z

Rebased, regenerated bazel and made the requested fixes. PTAL @chrislovecnm

chrislovecnm · 2017-10-08T19:10:58Z

/lgtm

k8s-github-robot · 2017-10-08T19:11:43Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: chrislovecnm

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~OWNERS~~ [chrislovecnm]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

k8s-github-robot · 2017-10-08T20:16:56Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2017-10-08T20:48:40Z

Automatic merge from submit-queue.

k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Sep 30, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 1, 2017

justinsb force-pushed the gce_containerized_mounter branch from 4341335 to c84beba Compare October 4, 2017 01:19

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Oct 4, 2017

justinsb force-pushed the gce_containerized_mounter branch from c84beba to e1e728c Compare October 4, 2017 01:41

chrislovecnm reviewed Oct 5, 2017

View reviewed changes

justinsb added 2 commits October 8, 2017 11:12

GCE: install containerized mounter on COS

3a38d05

The containerized mounter is a little tricky to install, with lots of bind mounts. This code path is only hit on GCE though.

Containerized mounter fixes per code review

af6a7ef

justinsb force-pushed the gce_containerized_mounter branch from e1e728c to af6a7ef Compare October 8, 2017 15:41

k8s-ci-robot assigned chrislovecnm Oct 8, 2017

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 8, 2017

k8s-github-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Oct 8, 2017

k8s-github-robot merged commit fe7ed14 into kubernetes:master Oct 8, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GCE: install containerized mounter on COS #3482

GCE: install containerized mounter on COS #3482

justinsb commented Sep 30, 2017

k8s-github-robot commented Oct 1, 2017

chrislovecnm commented Oct 5, 2017

chrislovecnm left a comment

chrislovecnm Sep 30, 2017

justinsb Oct 8, 2017

chrislovecnm Sep 30, 2017

justinsb Oct 8, 2017

chrislovecnm Sep 30, 2017

justinsb Oct 8, 2017

chrislovecnm Sep 30, 2017

justinsb Oct 8, 2017

chrislovecnm Sep 30, 2017

chrislovecnm Oct 5, 2017

justinsb Oct 8, 2017

chrislovecnm Oct 5, 2017

justinsb Oct 8, 2017

chrislovecnm Oct 5, 2017

justinsb Oct 8, 2017

chrislovecnm Oct 5, 2017

chrislovecnm Oct 5, 2017

justinsb Oct 8, 2017 •

edited

Loading

chrislovecnm Oct 5, 2017

justinsb Oct 8, 2017

chrislovecnm Oct 5, 2017

justinsb Oct 8, 2017

justinsb commented Oct 8, 2017

chrislovecnm commented Oct 8, 2017

k8s-github-robot commented Oct 8, 2017

k8s-github-robot commented Oct 8, 2017

k8s-github-robot commented Oct 8, 2017

GCE: install containerized mounter on COS #3482

GCE: install containerized mounter on COS #3482

Conversation

justinsb commented Sep 30, 2017

k8s-github-robot commented Oct 1, 2017

chrislovecnm commented Oct 5, 2017

chrislovecnm left a 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

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

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

justinsb Oct 8, 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

Choose a reason for hiding this comment

justinsb commented Oct 8, 2017

chrislovecnm commented Oct 8, 2017

k8s-github-robot commented Oct 8, 2017

k8s-github-robot commented Oct 8, 2017

k8s-github-robot commented Oct 8, 2017

justinsb Oct 8, 2017 •

edited

Loading