Skip to content

Commit

Permalink
Best effort to pick stable/active services for Rollouts (istio-ecosys…
Browse files Browse the repository at this point in the history 
…tem#242) (istio-ecosystem#141)

Co-authored-by: aattuluri <[email protected]>
  • Loading branch information
@aattuluri
2 people authored and GitHub Enterprise committed Jul 6, 2022
1 parent de341d6 commit 6df0ecf
Show file tree
Hide file tree
Showing 3 changed files with 139 additions and 43 deletions.
100 changes: 61 additions & 39 deletions admiral/pkg/clusters/handler.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,6 @@ func createDestinationRuleForLocal(remoteController *RemoteController, localDrNa
syncNamespace := common.GetSyncNamespace()
serviceInstance := getServiceForDeployment(remoteController, deploymentInstance)

serviceInstance = getServiceForDeployment(remoteController, deploymentInstance)
if serviceInstance == nil {
log.Warnf(LogFormatAdv, "Get", "Service", deploymentInstance.Name, deploymentInstance.Namespace, remoteController.ClusterID, "No matching service instance found")
return
Expand Down Expand Up @@ -860,58 +859,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 @@ -1001,3 +1014,12 @@ func deleteVirtualServicePostStateCheck(rc *RemoteController,syncNamespace strin
}
return nil
}

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 @@ -665,6 +665,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 @@ -721,6 +723,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 @@ -729,6 +739,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 @@ -777,7 +795,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 @@ -827,7 +847,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 @@ -945,6 +966,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 @@ -966,10 +1001,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 @@ -995,7 +1030,7 @@ func TestGetServiceForRolloutCanary(t *testing.T) {
name: "canaryRolloutHappyCase",
rollout: &canaryRollout,
rc: rcTemp,
result: resultForRandomMatch,
result: resultForEmptyStableServiceOnRollout,
}, {
name: "canaryRolloutWithStableService",
rollout: &canaryRolloutIstioVsMimatch,
Expand All @@ -1021,12 +1056,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 @@ -1054,6 +1097,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 @@ -1095,6 +1139,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 @@ -1120,6 +1176,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 @@ -1191,6 +1256,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 @@ -1222,6 +1288,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 @@ -1244,6 +1311,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 6df0ecf

Please sign in to comment.