From cf2608efa646edb5fb0dc3e4fbd53e7a6ad4c961 Mon Sep 17 00:00:00 2001 From: Lisheng Zheng Date: Fri, 3 Jul 2020 17:54:39 +0800 Subject: [PATCH] fix(limit): limit release history number (#159) (#160) * fix(limit): limit release history number * fix * change variable name * add a test case * add two test cases * fix lint * tweak comments * tweak code * tweak comments Co-authored-by: Lisheng Zheng Co-authored-by: Lisheng Zheng --- cmd/controller/app/controllers.go | 1 + cmd/controller/app/options/options.go | 5 +++ cmd/controller/app/release.go | 5 +++ pkg/controller/gc/controller.go | 51 ++++++++++++++++++++++---- pkg/controller/gc/controller_test.go | 52 +++++++++++++++++++++++++++ 5 files changed, 107 insertions(+), 7 deletions(-) create mode 100644 pkg/controller/gc/controller_test.go diff --git a/cmd/controller/app/controllers.go b/cmd/controller/app/controllers.go index dbfb8de5..01fac944 100644 --- a/cmd/controller/app/controllers.go +++ b/cmd/controller/app/controllers.go @@ -90,6 +90,7 @@ func startGCController(ctx ControllerContext) error { ctx.InformerStore, ctx.AvailableKinds, ctx.IgnoredKinds, + ctx.HistoryLimit, ) if err != nil { return err diff --git a/cmd/controller/app/options/options.go b/cmd/controller/app/options/options.go index 52eb078e..9e345cc2 100644 --- a/cmd/controller/app/options/options.go +++ b/cmd/controller/app/options/options.go @@ -26,6 +26,10 @@ type ReleaseServer struct { ResyncPeriod time.Duration // ReleaseResyncPeriod is the resync period to invoke informer event handler. ReleaseResyncPeriod time.Duration + + // The number of releaseHistory to retain to allow rollback. + // Defaults to 50. + HistoryLimit int32 } // NewReleaseServer creates a new CMServer with a default config. @@ -47,4 +51,5 @@ func (s *ReleaseServer) AddFlags(fs *pflag.FlagSet, allControllers []string) { fs.Int32Var(&s.ConcurrentStatusSyncs, "concurrent-status-syncs", s.ConcurrentStatusSyncs, "The number of status controller worker that are allowed to sync concurrently") fs.DurationVar(&s.ResyncPeriod, "resync-period", s.ResyncPeriod, "ResyncPeriod describes the period of informer resync") fs.DurationVar(&s.ReleaseResyncPeriod, "handler-resync-period", s.ReleaseResyncPeriod, "ReleaseResyncPeriod is the resync period to invoke informer event handler") + fs.Int32Var(&s.HistoryLimit, "history-limit", 50, "The number of releaseHistory to retain to allow rollback") } diff --git a/cmd/controller/app/release.go b/cmd/controller/app/release.go index 14c727a7..b238934c 100644 --- a/cmd/controller/app/release.go +++ b/cmd/controller/app/release.go @@ -50,11 +50,15 @@ type ControllerContext struct { Stop <-chan struct{} // ReleaseResyncPeriod is the resync period to invoke informer event handler for release ReleaseResyncPeriod time.Duration + // The number of releaseHistory to retain to allow rollback. + // Defaults to 50. + HistoryLimit int32 } // Run runs the ReleaseServer. This should never exit. func Run(s *options.ReleaseServer) error { glog.Infof("Initialize release server") + glog.Infof("Retain release history number %v", s.HistoryLimit) glog.Infof("Rudder Build Information, %v", version.Get().Pretty()) kubeConfig, err := clientcmd.BuildConfigFromFlags("", s.Kubeconfig) if err != nil { @@ -92,6 +96,7 @@ func Run(s *options.ReleaseServer) error { IgnoredKinds: IgnoredKinds(), Stop: stop, ReleaseResyncPeriod: s.ReleaseResyncPeriod, + HistoryLimit: s.HistoryLimit, } initializers, err := NewControllerInitializers(s.Controllers) if err != nil { diff --git a/pkg/controller/gc/controller.go b/pkg/controller/gc/controller.go index 53c7349c..6de3af08 100644 --- a/pkg/controller/gc/controller.go +++ b/pkg/controller/gc/controller.go @@ -1,6 +1,9 @@ package gc import ( + "fmt" + "strconv" + "strings" "sync" "time" @@ -175,19 +178,22 @@ type GarbageCollector struct { ignored []schema.GroupVersionKind // indicates which resources should be ignored workers int32 working int32 + historyLimit int32 } // NewGarbageCollector creates a garbage collector. func NewGarbageCollector(clients kube.ClientPool, codec kube.Codec, store store.IntegrationStore, targets, ignored []schema.GroupVersionKind, + historyLimit int32, ) (*GarbageCollector, error) { gc := &GarbageCollector{ - clients: clients, - codec: codec, - store: store, - resources: newReleaseResources(ignored), - queue: workqueue.NewRateLimitingQueue(workqueue.DefaultControllerRateLimiter()), - ignored: ignored, + clients: clients, + codec: codec, + store: store, + resources: newReleaseResources(ignored), + queue: workqueue.NewRateLimitingQueue(workqueue.DefaultControllerRateLimiter()), + ignored: ignored, + historyLimit: historyLimit, } releaseInformer, err := store.InformerFor(gvkRelease) if err != nil { @@ -385,7 +391,12 @@ func (gc *GarbageCollector) collect(release *releaseapi.Release) error { switch { case res.gvk == gvkReleaseHistory: // Check history - if releaseAlived { + ifRetain, err := gc.ifRetainHistory(release, res.name) + if err != nil { + glog.Errorf("get retain info for resource %s/%s failed %v", res.namespace, res.name, err) + return err + } + if releaseAlived && ifRetain { continue } fallthrough @@ -416,3 +427,29 @@ func (gc *GarbageCollector) collect(release *releaseapi.Release) error { return nil } + +// ifRetainHistory tell if retain the release history, it will retain the history which between +// (latestVersion-historyLimit : latestVersion] actually. +// Why don't use latestVersion as parameter directly, if you want acquire latestVersion, you need +// list all histories first which is not graceful. On the other hand in a sentence, the current +// policy also satisfies the demand of limiting history number because the current version will be +// equal with the latest version after updating the release. +func (gc *GarbageCollector) ifRetainHistory(rls *releaseapi.Release, rlsHistoryName string) (bool, error) { + // The rls name may be "hello" and rlsHistory name be "hellowe12-v0", the first validation will ignore it. + // But when invoke `strconv.Atoi` the digit string "12-v0" will failed. So it is still invalid. + if !strings.HasPrefix(rlsHistoryName, rls.Name) { + return false, fmt.Errorf("cur release history %v is not belong the release %v", rlsHistoryName, rls.Name) + } + // the "-v" occupy two byte + version, err := strconv.Atoi(rlsHistoryName[len(rls.Name)+2:]) + if err != nil { + return false, err + } + if version <= 0 { + return false, fmt.Errorf("cur release history %v version %v is invalid", rlsHistoryName, version) + } + if version+int(gc.historyLimit) > int(rls.Status.Version) { + return true, nil + } + return false, nil +} diff --git a/pkg/controller/gc/controller_test.go b/pkg/controller/gc/controller_test.go new file mode 100644 index 00000000..732f1b2a --- /dev/null +++ b/pkg/controller/gc/controller_test.go @@ -0,0 +1,52 @@ +package gc + +import ( + "fmt" + "reflect" + "testing" + + releaseapi "github.com/caicloud/clientset/pkg/apis/release/v1alpha1" + + metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +func newRelease(name string, version int32) *releaseapi.Release { + return &releaseapi.Release{ + ObjectMeta: metav1.ObjectMeta{ + Name: name, + }, + Status: releaseapi.ReleaseStatus{ + Version: version, + }, + } +} + +func TestIsRetainHistory(t *testing.T) { + gc := &GarbageCollector{ + historyLimit: 5, + } + testCases := []struct { + rls string + curVersion int32 + rlsHistory string + want bool + err error + }{ + {"hello", 3, "hello-v1", true, nil}, + {"hello", 10, "hello-v1", false, nil}, + {"hello", 3, "hello-v0", false, fmt.Errorf("cur release history hello-v0 version 0 is invalid")}, + {"hello", 3, "hello-v-9", false, fmt.Errorf("cur release history hello-v-9 version -9 is invalid")}, + {"hello", 10, "hello-v5", false, nil}, + {"hello", 10, "hello-v6", true, nil}, + {"hello", 10, "hello1-v6", false, nil}, + {"hello", 10, "hello-v20", true, nil}, + {"hello", 10, "hello1-v20", false, nil}, + } + + for _, ca := range testCases { + get, err := gc.ifRetainHistory(newRelease(ca.rls, ca.curVersion), ca.rlsHistory) + if reflect.DeepEqual(err, ca.want) || get != ca.want { + t.Errorf("limit %v got %v but condition is %v err %v", gc.historyLimit, get, ca, err) + } + } +}