Pv pvc by corentin1002 · Pull Request #141 · vapor-ware/kubetest

corentin1002 · 2019-09-02T12:35:11Z

Hello,

Currently, it is not possible to test a deployment with PV and PVC.
This pull request is for adding PersistentVolume and PersistentVolumeClaim support.

++

marcoceppi

Thank you so much for you contribution! I've just done a first pass at some simple formatting things. I'll assign this to a reviewer for a more in-depth review

marcoceppi

Formatting looks good to me! @edaniszewski should have a review shortly

edaniszewski

Largely, this looks good to me. I noticed some inconsistencies/incorrect docstrings that should be cleaned up. After that, I think this is good to go!

edaniszewski · 2019-09-03T13:24:18Z

+
+        By default, this will augment the PersistentVolume object with
+        the generated test case namespace. This behavior can be
+        disabled with the ``set_namespace`` flag.


This comment does not match up with the default value of the set_namespace parameter. This comment should either be changed, or set_namespace should default to True.

edaniszewski · 2019-09-03T13:25:16Z

+        Args:
+            path (str): The path to the PersistentVolumeClaim manifest.
+            set_namespace (bool): Enable/disable the automatic
+                augmentation of the Service namespace.


... of the Service namespace.

should be

... of the PersistentVolumeClaim namespace.

edaniszewski · 2019-09-03T13:31:39Z

+            namespace (str): The namespace to create the PersistentVolume under.
+                If the PersistentVolume was loaded via the kubetest client, the
+                namespace will already be set, so it is not needed here.
+                Otherwise, the namespace will need to be provided.


Need to update the docstring:

Remove Args block, as there is no namespace arg here

Update summary line, removing under the given namespace, since no namespace is given.

edaniszewski · 2019-09-03T13:32:34Z

+
+        This method expects the PersistentVolume to have been loaded or otherwise
+        assigned a namespace already. If it has not, the namespace will need
+        to be set manually.


Can remove mention of namespace, since the PV doesn't need it.

edaniszewski · 2019-09-03T13:41:17Z

+
+        PersistentVolumes do not have a "status" field to check, so we will
+        measure their readiness status by whether or not they exist
+        on the cluster.


this is technically not true, as there is a status field which holds a V1PersistentVolumeStatus -- that said, I'm not sure how helpful that necessarily is as it just gives the status of whether its available, used, or reclaimed (https://kubernetes.io/docs/concepts/storage/persistent-volumes/#phase)

There could be additional discussion as to whether we want to check that this enters the Available phase, but I'm not sold on that entirely, as a different object using the PV could be spun up and put it in the Bound phase quickly. That is to say, I think the implementation here for checking if its ready is ok, but I think the docstring should be updated to reflect that there is a status, but it may not be reliable for determining readiness for the reason mentioned above.

edaniszewski · 2019-09-03T13:45:27Z

+
+        PersistentVolumeClaims do not have a "status" field to check, so we will
+        measure their readiness status by whether or not they exist
+        on the cluster.


similar note as with the PV is_ready method -- there is a "status" field here, but to me its not certain that checking the phase necessarily guarantees that the object is ready.

Hello @edaniszewski
I have done some changes regarding the comments for being more consistent. Do not hesitate to tell me if something seems wrong

plyte · 2019-09-03T17:17:32Z

How is the Persistent volume being deleted? Usually kubetest when tearing down a test deletes the namespace. Since persistent volumes are not namespaced, we need to delete them individually.

In manager.py the teardown function deletes the namespace and then deletes the clusterolebindings individually since those dont have namespaces as well. If we add the persistence volumes to a list we can then iterate over them and call the delete function on each. This might fix this problem. One way we could test this would be through checking if a pv is infact deleted once the tear down function is called.

This of course causes more changes to be made during the setup as we need to register these persistent volumes from the marker (applymanifest). If we add a check in "apply_manifest_from_marker" in the markers.py function we should be able to register them accordingly from there.

edaniszewski · 2019-09-10T18:16:02Z

Sorry for the delay in circling back on this. I think the changes for the comments I left before all look good, but @plyte makes a good point here that since the PersistentVolume is not in the namespace set up for the test, it won't be automatically deleted on test cleanup. This leaves it up to the test author to manually delete the resource, which I'd prefer to avoid since its an easy step to miss.

I think the suggested solution of creating a marker to load/clean up the PV (like is done for ClusterRoleBindings), that should alleviate that issue.

corentin1002 · 2020-01-21T10:39:36Z

too many changes, follow #166 for PV

corentin1002 added 4 commits August 29, 2019 14:09

add PersistentVolume & PersistentVolumeClaim objects & methods

842fc9a

add tests for PV and PVC

37c78d6

fixing lint issue

58e6eae

testing PV and PVC in test_manifest.py

287b30b

corentin1002 marked this pull request as ready for review September 2, 2019 12:36

marcoceppi suggested changes Sep 2, 2019

View reviewed changes

Comment thread kubetest/client.py Outdated

Comment thread kubetest/client.py Outdated

marcoceppi requested a review from edaniszewski September 2, 2019 15:29

corentin1002 force-pushed the pv_pvc branch 2 times, most recently from b008af5 to 5f12489 Compare September 3, 2019 06:27

marcoceppi approved these changes Sep 3, 2019

View reviewed changes

edaniszewski suggested changes Sep 3, 2019

View reviewed changes

corentin1002 force-pushed the pv_pvc branch from 5f12489 to b1ee2f8 Compare September 4, 2019 07:48

create fixture for testing pv/pvc

e3e8758

corentin1002 force-pushed the pv_pvc branch from b1ee2f8 to e3e8758 Compare September 4, 2019 09:15

Corentin JACQUEMET added 2 commits January 20, 2020 09:28

delete pv after tests

71f14a1

resolv conflicts

d00af7f

corentin1002 force-pushed the pv_pvc branch from 9678e61 to 2e3a457 Compare January 20, 2020 13:50

add markers for persistentvolume

deb6b17

corentin1002 force-pushed the pv_pvc branch from 2e3a457 to deb6b17 Compare January 21, 2020 08:40

corentin1002 mentioned this pull request Jan 21, 2020

Add PV type #166

Open

corentin1002 closed this Jan 21, 2020

corentin1002 mentioned this pull request Jan 23, 2020

add PVC type #167

Merged

Uh oh!

Conversation

corentin1002 commented Sep 2, 2019

Uh oh!

marcoceppi left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

marcoceppi left a comment

Choose a reason for hiding this comment

Uh oh!

edaniszewski left a comment

Choose a reason for hiding this comment

Uh oh!

edaniszewski Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

edaniszewski Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

edaniszewski Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

edaniszewski Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

edaniszewski Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

edaniszewski Sep 3, 2019

Choose a reason for hiding this comment

Uh oh!

corentin1002 Sep 4, 2019

Choose a reason for hiding this comment

Uh oh!

plyte commented Sep 3, 2019

Uh oh!

edaniszewski commented Sep 10, 2019

Uh oh!

corentin1002 commented Jan 21, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants