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

Allow Not generating Canary Service && Fixed a bug caused by NOT considering case-insensitivity. #200

Merged
merged 2 commits into from
Mar 12, 2024
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
14 changes: 8 additions & 6 deletions api/v1alpha1/conversion.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@ package v1alpha1
import (
"fmt"

"strings"

"github.com/openkruise/rollouts/api/v1beta1"
"k8s.io/apimachinery/pkg/util/intstr"
utilpointer "k8s.io/utils/pointer"
Expand Down Expand Up @@ -74,7 +76,7 @@ func (src *Rollout) ConvertTo(dst conversion.Hub) error {
obj.Spec.Strategy.Canary.PatchPodTemplateMetadata.Labels[k] = v
}
}
if src.Annotations[RolloutStyleAnnotation] != string(PartitionRollingStyle) {
if !strings.EqualFold(src.Annotations[RolloutStyleAnnotation], string(PartitionRollingStyle)) {
obj.Spec.Strategy.Canary.EnableExtraWorkloadForCanary = true
}
if src.Annotations[TrafficRoutingAnnotation] != "" {
Expand Down Expand Up @@ -211,9 +213,9 @@ func (dst *Rollout) ConvertFrom(src conversion.Hub) error {
dst.Annotations = map[string]string{}
}
if srcV1beta1.Spec.Strategy.Canary.EnableExtraWorkloadForCanary {
dst.Annotations[RolloutStyleAnnotation] = string(CanaryRollingStyle)
dst.Annotations[RolloutStyleAnnotation] = strings.ToLower(string(CanaryRollingStyle))
} else {
dst.Annotations[RolloutStyleAnnotation] = string(PartitionRollingStyle)
dst.Annotations[RolloutStyleAnnotation] = strings.ToLower(string(PartitionRollingStyle))
}
if srcV1beta1.Spec.Strategy.Canary.TrafficRoutingRef != "" {
dst.Annotations[TrafficRoutingAnnotation] = srcV1beta1.Spec.Strategy.Canary.TrafficRoutingRef
Expand Down Expand Up @@ -336,7 +338,7 @@ func (src *BatchRelease) ConvertTo(dst conversion.Hub) error {
obj.Spec.ReleasePlan.PatchPodTemplateMetadata.Labels[k] = v
}
}
if src.Annotations[RolloutStyleAnnotation] != string(PartitionRollingStyle) {
if !strings.EqualFold(src.Annotations[RolloutStyleAnnotation], string(PartitionRollingStyle)) {
obj.Spec.ReleasePlan.EnableExtraWorkloadForCanary = true
}

Expand Down Expand Up @@ -416,9 +418,9 @@ func (dst *BatchRelease) ConvertFrom(src conversion.Hub) error {
dst.Annotations = map[string]string{}
}
if srcV1beta1.Spec.ReleasePlan.EnableExtraWorkloadForCanary {
dst.Annotations[RolloutStyleAnnotation] = string(CanaryRollingStyle)
dst.Annotations[RolloutStyleAnnotation] = strings.ToLower(string(CanaryRollingStyle))
} else {
dst.Annotations[RolloutStyleAnnotation] = string(PartitionRollingStyle)
dst.Annotations[RolloutStyleAnnotation] = strings.ToLower(string(PartitionRollingStyle))
}

// status
Expand Down
2 changes: 2 additions & 0 deletions api/v1alpha1/rollout_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -121,6 +121,8 @@ type CanaryStrategy struct {
// only support for canary deployment
// +optional
PatchPodTemplateMetadata *PatchPodTemplateMetadata `json:"patchPodTemplateMetadata,omitempty"`
// canary service will not be generated if DisableGenerateCanaryService is true
DisableGenerateCanaryService bool `json:"disableGenerateCanaryService,omitempty"`
}

type PatchPodTemplateMetadata struct {
Expand Down
2 changes: 2 additions & 0 deletions api/v1beta1/rollout_types.go
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,8 @@ type CanaryStrategy struct {
EnableExtraWorkloadForCanary bool `json:"enableExtraWorkloadForCanary,omitempty"`
// TrafficRoutingRef is TrafficRouting's Name
TrafficRoutingRef string `json:"trafficRoutingRef,omitempty"`
// canary service will not be generated if DisableGenerateCanaryService is true
DisableGenerateCanaryService bool `json:"disableGenerateCanaryService,omitempty"`
}

type PatchPodTemplateMetadata struct {
Expand Down
8 changes: 8 additions & 0 deletions config/crd/bases/rollouts.kruise.io_rollouts.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -99,6 +99,10 @@ spec:
description: CanaryStrategy defines parameters for a Replica Based
Canary
properties:
disableGenerateCanaryService:
description: canary service will not be generated if DisableGenerateCanaryService
is true
type: boolean
failureThreshold:
anyOf:
- type: integer
Expand Down Expand Up @@ -574,6 +578,10 @@ spec:
description: CanaryStrategy defines parameters for a Replica Based
Canary
properties:
disableGenerateCanaryService:
description: canary service will not be generated if DisableGenerateCanaryService
is true
type: boolean
enableExtraWorkloadForCanary:
description: 'If true, then it will create new deployment
for canary, such as: workload-demo-canary. When user verifies
Expand Down
19 changes: 10 additions & 9 deletions pkg/controller/rollout/rollout_progressing.go
Original file line number Diff line number Diff line change
Expand Up @@ -452,14 +452,15 @@ func newTrafficRoutingContext(c *RolloutContext) *trafficrouting.TrafficRoutingC
revisionLabelKey = c.Workload.RevisionLabelKey
}
return &trafficrouting.TrafficRoutingContext{
Key: fmt.Sprintf("Rollout(%s/%s)", c.Rollout.Namespace, c.Rollout.Name),
Namespace: c.Rollout.Namespace,
ObjectRef: c.Rollout.Spec.Strategy.Canary.TrafficRoutings,
Strategy: currentStep.TrafficRoutingStrategy,
OwnerRef: *metav1.NewControllerRef(c.Rollout, rolloutControllerKind),
RevisionLabelKey: revisionLabelKey,
StableRevision: c.NewStatus.CanaryStatus.StableRevision,
CanaryRevision: c.NewStatus.CanaryStatus.PodTemplateHash,
LastUpdateTime: c.NewStatus.CanaryStatus.LastUpdateTime,
Key: fmt.Sprintf("Rollout(%s/%s)", c.Rollout.Namespace, c.Rollout.Name),
Namespace: c.Rollout.Namespace,
ObjectRef: c.Rollout.Spec.Strategy.Canary.TrafficRoutings,
Strategy: currentStep.TrafficRoutingStrategy,
OwnerRef: *metav1.NewControllerRef(c.Rollout, rolloutControllerKind),
RevisionLabelKey: revisionLabelKey,
StableRevision: c.NewStatus.CanaryStatus.StableRevision,
CanaryRevision: c.NewStatus.CanaryStatus.PodTemplateHash,
LastUpdateTime: c.NewStatus.CanaryStatus.LastUpdateTime,
DisableGenerateCanaryService: c.Rollout.Spec.Strategy.Canary.DisableGenerateCanaryService,
}
}
19 changes: 12 additions & 7 deletions pkg/trafficrouting/manager.go
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,8 @@
CanaryRevision string
// newStatus.canaryStatus.LastUpdateTime
LastUpdateTime *metav1.Time
// won't work for Ingress and Gateway
DisableGenerateCanaryService bool
}

// Manager responsible for adjusting network resources
Expand All @@ -82,7 +84,7 @@
if err := m.Get(context.TODO(), types.NamespacedName{Namespace: c.Namespace, Name: sService}, service); err != nil {
return err
}
cService := getCanaryServiceName(sService, c.OnlyTrafficRouting)
cService := getCanaryServiceName(sService, c.OnlyTrafficRouting, c.DisableGenerateCanaryService)
// new network provider, ingress or gateway
trController, err := newNetworkProvider(m.Client, c, sService, cService)
if err != nil {
Expand Down Expand Up @@ -116,7 +118,7 @@
return false, err
}
// canary service name
canaryServiceName := getCanaryServiceName(trafficRouting.Service, c.OnlyTrafficRouting)
canaryServiceName := getCanaryServiceName(trafficRouting.Service, c.OnlyTrafficRouting, c.DisableGenerateCanaryService)
canaryService := &corev1.Service{}
canaryService.Namespace = stableService.Namespace
canaryService.Name = canaryServiceName
Expand All @@ -131,7 +133,7 @@

// end-to-end canary deployment scenario(a -> b -> c), if only b or c is released,
//and a is not released in this scenario, then the canary service is not needed.
if !c.OnlyTrafficRouting {
if !c.OnlyTrafficRouting && !c.DisableGenerateCanaryService {
if c.StableRevision == "" || c.CanaryRevision == "" {
klog.Warningf("%s stableRevision or podTemplateHash can not be empty, and wait a moment", c.Key)
return false, nil
Expand Down Expand Up @@ -206,7 +208,7 @@
trafficRouting.GracePeriodSeconds = defaultGracePeriodSeconds
}

cServiceName := getCanaryServiceName(trafficRouting.Service, c.OnlyTrafficRouting)
cServiceName := getCanaryServiceName(trafficRouting.Service, c.OnlyTrafficRouting, c.DisableGenerateCanaryService)
trController, err := newNetworkProvider(m.Client, c, trafficRouting.Service, cServiceName)
if err != nil {
klog.Errorf("%s newTrafficRoutingController failed: %s", c.Key, err.Error())
Expand All @@ -215,6 +217,7 @@

cService := &corev1.Service{ObjectMeta: metav1.ObjectMeta{Namespace: c.Namespace, Name: cServiceName}}
// if canary svc has been already cleaned up, just return
// even DisableGenerateCanaryService is true, canary svc still exists, because canary service is stable service
if err = m.Get(context.TODO(), client.ObjectKeyFromObject(cService), cService); err != nil {
if !errors.IsNotFound(err) {
klog.Errorf("%s get canary service(%s) failed: %s", c.Key, cServiceName, err.Error())
Expand Down Expand Up @@ -259,7 +262,7 @@
}
// end to end deployment, don't remove the canary service;
// because canary service is stable service
if !c.OnlyTrafficRouting {
if !c.OnlyTrafficRouting && !c.DisableGenerateCanaryService {
// remove canary service
err = m.Delete(context.TODO(), cService)
if err != nil && !errors.IsNotFound(err) {
Expand All @@ -281,6 +284,8 @@
StableService: sService,
TrafficConf: trafficRouting.CustomNetworkRefs,
OwnerRef: con.OwnerRef,
//only set for CustomController, never work for Ingress and Gateway
DisableGenerateCanaryService: con.DisableGenerateCanaryService,

Check warning on line 288 in pkg/trafficrouting/manager.go

View check run for this annotation

Codecov / codecov/patch

pkg/trafficrouting/manager.go#L288

Added line #L288 was not covered by tests
})
}
if trafficRouting.Ingress != nil {
Expand Down Expand Up @@ -375,8 +380,8 @@
return true, nil
}

func getCanaryServiceName(sService string, onlyTrafficRouting bool) string {
if onlyTrafficRouting {
func getCanaryServiceName(sService string, onlyTrafficRouting bool, disableGenerateCanaryService bool) string {
if onlyTrafficRouting || disableGenerateCanaryService {
return sService
}
return fmt.Sprintf("%s-canary", sService)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,8 +72,9 @@ type Config struct {
CanaryService string
StableService string
// network providers need to be created
TrafficConf []v1beta1.ObjectRef
OwnerRef metav1.OwnerReference
TrafficConf []v1beta1.ObjectRef
OwnerRef metav1.OwnerReference
DisableGenerateCanaryService bool
}

func NewCustomController(client client.Client, conf Config) (network.NetworkProvider, error) {
Expand Down
Loading