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

Remove outdated code from revision, etc dealing with services #3888

Merged
merged 2 commits into from
Apr 24, 2019
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
4 changes: 2 additions & 2 deletions pkg/activator/handler/handler.go
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,12 @@ import (
"github.com/knative/pkg/logging/logkey"
"github.com/knative/serving/pkg/activator"
"github.com/knative/serving/pkg/activator/util"
"github.com/knative/serving/pkg/apis/networking"
"github.com/knative/serving/pkg/apis/serving"
"github.com/knative/serving/pkg/apis/serving/v1alpha1"
pkghttp "github.com/knative/serving/pkg/http"
"github.com/knative/serving/pkg/network"
"github.com/knative/serving/pkg/queue"
"github.com/knative/serving/pkg/reconciler/revision/resources"

"go.opencensus.io/plugin/ochttp"
"go.opencensus.io/trace"
Expand Down Expand Up @@ -227,7 +227,7 @@ func (a *ActivationHandler) serviceHostName(rev *v1alpha1.Revision, serviceName
// Search for the appropriate port
port := int32(-1)
for _, p := range svc.Spec.Ports {
if p.Name == resources.ServicePortName(rev) {
if p.Name == networking.ServicePortName(rev.GetProtocol()) {
port = p.Port
break
}
Expand Down
5 changes: 2 additions & 3 deletions pkg/apis/autoscaling/v1alpha1/pa_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -87,10 +87,9 @@ type PodAutoscalerSpec struct {
// is responsible for quickly right-sizing.
ScaleTargetRef autoscalingv1.CrossVersionObjectReference `json:"scaleTargetRef"`

// ServiceName holds the name of a core Kubernetes Service resource that
// DeprecatedServiceName holds the name of a core Kubernetes Service resource that
// load balances over the pods referenced by the ScaleTargetRef.
// TODO(vagababov): deprecate.
ServiceName string `json:"serviceName"`
DeprecatedServiceName string `json:"serviceName"`

// The application-layer protocol. Matches `ProtocolType` inferred from the revision spec.
ProtocolType net.ProtocolType
Expand Down
3 changes: 0 additions & 3 deletions pkg/apis/autoscaling/v1alpha1/pa_validation.go
Original file line number Diff line number Diff line change
Expand Up @@ -71,9 +71,6 @@ func (rs *PodAutoscalerSpec) Validate(ctx context.Context) *apis.FieldError {
return apis.ErrMissingField(apis.CurrentField)
}
errs := validateReference(rs.ScaleTargetRef).ViaField("scaleTargetRef")
if rs.ServiceName == "" {
errs = errs.Also(apis.ErrMissingField("serviceName"))
}
errs = errs.Also(rs.ContainerConcurrency.Validate(ctx).
ViaField("containerConcurrency"))
return errs.Also(validateSKSFields(ctx, rs))
Expand Down
45 changes: 7 additions & 38 deletions pkg/apis/autoscaling/v1alpha1/pa_validation_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,6 @@ func TestPodAutoscalerSpecValidation(t *testing.T) {
name: "valid",
rs: &PodAutoscalerSpec{
ContainerConcurrency: 0,
ServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Expand All @@ -51,15 +50,13 @@ func TestPodAutoscalerSpecValidation(t *testing.T) {
}, {
name: "has missing scaleTargetRef",
rs: &PodAutoscalerSpec{
ContainerConcurrency: 0,
ServiceName: "foo",
ContainerConcurrency: 1,
},
want: apis.ErrMissingField("scaleTargetRef"),
}, {
name: "has missing scaleTargetRef kind",
rs: &PodAutoscalerSpec{
ContainerConcurrency: 0,
ServiceName: "foo",
ContainerConcurrency: 1,
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Name: "bar",
Expand All @@ -70,7 +67,6 @@ func TestPodAutoscalerSpecValidation(t *testing.T) {
name: "has missing scaleTargetRef apiVersion",
rs: &PodAutoscalerSpec{
ContainerConcurrency: 0,
ServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
Kind: "Deployment",
Name: "bar",
Expand All @@ -81,29 +77,16 @@ func TestPodAutoscalerSpecValidation(t *testing.T) {
name: "has missing scaleTargetRef name",
rs: &PodAutoscalerSpec{
ContainerConcurrency: 0,
ServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
},
},
want: apis.ErrMissingField("scaleTargetRef.name"),
}, {
name: "has missing serviceName",
rs: &PodAutoscalerSpec{
ContainerConcurrency: 0,
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Name: "bar",
},
},
want: apis.ErrMissingField("serviceName"),
}, {
name: "bad container concurrency",
rs: &PodAutoscalerSpec{
ContainerConcurrency: -1,
ServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Expand All @@ -116,7 +99,6 @@ func TestPodAutoscalerSpecValidation(t *testing.T) {
name: "multi invalid, bad concurrency and missing ref kind",
rs: &PodAutoscalerSpec{
ContainerConcurrency: -2,
ServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Name: "bar",
Expand All @@ -131,7 +113,7 @@ func TestPodAutoscalerSpecValidation(t *testing.T) {
t.Run(test.name, func(t *testing.T) {
got := test.rs.Validate(context.Background())
if diff := cmp.Diff(test.want.Error(), got.Error()); diff != "" {
t.Errorf("Validate (-want, +got) = %v", diff)
t.Errorf("Validate (-want, +got) = %s", diff)
}
})
}
Expand All @@ -152,7 +134,6 @@ func TestPodAutoscalerValidation(t *testing.T) {
},
},
Spec: PodAutoscalerSpec{
ServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Expand All @@ -172,7 +153,6 @@ func TestPodAutoscalerValidation(t *testing.T) {
},
},
Spec: PodAutoscalerSpec{
ServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Expand All @@ -191,7 +171,6 @@ func TestPodAutoscalerValidation(t *testing.T) {
},
},
Spec: PodAutoscalerSpec{
ServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Expand All @@ -211,7 +190,6 @@ func TestPodAutoscalerValidation(t *testing.T) {
},
},
Spec: PodAutoscalerSpec{
ServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Expand Down Expand Up @@ -239,7 +217,6 @@ func TestPodAutoscalerValidation(t *testing.T) {
},
Spec: PodAutoscalerSpec{
ContainerConcurrency: -1,
ServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Expand Down Expand Up @@ -274,7 +251,6 @@ func TestImmutableFields(t *testing.T) {
Name: "valid",
},
Spec: PodAutoscalerSpec{
ServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Expand All @@ -287,7 +263,6 @@ func TestImmutableFields(t *testing.T) {
Name: "valid",
},
Spec: PodAutoscalerSpec{
ServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Expand All @@ -303,7 +278,6 @@ func TestImmutableFields(t *testing.T) {
Name: "valid",
},
Spec: PodAutoscalerSpec{
ServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Expand All @@ -317,7 +291,6 @@ func TestImmutableFields(t *testing.T) {
Name: "valid",
},
Spec: PodAutoscalerSpec{
ServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Expand All @@ -333,7 +306,6 @@ func TestImmutableFields(t *testing.T) {
Name: "valid",
},
Spec: PodAutoscalerSpec{
ServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Expand All @@ -347,7 +319,6 @@ func TestImmutableFields(t *testing.T) {
},
Spec: PodAutoscalerSpec{
ContainerConcurrency: 1,
ServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Expand All @@ -371,7 +342,6 @@ func TestImmutableFields(t *testing.T) {
},
Spec: PodAutoscalerSpec{
ContainerConcurrency: 0,
ServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Expand All @@ -385,7 +355,6 @@ func TestImmutableFields(t *testing.T) {
},
Spec: PodAutoscalerSpec{
ContainerConcurrency: 1,
ServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Expand All @@ -408,7 +377,7 @@ func TestImmutableFields(t *testing.T) {
Name: "valid",
},
Spec: PodAutoscalerSpec{
ServiceName: "foo",
DeprecatedServiceName: "foo",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Expand All @@ -421,8 +390,8 @@ func TestImmutableFields(t *testing.T) {
Name: "valid",
},
Spec: PodAutoscalerSpec{
ContainerConcurrency: 1,
ServiceName: "food",
ContainerConcurrency: 1,
DeprecatedServiceName: "food",
ScaleTargetRef: autoscalingv1.CrossVersionObjectReference{
APIVersion: "apps/v1",
Kind: "Deployment",
Expand All @@ -439,7 +408,7 @@ func TestImmutableFields(t *testing.T) {
{v1alpha1.PodAutoscalerSpec}.ScaleTargetRef.Name:
-: "baz"
+: "bar"
{v1alpha1.PodAutoscalerSpec}.ServiceName:
{v1alpha1.PodAutoscalerSpec}.DeprecatedServiceName:
-: "food"
+: "foo"
`,
Expand Down
17 changes: 16 additions & 1 deletion pkg/apis/networking/register.go
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,9 @@ limitations under the License.

package networking

import "time"
import (
"time"
)

const (
// GroupName is the name for the networking API group.
Expand Down Expand Up @@ -48,6 +50,11 @@ const (
// ServiceTypeKey is the label key attached to a service specifying the type of service.
// e.g. Public, Metrics
ServiceTypeKey = GroupName + "/serviceType"

// ServicePortNameHTTP1 is the name of the external port of the service for HTTP/1.1
ServicePortNameHTTP1 = "http"
// ServicePortNameH2C is the name of the external port of the service for HTTP/2
ServicePortNameH2C = "http2"
)

// ServiceType is the enumeration type for the Kubernetes services
Expand All @@ -74,3 +81,11 @@ var (
// DefaultRetryCount will be set if Attempts not specified.
DefaultRetryCount = 3
)

// ServicePortName returns the port for the app level protocol.
func ServicePortName(proto ProtocolType) string {
if proto == ProtocolH2C {
return ServicePortNameH2C
}
return ServicePortNameHTTP1
}
1 change: 0 additions & 1 deletion pkg/reconciler/autoscaling/hpa/hpa_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -439,7 +439,6 @@ func pa(name, namespace string, options ...PodAutoscalerOption) *autoscalingv1al
Kind: "Deployment",
Name: name + "-deployment",
},
ServiceName: name + "-service",
ProtocolType: networking.ProtocolHTTP1,
},
}
Expand Down
26 changes: 0 additions & 26 deletions pkg/reconciler/autoscaling/kpa/kpa_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -104,7 +104,6 @@ func TestMetricsSvcIsReconciled(t *testing.T) {
defer logtesting.ClearAll()

rev := newTestRevision(testNamespace, testRevision)
ep := addEndpoint(makeEndpoints(rev))
kpa := revisionresources.MakeKPA(rev)
tests := []struct {
name string
Expand Down Expand Up @@ -283,8 +282,6 @@ func TestMetricsSvcIsReconciled(t *testing.T) {

servingClient.ServingV1alpha1().Revisions(testNamespace).Create(rev)
servingInformer.Serving().V1alpha1().Revisions().Informer().GetIndexer().Add(rev)
kubeClient.CoreV1().Endpoints(testNamespace).Create(ep)
kubeInformer.Core().V1().Endpoints().Informer().GetIndexer().Add(ep)
servingClient.AutoscalingV1alpha1().PodAutoscalers(testNamespace).Create(kpa)
servingInformer.Autoscaling().V1alpha1().PodAutoscalers().Informer().GetIndexer().Add(kpa)

Expand Down Expand Up @@ -386,15 +383,6 @@ func markResourceNotOwned(rType, name string) PodAutoscalerOption {
}
}

func makeTestEndpoints(num int, ns, n string) *corev1.Endpoints {
rev := newTestRevision(ns, n)
eps := makeEndpoints(rev)
for i := 0; i < num; i++ {
eps = addEndpoint(eps)
}
return eps
}

func TestReconcileAndScaleToZero(t *testing.T) {
// This test suite uses special decider that will
// force KPA to scale to 0.
Expand Down Expand Up @@ -1116,10 +1104,6 @@ func TestEmptyEndpoints(t *testing.T) {
rev := newTestRevision(testNamespace, testRevision)
servingClient.ServingV1alpha1().Revisions(testNamespace).Create(rev)
servingInformer.Serving().V1alpha1().Revisions().Informer().GetIndexer().Add(rev)
// This is empty still.
ep := makeEndpoints(rev)
kubeClient.CoreV1().Endpoints(testNamespace).Create(ep)
kubeInformer.Core().V1().Endpoints().Informer().GetIndexer().Add(ep)
kpa := revisionresources.MakeKPA(rev)
servingClient.AutoscalingV1alpha1().PodAutoscalers(testNamespace).Create(kpa)
servingInformer.Autoscaling().V1alpha1().PodAutoscalers().Informer().GetIndexer().Add(kpa)
Expand Down Expand Up @@ -1503,16 +1487,6 @@ func makeSKSPrivateEndpoints(num int, ns, n string) *corev1.Endpoints {
return eps
}

func makeEndpoints(rev *v1alpha1.Revision) *corev1.Endpoints {
service := revisionresources.MakeK8sService(rev)
return &corev1.Endpoints{
ObjectMeta: metav1.ObjectMeta{
Namespace: service.Namespace,
Name: service.Name,
},
}
}

func addEndpoint(ep *corev1.Endpoints) *corev1.Endpoints {
ep.Subsets = []corev1.EndpointSubset{{
Addresses: []corev1.EndpointAddress{{IP: "127.0.0.1"}},
Expand Down
1 change: 0 additions & 1 deletion pkg/reconciler/autoscaling/kpa/resources/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,6 @@ func TestMakeService(t *testing.T) {
Kind: "Deployment",
Name: "with-you",
},
ServiceName: "with-you-service",
},
}
selector := map[string]string{"cant": "stop"}
Expand Down
5 changes: 0 additions & 5 deletions pkg/reconciler/revision/resources/constants.go
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,6 @@ const (

autoscalerPort = 8080

// ServicePortNameHTTP1 is the name of the external port of the service for HTTP/1.1
ServicePortNameHTTP1 = "http"
// ServicePortNameH2C is the name of the external port of the service for HTTP/2
ServicePortNameH2C = "http2"

// ServicePort is the external port of the service
ServicePort = int32(80)
AppLabelKey = "app"
Expand Down
3 changes: 1 addition & 2 deletions pkg/reconciler/revision/resources/kpa.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,8 +48,7 @@ func MakeKPA(rev *v1alpha1.Revision) *kpa.PodAutoscaler {
Kind: "Deployment",
Name: names.Deployment(rev),
},
ServiceName: names.K8sService(rev),
ProtocolType: rev.GetProtocol(),
ProtocolType: rev.GetProtocol(),
},
}
}
Loading