Skip to content
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

Merged
merged 1 commit into from
Feb 25, 2019

Conversation

gman0
Copy link
Contributor

@gman0 gman0 commented Feb 20, 2019

In order to maintain idempotency when deleting k8s metadata entries, we may assume that non-existent entries are already deleted.

@gman0
Copy link
Contributor Author

gman0 commented Feb 20, 2019

PTAL @rootfs @mickymiek

@@ -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)
Copy link
Collaborator

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

Copy link
Contributor Author

@gman0 gman0 Feb 20, 2019

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

Copy link
Contributor Author

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

@@ -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)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here as well.

@gman0 gman0 force-pushed the cachepersister-idempotency-fix branch 2 times, most recently from daeda2c to 144df2a Compare February 20, 2019 18:28
@gman0
Copy link
Contributor Author

gman0 commented Feb 20, 2019

It should now return earlier ; also the log messages should be now a bit more descriptive.

@@ -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)
Copy link
Collaborator

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)

@gman0 gman0 force-pushed the cachepersister-idempotency-fix branch from 144df2a to 0235b9c Compare February 20, 2019 19:21
Copy link
Collaborator

@Madhu-1 Madhu-1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rootfs rootfs merged commit 4273408 into ceph:csi-v1.0 Feb 25, 2019
wilmardo pushed a commit to wilmardo/ceph-csi that referenced this pull request Jul 29, 2019
k8s metadata cache: delete shouldn't fail on NotFound errors
nixpanic pushed a commit to nixpanic/ceph-csi that referenced this pull request Nov 22, 2023
Syncing latest changes from devel for ceph-csi
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants