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

Implemented a second service for the collector #339

Merged
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
2 changes: 0 additions & 2 deletions deploy/examples/business-application-injected-sidecar.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -16,5 +16,3 @@ spec:
containers:
- name: myapp
image: jaegertracing/vertx-create-span:operator-e2e-tests


2 changes: 1 addition & 1 deletion pkg/deployment/agent.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ func (a *Agent) Get() *appsv1.DaemonSet {

// we only add the grpc host if we are adding the reporter type and there's no explicit value yet
if len(util.FindItem("--reporter.grpc.host-port=", args)) == 0 {
args = append(args, fmt.Sprintf("--reporter.grpc.host-port=dns:///%s.%s:14250", service.GetNameForCollectorService(a.jaeger), a.jaeger.Namespace))
args = append(args, fmt.Sprintf("--reporter.grpc.host-port=dns:///%s.%s:14250", service.GetNameForHeadlessCollectorService(a.jaeger), a.jaeger.Namespace))
}
}

Expand Down
5 changes: 2 additions & 3 deletions pkg/deployment/all-in-one.go
Original file line number Diff line number Diff line change
Expand Up @@ -175,11 +175,10 @@ func (a *AllInOne) Get() *appsv1.Deployment {
// Services returns a list of services to be deployed along with the all-in-one deployment
func (a *AllInOne) Services() []*corev1.Service {
labels := a.labels()
return []*corev1.Service{
service.NewCollectorService(a.jaeger, labels),
return append(service.NewCollectorServices(a.jaeger, labels),
service.NewQueryService(a.jaeger, labels),
service.NewAgentService(a.jaeger, labels),
}
)
}

func (a *AllInOne) labels() map[string]string {
Expand Down
2 changes: 1 addition & 1 deletion pkg/deployment/all-in-one_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ func TestAllInOneHasOwner(t *testing.T) {
func TestAllInOneNumberOfServices(t *testing.T) {
name := "TestNumberOfServices"
services := NewAllInOne(v1.NewJaeger(name)).Services()
assert.Len(t, services, 3) // collector, query, agent
assert.Len(t, services, 4) // collector (headless and cluster IP), query, agent

for _, svc := range services {
owners := svc.ObjectMeta.OwnerReferences
Expand Down
4 changes: 1 addition & 3 deletions pkg/deployment/collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -169,9 +169,7 @@ func (c *Collector) Get() *appsv1.Deployment {

// Services returns a list of services to be deployed along with the all-in-one deployment
func (c *Collector) Services() []*corev1.Service {
return []*corev1.Service{
service.NewCollectorService(c.jaeger, c.labels()),
}
return service.NewCollectorServices(c.jaeger, c.labels())
}

func (c *Collector) labels() map[string]string {
Expand Down
2 changes: 1 addition & 1 deletion pkg/deployment/collector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ func TestName(t *testing.T) {
func TestCollectorServices(t *testing.T) {
collector := NewCollector(v1.NewJaeger("TestName"))
svcs := collector.Services()
assert.Len(t, svcs, 1)
assert.Len(t, svcs, 2) // headless and cluster IP
}

func TestDefaultCollectorImage(t *testing.T) {
Expand Down
2 changes: 1 addition & 1 deletion pkg/inject/sidecar.go
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ func container(jaeger *v1.Jaeger) corev1.Container {

// we only add the grpc host if we are adding the reporter type and there's no explicit value yet
if len(util.FindItem("--reporter.grpc.host-port=", args)) == 0 {
args = append(args, fmt.Sprintf("--reporter.grpc.host-port=dns:///%s.%s:14250", service.GetNameForCollectorService(jaeger), jaeger.Namespace))
args = append(args, fmt.Sprintf("--reporter.grpc.host-port=dns:///%s.%s:14250", service.GetNameForHeadlessCollectorService(jaeger), jaeger.Namespace))
}
}

Expand Down
7 changes: 6 additions & 1 deletion pkg/inventory/service.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,13 +14,18 @@ type Service struct {
// ForServices builds an inventory of services based on the existing and desired states
func ForServices(existing []v1.Service, desired []v1.Service) Service {
update := []v1.Service{}
mcreate := serviceMap(desired)
mdelete := serviceMap(existing)
mcreate := serviceMap(desired)

for k, v := range mcreate {
if t, ok := mdelete[k]; ok {
tp := t.DeepCopy()

// we keep the ClusterIP that got assigned by the cluster, if it's empty in the "desired" and not empty on the "current"
if v.Spec.ClusterIP == "" && len(tp.Spec.ClusterIP) > 0 {
v.Spec.ClusterIP = tp.Spec.ClusterIP
}

// we can't blindly DeepCopyInto, so, we select what we bring from the new to the old object
tp.Spec = v.Spec
tp.ObjectMeta.OwnerReferences = v.ObjectMeta.OwnerReferences
Expand Down
4 changes: 4 additions & 0 deletions pkg/inventory/service_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ func TestServiceInventory(t *testing.T) {
},
Spec: v1.ServiceSpec{
ExternalName: "v1.example.com",
ClusterIP: "10.97.132.43", // got assigned by Kubernetes
},
}
updated := v1.Service{
Expand All @@ -28,6 +29,7 @@ func TestServiceInventory(t *testing.T) {
},
Spec: v1.ServiceSpec{
ExternalName: "v2.example.com",
ClusterIP: "", // will get assigned by Kubernetes
},
}
toDelete := v1.Service{
Expand All @@ -49,4 +51,6 @@ func TestServiceInventory(t *testing.T) {

assert.Len(t, inv.Delete, 1)
assert.Equal(t, "to-delete", inv.Delete[0].Name)

assert.Equal(t, toUpdate.Spec.ClusterIP, inv.Update[0].Spec.ClusterIP)
}
31 changes: 27 additions & 4 deletions pkg/service/collector.go
Original file line number Diff line number Diff line change
Expand Up @@ -9,10 +9,27 @@ import (
"github.com/jaegertracing/jaeger-operator/pkg/apis/jaegertracing/v1"
)

// NewCollectorService returns a new Kubernetes service for Jaeger Collector backed by the pods matching the selector
func NewCollectorService(jaeger *v1.Jaeger, selector map[string]string) *corev1.Service {
trueVar := true
// NewCollectorServices returns a new Kubernetes service for Jaeger Collector backed by the pods matching the selector
func NewCollectorServices(jaeger *v1.Jaeger, selector map[string]string) []*corev1.Service {
return []*corev1.Service{
headlessCollectorService(jaeger, selector),
clusteripCollectorService(jaeger, selector),
}
}

func headlessCollectorService(jaeger *v1.Jaeger, selector map[string]string) *corev1.Service {
svc := collectorService(jaeger, selector)
svc.Name = GetNameForHeadlessCollectorService(jaeger)
svc.Spec.ClusterIP = "None"
return svc
}

func clusteripCollectorService(jaeger *v1.Jaeger, selector map[string]string) *corev1.Service {
return collectorService(jaeger, selector)
}

func collectorService(jaeger *v1.Jaeger, selector map[string]string) *corev1.Service {
trueVar := true
return &corev1.Service{
TypeMeta: metav1.TypeMeta{
Kind: "Service",
Expand Down Expand Up @@ -41,7 +58,7 @@ func NewCollectorService(jaeger *v1.Jaeger, selector map[string]string) *corev1.
},
Spec: corev1.ServiceSpec{
Selector: selector,
ClusterIP: "None",
ClusterIP: "",
Ports: []corev1.ServicePort{
{
Name: "zipkin",
Expand All @@ -62,9 +79,15 @@ func NewCollectorService(jaeger *v1.Jaeger, selector map[string]string) *corev1.
},
},
}

}

// GetNameForCollectorService returns the service name for the collector in this Jaeger instance
func GetNameForCollectorService(jaeger *v1.Jaeger) string {
return fmt.Sprintf("%s-collector", jaeger.Name)
}

// GetNameForHeadlessCollectorService returns the headless service name for the collector in this Jaeger instance
func GetNameForHeadlessCollectorService(jaeger *v1.Jaeger) string {
return fmt.Sprintf("%s-collector-headless", jaeger.Name)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: This could take the output from GetNameForCollectorService and append -headless. Only minor though feel free to ignore.

}
24 changes: 22 additions & 2 deletions pkg/service/collector_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,10 @@ func TestCollectorServiceNameAndPorts(t *testing.T) {
selector := map[string]string{"app": "myapp", "jaeger": name, "jaeger-component": "collector"}

jaeger := v1.NewJaeger(name)
svc := NewCollectorService(jaeger, selector)
assert.Equal(t, svc.ObjectMeta.Name, fmt.Sprintf("%s-collector", name))
svcs := NewCollectorServices(jaeger, selector)

assert.Equal(t, svcs[0].Name, fmt.Sprintf("%s-collector-headless", name))
assert.Equal(t, svcs[1].Name, fmt.Sprintf("%s-collector", name))

ports := map[int32]bool{
9411: false,
Expand All @@ -24,6 +26,7 @@ func TestCollectorServiceNameAndPorts(t *testing.T) {
14268: false,
}

svc := svcs[0]
for _, port := range svc.Spec.Ports {
ports[port.Port] = true
}
Expand All @@ -32,4 +35,21 @@ func TestCollectorServiceNameAndPorts(t *testing.T) {
assert.Equal(t, v, true, "Expected port %v to be specified, but wasn't", k)
}

// we ensure the ports are the same for both services
assert.Equal(t, svcs[0].Spec.Ports, svcs[1].Spec.Ports)
}

func TestCollectorServiceWithClusterIPEmptyAndNone(t *testing.T) {
name := "TestCollectorServiceWithClusterIP"
selector := map[string]string{"app": "myapp", "jaeger": name, "jaeger-component": "collector"}

jaeger := v1.NewJaeger(name)
svcs := NewCollectorServices(jaeger, selector)

// we want two services, one headless (load balanced by the client, possibly via DNS)
// and one with a cluster IP (load balanced by kube-proxy)
assert.Len(t, svcs, 2)
assert.NotEqual(t, svcs[0].Name, svcs[1].Name) // they can't have the same name
assert.Equal(t, "None", svcs[0].Spec.ClusterIP)
assert.Len(t, svcs[1].Spec.ClusterIP, 0)
}
2 changes: 1 addition & 1 deletion pkg/service/query.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func NewQueryService(jaeger *v1.Jaeger, selector map[string]string) *corev1.Serv
},
Spec: corev1.ServiceSpec{
Selector: selector,
ClusterIP: "None",
ClusterIP: "",
Ports: []corev1.ServicePort{
{
Name: "query",
Expand Down
1 change: 1 addition & 0 deletions pkg/service/query_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ func TestQueryServiceNameAndPorts(t *testing.T) {
assert.Len(t, svc.Spec.Ports, 1)
assert.Equal(t, int32(16686), svc.Spec.Ports[0].Port)
assert.Equal(t, intstr.FromInt(16686), svc.Spec.Ports[0].TargetPort)
assert.Len(t, svc.Spec.ClusterIP, 0) // make sure we get a cluster IP
}

func TestQueryServiceNameAndPortsWithOAuthProxy(t *testing.T) {
Expand Down