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

Fix regression in chunk deletion for openstack provider #703

Merged

Conversation

shreyas-s-rao
Copy link
Collaborator

@shreyas-s-rao shreyas-s-rao commented Jan 6, 2024

What this PR does / why we need it:
This PR fixes a recent regression in chunk deletion behavior for Openstack Swift snapshots. Additionally, remove backward compatibility tests between v1 and v2 for garbage collection, since v1 is no longer supported. Support for v1 backup directory structure in other parts of the code should be gradually removed as well, but not in this PR, to avoid scope spill.

Which issue(s) this PR fixes:
Fixes #702

Special notes for your reviewer:
/cc @plkokanov
/invite @ishan16696 @anveshreddy18 @seshachalam-yv

Release note:

A regression in chunk deletion behavior for openstack provider has now been fixed.

@shreyas-s-rao shreyas-s-rao added platform/openstack OpenStack platform/infrastructure area/backup Backup related labels Jan 6, 2024
@shreyas-s-rao shreyas-s-rao added this to the v0.28.0 milestone Jan 6, 2024
@shreyas-s-rao shreyas-s-rao requested a review from a team as a code owner January 6, 2024 20:50
@gardener-robot gardener-robot added the needs/review Needs review label Jan 6, 2024
@gardener-robot gardener-robot added the size/s Size of pull request is small (see gardener-robot robot/bots/size.py) label Jan 6, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 6, 2024
@gardener-robot gardener-robot added size/l Size of pull request is large (see gardener-robot robot/bots/size.py) needs/second-opinion Needs second review by someone else and removed size/s Size of pull request is small (see gardener-robot robot/bots/size.py) labels Jan 6, 2024
@gardener-robot-ci-3 gardener-robot-ci-3 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 6, 2024
@ishan16696
Copy link
Member

unit tests failed:

Summarizing 3 Failures:
  [FAIL] Save, List, Fetch, Delete from mock snapstore When both v1 and v2 are present [It] When both v1 and v2 are present
  /tmp/build/bc4dbee3/pull-request-gardener.etcd-backup-restore-master-pr/pkg/snapstore/snapstore_test.go:217
  [FAIL] Save, List, Fetch, Delete from mock snapstore When Only v2 is present [It] When Only v2 is present
  /tmp/build/bc4dbee3/pull-request-gardener.etcd-backup-restore-master-pr/pkg/snapstore/snapstore_test.go:267
  [FAIL] Save, List, Fetch, Delete from mock snapstore When Only v1 is present [It] When Only v1 is present
  /tmp/build/bc4dbee3/pull-request-gardener.etcd-backup-restore-master-pr/pkg/snapstore/snapstore_test.go:165

Additionally, remove backward compatibility tests between v1 and v2 for garbage collection, since v1 is no longer supported. Support for v1 backup directory structure in other parts of the code should be gradually removed as well, but not in this PR, to avoid scope spill.

I would suggest let's address all of these in separate PR, let's keep this PR for bug fix, so that it become easy and quick to review. WDYT?

@ishan16696
Copy link
Member

unit tests also failed locally

[AfterSuite]
/Users/I539698/Desktop/review/etcd-backup-restore/pkg/snapstore/snapstore_suite_test.go:43
time="2024-01-08T14:53:39+05:30" level=info msg="Shutting down test server..."
[AfterSuite] PASSED [0.000 seconds]
------------------------------

Summarizing 3 Failures:
  [FAIL] Save, List, Fetch, Delete from mock snapstore When Only v1 is present [It] When Only v1 is present
  /Users/I539698/Desktop/review/etcd-backup-restore/pkg/snapstore/snapstore_test.go:165
  [FAIL] Save, List, Fetch, Delete from mock snapstore When both v1 and v2 are present [It] When both v1 and v2 are present
  /Users/I539698/Desktop/review/etcd-backup-restore/pkg/snapstore/snapstore_test.go:217
  [FAIL] Save, List, Fetch, Delete from mock snapstore When Only v2 is present [It] When Only v2 is present
  /Users/I539698/Desktop/review/etcd-backup-restore/pkg/snapstore/snapstore_test.go:267

Ran 115 of 115 Specs in 3.519 seconds
FAIL! -- 112 Passed | 3 Failed | 0 Pending | 0 Skipped
--- FAIL: TestSnapstore (3.52s)
FAIL

Ginkgo ran 1 suite in 8.47315375s

Test Suite Failed

I would suggest you can fix them in a separate PR.

@gardener-robot
Copy link

@ishan16696, @anveshreddy18, @seshachalam-yv You have pull request review open invite, please check

Copy link
Member

@ishan16696 ishan16696 left a comment

Choose a reason for hiding this comment

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

few nits

pkg/snapstore/swift_snapstore.go Outdated Show resolved Hide resolved
pkg/snapstore/swift_snapstore.go Outdated Show resolved Hide resolved
@gardener-robot gardener-robot added the needs/changes Needs (more) changes label Jan 9, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 9, 2024
@gardener-robot-ci-2 gardener-robot-ci-2 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 9, 2024
@shreyas-s-rao
Copy link
Collaborator Author

@ishan16696 thanks for your review. Addressed them. PTAL

@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 9, 2024
@shreyas-s-rao shreyas-s-rao added needs/cherry-pick Needs to be cherry-picked to older version merge/squash Should be merged via 'Squash and merge' labels Jan 9, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jan 9, 2024
Copy link
Contributor

@anveshreddy18 anveshreddy18 left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @shreyas-s-rao.
/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/changes Needs (more) changes needs/review Needs review needs/second-opinion Needs second review by someone else labels Jan 9, 2024
@gardener-robot-ci-1 gardener-robot-ci-1 added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jan 9, 2024
Copy link
Member

@ishan16696 ishan16696 left a comment

Choose a reason for hiding this comment

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

Thanks @shreyas-s-rao for quickly fixing this issue.
LGTM!!

@ishan16696
Copy link
Member

Please cherry-pick to branch rel-v0.24

@shreyas-s-rao shreyas-s-rao merged commit 727e957 into gardener:master Jan 9, 2024
9 checks passed
@gardener-robot gardener-robot added the status/closed Issue is closed (either delivered or triaged) label Jan 9, 2024
@shreyas-s-rao shreyas-s-rao deleted the fix/swift-chunk-deletion-gc branch January 9, 2024 15:04
shreyas-s-rao added a commit to shreyas-s-rao/etcd-backup-restore that referenced this pull request Jan 9, 2024
* Fix bug in chunk deletion for openstack

* Remove backward compatibility tests for GC, other minor fixes in snapshotter_test.go

* Fix unit tests

* Address review comments from @ishan16696

* Address review comments from @anveshreddy18
ishan16696 pushed a commit that referenced this pull request Jan 9, 2024
* Fix bug in chunk deletion for openstack

* Remove backward compatibility tests for GC, other minor fixes in snapshotter_test.go

* Fix unit tests

* Address review comments from @ishan16696

* Address review comments from @anveshreddy18
shreyas-s-rao added a commit to shreyas-s-rao/etcd-backup-restore that referenced this pull request Jan 11, 2024
* Fix bug in chunk deletion for openstack

* Remove backward compatibility tests for GC, other minor fixes in snapshotter_test.go

* Fix unit tests

* Address review comments from @ishan16696

* Address review comments from @anveshreddy18
@shreyas-s-rao shreyas-s-rao self-assigned this Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/backup Backup related merge/squash Should be merged via 'Squash and merge' needs/cherry-pick Needs to be cherry-picked to older version needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) platform/openstack OpenStack platform/infrastructure reviewed/lgtm Has approval for merging reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) size/l Size of pull request is large (see gardener-robot robot/bots/size.py) status/closed Issue is closed (either delivered or triaged)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] Chunk objects get wrongly garbage collected when using openstack swift
7 participants