Volume populator questions

[Liran Rotenberg]

Hi all,

We managed to integrate volume-populator to our product (Forklift - a tool to migrate VMs into kubevirt).

As part of the work we had some questions on the way:

1. Namespace limitation:
We noticed that the code doesn't allow running the populator pod on the same namespace the PVC is in - is there any particular reason? (https://github.com/kubernetes-csi/lib-volume-populator/blob/master/populator-machinery/controller.go#L344 and  https://github.com/kubernetes-csi/lib-volume-populator/blob/master/populator-machinery/controller.go#L425).

2. ServiceAccount/Secret:

Sometimes we want the populator pod to run with some permissions to let the populator pod do it's job. Currently there is no way to set it, unless you create the relevant SA. Would it make sense to have the option of getting input to the populator pod?

3. Static storage:

When trying out volume populator with pre-made PVs, the migration doesn't work. I think it's a race on the bounding of the PVC to the PV that prevents the populator from being executed. Any idea on that?

Regards,

Liran

[Alvaro Romero]

Hey Liran, I'm also interested in these questions. We are implementing our own populators in CDI and decided to write the controllers from scratch for reasons like your second question.

Regarding 1, I just assumed it was a self-imposed limitation to keep the populator's namespace as clean as possible. I saw it mentioned in the KEP but I think no further explanation is given.

Regards,

Álvaro

[Ben Swartzlander]

1) The namespace for the populator is meant to be a single dedicated namespace that's not used by anything other than the populator. The reason for this is because the temporary PVC' has to exist in some namespace, but we want it to be hidden. A dedicated namespace exists for this purpose. The populator pod has to run in the same namespace as PVC' so that it can attach to the PVC under the ordinary rules.

This design allows a single populator namespace to serve all the workloads across all the namespaces in the cluster (except the populator's own namespace). This design also allows multiple populators to coexist in the same cluster, each in its own namespace.

2) It would help if you gave some examples of the kind of permissions you think a populator pod might need, but I see no problems with that in principle. We should enhance the populator library to take a pre-created service account as an input.

3) Yes, the populator design assumes dynamic provisioning. If there are good reasons to populate static volumes we might be able to figure out a way to support that too, but I haven't thought through the details. In particular, I'm not sure what value static PVs provide that you'd want to use them. A dynamic provisioner can easily emulate static PVs by serving volumes from a fixed pool. Actually using static PVs directly seems like a legacy way of handling storage.

--
You received this message because you are subscribed to the Google Groups "kubernetes-sig-storage" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-st...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-storage/947b7d1f-410a-4c7f-a61d-fdef2e450be9n%40googlegroups.com.

[Ben Swartzlander]

> In our case we don't mind showing the temporary PVC'. Also, we didn't see any problem with multiple populators coexisting in the same cluster. We have one per each kind and each kind is per source.

This may be the case for your situation, but the library is written to support a wide variety of use cases where multiple populators are present, all of the user namespaces need to be supported, and temporary artifacts need to be hidden to avoid bad side effects.

> For example, if we wish to import from a source that we need credentials to connect. Then we need to provide it somehow to the populator pod. One way to do it is by querying an existing secret with the relevant data. But, we need permission to get the secret.

As part of the populator being integrated within another project, the project may have the permissions with its own service account to get it. In that case it makes sense to provide it without creating a new specific one. Without it we may get:

F0329 14:11:39.189291 1 ovirt-populator.go:200] secrets "gwe-663e7d55-f2e5-425c-8580-848c512b6db7-wtpsd" is forbidden: User "system:serviceaccount:<namespace>:default" cannot get resource "secrets" in API group "" in the namespace "konveyor-forklift"

>

> We also thought of updating the progress of the data transfer into the CR, you also need permissions for it.

So the library can be extended to do new things. Some of the stuff you mention is just a matter of giving your populator pod additional roles and adding the code to do those things. If you end up needing to change the library, and those changes are likely to be generally usable, please contribute them back to the upstream project.

> That isn't a priority from our side as well. But, if a user may come from a legacy environment with static PVs slowly upgrading and now starting to use volume-populator - it means he needs to set up his backend storage. If it's easy to do it might be worth it. I think for the very least it needs to be documented somewhere or validated.

Even if it's not a priority, it would be good to know exactly what's failing so we can think about fixing it, or finding other workarounds.

On Wed, Mar 29, 2023 at 11:12 AM Liran Rotenberg <lrot...@redhat.com> wrote:

On Tue, Mar 28, 2023 at 6:15 AM 'Ben Swartzlander' via kubernetes-sig-storage <kubernetes-...@googlegroups.com> wrote:

1) The namespace for the populator is meant to be a single dedicated namespace that's not used by anything other than the populator. The reason for this is because the temporary PVC' has to exist in some namespace, but we want it to be hidden. A dedicated namespace exists for this purpose. The populator pod has to run in the same namespace as PVC' so that it can attach to the PVC under the ordinary rules.

This design allows a single populator namespace to serve all the workloads across all the namespaces in the cluster (except the populator's own namespace). This design also allows multiple populators to coexist in the same cluster, each in its own namespace.

In our case we don't mind showing the temporary PVC'. Also, we didn't see any problem with multiple populators coexisting in the same cluster. We have one per each kind and each kind is per source.

2) It would help if you gave some examples of the kind of permissions you think a populator pod might need, but I see no problems with that in principle. We should enhance the populator library to take a pre-created service account as an input.

For example, if we wish to import from a source that we need credentials to connect. Then we need to provide it somehow to the populator pod. One way to do it is by querying an existing secret with the relevant data. But, we need permission to get the secret.

As part of the populator being integrated within another project, the project may have the permissions with its own service account to get it. In that case it makes sense to provide it without creating a new specific one. Without it we may get:

F0329 14:11:39.189291 1 ovirt-populator.go:200] secrets "gwe-663e7d55-f2e5-425c-8580-848c512b6db7-wtpsd" is forbidden: User "system:serviceaccount:<namespace>:default" cannot get resource "secrets" in API group "" in the namespace "konveyor-forklift"

We also thought of updating the progress of the data transfer into the CR, you also need permissions for it.

3) Yes, the populator design assumes dynamic provisioning. If there are good reasons to populate static volumes we might be able to figure out a way to support that too, but I haven't thought through the details. In particular, I'm not sure what value static PVs provide that you'd want to use them. A dynamic provisioner can easily emulate static PVs by serving volumes from a fixed pool. Actually using static PVs directly seems like a legacy way of handling storage.

That isn't a priority from our side as well. But, if a user may come from a legacy environment with static PVs slowly upgrading and now starting to use volume-populator - it means he needs to set up his backend storage. If it's easy to do it might be worth it. I think for the very least it needs to be documented somewhere or validated.

On Mon, Mar 27, 2023 at 7:37 AM Alvaro Romero <alro...@redhat.com> wrote:

Hey Liran, I'm also interested in these questions. We are implementing our own populators in CDI and decided to write the controllers from scratch for reasons like your second question.

Regarding 1, I just assumed it was a self-imposed limitation to keep the populator's namespace as clean as possible. I saw it mentioned in the KEP but I think no further explanation is given.

Regards,

Álvaro

On Sunday, March 26, 2023 at 3:27:27 PM UTC+2 Liran Rotenberg wrote:

Hi all,

We managed to integrate volume-populator to our product (Forklift - a tool to migrate VMs into kubevirt).

As part of the work we had some questions on the way:

1. Namespace limitation:
We noticed that the code doesn't allow running the populator pod on the same namespace the PVC is in - is there any particular reason? (https://github.com/kubernetes-csi/lib-volume-populator/blob/master/populator-machinery/controller.go#L344 and  https://github.com/kubernetes-csi/lib-volume-populator/blob/master/populator-machinery/controller.go#L425).

2. ServiceAccount/Secret:

Sometimes we want the populator pod to run with some permissions to let the populator pod do it's job. Currently there is no way to set it, unless you create the relevant SA. Would it make sense to have the option of getting input to the populator pod?

3. Static storage:

When trying out volume populator with pre-made PVs, the migration doesn't work. I think it's a race on the bounding of the PVC to the PV that prevents the populator from being executed. Any idea on that?

Regards,

Liran

--
You received this message because you are subscribed to the Google Groups "kubernetes-sig-storage" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-st...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-storage/947b7d1f-410a-4c7f-a61d-fdef2e450be9n%40googlegroups.com.

--

You received this message because you are subscribed to a topic in the Google Groups "kubernetes-sig-storage" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/kubernetes-sig-storage/i_mq38Cvrd4/unsubscribe.
To unsubscribe from this group and all its topics, send an email to kubernetes-sig-st...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-storage/CAJO2feDdxNop%2Bh-RG_AZOKSHEji%3DKaCCFH1VE%2BhKdPVxPeZXHw%40mail.gmail.com.

[Liran Rotenberg]

On Tue, Mar 28, 2023 at 6:15 AM 'Ben Swartzlander' via kubernetes-sig-storage <kubernetes-...@googlegroups.com> wrote:

1) The namespace for the populator is meant to be a single dedicated namespace that's not used by anything other than the populator. The reason for this is because the temporary PVC' has to exist in some namespace, but we want it to be hidden. A dedicated namespace exists for this purpose. The populator pod has to run in the same namespace as PVC' so that it can attach to the PVC under the ordinary rules.

This design allows a single populator namespace to serve all the workloads across all the namespaces in the cluster (except the populator's own namespace). This design also allows multiple populators to coexist in the same cluster, each in its own namespace.

In our case we don't mind showing the temporary PVC'. Also, we didn't see any problem with multiple populators coexisting in the same cluster. We have one per each kind and each kind is per source.

2) It would help if you gave some examples of the kind of permissions you think a populator pod might need, but I see no problems with that in principle. We should enhance the populator library to take a pre-created service account as an input.

For example, if we wish to import from a source that we need credentials to connect. Then we need to provide it somehow to the populator pod. One way to do it is by querying an existing secret with the relevant data. But, we need permission to get the secret.

As part of the populator being integrated within another project, the project may have the permissions with its own service account to get it. In that case it makes sense to provide it without creating a new specific one. Without it we may get:

F0329 14:11:39.189291 1 ovirt-populator.go:200] secrets "gwe-663e7d55-f2e5-425c-8580-848c512b6db7-wtpsd" is forbidden: User "system:serviceaccount:<namespace>:default" cannot get resource "secrets" in API group "" in the namespace "konveyor-forklift"

We also thought of updating the progress of the data transfer into the CR, you also need permissions for it.

3) Yes, the populator design assumes dynamic provisioning. If there are good reasons to populate static volumes we might be able to figure out a way to support that too, but I haven't thought through the details. In particular, I'm not sure what value static PVs provide that you'd want to use them. A dynamic provisioner can easily emulate static PVs by serving volumes from a fixed pool. Actually using static PVs directly seems like a legacy way of handling storage.

That isn't a priority from our side as well. But, if a user may come from a legacy environment with static PVs slowly upgrading and now starting to use volume-populator - it means he needs to set up his backend storage. If it's easy to do it might be worth it. I think for the very least it needs to be documented somewhere or validated.

On Mon, Mar 27, 2023 at 7:37 AM Alvaro Romero <alro...@redhat.com> wrote:

Hey Liran, I'm also interested in these questions. We are implementing our own populators in CDI and decided to write the controllers from scratch for reasons like your second question.

Regarding 1, I just assumed it was a self-imposed limitation to keep the populator's namespace as clean as possible. I saw it mentioned in the KEP but I think no further explanation is given.

Regards,

Álvaro

On Sunday, March 26, 2023 at 3:27:27 PM UTC+2 Liran Rotenberg wrote:

Hi all,

We managed to integrate volume-populator to our product (Forklift - a tool to migrate VMs into kubevirt).

As part of the work we had some questions on the way:

1. Namespace limitation:
We noticed that the code doesn't allow running the populator pod on the same namespace the PVC is in - is there any particular reason? (https://github.com/kubernetes-csi/lib-volume-populator/blob/master/populator-machinery/controller.go#L344 and  https://github.com/kubernetes-csi/lib-volume-populator/blob/master/populator-machinery/controller.go#L425).

2. ServiceAccount/Secret:

Sometimes we want the populator pod to run with some permissions to let the populator pod do it's job. Currently there is no way to set it, unless you create the relevant SA. Would it make sense to have the option of getting input to the populator pod?

3. Static storage:

When trying out volume populator with pre-made PVs, the migration doesn't work. I think it's a race on the bounding of the PVC to the PV that prevents the populator from being executed. Any idea on that?

Regards,

Liran

--
You received this message because you are subscribed to the Google Groups "kubernetes-sig-storage" group.
To unsubscribe from this group and stop receiving emails from it, send an email to kubernetes-sig-st...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-storage/947b7d1f-410a-4c7f-a61d-fdef2e450be9n%40googlegroups.com.

--

You received this message because you are subscribed to a topic in the Google Groups "kubernetes-sig-storage" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/kubernetes-sig-storage/i_mq38Cvrd4/unsubscribe.
To unsubscribe from this group and all its topics, send an email to kubernetes-sig-st...@googlegroups.com.
To view this discussion on the web visit https://groups.google.com/d/msgid/kubernetes-sig-storage/CAJO2feDdxNop%2Bh-RG_AZOKSHEji%3DKaCCFH1VE%2BhKdPVxPeZXHw%40mail.gmail.com.

[Liran Rotenberg]

On Wed, Mar 29, 2023 at 6:54 PM Ben Swartzlander <bswart...@google.com> wrote:

> In our case we don't mind showing the temporary PVC'. Also, we didn't see any problem with multiple populators coexisting in the same cluster. We have one per each kind and each kind is per source.

This may be the case for your situation, but the library is written to support a wide variety of use cases where multiple populators are present, all of the user namespaces need to be supported, and temporary artifacts need to be hidden to avoid bad side effects.

> For example, if we wish to import from a source that we need credentials to connect. Then we need to provide it somehow to the populator pod. One way to do it is by querying an existing secret with the relevant data. But, we need permission to get the secret.

As part of the populator being integrated within another project, the project may have the permissions with its own service account to get it. In that case it makes sense to provide it without creating a new specific one. Without it we may get:

F0329 14:11:39.189291 1 ovirt-populator.go:200] secrets "gwe-663e7d55-f2e5-425c-8580-848c512b6db7-wtpsd" is forbidden: User "system:serviceaccount:<namespace>:default" cannot get resource "secrets" in API group "" in the namespace "konveyor-forklift"

>

> We also thought of updating the progress of the data transfer into the CR, you also need permissions for it.

So the library can be extended to do new things. Some of the stuff you mention is just a matter of giving your populator pod additional roles and adding the code to do those things. If you end up needing to change the library, and those changes are likely to be generally usable, please contribute them back to the upstream project.

https://github.com/kubernetes-csi/lib-volume-populator/pull/89 

> That isn't a priority from our side as well. But, if a user may come from a legacy environment with static PVs slowly upgrading and now starting to use volume-populator - it means he needs to set up his backend storage. If it's easy to do it might be worth it. I think for the very least it needs to be documented somewhere or validated.

Even if it's not a priority, it would be good to know exactly what's failing so we can think about fixing it, or finding other workarounds.

Seems that the PV bounds fast, too fast. That causes a skip. https://github.com/kubernetes-csi/lib-volume-populator/blob/master/populator-machinery/controller.go#L523

[Ben Swartzlander]

Timing should not matter. If the PVC is being bound by something else, then it is a bug. We need to find the code that is binding these PVCs to PVs and introduce a check for non-empty dataSource.