Skip to content

Remove PV of OVA provider server #742

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

Merged
merged 1 commit into from
Feb 13, 2024
Merged

Remove PV of OVA provider server #742

merged 1 commit into from
Feb 13, 2024

Conversation

ahadas
Copy link
Member

@ahadas ahadas commented Feb 12, 2024

The way the PV for a pod that serves as an NFS server for an OVA provider is created doesn't allow its deletion by a CSI driver and therefore we need to take care of its cleanup when the OVA provider is deleted.

Here, we add a finalizer to OVA providers to prevent their deletion until we delete their PV.

Copy link

codecov bot commented Feb 12, 2024

Codecov Report

Attention: 871 lines in your changes are missing coverage. Please review.

Comparison is base (a55e08e) 0.00% compared to head (a5a6bb4) 19.18%.
Report is 175 commits behind head on main.

Files Patch % Lines
pkg/controller/plan/adapter/converter.go 15.04% 168 Missing and 7 partials ⚠️
...volume-populator/populator-machinery/controller.go 0.00% 132 Missing ⚠️
pkg/controller/plan/kubevirt.go 0.00% 127 Missing ⚠️
pkg/controller/plan/migration.go 0.00% 125 Missing ⚠️
...bhooks/mutating-webhook/mutators/secret-mutator.go 0.00% 74 Missing ⚠️
...ooks/mutating-webhook/mutators/provider-mutator.go 0.00% 59 Missing ⚠️
cmd/openstack-populator/openstack-populator.go 66.37% 36 Missing and 3 partials ⚠️
pkg/controller/plan/validation.go 0.00% 36 Missing ⚠️
cmd/ova-provider-server/ova-provider-server.go 0.00% 33 Missing ⚠️
pkg/lib/client/openshift/client.go 52.77% 15 Missing and 2 partials ⚠️
... and 11 more
Additional details and impacted files
@@            Coverage Diff            @@
##           main     #742       +/-   ##
=========================================
+ Coverage      0   19.18%   +19.18%     
=========================================
  Files         0       80       +80     
  Lines         0    15707    +15707     
=========================================
+ Hits          0     3013     +3013     
- Misses        0    12456    +12456     
- Partials      0      238      +238     
Flag Coverage Δ
unittests 19.18% <16.25%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

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

@liranr23
Copy link
Member

The way the PV for a pod that serves as an NFS server for an OVA provider is created doesn't allow its deletion by a CSI driver and therefore we need to take care of its cleanup when the OVA provider is deleted.

Why? and what can be done to do it on the creation? if possible.. i saw that we already set PersistentVolumeReclaimPolicy to delete.. but just wondering why it's stuck on this. maybe it holds finalizers within?

@ahadas
Copy link
Member Author

ahadas commented Feb 12, 2024

The way the PV for a pod that serves as an NFS server for an OVA provider is created doesn't allow its deletion by a CSI driver and therefore we need to take care of its cleanup when the OVA provider is deleted.

Why? and what can be done to do it on the creation? if possible.. i saw that we already set PersistentVolumeReclaimPolicy to delete.. but just wondering why it's stuck on this. maybe it holds finalizers within?

setting PersistentVolumeReclaimPolicy to delete has no meaning for a PV that is not allocated by a CSI driver that supports deletion.
what can be done to enable this? not much since the PV is not allocated dynamically, we allocate it manually to point to an NFS share that is provided by the user - in that case there's not much we can do other than deleting it "manually"

The way the PV for a pod that serves as an NFS server for an OVA
provider is created doesn't allow its deletion by a CSI driver and
therefore we need to take care of its cleanup when the OVA provider is
deleted.

Here, we add a finalizer to OVA providers to prevent their deletion
until we delete their PV.

Signed-off-by: Arik Hadas <ahadas@redhat.com>
Copy link

Quality Gate Passed Quality Gate passed

Issues
0 New issues

Measures
0 Security Hotspots
No data about Coverage
11.4% Duplication on New Code

See analysis details on SonarCloud

@@ -234,6 +236,30 @@ func (r Reconciler) Reconcile(ctx context.Context, request reconcile.Request) (r
}
}

if provider.DeletionTimestamp != nil && k8sutil.ContainsFinalizer(provider, api.OvaProviderFinalizer) {
Copy link
Member

Choose a reason for hiding this comment

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

I remembered another issue I had when looking into it, once the provider deleted the objects was no longer accessible, were you able to query its fields at this point?

Copy link
Member Author

@ahadas ahadas Feb 12, 2024

Choose a reason for hiding this comment

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

yeah, note that with a finalizer, an entity doesn't get removed until the finalizer is dropped - so when we get here, the provider still exists and has the DeletionTimestamp field set (that's the "update" we get), so we can still get its properties

@ahadas ahadas merged commit 59ed0ef into kubev2v:main Feb 13, 2024
@ahadas ahadas linked an issue Feb 13, 2024 that may be closed by this pull request
if specChanged {
patches := mutator.patchPayload()
patches = append(patches, util.PatchOperation{
Copy link
Member

Choose a reason for hiding this comment

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

not improtant but we can probably also use jsonpatch.CreateMergePatch

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.

PV isn't removed when an OVA provider is deleted
4 participants