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

rename user container to main container #15330

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open
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
2 changes: 1 addition & 1 deletion DEVELOPMENT.md
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
# Development
# Development
Copy link
Contributor

@skonto skonto Jul 1, 2024

Choose a reason for hiding this comment

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

revert the space?


This doc explains how to set up a development environment so you can get started
[contributing](https://www.knative.dev/contributing/) to `Knative Serving`. Also
Expand Down
20 changes: 10 additions & 10 deletions pkg/apis/config/defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -56,9 +56,9 @@ const (
// specified by the user, if `name:` is omitted.
DefaultInitContainerName = "init-container"

// DefaultUserContainerName is the default name we give to the container
// DefaultMainContainerName is the default name we give to the container
// specified by the user, if `name:` is omitted.
DefaultUserContainerName = "user-container"
DefaultMainContainerName = "user-container"
Copy link
Contributor

@skonto skonto Jul 1, 2024

Choose a reason for hiding this comment

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

Is it user-container or main-container? Are we planning to change the actual value? If so what will be the autogenerated name?


// DefaultContainerConcurrency is the default container concurrency. It will be set if ContainerConcurrency is not specified.
DefaultContainerConcurrency = 0
Expand All @@ -74,7 +74,7 @@ const (

var (
DefaultInitContainerNameTemplate = mustParseTemplate(DefaultInitContainerName)
DefaultUserContainerNameTemplate = mustParseTemplate(DefaultUserContainerName)
DefaultMainContainerNameTemplate = mustParseTemplate(DefaultMainContainerName)
)

func defaultDefaultsConfig() *Defaults {
Expand All @@ -84,7 +84,7 @@ func defaultDefaultsConfig() *Defaults {
RevisionResponseStartTimeoutSeconds: DefaultRevisionResponseStartTimeoutSeconds,
RevisionIdleTimeoutSeconds: DefaultRevisionIdleTimeoutSeconds,
InitContainerNameTemplate: DefaultInitContainerNameTemplate,
UserContainerNameTemplate: DefaultUserContainerNameTemplate,
MainContainerNameTemplate: DefaultMainContainerNameTemplate,
ContainerConcurrency: DefaultContainerConcurrency,
ContainerConcurrencyMaxLimit: DefaultMaxRevisionContainerConcurrency,
AllowContainerConcurrencyZero: DefaultAllowContainerConcurrencyZero,
Expand Down Expand Up @@ -114,7 +114,7 @@ func NewDefaultsConfigFromMap(data map[string]string) (*Defaults, error) {

if err := cm.Parse(data,
asTemplate("init-container-name-template", &nc.InitContainerNameTemplate),
asTemplate("container-name-template", &nc.UserContainerNameTemplate),
asTemplate("container-name-template", &nc.MainContainerNameTemplate),

cm.AsBool("allow-container-concurrency-zero", &nc.AllowContainerConcurrencyZero),
asTriState("enable-service-links", &nc.EnableServiceLinks, nil),
Expand Down Expand Up @@ -163,7 +163,7 @@ func NewDefaultsConfigFromMap(data map[string]string) (*Defaults, error) {
nc.ContainerConcurrency, 0, nc.ContainerConcurrencyMaxLimit, "container-concurrency")
}
// Check that the templates properly apply to ObjectMeta.
if err := nc.UserContainerNameTemplate.Execute(io.Discard, metav1.ObjectMeta{}); err != nil {
if err := nc.MainContainerNameTemplate.Execute(io.Discard, metav1.ObjectMeta{}); err != nil {
return nil, fmt.Errorf("error executing template: %w", err)
}
if err := nc.InitContainerNameTemplate.Execute(io.Discard, metav1.ObjectMeta{}); err != nil {
Expand Down Expand Up @@ -194,7 +194,7 @@ type Defaults struct {

InitContainerNameTemplate *ObjectMetaTemplate

UserContainerNameTemplate *ObjectMetaTemplate
MainContainerNameTemplate *ObjectMetaTemplate

ContainerConcurrency int64

Expand Down Expand Up @@ -226,9 +226,9 @@ func containerNameFromTemplate(ctx context.Context, tmpl *ObjectMetaTemplate) st
return buf.String()
}

// UserContainerName returns the name of the user container based on the context.
func (d Defaults) UserContainerName(ctx context.Context) string {
return containerNameFromTemplate(ctx, d.UserContainerNameTemplate)
// MainContainerName returns the name of the user container based on the context.
func (d Defaults) MainContainerName(ctx context.Context) string {
return containerNameFromTemplate(ctx, d.MainContainerNameTemplate)
}

// InitContainerName returns the name of the init container based on the context.
Expand Down
14 changes: 7 additions & 7 deletions pkg/apis/config/defaults_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -79,7 +79,7 @@ func TestDefaultsConfiguration(t *testing.T) {
RevisionIdleTimeoutSeconds: 123,
ContainerConcurrencyMaxLimit: 1984,
RevisionCPURequest: &oneTwoThree,
UserContainerNameTemplate: mustParseTemplate("{{.Name}}"),
MainContainerNameTemplate: mustParseTemplate("{{.Name}}"),
InitContainerNameTemplate: mustParseTemplate("{{.Name}}"),
EnableServiceLinks: ptr.Bool(true),
},
Expand All @@ -103,7 +103,7 @@ func TestDefaultsConfiguration(t *testing.T) {
MaxRevisionTimeoutSeconds: DefaultMaxRevisionTimeoutSeconds,
RevisionResponseStartTimeoutSeconds: DefaultRevisionResponseStartTimeoutSeconds,
InitContainerNameTemplate: DefaultInitContainerNameTemplate,
UserContainerNameTemplate: DefaultUserContainerNameTemplate,
MainContainerNameTemplate: DefaultMainContainerNameTemplate,
ContainerConcurrencyMaxLimit: DefaultMaxRevisionContainerConcurrency,
AllowContainerConcurrencyZero: true,
EnableServiceLinks: ptr.Bool(false),
Expand All @@ -119,7 +119,7 @@ func TestDefaultsConfiguration(t *testing.T) {
MaxRevisionTimeoutSeconds: DefaultMaxRevisionTimeoutSeconds,
RevisionResponseStartTimeoutSeconds: DefaultRevisionResponseStartTimeoutSeconds,
InitContainerNameTemplate: DefaultInitContainerNameTemplate,
UserContainerNameTemplate: DefaultUserContainerNameTemplate,
MainContainerNameTemplate: DefaultMainContainerNameTemplate,
ContainerConcurrencyMaxLimit: DefaultMaxRevisionContainerConcurrency,
AllowContainerConcurrencyZero: true,
EnableServiceLinks: nil,
Expand Down Expand Up @@ -214,7 +214,7 @@ func TestDefaultsConfiguration(t *testing.T) {
ContainerConcurrencyMaxLimit: DefaultMaxRevisionContainerConcurrency,
AllowContainerConcurrencyZero: DefaultAllowContainerConcurrencyZero,
EnableServiceLinks: ptr.Bool(false),
UserContainerNameTemplate: mustParseTemplate("{{.Name}}"),
MainContainerNameTemplate: mustParseTemplate("{{.Name}}"),
InitContainerNameTemplate: mustParseTemplate("my-template"),
},
data: map[string]string{
Expand All @@ -235,7 +235,7 @@ func TestDefaultsConfiguration(t *testing.T) {
AllowContainerConcurrencyZero: DefaultAllowContainerConcurrencyZero,
EnableServiceLinks: ptr.Bool(false),
InitContainerNameTemplate: DefaultInitContainerNameTemplate,
UserContainerNameTemplate: DefaultUserContainerNameTemplate,
MainContainerNameTemplate: DefaultMainContainerNameTemplate,
},
}}

Expand Down Expand Up @@ -308,8 +308,8 @@ func TestTemplating(t *testing.T) {
Namespace: "guardians",
})

if got, want := def.UserContainerName(ctx), test.want; got != want {
t.Errorf("UserContainerName() = %v, wanted %v", got, want)
if got, want := def.MainContainerName(ctx), test.want; got != want {
t.Errorf("MainContainerName() = %v, wanted %v", got, want)
}

if got, want := def.InitContainerName(ctx), test.want; got != want {
Expand Down
4 changes: 2 additions & 2 deletions pkg/apis/config/zz_generated.deepcopy.go

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

26 changes: 13 additions & 13 deletions pkg/apis/serving/k8s_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -387,7 +387,7 @@ func ValidatePodSpec(ctx context.Context, ps corev1.PodSpec) *apis.FieldError {
case 0:
errs = errs.Also(apis.ErrMissingField("containers"))
case 1:
errs = errs.Also(ValidateUserContainer(ctx, ps.Containers[0], volumes, port).
errs = errs.Also(ValidateMainContainer(ctx, ps.Containers[0], volumes, port).
ViaFieldIndex("containers", 0))
default:
errs = errs.Also(validateContainers(ctx, ps.Containers, volumes, port))
Expand Down Expand Up @@ -444,7 +444,7 @@ func validateContainers(ctx context.Context, containers []corev1.Container, volu
if len(containers[i].Ports) == 0 {
errs = errs.Also(validateSidecarContainer(WithinSidecarContainer(ctx), containers[i], volumes).ViaFieldIndex("containers", i))
} else {
errs = errs.Also(ValidateUserContainer(WithinUserContainer(ctx), containers[i], volumes, port).ViaFieldIndex("containers", i))
errs = errs.Also(ValidateMainContainer(WithinMainContainer(ctx), containers[i], volumes, port).ViaFieldIndex("containers", i))
}
}
return errs
Expand Down Expand Up @@ -550,8 +550,8 @@ func validateInitContainer(ctx context.Context, container corev1.Container, volu
return errs.Also(validate(WithinInitContainer(ctx), container, volumes))
}

// ValidateUserContainer validate fields for serving containers
func ValidateUserContainer(ctx context.Context, container corev1.Container, volumes map[string]corev1.Volume, port corev1.ContainerPort) (errs *apis.FieldError) {
// ValidateMainContainer validate fields for serving containers
func ValidateMainContainer(ctx context.Context, container corev1.Container, volumes map[string]corev1.Volume, port corev1.ContainerPort) (errs *apis.FieldError) {
// Liveness Probes
errs = errs.Also(validateProbe(container.LivenessProbe, &port, true).ViaField("livenessProbe"))
// Readiness Probes
Expand Down Expand Up @@ -757,12 +757,12 @@ func validateContainerPortBasic(port corev1.ContainerPort) *apis.FieldError {
return errs
}

func validateReadinessProbe(p *corev1.Probe, port *corev1.ContainerPort, isUserContainer bool) *apis.FieldError {
func validateReadinessProbe(p *corev1.Probe, port *corev1.ContainerPort, isMainContainer bool) *apis.FieldError {
if p == nil {
return nil
}

errs := validateProbe(p, port, isUserContainer)
errs := validateProbe(p, port, isMainContainer)

if p.PeriodSeconds < 0 {
errs = errs.Also(apis.ErrOutOfBoundsValue(p.PeriodSeconds, 0, math.MaxInt32, "periodSeconds"))
Expand Down Expand Up @@ -804,7 +804,7 @@ func validateReadinessProbe(p *corev1.Probe, port *corev1.ContainerPort, isUserC
return errs
}

func validateProbe(p *corev1.Probe, port *corev1.ContainerPort, isUserContainer bool) *apis.FieldError {
func validateProbe(p *corev1.Probe, port *corev1.ContainerPort, isMainContainer bool) *apis.FieldError {
if p == nil {
return nil
}
Expand All @@ -819,7 +819,7 @@ func validateProbe(p *corev1.Probe, port *corev1.ContainerPort, isUserContainer
handlers = append(handlers, "httpGet")
errs = errs.Also(apis.CheckDisallowedFields(*h.HTTPGet, *HTTPGetActionMask(h.HTTPGet))).ViaField("httpGet")
getPort := h.HTTPGet.Port
if isUserContainer {
if isMainContainer {
if getPort.StrVal != "" && getPort.StrVal != port.Name {
errs = errs.Also(apis.ErrInvalidValue(getPort.String(), "httpGet.port", "Probe port must match container port"))
}
Expand All @@ -833,7 +833,7 @@ func validateProbe(p *corev1.Probe, port *corev1.ContainerPort, isUserContainer
handlers = append(handlers, "tcpSocket")
errs = errs.Also(apis.CheckDisallowedFields(*h.TCPSocket, *TCPSocketActionMask(h.TCPSocket))).ViaField("tcpSocket")
tcpPort := h.TCPSocket.Port
if isUserContainer {
if isMainContainer {
if tcpPort.StrVal != "" && tcpPort.StrVal != port.Name {
errs = errs.Also(apis.ErrInvalidValue(tcpPort.String(), "tcpSocket.port", "Probe port must match container port"))
}
Expand Down Expand Up @@ -983,12 +983,12 @@ func warnDefaultContainerSecurityContext(_ context.Context, psc *corev1.PodSecur

// This is attached to contexts as they are passed down through a user container
// being validated.
type userContainer struct{}
type mainContainer struct{}

// WithinUserContainer notes on the context that further validation or defaulting
// WithinMainContainer notes on the context that further validation or defaulting
// is within the context of a user container in the revision.
func WithinUserContainer(ctx context.Context) context.Context {
return context.WithValue(ctx, userContainer{}, struct{}{})
func WithinMainContainer(ctx context.Context) context.Context {
return context.WithValue(ctx, mainContainer{}, struct{}{})
}

// This is attached to contexts as they are passed down through a sidecar container
Expand Down
6 changes: 3 additions & 3 deletions pkg/apis/serving/k8s_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -1492,7 +1492,7 @@ func TestPodSpecFieldRefValidation(t *testing.T) {
}
}

func TestUserContainerValidation(t *testing.T) {
func TestMainContainerValidation(t *testing.T) {
tests := []containerValidationTestCase{
{
name: "has a lifecycle",
Expand Down Expand Up @@ -1961,10 +1961,10 @@ func TestUserContainerValidation(t *testing.T) {
}
port, err := validateContainersPorts([]corev1.Container{test.c})

got := err.Also(ValidateUserContainer(ctx, test.c, test.volumes, port))
got := err.Also(ValidateMainContainer(ctx, test.c, test.volumes, port))
got = got.Filter(apis.ErrorLevel)
if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" {
t.Errorf("ValidateUserContainer (-want, +got): \n%s", diff)
t.Errorf("ValidateMainContainer (-want, +got): \n%s", diff)
}
})
}
Expand Down
6 changes: 3 additions & 3 deletions pkg/apis/serving/v1/configuration_defaults_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ func TestConfigurationDefaulting(t *testing.T) {
Spec: RevisionSpec{
PodSpec: corev1.PodSpec{
Containers: []corev1.Container{{
Name: config.DefaultUserContainerName,
Name: config.DefaultMainContainerName,
Image: "busybox",
Resources: defaultResources,
ReadinessProbe: defaultProbe,
Expand Down Expand Up @@ -107,7 +107,7 @@ func TestConfigurationDefaulting(t *testing.T) {
PodSpec: corev1.PodSpec{
EnableServiceLinks: ptr.Bool(false),
Containers: []corev1.Container{{
Name: config.DefaultUserContainerName,
Name: config.DefaultMainContainerName,
Image: "busybox",
Resources: defaultResources,
ReadinessProbe: defaultProbe,
Expand Down Expand Up @@ -144,7 +144,7 @@ func TestConfigurationDefaulting(t *testing.T) {
PodSpec: corev1.PodSpec{
EnableServiceLinks: ptr.Bool(true),
Containers: []corev1.Container{{
Name: config.DefaultUserContainerName,
Name: config.DefaultMainContainerName,
Image: "busybox",
Resources: defaultResources,
ReadinessProbe: defaultProbe,
Expand Down
8 changes: 4 additions & 4 deletions pkg/apis/serving/v1/revision_defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,8 @@ func (rs *RevisionSpec) SetDefaults(ctx context.Context) {
for idx := range rs.PodSpec.InitContainers {
containerNames.Insert(rs.PodSpec.InitContainers[idx].Name)
}
defaultUserContainerName := cfg.Defaults.UserContainerName(ctx)
applyDefaultContainerNames(rs.PodSpec.Containers, containerNames, defaultUserContainerName)
defaultMainContainerName := cfg.Defaults.MainContainerName(ctx)
applyDefaultContainerNames(rs.PodSpec.Containers, containerNames, defaultMainContainerName)
defaultInitContainerName := cfg.Defaults.InitContainerName(ctx)
applyDefaultContainerNames(rs.PodSpec.InitContainers, containerNames, defaultInitContainerName)
for idx := range rs.PodSpec.Containers {
Expand Down Expand Up @@ -132,7 +132,7 @@ func (rs *RevisionSpec) applyDefault(ctx context.Context, container *corev1.Cont
// If there are multiple containers then default probes will be applied to the container where user specified PORT
// default probes will not be applied for non serving containers
if len(rs.PodSpec.Containers) == 1 || len(container.Ports) != 0 {
rs.applyUserContainerDefaultReadinessProbe(container)
rs.applyMainContainerDefaultReadinessProbe(container)
}
rs.applyReadinessProbeDefaults(container)
rs.applyGRPCProbeDefaults(container)
Expand All @@ -155,7 +155,7 @@ func (rs *RevisionSpec) applyDefault(ctx context.Context, container *corev1.Cont
}
}

func (*RevisionSpec) applyUserContainerDefaultReadinessProbe(container *corev1.Container) {
func (*RevisionSpec) applyMainContainerDefaultReadinessProbe(container *corev1.Container) {
if container.ReadinessProbe == nil {
container.ReadinessProbe = &corev1.Probe{}
}
Expand Down
Loading