Skip to content
This repository has been archived by the owner on Mar 28, 2020. It is now read-only.

cluster: add option to specify busybox repository #1928

Merged
merged 6 commits into from
Mar 6, 2018
Merged
Show file tree
Hide file tree
Changes from 3 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
8 changes: 8 additions & 0 deletions pkg/apis/etcd/v1beta2/cluster.go
Original file line number Diff line number Diff line change
Expand Up @@ -143,6 +143,14 @@ type PodPolicy struct {
// etcd cluster.
// The "etcd.version" annotation is reserved for the internal use of the etcd operator.
Annotations map[string]string `json:"annotations,omitempty"`

// busybox:latest uses uclibc which contains a bug that sometimes prevents name resolution
Copy link
Member

Choose a reason for hiding this comment

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

Can you put these comment below on the BusyboxVersion comment? They are more related there.

// More info: https://github.com/docker-library/busybox/issues/27
// busybox init container repository. default is busybox
BusyboxRepository string `json:"busyboxRepository,omitempty"`
Copy link
Member

Choose a reason for hiding this comment

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

Sorry to bother gain.
But after a second though, it would be better and easier to combine BusyboxRepository and BusyboxVersion into one field, e.g. BusyboxImage. Default to "busybox:1.28.0-glibc".


// busybox init container version. defaults to 1.28.0-glibc
BusyboxVersion string `json:"busyboxVersion,omitempty"`
}

// TODO: move this to initializer
Expand Down
22 changes: 20 additions & 2 deletions pkg/util/k8sutil/k8sutil.go
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,9 @@ const (
// AnnotationScope annotation name for defining instance scope. Used for specifing cluster wide clusters.
AnnotationScope = "etcd.database.coreos.com/scope"
//AnnotationClusterWide annotation value for cluster wide clusters.
AnnotationClusterWide = "clusterwide"
AnnotationClusterWide = "clusterwide"
defaultBusyboxRepository = "busybox"
Copy link
Member

Choose a reason for hiding this comment

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

Add a blank line between public and private var?

defaultBusyboxVersion = "1.28.0-glibc"
)

const TolerateUnreadyEndpointsAnnotation = "service.alpha.kubernetes.io/tolerate-unready-endpoints"
Expand Down Expand Up @@ -134,6 +136,21 @@ func ImageName(repo, version string) string {
return fmt.Sprintf("%s:v%v", repo, version)
}

func ImageNameBusybox(policy *api.PodPolicy) string {
Copy link
Member

Choose a reason for hiding this comment

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

Make this method private by uncapitalize it imageName...
It is not used outside this package.

// set defaults for busybox init container
Copy link
Contributor

@fanminshi fanminshi Mar 5, 2018

Choose a reason for hiding this comment

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

move the comment on top of imageNameBusybox?
and change the comment to be imageNameBusybox sets the default image for busybox init container

repo := defaultBusyboxRepository
version := defaultBusyboxVersion

if policy != nil && len(policy.BusyboxRepository) > 0 {
repo = policy.BusyboxRepository
}

Copy link
Contributor

Choose a reason for hiding this comment

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

no need to have a newline here.
also is the following simpler?

if policy != nil && len(policy.BusyboxImage) > 0 {
		return policy.BusyboxImage
}
return defaultBusyboxImage

if policy != nil && len(policy.BusyboxVersion) > 0 {
version = policy.BusyboxVersion
}
return fmt.Sprintf("%s:%v", repo, version)
}

func PodWithNodeSelector(p *v1.Pod, ns map[string]string) *v1.Pod {
p.Spec.NodeSelector = ns
return p
Expand Down Expand Up @@ -355,7 +372,8 @@ func newEtcdPod(m *etcdutil.Member, initialCluster []string, clusterName, state,
InitContainers: []v1.Container{{
// busybox:latest uses uclibc which contains a bug that sometimes prevents name resolution
// More info: https://github.com/docker-library/busybox/issues/27
Image: "busybox:1.28.0-glibc",
//Image default: "busybox:1.28.0-glibc",
Image: ImageNameBusybox(cs.Pod),
Name: "check-dns",
// In etcd 3.2, TLS listener will do a reverse-DNS lookup for pod IP -> hostname.
// If DNS entry is not warmed up, it will return empty result and peer connection will be rejected.
Expand Down
51 changes: 51 additions & 0 deletions pkg/util/k8sutil/k8sutils_test.go
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Copyright 2017 The etcd-operator Authors
//
// Licensed under the Apache License, Version 2.0 (the "License");
// you may not use this file except in compliance with the License.
// You may obtain a copy of the License at
//
// http://www.apache.org/licenses/LICENSE-2.0
//
// Unless required by applicable law or agreed to in writing, software
// distributed under the License is distributed on an "AS IS" BASIS,
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
// See the License for the specific language governing permissions and
// limitations under the License.

package k8sutil

import (
"fmt"
"testing"

api "github.com/coreos/etcd-operator/pkg/apis/etcd/v1beta2"
)

func TestDefaultBusyboxImageName(t *testing.T) {
policy := &api.PodPolicy{}
image := ImageNameBusybox(policy)
expected := fmt.Sprintf("%s:%v", defaultBusyboxRepository, defaultBusyboxVersion)
if image != expected {
t.Errorf("expect image=%s, get=%s", expected, image)
}
}

func TestDefaultNilBusyboxImageName(t *testing.T) {
image := ImageNameBusybox(nil)
expected := fmt.Sprintf("%s:%v", defaultBusyboxRepository, defaultBusyboxVersion)
if image != expected {
t.Errorf("expect image=%s, get=%s", expected, image)
}
}

func TestSetBusyboxImageName(t *testing.T) {
policy := &api.PodPolicy{
BusyboxVersion: "1.3.2",
BusyboxRepository: "myRepo/busybox",
}
image := ImageNameBusybox(policy)
expected := fmt.Sprintf("%s:%v", "myRepo/busybox", "1.3.2")
if image != expected {
t.Errorf("expect image=%s, get=%s", expected, image)
}
}