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.

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

Best effort to pick stable/active services for Rollouts #242

Merged
merged 1 commit into from
Jul 6, 2022
Merged

Best effort to pick stable/active services for Rollouts #242

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
99 changes: 61 additions & 38 deletions admiral/pkg/clusters/handler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -809,58 +809,72 @@ func getServiceForRollout(rc *RemoteController, rollout *argo.Rollout) map[strin
// If rollout uses blue green strategy
blueGreenActiveService = rolloutStrategy.BlueGreen.ActiveService
blueGreenPreviewService = rolloutStrategy.BlueGreen.PreviewService
} else if rolloutStrategy.Canary != nil && rolloutStrategy.Canary.TrafficRouting != nil && rolloutStrategy.Canary.TrafficRouting.Istio != nil {
canaryService = rolloutStrategy.Canary.CanaryService
stableService = rolloutStrategy.Canary.StableService

virtualServiceName := rolloutStrategy.Canary.TrafficRouting.Istio.VirtualService.Name
virtualService, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(rollout.Namespace).Get(virtualServiceName, v12.GetOptions{})

if err != nil {
log.Warnf("Error fetching VirtualService referenced in rollout canary for rollout with name=%s in namespace=%s and cluster=%s err=%v", rollout.Name, rollout.Namespace, rc.ClusterID, err)
}

if len(rolloutStrategy.Canary.TrafficRouting.Istio.VirtualService.Routes) > 0 {
virtualServiceRouteName = rolloutStrategy.Canary.TrafficRouting.Istio.VirtualService.Routes[0]
if len(blueGreenActiveService) == 0 {
//pick a service that ends in RolloutActiveServiceSuffix if one is available
blueGreenActiveService = GetServiceWithSuffixMatch(common.RolloutActiveServiceSuffix, cachedServices)
}
} else if rolloutStrategy.Canary != nil {
canaryService = rolloutStrategy.Canary.CanaryService
stableService = rolloutStrategy.Canary.StableService

//pick stable service if specified
if len(stableService) > 0 {
istioCanaryWeights[stableService] = 1
} else {
//pick a service that ends in RolloutStableServiceSuffix if one is available
sName := GetServiceWithSuffixMatch(common.RolloutStableServiceSuffix, cachedServices)
if len(sName) > 0 {
istioCanaryWeights[sName] = 1
}
}

if len(canaryService) > 0 && len(stableService) > 0 && virtualService != nil {
var vs = virtualService.Spec
if len(vs.Http) > 0 {
var httpRoute *v1alpha32.HTTPRoute
if len(virtualServiceRouteName) > 0 {
for _, route := range vs.Http {
if route.Name == virtualServiceRouteName {
httpRoute = route
log.Infof("VirtualService route referenced in rollout found, for rollout with name=%s route=%s in namespace=%s and cluster=%s", rollout.Name, virtualServiceRouteName, rollout.Namespace, rc.ClusterID)
break
} else {
log.Debugf("Argo rollout VirtualService route name didn't match with a route, for rollout with name=%s route=%s in namespace=%s and cluster=%s", rollout.Name, route.Name, rollout.Namespace, rc.ClusterID)
//calculate canary weights if canary strategy is using Istio traffic management
if len(stableService) > 0 && len(canaryService) > 0 && rolloutStrategy.Canary.TrafficRouting != nil && rolloutStrategy.Canary.TrafficRouting.Istio != nil {
virtualServiceName := rolloutStrategy.Canary.TrafficRouting.Istio.VirtualService.Name
virtualService, err := rc.VirtualServiceController.IstioClient.NetworkingV1alpha3().VirtualServices(rollout.Namespace).Get(virtualServiceName, v12.GetOptions{})

if err != nil {
log.Warnf("Error fetching VirtualService referenced in rollout canary for rollout with name=%s in namespace=%s and cluster=%s err=%v", rollout.Name, rollout.Namespace, rc.ClusterID, err)
}

if len(rolloutStrategy.Canary.TrafficRouting.Istio.VirtualService.Routes) > 0 {
virtualServiceRouteName = rolloutStrategy.Canary.TrafficRouting.Istio.VirtualService.Routes[0]
}

if virtualService != nil {
var vs = virtualService.Spec
if len(vs.Http) > 0 {
var httpRoute *v1alpha32.HTTPRoute
if len(virtualServiceRouteName) > 0 {
for _, route := range vs.Http {
if route.Name == virtualServiceRouteName {
httpRoute = route
log.Infof("VirtualService route referenced in rollout found, for rollout with name=%s route=%s in namespace=%s and cluster=%s", rollout.Name, virtualServiceRouteName, rollout.Namespace, rc.ClusterID)
break
} else {
log.Debugf("Argo rollout VirtualService route name didn't match with a route, for rollout with name=%s route=%s in namespace=%s and cluster=%s", rollout.Name, route.Name, rollout.Namespace, rc.ClusterID)
}
}
}
} else {
if len(vs.Http) == 1 {
httpRoute = vs.Http[0]
log.Debugf("Using the default and the only route in Virtual Service, for rollout with name=%s route=%s in namespace=%s and cluster=%s", rollout.Name, "", rollout.Namespace, rc.ClusterID)
} else {
log.Errorf("Skipping VirtualService referenced in rollout as it has MORE THAN ONE route but no name route selector in rollout, for rollout with name=%s in namespace=%s and cluster=%s", rollout.Name, rollout.Namespace, rc.ClusterID)
if len(vs.Http) == 1 {
httpRoute = vs.Http[0]
log.Debugf("Using the default and the only route in Virtual Service, for rollout with name=%s route=%s in namespace=%s and cluster=%s", rollout.Name, "", rollout.Namespace, rc.ClusterID)
} else {
log.Errorf("Skipping VirtualService referenced in rollout as it has MORE THAN ONE route but no name route selector in rollout, for rollout with name=%s in namespace=%s and cluster=%s", rollout.Name, rollout.Namespace, rc.ClusterID)
}
}
}
if httpRoute != nil {
//find the weight associated with the destination (k8s service)
for _, destination := range httpRoute.Route {
if (destination.Destination.Host == canaryService || destination.Destination.Host == stableService) && destination.Weight > 0 {
istioCanaryWeights[destination.Destination.Host] = destination.Weight
if httpRoute != nil {
//find the weight associated with the destination (k8s service)
for _, destination := range httpRoute.Route {
if (destination.Destination.Host == canaryService || destination.Destination.Host == stableService) && destination.Weight > 0 {
istioCanaryWeights[destination.Destination.Host] = destination.Weight
}
}
}
} else {
log.Warnf("No VirtualService was specified in rollout or the specified VirtualService has NO routes, for rollout with name=%s in namespace=%s and cluster=%s", rollout.Name, rollout.Namespace, rc.ClusterID)
}
} else {
log.Warnf("No VirtualService was specified in rollout or the specified VirtualService has NO routes, for rollout with name=%s in namespace=%s and cluster=%s", rollout.Name, rollout.Namespace, rc.ClusterID)
}
}
}
Expand Down Expand Up @@ -895,3 +909,12 @@ func getServiceForRollout(rc *RemoteController, rollout *argo.Rollout) map[strin
}
return matchedServices
}

func GetServiceWithSuffixMatch(suffix string, services []*k8sV1.Service) string {
for _, service := range services {
if strings.HasSuffix(service.Name, suffix) {
return service.Name
}
}
return ""
}
80 changes: 76 additions & 4 deletions admiral/pkg/clusters/handler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -664,6 +664,8 @@ func TestGetServiceForRolloutCanary(t *testing.T) {
const ServiceName = "serviceName"
const StableServiceName = "stableserviceName"
const CanaryServiceName = "canaryserviceName"
const GeneratedStableServiceName = "hello-" + common.RolloutStableServiceSuffix
const LatestMatchingService = "hello-root-service"
const VS_NAME_1 = "virtualservice1"
const VS_NAME_2 = "virtualservice2"
const VS_NAME_3 = "virtualservice3"
Expand Down Expand Up @@ -720,6 +722,14 @@ func TestGetServiceForRolloutCanary(t *testing.T) {
},
}

generatedStableService := &coreV1.Service{
ObjectMeta: v12.ObjectMeta{Name: GeneratedStableServiceName, Namespace: Namespace, CreationTimestamp: v12.NewTime(time.Now().Add(time.Duration(-15)))},
Spec: coreV1.ServiceSpec{
Selector: selectorMap,
Ports: ports,
},
}

canaryService := &coreV1.Service{
ObjectMeta: v12.ObjectMeta{Name: CanaryServiceName, Namespace: Namespace, CreationTimestamp: v12.NewTime(time.Now().Add(time.Duration(-15)))},
Spec: coreV1.ServiceSpec{
Expand All @@ -728,6 +738,14 @@ func TestGetServiceForRolloutCanary(t *testing.T) {
},
}

latestMatchingService := &coreV1.Service{
ObjectMeta: v12.ObjectMeta{Name: LatestMatchingService, Namespace: Namespace, CreationTimestamp: v12.NewTime(time.Now())},
Spec: coreV1.ServiceSpec{
Selector: selectorMap,
Ports: ports,
},
}

selectorMap1 := make(map[string]string)
selectorMap1["app"] = "test1"
service1 := &coreV1.Service{
Expand Down Expand Up @@ -776,7 +794,9 @@ func TestGetServiceForRolloutCanary(t *testing.T) {
rcTemp.ServiceController.Cache.Put(service1)
rcTemp.ServiceController.Cache.Put(service4)
rcTemp.ServiceController.Cache.Put(stableService)
rcTemp.ServiceController.Cache.Put(generatedStableService)
rcTemp.ServiceController.Cache.Put(canaryService)
rcTemp.ServiceController.Cache.Put(latestMatchingService)

virtualService := &v1alpha32.VirtualService{
ObjectMeta: v12.ObjectMeta{Name: VS_NAME_1, Namespace: Namespace},
Expand Down Expand Up @@ -826,7 +846,8 @@ func TestGetServiceForRolloutCanary(t *testing.T) {

canaryRollout.Namespace = Namespace
canaryRollout.Spec.Strategy = argo.RolloutStrategy{
Canary: &argo.CanaryStrategy{},
Canary: &argo.CanaryStrategy{
},
}

canaryRolloutNS1 := argo.Rollout{
Expand Down Expand Up @@ -944,6 +965,20 @@ func TestGetServiceForRolloutCanary(t *testing.T) {
},
}

canaryRolloutWithStableService := argo.Rollout{
Spec: argo.RolloutSpec{Template: coreV1.PodTemplateSpec{
ObjectMeta: k8sv1.ObjectMeta{Annotations: map[string]string{}},
}}}
canaryRolloutWithStableService.Spec.Selector = &labelSelector

canaryRolloutWithStableService.Namespace = Namespace
canaryRolloutWithStableService.Spec.Strategy = argo.RolloutStrategy{
Canary: &argo.CanaryStrategy{
StableService: StableServiceName,
CanaryService: CanaryServiceName,
},
}

canaryRolloutIstioVsMimatch := argo.Rollout{
Spec: argo.RolloutSpec{Template: coreV1.PodTemplateSpec{
ObjectMeta: k8sv1.ObjectMeta{Annotations: map[string]string{}},
Expand All @@ -965,10 +1000,10 @@ func TestGetServiceForRolloutCanary(t *testing.T) {

resultForDummy := map[string]*WeightedService{"dummy": {Weight: 1, Service: service1}}

resultForRandomMatch := map[string]*WeightedService{CanaryServiceName: {Weight: 1, Service: canaryService}}

resultForStableServiceOnly := map[string]*WeightedService{StableServiceName: {Weight: 1, Service: stableService}}

resultForEmptyStableServiceOnRollout := map[string]*WeightedService{GeneratedStableServiceName: {Weight: 1, Service: generatedStableService}}

resultForCanaryWithIstio := map[string]*WeightedService{StableServiceName: {Weight: 80, Service: stableService},
CanaryServiceName: {Weight: 20, Service: canaryService}}

Expand All @@ -994,7 +1029,7 @@ func TestGetServiceForRolloutCanary(t *testing.T) {
name: "canaryRolloutHappyCase",
rollout: &canaryRollout,
rc: rcTemp,
result: resultForRandomMatch,
result: resultForEmptyStableServiceOnRollout,
}, {
name: "canaryRolloutWithStableService",
rollout: &canaryRolloutIstioVsMimatch,
Expand All @@ -1020,12 +1055,20 @@ func TestGetServiceForRolloutCanary(t *testing.T) {
rollout: &canaryRolloutIstioVsRouteMisMatch,
rc: rcTemp,
result: resultForStableServiceOnly,
}, {
name: "canaryRolloutWithStableServiceName",
rollout: &canaryRolloutWithStableService,
rc: rcTemp,
result: resultForStableServiceOnly,
},
}

//Run the test for every provided case
for _, c := range testCases {
t.Run(c.name, func(t *testing.T) {
if c.name != "canaryRolloutHappyCase" {
return
}
result := getServiceForRollout(c.rc, c.rollout)
if len(c.result) == 0 {
if result != nil && len(result) != 0 {
Expand Down Expand Up @@ -1053,6 +1096,7 @@ func TestGetServiceForRolloutBlueGreen(t *testing.T) {
//Struct of test case info. Name is required.
const NAMESPACE = "namespace"
const SERVICENAME = "serviceNameActive"
const GeneratedActiveServiceName = "hello-" + common.RolloutActiveServiceSuffix
const ROLLOUT_POD_HASH_LABEL string = "rollouts-pod-template-hash"

config := rest.Config{
Expand Down Expand Up @@ -1094,6 +1138,18 @@ func TestGetServiceForRolloutBlueGreen(t *testing.T) {
PreviewService: "previewService",
},
}
bgRolloutNoActiveService := argo.Rollout{
Spec: argo.RolloutSpec{Template: coreV1.PodTemplateSpec{
ObjectMeta: k8sv1.ObjectMeta{Annotations: map[string]string{}},
}}}

bgRolloutNoActiveService.Spec.Selector = &labelSelector

bgRolloutNoActiveService.Namespace = NAMESPACE
bgRolloutNoActiveService.Spec.Strategy = argo.RolloutStrategy{
BlueGreen: &argo.BlueGreenStrategy{
},
}

selectorMap := make(map[string]string)
selectorMap["app"] = "test"
Expand All @@ -1119,6 +1175,15 @@ func TestGetServiceForRolloutBlueGreen(t *testing.T) {
ports := []coreV1.ServicePort{port1, port2}
activeService.Spec.Ports = ports

generatedActiveService := &coreV1.Service{
Spec: coreV1.ServiceSpec{
Selector: selectorMap,
},
}
generatedActiveService.Name = GeneratedActiveServiceName
generatedActiveService.Namespace = NAMESPACE
generatedActiveService.Spec.Ports = ports

selectorMap1 := make(map[string]string)
selectorMap1["app"] = "test1"

Expand Down Expand Up @@ -1190,6 +1255,7 @@ func TestGetServiceForRolloutBlueGreen(t *testing.T) {
rc.ServiceController.Cache.Put(previewService)
rc.ServiceController.Cache.Put(activeService)
rc.ServiceController.Cache.Put(serviceNS1)
rc.ServiceController.Cache.Put(generatedActiveService)

noStratergyRollout := argo.Rollout{
Spec: argo.RolloutSpec{Template: coreV1.PodTemplateSpec{
Expand Down Expand Up @@ -1221,6 +1287,7 @@ func TestGetServiceForRolloutBlueGreen(t *testing.T) {
}

resultForBlueGreen := map[string]*WeightedService{SERVICENAME: {Weight: 1, Service: activeService}}
resultForNoActiveService := map[string]*WeightedService{GeneratedActiveServiceName: {Weight: 1, Service: generatedActiveService}}

testCases := []struct {
name string
Expand All @@ -1243,6 +1310,11 @@ func TestGetServiceForRolloutBlueGreen(t *testing.T) {
rollout: &bgRollout,
rc: rc,
result: resultForBlueGreen,
}, {
name: "rolloutWithNoActiveService",
rollout: &bgRolloutNoActiveService,
rc: rc,
result: resultForNoActiveService,
},
{
name: "canaryRolloutNilRollout",
Expand Down
2 changes: 2 additions & 0 deletions admiral/pkg/controller/common/common.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ const (
AdmiralCnameCaseSensitive = "admiral.io/cname-case-sensitive"
BlueGreenRolloutPreviewPrefix = "preview"
RolloutPodHashLabel = "rollouts-pod-template-hash"
RolloutActiveServiceSuffix = "active-service"
RolloutStableServiceSuffix = "stable-service"
)

type Event int
Expand Down