Clean up temporary resources when the original PVC gets deleted

[dannawang0221]

What type of PR is this?

Uncomment only one /kind <> line, hit enter to put that in a new line, and remove leading whitespaces from that line:

/kind api-change
/kind bug
/kind cleanup
/kind design
/kind documentation
/kind failing-test
/kind feature
/kind flake

What this PR does / why we need it:
With the current implementation, volume populator doesn't support PVC deletion during volume population. The finalizer will block deleting. If users want to force delete their PVCs, they need to remove the finalizer and manually clean up the temporary resource created by volume populators. This is hard to maintain and might end up with resource leak if users forget to delete them.

Which issue(s) this PR fixes:

Fixes #178

Special notes for your reviewer:

Does this PR introduce a user-facing change?:

Allow users to delete their PVC during volume population and clean up temporary resources create by volume populators

[dannawang0221]

/assign @msau42

[dannawang0221]

/assign @pwschuurman

[dannawang0221]

/assign @sunnylovestiramisu

[dannawang0221]

/retest

[pwschuurman]

It seems like these fields are trying to exclude some comparison. I would prefer to exclude something and still use reflect.DeepEqual, rather than (I think this is something we'd need to do using cmp, rather than reflect). This would make the test more resilient.

Can you at least comment on the specific test cases what you want to ignore?

[dannawang0221]

sure,I'll add comments to explain why these flags are used. I have reflect.DeepEqual for most tests. Only for the test which the object gets updated by the volume populator I have this flag set to true because for some reason reflect.DeepEqual return false even if all the fields print out look exactly the same.

[dannawang0221]

updated

[sunnylovestiramisu]

I noticed that the controller code itself historically does not have too many logs, do we think adding some logs to indicate the process can help debugging?

[dannawang0221]

I noticed that the controller code itself historically does not have too many logs, do we think adding some logs to indicate the process can help debugging?

I have thought about this, but not sure what would be the best practice, is there any concern of having too many logs?

[sunnylovestiramisu]

I noticed that the controller code itself historically does not have too many logs, do we think adding some logs to indicate the process can help debugging?

I have thought about this, but not sure what would be the best practice, is there any concern of having too many logs?

We can define a different log level for these logs. For example 4=Debug level verbosity(just an example).

[dannawang0221]

I noticed that the controller code itself historically does not have too many logs, do we think adding some logs to indicate the process can help debugging?

I have thought about this, but not sure what would be the best practice, is there any concern of having too many logs?

We can define a different log level for these logs. For example 4=Debug level verbosity(just an example).

I agree we should add more logs. The current log coverage is too low, which makes it very hard to debug

[sunnylovestiramisu]

Overall LGTM

[msau42]

Danna and I discussed offline. It looks like existing logic for removing the finalizer doesn't wait for the Pod and PVC' resources to be fully deleted. This may be something we still want to block PVC deletion on.

[dannawang0221]

Danna and I discussed offline. It looks like existing logic for removing the finalizer doesn't wait for the Pod and PVC' resources to be fully deleted. This may be something we still want to block PVC deletion on.

Open an issue to track this #198

[msau42]

/lgtm
/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dannawang0221, msau42

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• OWNERS [msau42]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment