From c34fd83365b9feb3fa56f7790c9a9e531029550c Mon Sep 17 00:00:00 2001 From: justinsb Date: Thu, 14 Oct 2021 15:39:51 -0400 Subject: [PATCH] Add SystemGeneration to channel version tracker This allows us to reapply a manifest when we introduce new functionality, such as pruning. Otherwise an old version can apply the manifest, mark the manifest as applied, and we won't reapply. --- channels/pkg/api/channel.go | 2 +- channels/pkg/channels/addon.go | 11 +++--- channels/pkg/channels/addons.go | 2 +- channels/pkg/channels/addons_test.go | 32 +++++++++++++---- channels/pkg/channels/channel_version.go | 46 +++++++++++++++++------- nodeup/pkg/model/update_service.go | 2 +- 6 files changed, 68 insertions(+), 27 deletions(-) diff --git a/channels/pkg/api/channel.go b/channels/pkg/api/channel.go index 61657f5ea8..39ddd1da90 100644 --- a/channels/pkg/api/channel.go +++ b/channels/pkg/api/channel.go @@ -46,7 +46,7 @@ type AddonSpec struct { // Manifest is the URL to the manifest that should be applied Manifest *string `json:"manifest,omitempty"` - // Manifesthash is the sha256 hash of our manifest + // ManifestHash is the sha256 hash of our manifest ManifestHash string `json:"manifestHash,omitempty"` // KubernetesVersion is a semver version range on which this version of the addon can be applied diff --git a/channels/pkg/channels/addon.go b/channels/pkg/channels/addon.go index d51269507d..33abc2fcab 100644 --- a/channels/pkg/channels/addon.go +++ b/channels/pkg/channels/addon.go @@ -73,7 +73,7 @@ func (m *AddonMenu) MergeAddons(o *AddonMenu) { if existing == nil { m.Addons[k] = v } else { - if v.ChannelVersion().replaces(existing.ChannelVersion()) { + if v.ChannelVersion().replaces(k, existing.ChannelVersion()) { m.Addons[k] = v } } @@ -82,9 +82,10 @@ func (m *AddonMenu) MergeAddons(o *AddonMenu) { func (a *Addon) ChannelVersion() *ChannelVersion { return &ChannelVersion{ - Channel: &a.ChannelName, - Id: a.Spec.Id, - ManifestHash: a.Spec.ManifestHash, + Channel: &a.ChannelName, + Id: a.Spec.Id, + ManifestHash: a.Spec.ManifestHash, + SystemGeneration: CurrentSystemGeneration, } } @@ -120,7 +121,7 @@ func (a *Addon) GetRequiredUpdates(ctx context.Context, k8sClient kubernetes.Int } } - if existingVersion != nil && !newVersion.replaces(existingVersion) { + if existingVersion != nil && !newVersion.replaces(a.Name, existingVersion) { newVersion = nil } diff --git a/channels/pkg/channels/addons.go b/channels/pkg/channels/addons.go index e48ceaa41b..f6f023f678 100644 --- a/channels/pkg/channels/addons.go +++ b/channels/pkg/channels/addons.go @@ -74,7 +74,7 @@ func (a *Addons) GetCurrent(kubernetesVersion semver.Version) (*AddonMenu, error name := addon.Name existing := menu.Addons[name] - if existing == nil || addon.ChannelVersion().replaces(existing.ChannelVersion()) { + if existing == nil || addon.ChannelVersion().replaces(name, existing.ChannelVersion()) { menu.Addons[name] = addon } } diff --git a/channels/pkg/channels/addons_test.go b/channels/pkg/channels/addons_test.go index 0cd03ba49b..e4c0cc639c 100644 --- a/channels/pkg/channels/addons_test.go +++ b/channels/pkg/channels/addons_test.go @@ -85,6 +85,9 @@ func Test_Filtering(t *testing.T) { } func Test_Replacement(t *testing.T) { + hash1 := "3544de6578b2b582c0323b15b7b05a28c60b9430" + hash2 := "ea9e79bf29adda450446487d65a8fc6b3fdf8c2b" + grid := []struct { Old *ChannelVersion New *ChannelVersion @@ -92,23 +95,38 @@ func Test_Replacement(t *testing.T) { }{ //Test ManifestHash Changes { - Old: &ChannelVersion{Id: "a", ManifestHash: "3544de6578b2b582c0323b15b7b05a28c60b9430"}, - New: &ChannelVersion{Id: "a", ManifestHash: "3544de6578b2b582c0323b15b7b05a28c60b9430"}, + Old: &ChannelVersion{Id: "a", ManifestHash: hash1}, + New: &ChannelVersion{Id: "a", ManifestHash: hash1}, Replaces: false, }, { Old: &ChannelVersion{Id: "a", ManifestHash: ""}, - New: &ChannelVersion{Id: "a", ManifestHash: "3544de6578b2b582c0323b15b7b05a28c60b9430"}, + New: &ChannelVersion{Id: "a", ManifestHash: hash1}, Replaces: true, }, { - Old: &ChannelVersion{Id: "a", ManifestHash: "3544de6578b2b582c0323b15b7b05a28c60b9430"}, - New: &ChannelVersion{Id: "a", ManifestHash: "ea9e79bf29adda450446487d65a8fc6b3fdf8c2b"}, + Old: &ChannelVersion{Id: "a", ManifestHash: hash1}, + New: &ChannelVersion{Id: "a", ManifestHash: hash2}, Replaces: true, }, + { + Old: &ChannelVersion{Id: "a", ManifestHash: hash1}, + New: &ChannelVersion{Id: "a", ManifestHash: hash1, SystemGeneration: 1}, + Replaces: true, + }, + { + Old: &ChannelVersion{Id: "a", ManifestHash: hash1, SystemGeneration: 1}, + New: &ChannelVersion{Id: "a", ManifestHash: hash1}, + Replaces: false, + }, + { + Old: &ChannelVersion{Id: "a", ManifestHash: hash1, SystemGeneration: 1}, + New: &ChannelVersion{Id: "a", ManifestHash: hash1, SystemGeneration: 1}, + Replaces: false, + }, } for _, g := range grid { - actual := g.New.replaces(g.Old) + actual := g.New.replaces(t.Name(), g.Old) if actual != g.Replaces { t.Errorf("unexpected result from %v -> %v, expect %t. actual %v", g.Old, g.New, g.Replaces, actual) } @@ -218,7 +236,7 @@ func Test_NeedsRollingUpdate(t *testing.T) { ctx := context.Background() annotations := map[string]string{ - "addons.k8s.io/test": "{\"manifestHash\":\"originalHash\"}", + "addons.k8s.io/test": "{\"manifestHash\":\"originalHash\",\"systemGeneration\": 1}", } if len(g.originalAnnotations) > 0 { annotations = g.originalAnnotations diff --git a/channels/pkg/channels/channel_version.go b/channels/pkg/channels/channel_version.go index 3e4fb33444..54aca2fe48 100644 --- a/channels/pkg/channels/channel_version.go +++ b/channels/pkg/channels/channel_version.go @@ -20,6 +20,7 @@ import ( "context" "encoding/json" "fmt" + "strconv" "strings" certmanager "github.com/jetstack/cert-manager/pkg/client/clientset/versioned" @@ -38,10 +39,20 @@ type Channel struct { Name string } +// CurrentSystemGeneration holds our current SystemGeneration value. +// Version history: +// 0 Pre-history (and the default value); versions prior to prune. +// 1 Prune functionality introduced. +const CurrentSystemGeneration = 1 + type ChannelVersion struct { Channel *string `json:"channel,omitempty"` Id string `json:"id,omitempty"` ManifestHash string `json:"manifestHash,omitempty"` + + // SystemGeneration holds the generation of the channels functionality. + // It is used so that we reapply when we introduce new features, such as prune. + SystemGeneration int `json:"systemGeneration,omitempty"` } func stringValue(s *string) string { @@ -59,6 +70,7 @@ func (c *ChannelVersion) String() string { if c.ManifestHash != "" { s += " ManifestHash=" + c.ManifestHash } + s += " SystemGeneration=" + strconv.Itoa(c.SystemGeneration) return s } @@ -102,21 +114,31 @@ func (c *Channel) AnnotationName() string { return AnnotationPrefix + c.Name } -func (c *ChannelVersion) replaces(existing *ChannelVersion) bool { - klog.V(4).Infof("Checking existing channel: %v compared to new channel: %v", existing, c) +func (c *ChannelVersion) replaces(name string, existing *ChannelVersion) bool { + klog.V(6).Infof("Checking existing config for %q: %v compared to new channel: %v", name, existing, c) - if c.Id == existing.Id { - // Same id; check manifests - if c.ManifestHash == existing.ManifestHash { - klog.V(4).Infof("Manifest Match") - return false - } - klog.V(4).Infof("Channels had same ids %q, %q but different ManifestHash (%q vs %q); will replace", c.Id, c.ManifestHash, existing.ManifestHash) - } else { - klog.V(4).Infof("Channels had different ids (%q vs %q); will replace", c.Id, existing.Id) + if c.Id != existing.Id { + klog.V(4).Infof("cluster has different ids for %q (%q vs %q); will replace", name, c.Id, existing.Id) + return true } - return true + if c.ManifestHash != existing.ManifestHash { + klog.V(4).Infof("cluster has different ManifestHash for %q (%q vs %q); will replace", name, c.ManifestHash, existing.ManifestHash) + return true + } + + if existing.SystemGeneration != c.SystemGeneration { + if existing.SystemGeneration > c.SystemGeneration { + klog.V(4).Infof("cluster has newer SystemGeneration for %q (%v vs %v), will not replace", name, existing.SystemGeneration, c.SystemGeneration) + return false + } else { + klog.V(4).Infof("cluster has different SystemGeneration for %q (%v vs %v); will replace", name, existing.SystemGeneration, c.SystemGeneration) + return true + } + } + + klog.V(4).Infof("manifest Match for %q: %v", name, existing) + return false } func (c *Channel) GetInstalledVersion(ctx context.Context, k8sClient kubernetes.Interface) (*ChannelVersion, error) { diff --git a/nodeup/pkg/model/update_service.go b/nodeup/pkg/model/update_service.go index 1a970fe7bb..80ff4769f5 100644 --- a/nodeup/pkg/model/update_service.go +++ b/nodeup/pkg/model/update_service.go @@ -92,7 +92,7 @@ func (b *UpdateServiceBuilder) buildDebianPackage(c *fi.ModelBuilderContext) { ` } else { - klog.Infof("Detected OS %s; installing %s package", b.Distribution, debianPackageName) + klog.Infof("Detected OS %v; installing %s package", b.Distribution, debianPackageName) c.AddTask(&nodetasks.Package{Name: debianPackageName}) contents = `APT::Periodic::Update-Package-Lists "1";