Skip to content

Set k8s normal default when assuming non aggressive probe is used #4864

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.

Sign up for GitHub

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

Closed
nak3 wants to merge 4 commits into from
Closed

Set k8s normal default when assuming non aggressive probe is used #4864

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
04d2300
Set k8s normal default when assuming non aggressive probe is used
nak3 Jul 22, 2019
6520af2
Fix typo
nak3 Jul 22, 2019
c07c184
Make test more readable
nak3 Jul 22, 2019
d2d4f55
Make loop more readable
nak3 Jul 22, 2019
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
65 changes: 41 additions & 24 deletions pkg/apis/serving/v1beta1/revision_defaults.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,51 +45,68 @@ func (rs *RevisionSpec) SetDefaults(ctx context.Context) {
}

for idx := range rs.PodSpec.Containers {
if rs.PodSpec.Containers[idx].Name == "" {
rs.PodSpec.Containers[idx].Name = cfg.Defaults.UserContainerName(ctx)
ctr := &rs.PodSpec.Containers[idx]

if ctr.Name == "" {
ctr.Name = cfg.Defaults.UserContainerName(ctx)
}

if rs.PodSpec.Containers[idx].Resources.Requests == nil {
rs.PodSpec.Containers[idx].Resources.Requests = corev1.ResourceList{}
if ctr.Resources.Requests == nil {
ctr.Resources.Requests = corev1.ResourceList{}
}
if _, ok := rs.PodSpec.Containers[idx].Resources.Requests[corev1.ResourceCPU]; !ok {
if _, ok := ctr.Resources.Requests[corev1.ResourceCPU]; !ok {
if rsrc := cfg.Defaults.RevisionCPURequest; rsrc != nil {
rs.PodSpec.Containers[idx].Resources.Requests[corev1.ResourceCPU] = *rsrc
ctr.Resources.Requests[corev1.ResourceCPU] = *rsrc
}
}
if _, ok := rs.PodSpec.Containers[idx].Resources.Requests[corev1.ResourceMemory]; !ok {
if _, ok := ctr.Resources.Requests[corev1.ResourceMemory]; !ok {
if rsrc := cfg.Defaults.RevisionMemoryRequest; rsrc != nil {
rs.PodSpec.Containers[idx].Resources.Requests[corev1.ResourceMemory] = *rsrc
ctr.Resources.Requests[corev1.ResourceMemory] = *rsrc
}
}

if rs.PodSpec.Containers[idx].Resources.Limits == nil {
rs.PodSpec.Containers[idx].Resources.Limits = corev1.ResourceList{}
if ctr.Resources.Limits == nil {
ctr.Resources.Limits = corev1.ResourceList{}
}
if _, ok := rs.PodSpec.Containers[idx].Resources.Limits[corev1.ResourceCPU]; !ok {
if _, ok := ctr.Resources.Limits[corev1.ResourceCPU]; !ok {
if rsrc := cfg.Defaults.RevisionCPULimit; rsrc != nil {
rs.PodSpec.Containers[idx].Resources.Limits[corev1.ResourceCPU] = *rsrc
ctr.Resources.Limits[corev1.ResourceCPU] = *rsrc
}
}
if _, ok := rs.PodSpec.Containers[idx].Resources.Limits[corev1.ResourceMemory]; !ok {
if _, ok := ctr.Resources.Limits[corev1.ResourceMemory]; !ok {
if rsrc := cfg.Defaults.RevisionMemoryLimit; rsrc != nil {
rs.PodSpec.Containers[idx].Resources.Limits[corev1.ResourceMemory] = *rsrc
ctr.Resources.Limits[corev1.ResourceMemory] = *rsrc
}
}
if rs.PodSpec.Containers[idx].ReadinessProbe == nil {
rs.PodSpec.Containers[idx].ReadinessProbe = &corev1.Probe{}

Copy link
Copy Markdown
Contributor

@markusthoemmes markusthoemmes Jul 22, 2019

Choose a reason for hiding this comment

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

Maybe fetch a pointer to the probe once to make this a bit more readable.

readinessProbe := &rs.PodSpec.Containers[idx].ReadinessProbe

I think that works, feel free to ignore if it doesn't.

Copy link
Copy Markdown
Contributor Author

@nak3 nak3 Jul 22, 2019

Choose a reason for hiding this comment

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

Yes, it worked and it makes more readable! Thank you for the suggestion. Updated.

Copy link
Copy Markdown
Contributor

@markusthoemmes markusthoemmes Jul 22, 2019

Choose a reason for hiding this comment

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

Ugh, but now you need to derefence on every call. May I instead suggest you obtain a pointer to the container at the start of the loop?

ctr := &rs.PodSpec.Containers[idx]

You can then replace a lot of boilerplate in the whole function and simplify all of the below to ctr.ReadinessProbe... which is simple enough I think. No dereferences should be needed then.

Copy link
Copy Markdown
Contributor Author

@nak3 nak3 Jul 22, 2019

Choose a reason for hiding this comment

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

Thank you! Updated. (Sorry, I should have noticed it by myself.)

if ctr.ReadinessProbe == nil {
ctr.ReadinessProbe = &corev1.Probe{}
}
if ctr.ReadinessProbe.TCPSocket == nil &&
ctr.ReadinessProbe.HTTPGet == nil &&
ctr.ReadinessProbe.Exec == nil {
ctr.ReadinessProbe.TCPSocket = &corev1.TCPSocketAction{}
}
if rs.PodSpec.Containers[idx].ReadinessProbe.TCPSocket == nil &&
rs.PodSpec.Containers[idx].ReadinessProbe.HTTPGet == nil &&
rs.PodSpec.Containers[idx].ReadinessProbe.Exec == nil {
rs.PodSpec.Containers[idx].ReadinessProbe.TCPSocket = &corev1.TCPSocketAction{}
if ctr.ReadinessProbe.SuccessThreshold == 0 {
ctr.ReadinessProbe.SuccessThreshold = 1
}

if rs.PodSpec.Containers[idx].ReadinessProbe.SuccessThreshold == 0 {
rs.PodSpec.Containers[idx].ReadinessProbe.SuccessThreshold = 1
// If any of FailureThreshold, TimeoutSeconds or PeriodSeconds are greater than 0,
// standard k8s-style would be used so setting normal k8s default values.
if ctr.ReadinessProbe.FailureThreshold > 0 ||
ctr.ReadinessProbe.TimeoutSeconds > 0 ||
ctr.ReadinessProbe.PeriodSeconds > 0 {
if ctr.ReadinessProbe.FailureThreshold == 0 {
ctr.ReadinessProbe.FailureThreshold = 3
}
if ctr.ReadinessProbe.TimeoutSeconds == 0 {
ctr.ReadinessProbe.TimeoutSeconds = 1
}
if ctr.ReadinessProbe.PeriodSeconds == 0 {
ctr.ReadinessProbe.PeriodSeconds = 10
}
}

vms := rs.PodSpec.Containers[idx].VolumeMounts
vms := ctr.VolumeMounts
for i := range vms {
vms[i].ReadOnly = true
}
Expand Down
34 changes: 34 additions & 0 deletions pkg/apis/serving/v1beta1/revision_defaults_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -243,6 +243,40 @@ func TestRevisionDefaulting(t *testing.T) {
},
},
},
}, {
name: "defaulting non aggressive probe",
in: &Revision{
Spec: RevisionSpec{
PodSpec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: config.DefaultUserContainerName,
ReadinessProbe: &corev1.Probe{
FailureThreshold: 3,
},
}},
},
},
},
want: &Revision{
Spec: RevisionSpec{
TimeoutSeconds: ptr.Int64(config.DefaultRevisionTimeoutSeconds),
PodSpec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: config.DefaultUserContainerName,
Resources: defaultResources,
ReadinessProbe: &corev1.Probe{
TimeoutSeconds: 1,
PeriodSeconds: 10,
FailureThreshold: 3,
SuccessThreshold: 1,
Handler: corev1.Handler{
TCPSocket: &corev1.TCPSocketAction{},
},
},
}},
},
},
},
}}

for _, test := range tests {
Expand Down