From 79d6644c9d798b4ce683b56d45040e49754bf316 Mon Sep 17 00:00:00 2001 From: Ivan Kozlovic Date: Fri, 18 Feb 2022 17:13:53 -0700 Subject: [PATCH 1/2] KV: changing discard policy would fail on srv upgrade This is related to #900 that introduced the change of discard policy if connecting to a server v2.7.2+. The CreateKeyValue() API calls AddStream(). If the stream already exists and the configuration is identical, the call succeeds (idempotent). However, since we now try to set the discard policy to "new" when connecting to a server v2.7.2+, the call will fail if the KV store already existed (CreateKeyValue() was called with a v2.7.1 server and stream was created with DiscardOld). The approach here is that if AddStream() fails with an "already in use" error, then we will lookup the stream info, and if we detect that the configuration is same (except for the discard policy), then we call UpdateStream() with the new discard policy. The problematic part is that the client side configuration does not set some of the fields (or their value is 0), but then the server sets either some defaults (like Duplicates set to 2min) or replaces 0 to -1. So the info we get back and the config we have need to be tweaked before being compared. This is really hacky and prone to break if server were to change some defaults. Signed-off-by: Ivan Kozlovic --- kv.go | 36 +++++++++++++++++++++++++- kv_test.go | 69 +++++++++++++++++++++++++++++++++++++++++++++++++ test/kv_test.go | 43 ------------------------------ 3 files changed, 104 insertions(+), 44 deletions(-) create mode 100644 kv_test.go diff --git a/kv.go b/kv.go index 0b75054d9..435314c17 100644 --- a/kv.go +++ b/kv.go @@ -17,6 +17,7 @@ import ( "context" "errors" "fmt" + "reflect" "regexp" "strconv" "strings" @@ -346,7 +347,21 @@ func (js *js) CreateKeyValue(cfg *KeyValueConfig) (KeyValue, error) { } if _, err := js.AddStream(scfg); err != nil { - return nil, err + // If we have a failure to add, it could be because we have + // a config change if the KV was created against a pre 2.7.2 + // and we are now moving to a v2.7.2+. If that is the case + // and the only difference is the discard policy, then update + // the stream. + if strings.Contains(err.Error(), "already in use") { + if si, _ := js.StreamInfo(scfg.Name); si != nil { + if streamCfgSameSansDiscard(&si.Config, scfg) { + _, err = js.UpdateStream(scfg) + } + } + } + if err != nil { + return nil, err + } } kv := &kvs{ @@ -360,6 +375,25 @@ func (js *js) CreateKeyValue(cfg *KeyValueConfig) (KeyValue, error) { return kv, nil } +func streamCfgSameSansDiscard(srvCfg, ourCfg *StreamConfig) bool { + // The server sets some values (like -1) when sending 0. + // So we need to align to what the server may return to + // have a meaningfull comparison. + ours := *ourCfg + if ours.MaxMsgSize == 0 { + ours.MaxMsgSize = -1 + } + if ours.MaxBytes == 0 { + ours.MaxBytes = -1 + } + ours.Duplicates = 2 * time.Minute + ours.MaxMsgs = -1 + ours.MaxConsumers = -1 + // Set our discard to what server has for the comparison + ours.Discard = srvCfg.Discard + return reflect.DeepEqual(srvCfg, &ours) +} + // DeleteKeyValue will delete this KeyValue store (JetStream stream). func (js *js) DeleteKeyValue(bucket string) error { if !validBucketRe.MatchString(bucket) { diff --git a/kv_test.go b/kv_test.go new file mode 100644 index 000000000..a7cd03e20 --- /dev/null +++ b/kv_test.go @@ -0,0 +1,69 @@ +// Copyright 2022 The NATS Authors +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package nats + +import ( + "testing" +) + +func TestKeyValueDiscardOldToDiscardNew(t *testing.T) { + s := RunBasicJetStreamServer() + defer shutdownJSServerAndRemoveStorage(t, s) + + nc, js := jsClient(t, s) + defer nc.Close() + + checkDiscard := func(expected DiscardPolicy) KeyValue { + t.Helper() + kv, err := js.CreateKeyValue(&KeyValueConfig{Bucket: "TEST", History: 1}) + if err != nil { + t.Fatalf("Error creating store: %v", err) + } + si, err := js.StreamInfo("KV_TEST") + if err != nil { + t.Fatalf("Error getting stream info: %v", err) + } + if si.Config.Discard != expected { + t.Fatalf("Expected discard policy %v, got %+v", expected, si) + } + return kv + } + + // We are going to go from 2.7.1->2.7.2->2.7.1 and 2.7.2 again. + for i := 0; i < 2; i++ { + // Change the server version in the connection to + // create as-if we were connecting to a v2.7.1 server. + nc.mu.Lock() + nc.info.Version = "2.7.1" + nc.mu.Unlock() + + kv := checkDiscard(DiscardOld) + if i == 0 { + if _, err := kv.PutString("foo", "value"); err != nil { + t.Fatalf("Error adding key: %v", err) + } + } + + // Now change version to 2.7.2 + nc.mu.Lock() + nc.info.Version = "2.7.2" + nc.mu.Unlock() + + kv = checkDiscard(DiscardNew) + // Make sure the key still exists + if e, err := kv.Get("foo"); err != nil || string(e.Value()) != "value" { + t.Fatalf("Error getting key: err=%v e=%+v", err, e) + } + } +} diff --git a/test/kv_test.go b/test/kv_test.go index 831ad1315..7568f43bb 100644 --- a/test/kv_test.go +++ b/test/kv_test.go @@ -18,7 +18,6 @@ import ( "fmt" "os" "reflect" - "regexp" "strconv" "strings" "testing" @@ -558,48 +557,6 @@ func TestKeyValueKeys(t *testing.T) { } } -func TestKeyValueDiscardNew(t *testing.T) { - s := RunBasicJetStreamServer() - defer shutdownJSServerAndRemoveStorage(t, s) - - nc, js := jsClient(t, s) - defer nc.Close() - - kv, err := js.CreateKeyValue(&nats.KeyValueConfig{Bucket: "TEST", History: 1, MaxBytes: 256}) - expectOk(t, err) - - vc := func() (major, minor, patch int) { - semVerRe := regexp.MustCompile(`\Av?([0-9]+)\.?([0-9]+)?\.?([0-9]+)?`) - m := semVerRe.FindStringSubmatch(nc.ConnectedServerVersion()) - expectOk(t, err) - major, err = strconv.Atoi(m[1]) - expectOk(t, err) - minor, err = strconv.Atoi(m[2]) - expectOk(t, err) - patch, err = strconv.Atoi(m[3]) - expectOk(t, err) - return major, minor, patch - } - - major, minor, patch := vc() - status, err := kv.Status() - expectOk(t, err) - kvs := status.(*nats.KeyValueBucketStatus) - si := kvs.StreamInfo() - - // If we are 2.7.1 or below DiscardOld should be used. - // If 2.7.2 or above should be DiscardNew - if major <= 2 && minor <= 7 && patch <= 1 { - if si.Config.Discard != nats.DiscardOld { - t.Fatalf("Expected Discard Old for server version %d.%d.%d", major, minor, patch) - } - } else { - if si.Config.Discard != nats.DiscardNew { - t.Fatalf("Expected Discard New for server version %d.%d.%d", major, minor, patch) - } - } -} - func TestKeyValueCrossAccounts(t *testing.T) { conf := createConfFile(t, []byte(` jetstream: enabled From 5bdaa1fe036d1161bcbbb5b788648794327267d4 Mon Sep 17 00:00:00 2001 From: Ivan Kozlovic Date: Fri, 18 Feb 2022 18:40:51 -0700 Subject: [PATCH 2/2] Updates based on code review Signed-off-by: Ivan Kozlovic --- jsm.go | 3 +++ kv.go | 43 ++++++++++++++++++++----------------------- nats.go | 1 + 3 files changed, 24 insertions(+), 23 deletions(-) diff --git a/jsm.go b/jsm.go index 87ab37cec..129fb7642 100644 --- a/jsm.go +++ b/jsm.go @@ -587,6 +587,9 @@ func (js *js) AddStream(cfg *StreamConfig, opts ...JSOpt) (*StreamInfo, error) { return nil, err } if resp.Error != nil { + if resp.Error.ErrorCode == 10058 { + return nil, ErrStreamNameAlreadyInUse + } return nil, errors.New(resp.Error.Description) } diff --git a/kv.go b/kv.go index 435314c17..da33774e8 100644 --- a/kv.go +++ b/kv.go @@ -327,18 +327,31 @@ func (js *js) CreateKeyValue(cfg *KeyValueConfig) (KeyValue, error) { replicas = 1 } + // We will set explicitly some values so that we can do comparison + // if we get an "already in use" error and need to check if it is same. + maxBytes := cfg.MaxBytes + if maxBytes == 0 { + maxBytes = -1 + } + maxMsgSize := cfg.MaxValueSize + if maxMsgSize == 0 { + maxMsgSize = -1 + } scfg := &StreamConfig{ Name: fmt.Sprintf(kvBucketNameTmpl, cfg.Bucket), Description: cfg.Description, Subjects: []string{fmt.Sprintf(kvSubjectsTmpl, cfg.Bucket)}, MaxMsgsPerSubject: history, - MaxBytes: cfg.MaxBytes, + MaxBytes: maxBytes, MaxAge: cfg.TTL, - MaxMsgSize: cfg.MaxValueSize, + MaxMsgSize: maxMsgSize, Storage: cfg.Storage, Replicas: replicas, AllowRollup: true, DenyDelete: true, + Duplicates: 2 * time.Minute, + MaxMsgs: -1, + MaxConsumers: -1, } // If we are at server version 2.7.2 or above use DiscardNew. We can not use DiscardNew for 2.7.1 or below. @@ -352,9 +365,12 @@ func (js *js) CreateKeyValue(cfg *KeyValueConfig) (KeyValue, error) { // and we are now moving to a v2.7.2+. If that is the case // and the only difference is the discard policy, then update // the stream. - if strings.Contains(err.Error(), "already in use") { + if err == ErrStreamNameAlreadyInUse { if si, _ := js.StreamInfo(scfg.Name); si != nil { - if streamCfgSameSansDiscard(&si.Config, scfg) { + // To compare, make the server's stream info discard + // policy same than ours. + si.Config.Discard = scfg.Discard + if reflect.DeepEqual(&si.Config, scfg) { _, err = js.UpdateStream(scfg) } } @@ -375,25 +391,6 @@ func (js *js) CreateKeyValue(cfg *KeyValueConfig) (KeyValue, error) { return kv, nil } -func streamCfgSameSansDiscard(srvCfg, ourCfg *StreamConfig) bool { - // The server sets some values (like -1) when sending 0. - // So we need to align to what the server may return to - // have a meaningfull comparison. - ours := *ourCfg - if ours.MaxMsgSize == 0 { - ours.MaxMsgSize = -1 - } - if ours.MaxBytes == 0 { - ours.MaxBytes = -1 - } - ours.Duplicates = 2 * time.Minute - ours.MaxMsgs = -1 - ours.MaxConsumers = -1 - // Set our discard to what server has for the comparison - ours.Discard = srvCfg.Discard - return reflect.DeepEqual(srvCfg, &ours) -} - // DeleteKeyValue will delete this KeyValue store (JetStream stream). func (js *js) DeleteKeyValue(bucket string) error { if !validBucketRe.MatchString(bucket) { diff --git a/nats.go b/nats.go index 3bf5b4d57..bc2fe5d56 100644 --- a/nats.go +++ b/nats.go @@ -160,6 +160,7 @@ var ( ErrMsgNotFound = errors.New("nats: message not found") ErrMsgAlreadyAckd = errors.New("nats: message was already acknowledged") ErrStreamInfoMaxSubjects = errors.New("nats: subject details would exceed maximum allowed") + ErrStreamNameAlreadyInUse = errors.New("nats: stream name already in use") ) func init() {