-
Notifications
You must be signed in to change notification settings - Fork 561
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
k8s metadata cache: delete shouldn't fail on NotFound errors #216
Conversation
PTAL @rootfs @mickymiek |
pkg/util/k8scmcache.go
Outdated
@@ -172,6 +172,9 @@ func (k8scm *K8sCMCache) Get(identifier string, data interface{}) error { | |||
func (k8scm *K8sCMCache) Delete(identifier string) error { | |||
err := k8scm.Client.CoreV1().ConfigMaps(k8scm.Namespace).Delete(identifier, nil) | |||
if err != nil { | |||
if apierrs.IsNotFound(err) { | |||
klog.V(4).Infof("k8s-cm-cache: assuming metadata configmap %s already deleted", identifier) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can we return nil
to avoid printing log at line 180 klog.V(4).Infof("k8s-cm-cache: successfully deleted metadata configmap %s", identifier)
@gman0 I have one question.
if user manually deletes this entry from the config map by this changes we may end up in having stale volume at backend
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely, that's why it says it's only assuming
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And yes, I'll fix that
pkg/util/nodecache.go
Outdated
@@ -152,7 +152,9 @@ func (nc *NodeCache) Delete(identifier string) error { | |||
file := path.Join(nc.BasePath, cacheDir, identifier+".json") | |||
err := os.Remove(file) | |||
if err != nil { | |||
if err != os.ErrNotExist { | |||
if err == os.ErrNotExist { | |||
klog.V(4).Infof("node-cache: assuming metadata storage file %s already deleted", file) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here as well.
daeda2c
to
144df2a
Compare
It should now return earlier ; also the log messages should be now a bit more descriptive. |
pkg/util/k8scmcache.go
Outdated
@@ -128,7 +128,7 @@ func (k8scm *K8sCMCache) Create(identifier string, data interface{}) error { | |||
} | |||
dataJSON, err := json.Marshal(data) | |||
if err != nil { | |||
return errors.Wrap(err, "k8s-cm-cache: marshal error") | |||
return errors.Wrapf(err, "k8s-cm-cache: JSON marshaling failed for configmap %s: %v", identifier, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wrapf
already wrap err
with string i think we can skip adding err at last
simply it will become
errors.Wrapf(err, "k8s-cm-cache: JSON marshaling failed for configmap %s:",identifier)
144df2a
to
0235b9c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
k8s metadata cache: delete shouldn't fail on NotFound errors
Syncing latest changes from devel for ceph-csi
In order to maintain idempotency when deleting k8s metadata entries, we may assume that non-existent entries are already deleted.