Skip to content

pb-4887: Added fix in handling the already exists error while creating#1567

Merged
siva-portworx merged 1 commit intomasterfrom
pb-4887
Nov 16, 2023
Merged

pb-4887: Added fix in handling the already exists error while creating#1567
siva-portworx merged 1 commit intomasterfrom
pb-4887

Conversation

@siva-portworx
Copy link
Copy Markdown
Contributor

What type of PR is this?

Uncomment only one and also add the corresponding label in the PR:
bug

What this PR does / why we need it:

    pb-4887: Added fix in handling the already exists error while creating
    volumesnapshotclass during restore.

            - If we have csi-snapshot-webhook webhook, the creation of
              volumesnapshotclass fails with different eror message for already exists error.
            - So add a get call and then calling create volumesnapshot call,
              if Get failed with NotFound error.

Does this PR change a user-facing CRD or CLI?:
no

Is a release note needed?:

Issue: csi based restore fails on some of the setup that had csi-snapshot-webhook admin webhook as we get different error for alreadyexist error, while creating the volumesnapshotclass resource.
User Impact: CSI based restore was failing on the setup that has csi-snapshot-webhook admin webhook
Resolution: Added a get call before create call, such that create call will happen only the get failed with "NotFound" error.

Does this change need to be cherry-picked to a release branch?:
23.9

Testing:

  1. Validated the Native CSI restore and kdmp localsnapshot restore, which was failing with out fix.

volumesnapshotclass during restore.

	- If we have csi-snapshot-webhook webhook, the creation of
	  volumesnapshotclass fails with different eror message for already exists error.
	- So add a get call and then calling create volumesnapshot call,
	  if Get failed with NotFound error.
@cnbu-jenkins
Copy link
Copy Markdown
Collaborator

Can one of the admins verify this patch?

Copy link
Copy Markdown
Contributor

@px-kesavan px-kesavan left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Copy Markdown

@kshithijiyer-px kshithijiyer-px left a comment

Choose a reason for hiding this comment

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

LGTM

Comment thread drivers/volume/csi/csi.go
Comment thread drivers/volume/csi/csi.go
Copy link
Copy Markdown
Collaborator

@mkoppal-px mkoppal-px left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link
Copy Markdown

codecov bot commented Nov 16, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (ade305c) 66.47% compared to head (7e7c35b) 66.53%.
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1567      +/-   ##
==========================================
+ Coverage   66.47%   66.53%   +0.05%     
==========================================
  Files          43       43              
  Lines        5453     5453              
==========================================
+ Hits         3625     3628       +3     
+ Misses       1493     1491       -2     
+ Partials      335      334       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@siva-portworx siva-portworx merged commit bbd30a4 into master Nov 16, 2023
Comment thread drivers/volume/csi/csi.go
vsClass.(*kSnapshotv1.VolumeSnapshotClass).ResourceVersion = ""
vsClass.(*kSnapshotv1.VolumeSnapshotClass).UID = ""
newVSClass, err = c.snapshotClient.SnapshotV1().VolumeSnapshotClasses().Create(context.TODO(), vsClass.(*kSnapshotv1.VolumeSnapshotClass), metav1.CreateOptions{})
_, err = c.snapshotClient.SnapshotV1().VolumeSnapshotClasses().Get(context.TODO(), vsClass.(*kSnapshotv1.VolumeSnapshotClass).Name, metav1.GetOptions{})
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

why not return the vsClass object returned by this Get function?

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.

8 participants