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

enable build pods to tolerate running on crio while talking to docker #16491

Merged
merged 3 commits into from
Sep 28, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
15 changes: 15 additions & 0 deletions Godeps/Godeps.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

10 changes: 8 additions & 2 deletions pkg/build/builder/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -305,9 +305,15 @@ func (d *DockerBuilder) dockerBuild(dir string, tag string, secrets []buildapi.S
NoCache: noCache,
Pull: forcePull,
BuildArgs: buildArgs,
NetworkMode: string(getDockerNetworkMode()),
}

network, resolvConfHostPath, err := getContainerNetworkConfig()
if err != nil {
return err
}
opts.NetworkMode = network
if len(resolvConfHostPath) != 0 {
opts.BuildBinds = fmt.Sprintf("[\"%s:/etc/resolv.conf\"]", resolvConfHostPath)
}
// Though we are capped on memory and cpu at the cgroup parent level,
// some build containers care what their memory limit is so they can
// adapt, thus we need to set the memory limit at the container level
Expand Down
11 changes: 10 additions & 1 deletion pkg/build/builder/sti.go
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,11 @@ func (s *S2IBuilder) Build() error {
}
}

networkMode, resolvConfHostPath, err := getContainerNetworkConfig()
if err != nil {
return err
}

config := &s2iapi.Config{
// Save some processing time by not cleaning up (the container will go away anyway)
PreserveWorkingDir: true,
Expand All @@ -182,7 +187,7 @@ func (s *S2IBuilder) Build() error {

Environment: buildEnvVars(s.build, sourceInfo),
Labels: s2iBuildLabels(s.build, sourceInfo),
DockerNetworkMode: getDockerNetworkMode(),
DockerNetworkMode: s2iapi.DockerNetworkMode(networkMode),

Source: &s2igit.URL{URL: url.URL{Path: srcDir}, Type: s2igit.URLTypeLocal},
ContextDir: contextDir,
Expand All @@ -197,6 +202,10 @@ func (s *S2IBuilder) Build() error {
BlockOnBuild: true,
}

if len(resolvConfHostPath) != 0 {
config.BuildVolumes = []string{fmt.Sprintf("%s:/etc/resolv.conf", resolvConfHostPath)}
}

if s.build.Spec.Strategy.SourceStrategy.ForcePull {
glog.V(4).Infof("With force pull true, setting policies to %s", s2iapi.PullAlways)
config.BuilderPullPolicy = s2iapi.PullAlways
Expand Down
55 changes: 43 additions & 12 deletions pkg/build/builder/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package builder

import (
"bufio"
"errors"
"fmt"
"io"
"io/ioutil"
Expand All @@ -11,6 +12,10 @@ import (
"strconv"
"strings"

"github.com/google/cadvisor/container/crio"
crioclient "github.com/kubernetes-incubator/cri-o/client"
"github.com/kubernetes-incubator/cri-o/pkg/annotations"

docker "github.com/fsouza/go-dockerclient"

s2iapi "github.com/openshift/source-to-image/pkg/api"
Expand All @@ -20,17 +25,22 @@ import (

var (
// procCGroupPattern is a regular expression that parses the entries in /proc/self/cgroup
procCGroupPattern = regexp.MustCompile(`\d+:([a-z_,]+):/.*/(docker-|)([a-z0-9]+).*`)
procCGroupPattern = regexp.MustCompile(`\d+:([a-z_,]+):/.*/(\w+-|)([a-z0-9]+).*`)
)

// readNetClsCGroup parses /proc/self/cgroup in order to determine the container id that can be used
// the network namespace that this process is running on.
func readNetClsCGroup(reader io.Reader) string {
cgroups := make(map[string]string)
// the network namespace that this process is running on, it returns the cgroup and container type
// (docker vs crio).
func readNetClsCGroup(reader io.Reader) (string, string) {

containerType := "docker"

cgroups := make(map[string]string)
scanner := bufio.NewScanner(reader)
for scanner.Scan() {
if match := procCGroupPattern.FindStringSubmatch(scanner.Text()); match != nil {
containerType = strings.TrimSuffix(match[2], "-")

list := strings.Split(match[1], ",")
containerId := match[3]
if len(list) > 0 {
Expand All @@ -46,26 +56,47 @@ func readNetClsCGroup(reader io.Reader) string {
names := []string{"net_cls", "cpu"}
for _, group := range names {
if value, ok := cgroups[group]; ok {
return value
return value, containerType
}
}

return ""
return "", containerType
}

// getDockerNetworkMode determines whether the builder is running as a container
// getContainerNetworkConfig determines whether the builder is running as a container
// by examining /proc/self/cgroup. This context is then passed to source-to-image.
func getDockerNetworkMode() s2iapi.DockerNetworkMode {
// It returns a suitable argument for NetworkMode. If the container platform is
// CRI-O, it also returns a path for /etc/resolv.conf, suitable for bindmounting.
func getContainerNetworkConfig() (string, string, error) {
file, err := os.Open("/proc/self/cgroup")
if err != nil {
return ""
return "", "", err
}
defer file.Close()

if id := readNetClsCGroup(file); id != "" {
return s2iapi.NewDockerNetworkModeContainer(id)
if id, containerType := readNetClsCGroup(file); id != "" {
glog.V(5).Infof("container type=%s", containerType)
if containerType != "crio" {
return s2iapi.DockerNetworkModeContainerPrefix + id, "", nil
}

crioClient, err := crioclient.New(crio.CrioSocket)
if err != nil {
return "", "", err
}
info, err := crioClient.ContainerInfo(id)
if err != nil {
return "", "", err
}
pid := strconv.Itoa(info.Pid)
resolvConfHostPath := info.CrioAnnotations[annotations.ResolvPath]
if len(resolvConfHostPath) == 0 {
return "", "", errors.New("/etc/resolv.conf hostpath is empty")
}

return fmt.Sprintf("netns:/proc/%s/ns/net", pid), resolvConfHostPath, nil
}
return ""
return "", "", nil
}

// GetCGroupLimits returns a struct populated with cgroup limit values gathered
Expand Down
4 changes: 2 additions & 2 deletions pkg/build/builder/util_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ func TestCGroups_CentOS7_Docker1_7(t *testing.T) {
1:name=systemd:/system.slice/docker.service
`
buffer := bytes.NewBufferString(example)
containerId := readNetClsCGroup(buffer)
containerId, _ := readNetClsCGroup(buffer)

if containerId != "5617ed7e7e487d2c4dd2e013e361109b4eceabfe3fa8c7aea9e37498b1aed5fa" {
t.Errorf("got %s, expected 5617ed7e7e487d2c4dd2e013e361109b4eceabfe3fa8c7aea9e37498b1aed5fa", containerId)
Expand All @@ -38,7 +38,7 @@ func TestCGroups_Ubuntu_Docker1_9(t *testing.T) {
3:cpuset:/docker/bfea6eb2d60179355e370a5d277d496eb0fe75d9a5a47c267221e87dbbbbc93b
2:name=systemd:/`
buffer := bytes.NewBufferString(example)
containerId := readNetClsCGroup(buffer)
containerId, _ := readNetClsCGroup(buffer)

if containerId != "bfea6eb2d60179355e370a5d277d496eb0fe75d9a5a47c267221e87dbbbbc93b" {
t.Errorf("got %s, expected bfea6eb2d60179355e370a5d277d496eb0fe75d9a5a47c267221e87dbbbbc93b", containerId)
Expand Down
1 change: 1 addition & 0 deletions pkg/build/controller/strategy/docker.go
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,7 @@ func (bs *DockerBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e

setOwnerReference(pod, build)
setupDockerSocket(pod)
setupCrioSocket(pod)
setupDockerSecrets(pod, &pod.Spec.Containers[0], build.Spec.Output.PushSecret, strategy.PullSecret, build.Spec.Source.Images)
// For any secrets the user wants to reference from their Assemble script or Dockerfile, mount those
// secrets into the main container. The main container includes logic to copy them from the mounted
Expand Down
14 changes: 8 additions & 6 deletions pkg/build/controller/strategy/docker_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import (
"strings"
"testing"

"github.com/google/cadvisor/container/crio"

"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -63,20 +65,20 @@ func TestDockerCreateBuildPod(t *testing.T) {
t.Errorf("Expected environment keys:\n%v\ngot keys\n%v", expectedKeys, gotKeys)
}

// the pod has 5 volumes but the git source secret is not mounted into the main container.
if len(container.VolumeMounts) != 4 {
t.Fatalf("Expected 4 volumes in container, got %d", len(container.VolumeMounts))
// the pod has 6 volumes but the git source secret is not mounted into the main container.
if len(container.VolumeMounts) != 5 {
t.Fatalf("Expected 5 volumes in container, got %d", len(container.VolumeMounts))
}
if *actual.Spec.ActiveDeadlineSeconds != 60 {
t.Errorf("Expected ActiveDeadlineSeconds 60, got %d", *actual.Spec.ActiveDeadlineSeconds)
}
for i, expected := range []string{buildutil.BuildWorkDirMount, dockerSocketPath, DockerPushSecretMountPath, DockerPullSecretMountPath} {
for i, expected := range []string{buildutil.BuildWorkDirMount, dockerSocketPath, crio.CrioSocket, DockerPushSecretMountPath, DockerPullSecretMountPath} {
if container.VolumeMounts[i].MountPath != expected {
t.Fatalf("Expected %s in VolumeMount[%d], got %s", expected, i, container.VolumeMounts[i].MountPath)
}
}
if len(actual.Spec.Volumes) != 5 {
t.Fatalf("Expected 5 volumes in Build pod, got %d", len(actual.Spec.Volumes))
if len(actual.Spec.Volumes) != 6 {
t.Fatalf("Expected 6 volumes in Build pod, got %d", len(actual.Spec.Volumes))
}
if !kapihelper.Semantic.DeepEqual(container.Resources, util.CopyApiResourcesToV1Resources(&build.Spec.Resources)) {
t.Fatalf("Expected actual=expected, %v != %v", container.Resources, build.Spec.Resources)
Expand Down
1 change: 1 addition & 0 deletions pkg/build/controller/strategy/sti.go
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ func (bs *SourceBuildStrategy) CreateBuildPod(build *buildapi.Build) (*v1.Pod, e

setOwnerReference(pod, build)
setupDockerSocket(pod)
setupCrioSocket(pod)
Copy link
Member

Choose a reason for hiding this comment

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

Should there be a check to add this only if the socket source path exists?

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 didn't see a good way to do that(after all this logic is running in a controller that doesn't know anything about the target node for the build pod) and it doesn't do any harm to do the mount even if the source path doesn't exist.

Copy link
Member

Choose a reason for hiding this comment

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

So, the controller doesn't add the mount to the pod if it can't find the source? If the mount is passed down to the runc config.json, it will fail to start the container if it can't find the source.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the controller adds the mount. if runc is the container engine, then the source will be there.

and docker seemingly does not care if it does not exist, because this is working when i run using docker as the container engine.

setupDockerSecrets(pod, &pod.Spec.Containers[0], build.Spec.Output.PushSecret, strategy.PullSecret, build.Spec.Source.Images)
// For any secrets the user wants to reference from their Assemble script or Dockerfile, mount those
// secrets into the main container. The main container includes logic to copy them from the mounted
Expand Down
14 changes: 8 additions & 6 deletions pkg/build/controller/strategy/sti_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
"strings"
"testing"

"github.com/google/cadvisor/container/crio"

"k8s.io/apimachinery/pkg/api/resource"
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
"k8s.io/apimachinery/pkg/runtime"
Expand Down Expand Up @@ -98,17 +100,17 @@ func testSTICreateBuildPod(t *testing.T, rootAllowed bool) {
t.Errorf("Expected environment keys:\n%v\ngot keys\n%v", expectedKeys, gotKeys)
}

// the pod has 5 volumes but the git source secret is not mounted into the main container.
if len(container.VolumeMounts) != 4 {
t.Fatalf("Expected 4 volumes in container, got %d", len(container.VolumeMounts))
// the pod has 6 volumes but the git source secret is not mounted into the main container.
if len(container.VolumeMounts) != 5 {
t.Fatalf("Expected 5 volumes in container, got %d", len(container.VolumeMounts))
}
for i, expected := range []string{buildutil.BuildWorkDirMount, dockerSocketPath, DockerPushSecretMountPath, DockerPullSecretMountPath} {
for i, expected := range []string{buildutil.BuildWorkDirMount, dockerSocketPath, crio.CrioSocket, DockerPushSecretMountPath, DockerPullSecretMountPath} {
if container.VolumeMounts[i].MountPath != expected {
t.Fatalf("Expected %s in VolumeMount[%d], got %s", expected, i, container.VolumeMounts[i].MountPath)
}
}
if len(actual.Spec.Volumes) != 5 {
t.Fatalf("Expected 5 volumes in Build pod, got %d", len(actual.Spec.Volumes))
if len(actual.Spec.Volumes) != 6 {
t.Fatalf("Expected 6 volumes in Build pod, got %d", len(actual.Spec.Volumes))
}
if *actual.Spec.ActiveDeadlineSeconds != 60 {
t.Errorf("Expected ActiveDeadlineSeconds 60, got %d", *actual.Spec.ActiveDeadlineSeconds)
Expand Down
27 changes: 26 additions & 1 deletion pkg/build/controller/strategy/util.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,12 +6,14 @@ import (
"strconv"
"strings"

"github.com/golang/glog"
"github.com/google/cadvisor/container/crio"

metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
kvalidation "k8s.io/apimachinery/pkg/util/validation"
kapi "k8s.io/kubernetes/pkg/api"
"k8s.io/kubernetes/pkg/api/v1"

"github.com/golang/glog"
"github.com/openshift/origin/pkg/api/apihelpers"
buildapi "github.com/openshift/origin/pkg/build/apis/build"
buildapiv1 "github.com/openshift/origin/pkg/build/apis/build/v1"
Expand Down Expand Up @@ -91,6 +93,29 @@ func setupDockerSocket(pod *v1.Pod) {
}
}

// setupCrioSocket configures the pod to support the host's Crio socket
func setupCrioSocket(pod *v1.Pod) {
crioSocketVolume := v1.Volume{
Name: "crio-socket",
VolumeSource: v1.VolumeSource{
HostPath: &v1.HostPathVolumeSource{
Path: crio.CrioSocket,
},
},
}

crioSocketVolumeMount := v1.VolumeMount{
Name: "crio-socket",
MountPath: crio.CrioSocket,
}

pod.Spec.Volumes = append(pod.Spec.Volumes,
crioSocketVolume)
pod.Spec.Containers[0].VolumeMounts =
append(pod.Spec.Containers[0].VolumeMounts,
crioSocketVolumeMount)
}

// mountSecretVolume is a helper method responsible for actual mounting secret
// volumes into a pod.
func mountSecretVolume(pod *v1.Pod, container *v1.Container, secretName, mountPath, volumeSuffix string) {
Expand Down

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading