Skip to content

Commit

Permalink
Best effort to pick stable/active services for Rollouts (#242)
Browse files Browse the repository at this point in the history
  • Loading branch information
@aattuluri
aattuluri committed Jul 6, 2022
1 parent f488367 commit 5541b68
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 42 deletions.
99 changes: 61 additions & 38 deletions admiral/pkg/clusters/handler.go
View file
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
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
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

0 comments on commit 5541b68

Please sign in to comment.