-
Notifications
You must be signed in to change notification settings - Fork 741
cluster: add option to specify busybox repository #1928
Conversation
add 2 options in EtcdCluster.spec: busyboxRepository. Defaults to "busybox" busyboxVersion. Defaults to "1.28.0-glibc"
Can one of the admins verify this patch? |
2 similar comments
Can one of the admins verify this patch? |
Can one of the admins verify this patch? |
pkg/apis/etcd/v1beta2/cluster.go
Outdated
@@ -99,6 +101,15 @@ type ClusterSpec struct { | |||
|
|||
// etcd cluster TLS configuration | |||
TLS *TLSPolicy `json:"TLS,omitempty"` | |||
|
|||
// busybox:latest uses uclibc which contains a bug that sometimes prevents name resolution |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't bloat the top level spec.
I would suggest adding these to PodPolicy.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hongchaodeng OK. I'll do that.
Any other suggestions while i'm on it?
Move definition to PodPolicy Add unit-test
Since you changed the API, please also update the generated code: https://github.com/coreos/etcd-operator/blob/master/hack/k8s/codegen/README.md After that I can enable the test on this PR. |
@hongchaodeng Running the script did not change the generated code. Is it possible, or am I missing anything? |
What's the output of running the command? |
Generating deepcopy funcs |
@etcd-bot ok to test |
gofmt check failed:
|
fix formatting
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code looks good. Just some minor nits.
Almost dismiss the update. Please ping me when you made the change again. Thanks!
pkg/apis/etcd/v1beta2/cluster.go
Outdated
@@ -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 |
There was a problem hiding this comment.
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.
pkg/util/k8sutil/k8sutil.go
Outdated
@@ -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" |
There was a problem hiding this comment.
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?
pkg/util/k8sutil/k8sutil.go
Outdated
@@ -134,6 +136,21 @@ func ImageName(repo, version string) string { | |||
return fmt.Sprintf("%s:v%v", repo, version) | |||
} | |||
|
|||
func ImageNameBusybox(policy *api.PodPolicy) string { |
There was a problem hiding this comment.
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.
fix minor formatting issues
@hongchaodeng Fixed your comments. |
pkg/apis/etcd/v1beta2/cluster.go
Outdated
@@ -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 init container repository. default is busybox | |||
BusyboxRepository string `json:"busyboxRepository,omitempty"` |
There was a problem hiding this comment.
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".
I think it is better with two properties, as it is the same as the etcd
image and tag. If you still believe it is better to combine, I'll do it. No
problem
…On Mar 5, 2018 21:12, "Hongchao Deng" ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/apis/etcd/v1beta2/cluster.go
<#1928 (comment)>:
> @@ -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 init container repository. default is busybox
+ BusyboxRepository string `json:"busyboxRepository,omitempty"`
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".
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1928 (review)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AF2uY-s_f7XIelyvMM_c-4kOQMTFBFI7ks5tbY4lgaJpZM4SJQum>
.
|
The etcd image is a special case. We expect that most of time users would only upgrade versions, aka tags. But that's not the case here. |
Ok.
…On Mar 5, 2018 21:18, "Hongchao Deng" ***@***.***> wrote:
as it is the same as the etcd image and tag.
The etcd image is a special case. We expect that most of time users would
only upgrade versions, aka tags. But that's not the case here.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#1928 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AF2uY_oTPeev6Go09vXNYuMGAud_h3kWks5tbY9tgaJpZM4SJQum>
.
|
LGTM |
@fanminshi @hasbro17 |
pkg/util/k8sutil/k8sutil.go
Outdated
func imageNameBusybox(policy *api.PodPolicy) string { | ||
// set defaults for busybox init container | ||
image := defaultBusyboxImage | ||
|
There was a problem hiding this comment.
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
pkg/util/k8sutil/k8sutil.go
Outdated
@@ -134,6 +136,16 @@ func ImageName(repo, version string) string { | |||
return fmt.Sprintf("%s:v%v", repo, version) | |||
} | |||
|
|||
func imageNameBusybox(policy *api.PodPolicy) string { | |||
// set defaults for busybox init container |
There was a problem hiding this comment.
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
fix minor formatting issues
@fanminshi Updated the PR. |
lgtm |
@yehiyam @hongchaodeng are there any examples on how to use this to pass in an alternate repo for busybox? |
@herikwebb you can look at an example in our helm chart |
@yehiyam great, thanks! |
cluster: add option to specify busybox repository
add 2 options in EtcdCluster.spec:
busyboxRepository. Defaults to "busybox"
busyboxVersion. Defaults to "1.28.0-glibc"
for #1927