From cf1a1b9dd358cc7a42a73e80599acd13e7a27424 Mon Sep 17 00:00:00 2001 From: Nikita Bhatia Date: Fri, 28 Apr 2023 10:52:34 +0530 Subject: [PATCH 1/5] PWX-30230 : Create all px configmaps in same namespace as portworx Signed-off-by: Nikita Bhatia --- k8s/core/configmap/configmap.go | 229 ++++++++---------------- k8s/core/configmap/configmap_lock_v1.go | 10 +- k8s/core/configmap/configmap_lock_v2.go | 20 +-- k8s/core/configmap/coreconfigmap.go | 192 ++++++++++++++++++++ k8s/core/configmap/types.go | 7 + 5 files changed, 286 insertions(+), 172 deletions(-) create mode 100644 k8s/core/configmap/coreconfigmap.go diff --git a/k8s/core/configmap/configmap.go b/k8s/core/configmap/configmap.go index ed50972b..849c1ede 100644 --- a/k8s/core/configmap/configmap.go +++ b/k8s/core/configmap/configmap.go @@ -1,188 +1,103 @@ package configmap import ( - "fmt" - "strings" "time" - "github.com/portworx/sched-ops/k8s/core" - "github.com/sirupsen/logrus" - corev1 "k8s.io/api/core/v1" - k8s_errors "k8s.io/apimachinery/pkg/api/errors" - meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" + "github.com/pborman/uuid" + log "github.com/sirupsen/logrus" ) -// New returns the ConfigMap interface. It also creates a new -// configmap in k8s for the given name if not present and puts the data in it. -func New( - name string, - data map[string]string, - lockTimeout time.Duration, - lockAttempts uint, - v2LockRefreshDuration time.Duration, - v2LockK8sLockTTL time.Duration, -) (ConfigMap, error) { - if data == nil { - data = make(map[string]string) - } - - labels := map[string]string{ - configMapUserLabelKey: TruncateLabel(name), - } - data[pxOwnerKey] = "" - - cm := &corev1.ConfigMap{ - ObjectMeta: meta_v1.ObjectMeta{ - Name: name, - Namespace: k8sSystemNamespace, - Labels: labels, - }, - Data: data, - } +func (c *configMap) Instance() ConfigMap { - if _, err := core.Instance().CreateConfigMap(cm); err != nil && - !k8s_errors.IsAlreadyExists(err) { - return nil, fmt.Errorf("failed to create configmap %v: %v", - name, err) - } - - if v2LockK8sLockTTL == 0 { - v2LockK8sLockTTL = v2DefaultK8sLockTTL - } + //iskvdbhealthy + //kvdb.instance - if v2LockRefreshDuration == 0 { - v2LockRefreshDuration = v2DefaultK8sLockRefreshDuration + if c.pxNs == c.config.nameSpace { + //fresh install + //upgrade completed + return c.config + } else { + existingConfig := c.config + c.copylock.Lock(uuid.New()) + defer c.copylock.Unlock() + lockMap, err := c.copylock.Get() + if err != nil { + log.Error("Error during fetching data from copy lock %s", err) + return existingConfig + } + status := lockMap["UPGRADE_DONE"] + if status == "true" { + // upgrade is completed + //create configmap in portworx namespace + newConfig := &coreConfigMap{ + name: existingConfig.name, + defaultLockHoldTimeout: existingConfig.defaultLockHoldTimeout, + kLocksV2: existingConfig.kLocksV2, + lockAttempts: existingConfig.lockAttempts, + lockRefreshDuration: existingConfig.lockRefreshDuration, + lockK8sLockTTL: existingConfig.lockK8sLockTTL, + nameSpace: "portworx", + } + + configData, err := existingConfig.Get() + if err != nil { + log.Errorf("Error during fetching data from old config map %s", err) + return existingConfig + } + //copy data from old configmap to new configmap + if err = newConfig.Update(configData); err != nil { + log.Errorf("Error during copying data from old config map %s", err) + return existingConfig + } + + //delete old configmap + err = c.config.Delete() + if err != nil { + log.Errorf("Error during deleting configmap %s in namespace %s ", c.config.name, c.config.nameSpace) + } + c.config = newConfig + } else { + return existingConfig + } } - - return &configMap{ - name: name, - defaultLockHoldTimeout: lockTimeout, - kLocksV2: map[string]*k8sLock{}, - lockAttempts: lockAttempts, - lockRefreshDuration: v2LockRefreshDuration, - lockK8sLockTTL: v2LockK8sLockTTL, - }, nil + return c.config } func (c *configMap) Get() (map[string]string, error) { - cm, err := core.Instance().GetConfigMap( - c.name, - k8sSystemNamespace, - ) - if err != nil { - return nil, err - } - - return cm.Data, nil + return c.Instance().Get() } func (c *configMap) Delete() error { - return core.Instance().DeleteConfigMap( - c.name, - k8sSystemNamespace, - ) + return c.Instance().Delete() } func (c *configMap) Patch(data map[string]string) error { - var ( - err error - cm *corev1.ConfigMap - ) - for retries := 0; retries < maxConflictRetries; retries++ { - cm, err = core.Instance().GetConfigMap( - c.name, - k8sSystemNamespace, - ) - if err != nil { - return err - } - - if cm.Data == nil { - cm.Data = make(map[string]string, 0) - } - - for k, v := range data { - cm.Data[k] = v - } - _, err = core.Instance().UpdateConfigMap(cm) - if k8s_errors.IsConflict(err) { - // try again - continue - } - return err - } - return err + return c.Instance().Patch(data) } func (c *configMap) Update(data map[string]string) error { - var ( - err error - cm *corev1.ConfigMap - ) - for retries := 0; retries < maxConflictRetries; retries++ { - cm, err = core.Instance().GetConfigMap( - c.name, - k8sSystemNamespace, - ) - if err != nil { - return err - } - cm.Data = data - _, err = core.Instance().UpdateConfigMap(cm) - if k8s_errors.IsConflict(err) { - // try again - continue - } - return err - } - return err + return c.Instance().Update(data) } -// SetFatalCb sets the fatal callback for the package which will get invoked in panic situations -func SetFatalCb(fb FatalCb) { - fatalCb = fb +func (c *configMap) Lock(id string) error { + return c.Instance().Lock(id) } -func configMapLog(fn, name, owner, key string, err error) *logrus.Entry { - if len(owner) > 0 && len(key) > 0 { - return logrus.WithFields(logrus.Fields{ - "Module": "ConfigMap", - "Name": name, - "Owner": owner, - "Key": key, - "Function": fn, - "Error": err, - }) - } - if len(owner) > 0 { - return logrus.WithFields(logrus.Fields{ - "Module": "ConfigMap", - "Name": name, - "Owner": owner, - "Function": fn, - "Error": err, - }) - } - return logrus.WithFields(logrus.Fields{ - "Module": "ConfigMap", - "Name": name, - "Function": fn, - "Error": err, - }) +func (c *configMap) LockWithHoldTimeout(id string, holdTimeout time.Duration) error { + return c.Instance().LockWithHoldTimeout(id, holdTimeout) } -// GetName is a helper function that returns a valid k8s -// configmap name given a prefix identifying the component using -// the configmap and a clusterID -func GetName(prefix, clusterID string) string { - return prefix + strings.ToLower(configMapNameRegex.ReplaceAllString(clusterID, "")) +func (c *configMap) LockWithKey(owner, key string) error { + return c.Instance().LockWithKey(owner, key) } -// TruncateLabel is a helper function that returns a valid k8s -// label stripped down to 63 characters. It removes the trailing characters -func TruncateLabel(label string) string { - if len(label) > 63 { - return label[:63] - } - return label +func (c *configMap) Unlock() error { + return c.Instance().Unlock() +} + +func (c *configMap) UnlockWithKey(key string) error { + return c.Instance().UnlockWithKey(key) +} +func (c *configMap) IsKeyLocked(key string) (bool, string, error) { + return c.Instance().IsKeyLocked(key) } diff --git a/k8s/core/configmap/configmap_lock_v1.go b/k8s/core/configmap/configmap_lock_v1.go index ea98317b..9d545ffb 100644 --- a/k8s/core/configmap/configmap_lock_v1.go +++ b/k8s/core/configmap/configmap_lock_v1.go @@ -8,11 +8,11 @@ import ( k8s_errors "k8s.io/apimachinery/pkg/api/errors" ) -func (c *configMap) Lock(id string) error { +func (c *coreConfigMap) Lock(id string) error { return c.LockWithHoldTimeout(id, c.defaultLockHoldTimeout) } -func (c *configMap) LockWithHoldTimeout(id string, holdTimeout time.Duration) error { +func (c *coreConfigMap) LockWithHoldTimeout(id string, holdTimeout time.Duration) error { fn := "LockWithHoldTimeout" count := uint(0) // try acquiring a lock on the ConfigMap @@ -40,7 +40,7 @@ func (c *configMap) LockWithHoldTimeout(id string, holdTimeout time.Duration) er return nil } -func (c *configMap) Unlock() error { +func (c *coreConfigMap) Unlock() error { fn := "Unlock" // Get the existing ConfigMap c.kLockV1.Lock() @@ -91,7 +91,7 @@ func (c *configMap) Unlock() error { return err } -func (c *configMap) tryLockV1(id string, refresh bool) (string, error) { +func (c *coreConfigMap) tryLockV1(id string, refresh bool) (string, error) { // Get the existing ConfigMap cm, err := core.Instance().GetConfigMap( c.name, @@ -140,7 +140,7 @@ increase_expiry: return id, nil } -func (c *configMap) refreshLockV1(id string) { +func (c *coreConfigMap) refreshLockV1(id string) { fn := "refreshLock" refresh := time.NewTicker(v1DefaultK8sLockRefreshDuration) var ( diff --git a/k8s/core/configmap/configmap_lock_v2.go b/k8s/core/configmap/configmap_lock_v2.go index 794c7c03..d09de63a 100644 --- a/k8s/core/configmap/configmap_lock_v2.go +++ b/k8s/core/configmap/configmap_lock_v2.go @@ -12,7 +12,7 @@ import ( k8s_errors "k8s.io/apimachinery/pkg/api/errors" ) -func (c *configMap) LockWithKey(owner, key string) error { +func (c *coreConfigMap) LockWithKey(owner, key string) error { if key == "" { return fmt.Errorf("key cannot be empty") } @@ -54,7 +54,7 @@ func (c *configMap) LockWithKey(owner, key string) error { return nil } -func (c *configMap) UnlockWithKey(key string) error { +func (c *coreConfigMap) UnlockWithKey(key string) error { if key == "" { return fmt.Errorf("key cannot be empty") } @@ -136,7 +136,7 @@ func (c *configMap) UnlockWithKey(key string) error { return err } -func (c *configMap) IsKeyLocked(key string) (bool, string, error) { +func (c *coreConfigMap) IsKeyLocked(key string) (bool, string, error) { // Get the existing ConfigMap cm, err := core.Instance().GetConfigMap( c.name, @@ -170,7 +170,7 @@ func (c *configMap) IsKeyLocked(key string) (bool, string, error) { return false, "", nil } -func (c *configMap) tryLock(owner string, key string) (string, error) { +func (c *coreConfigMap) tryLock(owner string, key string) (string, error) { // Get the existing ConfigMap cm, err := core.Instance().GetConfigMap( c.name, @@ -209,7 +209,7 @@ func (c *configMap) tryLock(owner string, key string) (string, error) { // parseLocks reads the lock data from the given ConfigMap and then converts it to: // * a map of keys to lock owners // * a map of keys to lock expiration times -func (c *configMap) parseLocks(cm *v1.ConfigMap) (map[string]string, map[string]time.Time, error) { +func (c *coreConfigMap) parseLocks(cm *v1.ConfigMap) (map[string]string, map[string]time.Time, error) { // Check all the locks: will be an empty string if key is not present indicating no lock parsedLocks := []lockData{} if lock, ok := cm.Data[pxLockKey]; ok && len(lock) > 0 { @@ -233,7 +233,7 @@ func (c *configMap) parseLocks(cm *v1.ConfigMap) (map[string]string, map[string] // checkAndTakeLock tries to take the given lock (owner, key) given the current state of the lock // (lockOwners, lockExpirations). -func (c *configMap) checkAndTakeLock( +func (c *coreConfigMap) checkAndTakeLock( owner, key string, lockOwners map[string]string, lockExpirations map[string]time.Time, @@ -279,7 +279,7 @@ func (c *configMap) checkAndTakeLock( // generateConfigMapData converts the given lock data (lockOwners, lockExpirations) to JSON and // stores it in the given ConfigMap. -func (c *configMap) generateConfigMapData(cm *v1.ConfigMap, lockOwners map[string]string, lockExpirations map[string]time.Time) error { +func (c *coreConfigMap) generateConfigMapData(cm *v1.ConfigMap, lockOwners map[string]string, lockExpirations map[string]time.Time) error { var locks []lockData for key, lockOwner := range lockOwners { locks = append(locks, lockData{ @@ -297,7 +297,7 @@ func (c *configMap) generateConfigMapData(cm *v1.ConfigMap, lockOwners map[strin return nil } -func (c *configMap) updateConfigMap(cm *v1.ConfigMap) (bool, error) { +func (c *coreConfigMap) updateConfigMap(cm *v1.ConfigMap) (bool, error) { if _, err := core.Instance().UpdateConfigMap(cm); err != nil { return k8s_errors.IsConflict(err), err } @@ -308,7 +308,7 @@ func (c *configMap) updateConfigMap(cm *v1.ConfigMap) (bool, error) { // It keeps the lock refreshed in k8s until we call Unlock. This is so that if the // node dies, the lock can have a short timeout and expire quickly but we can still // take longer-term locks. -func (c *configMap) refreshLock(id, key string) { +func (c *coreConfigMap) refreshLock(id, key string) { fn := "refreshLock" refresh := time.NewTicker(c.lockRefreshDuration) var ( @@ -356,7 +356,7 @@ func (c *configMap) refreshLock(id, key string) { } -func (c *configMap) checkLockTimeout(holdTimeout time.Duration, startTime time.Time, id string) { +func (c *coreConfigMap) checkLockTimeout(holdTimeout time.Duration, startTime time.Time, id string) { if holdTimeout > 0 && time.Since(startTime) > holdTimeout { panicMsg := fmt.Sprintf("Lock hold timeout (%v) triggered for K8s configmap lock key %s", holdTimeout, id) if fatalCb != nil { diff --git a/k8s/core/configmap/coreconfigmap.go b/k8s/core/configmap/coreconfigmap.go new file mode 100644 index 00000000..861f565f --- /dev/null +++ b/k8s/core/configmap/coreconfigmap.go @@ -0,0 +1,192 @@ +package configmap + +import ( + "fmt" + "strings" + "time" + + "github.com/portworx/sched-ops/k8s/core" + "github.com/sirupsen/logrus" + corev1 "k8s.io/api/core/v1" + k8s_errors "k8s.io/apimachinery/pkg/api/errors" + meta_v1 "k8s.io/apimachinery/pkg/apis/meta/v1" +) + +// New returns the ConfigMap interface. It also creates a new +// configmap in k8s for the given name if not present and puts the data in it. +func New( + name string, + data map[string]string, + lockTimeout time.Duration, + lockAttempts uint, + v2LockRefreshDuration time.Duration, + v2LockK8sLockTTL time.Duration, + ns string, +) (ConfigMap, error) { + if data == nil { + data = make(map[string]string) + } + + //if copylock not created, then create + + labels := map[string]string{ + configMapUserLabelKey: TruncateLabel(name), + } + data[pxOwnerKey] = "" + + cm := &corev1.ConfigMap{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: name, + Namespace: ns, + Labels: labels, + }, + Data: data, + } + + if _, err := core.Instance().CreateConfigMap(cm); err != nil && + !k8s_errors.IsAlreadyExists(err) { + return nil, fmt.Errorf("failed to create configmap %v: %v", + name, err) + } + + if v2LockK8sLockTTL == 0 { + v2LockK8sLockTTL = v2DefaultK8sLockTTL + } + + if v2LockRefreshDuration == 0 { + v2LockRefreshDuration = v2DefaultK8sLockRefreshDuration + } + + return &coreConfigMap{ + name: name, + defaultLockHoldTimeout: lockTimeout, + kLocksV2: map[string]*k8sLock{}, + lockAttempts: lockAttempts, + lockRefreshDuration: v2LockRefreshDuration, + lockK8sLockTTL: v2LockK8sLockTTL, + nameSpace: ns, + }, nil +} + +func (c *coreConfigMap) Get() (map[string]string, error) { + cm, err := core.Instance().GetConfigMap( + c.name, + c.nameSpace, + ) + if err != nil { + return nil, err + } + + return cm.Data, nil +} + +func (c *coreConfigMap) Delete() error { + return core.Instance().DeleteConfigMap( + c.name, + c.nameSpace, + ) +} + +func (c *coreConfigMap) Patch(data map[string]string) error { + var ( + err error + cm *corev1.ConfigMap + ) + for retries := 0; retries < maxConflictRetries; retries++ { + cm, err = core.Instance().GetConfigMap( + c.name, + c.nameSpace, + ) + if err != nil { + return err + } + + if cm.Data == nil { + cm.Data = make(map[string]string, 0) + } + + for k, v := range data { + cm.Data[k] = v + } + _, err = core.Instance().UpdateConfigMap(cm) + if k8s_errors.IsConflict(err) { + // try again + continue + } + return err + } + return err +} + +func (c *coreConfigMap) Update(data map[string]string) error { + var ( + err error + cm *corev1.ConfigMap + ) + for retries := 0; retries < maxConflictRetries; retries++ { + cm, err = core.Instance().GetConfigMap( + c.name, + c.nameSpace, + ) + if err != nil { + return err + } + cm.Data = data + _, err = core.Instance().UpdateConfigMap(cm) + if k8s_errors.IsConflict(err) { + // try again + continue + } + return err + } + return err +} + +// SetFatalCb sets the fatal callback for the package which will get invoked in panic situations +func SetFatalCb(fb FatalCb) { + fatalCb = fb +} + +func configMapLog(fn, name, owner, key string, err error) *logrus.Entry { + if len(owner) > 0 && len(key) > 0 { + return logrus.WithFields(logrus.Fields{ + "Module": "ConfigMap", + "Name": name, + "Owner": owner, + "Key": key, + "Function": fn, + "Error": err, + }) + } + if len(owner) > 0 { + return logrus.WithFields(logrus.Fields{ + "Module": "ConfigMap", + "Name": name, + "Owner": owner, + "Function": fn, + "Error": err, + }) + } + return logrus.WithFields(logrus.Fields{ + "Module": "ConfigMap", + "Name": name, + "Function": fn, + "Error": err, + }) +} + +// GetName is a helper function that returns a valid k8s +// configmap name given a prefix identifying the component using +// the configmap and a clusterID +func GetName(prefix, clusterID string) string { + return prefix + strings.ToLower(configMapNameRegex.ReplaceAllString(clusterID, "")) +} + +// TruncateLabel is a helper function that returns a valid k8s +// label stripped down to 63 characters. It removes the trailing characters +func TruncateLabel(label string) string { + if len(label) > 63 { + return label[:63] + } + return label +} diff --git a/k8s/core/configmap/types.go b/k8s/core/configmap/types.go index 97b43d1d..4722526b 100644 --- a/k8s/core/configmap/types.go +++ b/k8s/core/configmap/types.go @@ -58,7 +58,14 @@ var ( type FatalCb func(format string, args ...interface{}) type configMap struct { + config *coreConfigMap + pxNs string + copylock *coreConfigMap +} + +type coreConfigMap struct { name string + nameSpace string kLockV1 k8sLock kLocksV2Mutex sync.Mutex kLocksV2 map[string]*k8sLock From a4ecedec4d06f3800b53eb1d73d144c589e1c912 Mon Sep 17 00:00:00 2001 From: Nikita Bhatia Date: Fri, 28 Apr 2023 16:33:28 +0530 Subject: [PATCH 2/5] Fix failing test Signed-off-by: Nikita Bhatia --- k8s/core/configmap/configmap.go | 6 +- k8s/core/configmap/configmap_lock_v1.go | 4 +- k8s/core/configmap/configmap_lock_v1_test.go | 4 +- k8s/core/configmap/configmap_lock_v2.go | 6 +- k8s/core/configmap/configmap_lock_v2_test.go | 2 +- k8s/core/configmap/configmap_test.go | 90 ++++++++++++++++++++ k8s/core/configmap/coreconfigmap.go | 7 +- 7 files changed, 107 insertions(+), 12 deletions(-) create mode 100644 k8s/core/configmap/configmap_test.go diff --git a/k8s/core/configmap/configmap.go b/k8s/core/configmap/configmap.go index 849c1ede..10e55089 100644 --- a/k8s/core/configmap/configmap.go +++ b/k8s/core/configmap/configmap.go @@ -3,7 +3,7 @@ package configmap import ( "time" - "github.com/pborman/uuid" + "github.com/google/uuid" log "github.com/sirupsen/logrus" ) @@ -18,11 +18,11 @@ func (c *configMap) Instance() ConfigMap { return c.config } else { existingConfig := c.config - c.copylock.Lock(uuid.New()) + c.copylock.Lock(uuid.New().String()) defer c.copylock.Unlock() lockMap, err := c.copylock.Get() if err != nil { - log.Error("Error during fetching data from copy lock %s", err) + log.Errorf("Error during fetching data from copy lock %s", err) return existingConfig } status := lockMap["UPGRADE_DONE"] diff --git a/k8s/core/configmap/configmap_lock_v1.go b/k8s/core/configmap/configmap_lock_v1.go index 9d545ffb..3a15037c 100644 --- a/k8s/core/configmap/configmap_lock_v1.go +++ b/k8s/core/configmap/configmap_lock_v1.go @@ -60,7 +60,7 @@ func (c *coreConfigMap) Unlock() error { for retries := 0; retries < maxConflictRetries; retries++ { cm, err = core.Instance().GetConfigMap( c.name, - k8sSystemNamespace, + c.nameSpace, ) if err != nil { // A ConfigMap should always be created. @@ -95,7 +95,7 @@ func (c *coreConfigMap) tryLockV1(id string, refresh bool) (string, error) { // Get the existing ConfigMap cm, err := core.Instance().GetConfigMap( c.name, - k8sSystemNamespace, + c.nameSpace, ) if err != nil { // A ConfigMap should always be created. diff --git a/k8s/core/configmap/configmap_lock_v1_test.go b/k8s/core/configmap/configmap_lock_v1_test.go index 03e29338..b7f2316a 100644 --- a/k8s/core/configmap/configmap_lock_v1_test.go +++ b/k8s/core/configmap/configmap_lock_v1_test.go @@ -13,7 +13,7 @@ import ( func TestLock(t *testing.T) { fakeClient := fakek8sclient.NewSimpleClientset() coreops.SetInstance(coreops.New(fakeClient)) - cm, err := New("px-configmaps-test", nil, lockTimeout, 5, 0, 0) + cm, err := New("px-configmaps-test", nil, lockTimeout, 5, 0, 0, "test-ns") require.NoError(t, err, "Unexpected error on New") fmt.Println("testLock") @@ -112,7 +112,7 @@ func TestLockWithHoldTimeout(t *testing.T) { customHoldTimeout := defaultHoldTimeout + v1DefaultK8sLockRefreshDuration + 10*time.Second fakeClient := fakek8sclient.NewSimpleClientset() coreops.SetInstance(coreops.New(fakeClient)) - cm, err := New("px-configmaps-test", nil, defaultHoldTimeout, 5, 0, 0) + cm, err := New("px-configmaps-test", nil, defaultHoldTimeout, 5, 0, 0, "test-ns") require.NoError(t, err, "Unexpected error on New") fmt.Println("TestLockWithHoldTimeout") diff --git a/k8s/core/configmap/configmap_lock_v2.go b/k8s/core/configmap/configmap_lock_v2.go index d09de63a..12c5fa42 100644 --- a/k8s/core/configmap/configmap_lock_v2.go +++ b/k8s/core/configmap/configmap_lock_v2.go @@ -89,7 +89,7 @@ func (c *coreConfigMap) UnlockWithKey(key string) error { for retries := 0; retries < maxConflictRetries; retries++ { cm, err = core.Instance().GetConfigMap( c.name, - k8sSystemNamespace, + c.nameSpace, ) if err != nil { // A ConfigMap should always be created. @@ -140,7 +140,7 @@ func (c *coreConfigMap) IsKeyLocked(key string) (bool, string, error) { // Get the existing ConfigMap cm, err := core.Instance().GetConfigMap( c.name, - k8sSystemNamespace, + c.nameSpace, ) if err != nil { return false, "", err @@ -174,7 +174,7 @@ func (c *coreConfigMap) tryLock(owner string, key string) (string, error) { // Get the existing ConfigMap cm, err := core.Instance().GetConfigMap( c.name, - k8sSystemNamespace, + c.nameSpace, ) if err != nil { // A ConfigMap should always be created. diff --git a/k8s/core/configmap/configmap_lock_v2_test.go b/k8s/core/configmap/configmap_lock_v2_test.go index 74955bc6..d317af1f 100644 --- a/k8s/core/configmap/configmap_lock_v2_test.go +++ b/k8s/core/configmap/configmap_lock_v2_test.go @@ -17,7 +17,7 @@ const ( func TestMultilock(t *testing.T) { fakeClient := fakek8sclient.NewSimpleClientset() coreops.SetInstance(coreops.New(fakeClient)) - cm, err := New("px-configmaps-test", nil, lockTimeout, 3, 0, 0) + cm, err := New("px-configmaps-test", nil, lockTimeout, 3, 0, 0, "test-ns") require.NoError(t, err, "Unexpected error on New") fmt.Println("testMultilock") diff --git a/k8s/core/configmap/configmap_test.go b/k8s/core/configmap/configmap_test.go new file mode 100644 index 00000000..dc60256a --- /dev/null +++ b/k8s/core/configmap/configmap_test.go @@ -0,0 +1,90 @@ +package configmap + +import ( + "fmt" + "testing" + + "github.com/stretchr/testify/require" + + coreops "github.com/portworx/sched-ops/k8s/core" + fakek8sclient "k8s.io/client-go/kubernetes/fake" +) + +func TestGetConfigMap(t *testing.T) { + fakeClient := fakek8sclient.NewSimpleClientset() + coreops.SetInstance(coreops.New(fakeClient)) + + configData := map[string]string{ + "key1": "val1", + } + cm, err := New("px-configmaps-test", configData, lockTimeout, 5, 0, 0, "test-ns") + fmt.Println("cm : ", cm) + require.NoError(t, err, "Unexpected error in creating configmap") + + resultMap, err := cm.Get() + require.NoError(t, err, "Unexpected error in getting configmap") + require.Contains(t, resultMap, "key1") + fmt.Println(resultMap) +} + +func TestDeleteConfigMap(t *testing.T) { + fakeClient := fakek8sclient.NewSimpleClientset() + coreops.SetInstance(coreops.New(fakeClient)) + + configData := map[string]string{ + "key1": "val1", + } + + cm, err := New("px-configmaps-test", configData, lockTimeout, 5, 0, 0, "test-ns") + require.NoError(t, err, "Unexpected error in creating configmap") + + err = cm.Delete() + require.NoError(t, err, "Unexpected error in delete") + +} + +func TestPatchConfigMap(t *testing.T) { + fakeClient := fakek8sclient.NewSimpleClientset() + coreops.SetInstance(coreops.New(fakeClient)) + + configData := map[string]string{ + "key1": "val1", + } + + cm, err := New("px-configmaps-test", configData, lockTimeout, 5, 0, 0, "test-ns") + require.NoError(t, err, "Unexpected error in creating configmap") + + dummyData := map[string]string{ + "key2": "val2", + } + + err = cm.Patch(dummyData) + require.NoError(t, err, "Unexpected error in Patch") + resultMap, err := cm.Get() + require.Contains(t, resultMap, "key1") + require.Contains(t, resultMap, "key2") + fmt.Println(resultMap) +} + +func TestUpdateConfigMap(t *testing.T) { + fakeClient := fakek8sclient.NewSimpleClientset() + coreops.SetInstance(coreops.New(fakeClient)) + + configData := map[string]string{ + "key1": "val1", + } + + cm, err := New("px-configmaps-test", configData, lockTimeout, 5, 0, 0, "test-ns") + require.NoError(t, err, "Unexpected error in creating configmap") + + dummyData := map[string]string{ + "key2": "val2", + } + + err = cm.Update(dummyData) + require.NoError(t, err, "Unexpected error in Update") + resultMap, err := cm.Get() + require.NotContains(t, resultMap, "key1") + require.Contains(t, resultMap, "key2") + fmt.Println(resultMap) +} diff --git a/k8s/core/configmap/coreconfigmap.go b/k8s/core/configmap/coreconfigmap.go index 861f565f..928729f2 100644 --- a/k8s/core/configmap/coreconfigmap.go +++ b/k8s/core/configmap/coreconfigmap.go @@ -57,7 +57,7 @@ func New( v2LockRefreshDuration = v2DefaultK8sLockRefreshDuration } - return &coreConfigMap{ + config := &coreConfigMap{ name: name, defaultLockHoldTimeout: lockTimeout, kLocksV2: map[string]*k8sLock{}, @@ -65,6 +65,11 @@ func New( lockRefreshDuration: v2LockRefreshDuration, lockK8sLockTTL: v2LockK8sLockTTL, nameSpace: ns, + } + return &configMap{ + config: config, + pxNs: ns, + copylock: nil, }, nil } From 7d335c9dc99f1008c3b680c7eeadf5a04db3ef18 Mon Sep 17 00:00:00 2001 From: Nikita Bhatia Date: Mon, 1 May 2023 22:26:22 +0530 Subject: [PATCH 3/5] create copylock configmap Signed-off-by: Nikita Bhatia --- k8s/core/configmap/configmap.go | 31 +++++++++++-------------- k8s/core/configmap/coreconfigmap.go | 36 +++++++++++++++++++++++++---- k8s/core/configmap/types.go | 11 ++++++--- k8s/core/configmaps.go | 2 ++ 4 files changed, 55 insertions(+), 25 deletions(-) diff --git a/k8s/core/configmap/configmap.go b/k8s/core/configmap/configmap.go index 10e55089..965587d5 100644 --- a/k8s/core/configmap/configmap.go +++ b/k8s/core/configmap/configmap.go @@ -7,26 +7,23 @@ import ( log "github.com/sirupsen/logrus" ) -func (c *configMap) Instance() ConfigMap { - - //iskvdbhealthy - //kvdb.instance +func (c *configMap) Instance() *coreConfigMap { if c.pxNs == c.config.nameSpace { - //fresh install - //upgrade completed + //fresh install ot upgrade completed return c.config } else { existingConfig := c.config c.copylock.Lock(uuid.New().String()) defer c.copylock.Unlock() - lockMap, err := c.copylock.Get() + + lockMap, err := c.copylock.get() if err != nil { log.Errorf("Error during fetching data from copy lock %s", err) return existingConfig } - status := lockMap["UPGRADE_DONE"] - if status == "true" { + status := lockMap[upgradeCompletedStatus] + if status == true { // upgrade is completed //create configmap in portworx namespace newConfig := &coreConfigMap{ @@ -36,22 +33,22 @@ func (c *configMap) Instance() ConfigMap { lockAttempts: existingConfig.lockAttempts, lockRefreshDuration: existingConfig.lockRefreshDuration, lockK8sLockTTL: existingConfig.lockK8sLockTTL, - nameSpace: "portworx", + nameSpace: pxNamespace, } - configData, err := existingConfig.Get() + configData, err := existingConfig.get() if err != nil { log.Errorf("Error during fetching data from old config map %s", err) return existingConfig } //copy data from old configmap to new configmap - if err = newConfig.Update(configData); err != nil { + if err = newConfig.update(configData); err != nil { log.Errorf("Error during copying data from old config map %s", err) return existingConfig } //delete old configmap - err = c.config.Delete() + err = c.config.delete() if err != nil { log.Errorf("Error during deleting configmap %s in namespace %s ", c.config.name, c.config.nameSpace) } @@ -64,19 +61,19 @@ func (c *configMap) Instance() ConfigMap { } func (c *configMap) Get() (map[string]string, error) { - return c.Instance().Get() + return c.Instance().get() } func (c *configMap) Delete() error { - return c.Instance().Delete() + return c.Instance().delete() } func (c *configMap) Patch(data map[string]string) error { - return c.Instance().Patch(data) + return c.Instance().patch(data) } func (c *configMap) Update(data map[string]string) error { - return c.Instance().Update(data) + return c.Instance().update(data) } func (c *configMap) Lock(id string) error { diff --git a/k8s/core/configmap/coreconfigmap.go b/k8s/core/configmap/coreconfigmap.go index 928729f2..3ac72149 100644 --- a/k8s/core/configmap/coreconfigmap.go +++ b/k8s/core/configmap/coreconfigmap.go @@ -66,14 +66,40 @@ func New( lockK8sLockTTL: v2LockK8sLockTTL, nameSpace: ns, } + + cm1 := &corev1.ConfigMap{ + ObjectMeta: meta_v1.ObjectMeta{ + Name: pxCopyLockConfigMap, + Namespace: pxNamespace, + }, + Data: map[string]string{ + upgradeCompletedStatus: true, + }, + } + + copyLock := &coreConfigMap{} + _, err := core.Instance().CreateConfigMap(cm1) + + if err != nil && !k8s_errors.IsAlreadyExists(err) { + fmt.Println("Failed to create configmap-copylock") + return nil, fmt.Errorf("failed to create configmap %v: %v", + name, err) + } else { + copyLock.name = pxCopyLockConfigMap + copyLock.kLocksV2 = map[string]*k8sLock{} + copyLock.lockRefreshDuration = v2LockRefreshDuration + copyLock.lockK8sLockTTL = v2LockK8sLockTTL + copyLock.nameSpace = pxNamespace + } + return &configMap{ config: config, pxNs: ns, - copylock: nil, + copylock: copyLock, }, nil } -func (c *coreConfigMap) Get() (map[string]string, error) { +func (c *coreConfigMap) get() (map[string]string, error) { cm, err := core.Instance().GetConfigMap( c.name, c.nameSpace, @@ -85,14 +111,14 @@ func (c *coreConfigMap) Get() (map[string]string, error) { return cm.Data, nil } -func (c *coreConfigMap) Delete() error { +func (c *coreConfigMap) delete() error { return core.Instance().DeleteConfigMap( c.name, c.nameSpace, ) } -func (c *coreConfigMap) Patch(data map[string]string) error { +func (c *coreConfigMap) patch(data map[string]string) error { var ( err error cm *corev1.ConfigMap @@ -123,7 +149,7 @@ func (c *coreConfigMap) Patch(data map[string]string) error { return err } -func (c *coreConfigMap) Update(data map[string]string) error { +func (c *coreConfigMap) update(data map[string]string) error { var ( err error cm *corev1.ConfigMap diff --git a/k8s/core/configmap/types.go b/k8s/core/configmap/types.go index 4722526b..35b42c41 100644 --- a/k8s/core/configmap/types.go +++ b/k8s/core/configmap/types.go @@ -41,9 +41,14 @@ const ( // objects. pxLockKey = "px-lock" - lockSleepDuration = 1 * time.Second - configMapUserLabelKey = "user" - maxConflictRetries = 3 + lockSleepDuration = 1 * time.Second + configMapUserLabelKey = "user" + maxConflictRetries = 3 + upgradeCompletedStatus = "UPGRADE_DONE" + true = "true" + false = "false" + pxNamespace = "portworx" + pxCopyLockConfigMap = "configmaps-copylock" ) var ( diff --git a/k8s/core/configmaps.go b/k8s/core/configmaps.go index dc0b1e86..57bd7432 100644 --- a/k8s/core/configmaps.go +++ b/k8s/core/configmaps.go @@ -2,6 +2,7 @@ package core import ( "context" + "fmt" "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" @@ -27,6 +28,7 @@ type ConfigMapOps interface { // GetConfigMap gets the config map object for the given name and namespace func (c *Client) GetConfigMap(name string, namespace string) (*corev1.ConfigMap, error) { + fmt.Println("Searching for configmap ", name, " in ns ", namespace) if err := c.initClient(); err != nil { return nil, err } From ad7485aeecca17dad63bc41e20bafb6f629f58d6 Mon Sep 17 00:00:00 2001 From: Nikita Bhatia Date: Mon, 1 May 2023 22:44:22 +0530 Subject: [PATCH 4/5] Fix failing test Signed-off-by: Nikita Bhatia --- k8s/core/configmap/configmap.go | 77 +++++++++++++++-------------- k8s/core/configmap/coreconfigmap.go | 14 +++--- k8s/core/configmap/types.go | 4 +- 3 files changed, 48 insertions(+), 47 deletions(-) diff --git a/k8s/core/configmap/configmap.go b/k8s/core/configmap/configmap.go index 965587d5..4747b94b 100644 --- a/k8s/core/configmap/configmap.go +++ b/k8s/core/configmap/configmap.go @@ -12,51 +12,52 @@ func (c *configMap) Instance() *coreConfigMap { if c.pxNs == c.config.nameSpace { //fresh install ot upgrade completed return c.config - } else { - existingConfig := c.config - c.copylock.Lock(uuid.New().String()) - defer c.copylock.Unlock() + } + + existingConfig := c.config + c.copylock.Lock(uuid.New().String()) + defer c.copylock.Unlock() + + lockMap, err := c.copylock.get() + if err != nil { + log.Errorf("Error during fetching data from copy lock %s", err) + return existingConfig + } + status := lockMap[upgradeCompletedStatus] + if status == trueString { + // upgrade is completed + //create configmap in portworx namespace + newConfig := &coreConfigMap{ + name: existingConfig.name, + defaultLockHoldTimeout: existingConfig.defaultLockHoldTimeout, + kLocksV2: existingConfig.kLocksV2, + lockAttempts: existingConfig.lockAttempts, + lockRefreshDuration: existingConfig.lockRefreshDuration, + lockK8sLockTTL: existingConfig.lockK8sLockTTL, + nameSpace: pxNamespace, + } - lockMap, err := c.copylock.get() + configData, err := existingConfig.get() if err != nil { - log.Errorf("Error during fetching data from copy lock %s", err) + log.Errorf("Error during fetching data from old config map %s", err) return existingConfig } - status := lockMap[upgradeCompletedStatus] - if status == true { - // upgrade is completed - //create configmap in portworx namespace - newConfig := &coreConfigMap{ - name: existingConfig.name, - defaultLockHoldTimeout: existingConfig.defaultLockHoldTimeout, - kLocksV2: existingConfig.kLocksV2, - lockAttempts: existingConfig.lockAttempts, - lockRefreshDuration: existingConfig.lockRefreshDuration, - lockK8sLockTTL: existingConfig.lockK8sLockTTL, - nameSpace: pxNamespace, - } - - configData, err := existingConfig.get() - if err != nil { - log.Errorf("Error during fetching data from old config map %s", err) - return existingConfig - } - //copy data from old configmap to new configmap - if err = newConfig.update(configData); err != nil { - log.Errorf("Error during copying data from old config map %s", err) - return existingConfig - } - - //delete old configmap - err = c.config.delete() - if err != nil { - log.Errorf("Error during deleting configmap %s in namespace %s ", c.config.name, c.config.nameSpace) - } - c.config = newConfig - } else { + //copy data from old configmap to new configmap + if err = newConfig.update(configData); err != nil { + log.Errorf("Error during copying data from old config map %s", err) return existingConfig } + + //delete old configmap + err = c.config.delete() + if err != nil { + log.Errorf("Error during deleting configmap %s in namespace %s ", c.config.name, c.config.nameSpace) + } + c.config = newConfig + } else { + return existingConfig } + return c.config } diff --git a/k8s/core/configmap/coreconfigmap.go b/k8s/core/configmap/coreconfigmap.go index 3ac72149..a23ee563 100644 --- a/k8s/core/configmap/coreconfigmap.go +++ b/k8s/core/configmap/coreconfigmap.go @@ -73,7 +73,7 @@ func New( Namespace: pxNamespace, }, Data: map[string]string{ - upgradeCompletedStatus: true, + upgradeCompletedStatus: trueString, }, } @@ -84,14 +84,14 @@ func New( fmt.Println("Failed to create configmap-copylock") return nil, fmt.Errorf("failed to create configmap %v: %v", name, err) - } else { - copyLock.name = pxCopyLockConfigMap - copyLock.kLocksV2 = map[string]*k8sLock{} - copyLock.lockRefreshDuration = v2LockRefreshDuration - copyLock.lockK8sLockTTL = v2LockK8sLockTTL - copyLock.nameSpace = pxNamespace } + copyLock.name = pxCopyLockConfigMap + copyLock.kLocksV2 = map[string]*k8sLock{} + copyLock.lockRefreshDuration = v2LockRefreshDuration + copyLock.lockK8sLockTTL = v2LockK8sLockTTL + copyLock.nameSpace = pxNamespace + return &configMap{ config: config, pxNs: ns, diff --git a/k8s/core/configmap/types.go b/k8s/core/configmap/types.go index 35b42c41..a74210bc 100644 --- a/k8s/core/configmap/types.go +++ b/k8s/core/configmap/types.go @@ -45,8 +45,8 @@ const ( configMapUserLabelKey = "user" maxConflictRetries = 3 upgradeCompletedStatus = "UPGRADE_DONE" - true = "true" - false = "false" + trueString = "true" + falseString = "false" pxNamespace = "portworx" pxCopyLockConfigMap = "configmaps-copylock" ) From 359d22f1fc283db954399eb6d989b251cf7e4145 Mon Sep 17 00:00:00 2001 From: Nikita Bhatia Date: Mon, 1 May 2023 22:47:31 +0530 Subject: [PATCH 5/5] remove debug log Signed-off-by: Nikita Bhatia --- k8s/core/configmaps.go | 2 -- 1 file changed, 2 deletions(-) diff --git a/k8s/core/configmaps.go b/k8s/core/configmaps.go index 57bd7432..dc0b1e86 100644 --- a/k8s/core/configmaps.go +++ b/k8s/core/configmaps.go @@ -2,7 +2,6 @@ package core import ( "context" - "fmt" "github.com/sirupsen/logrus" corev1 "k8s.io/api/core/v1" @@ -28,7 +27,6 @@ type ConfigMapOps interface { // GetConfigMap gets the config map object for the given name and namespace func (c *Client) GetConfigMap(name string, namespace string) (*corev1.ConfigMap, error) { - fmt.Println("Searching for configmap ", name, " in ns ", namespace) if err := c.initClient(); err != nil { return nil, err }