Skip to content
This repository has been archived by the owner on Mar 7, 2023. It is now read-only.

build: radosgw binaries have been moved to /radosgw directory in gateway's Dockerfiles #180

Merged
merged 1 commit into from
Oct 31, 2022

Conversation

giubacc
Copy link

@giubacc giubacc commented Oct 17, 2022

This patch is needed as prerequisite for: #164

Signed-off-by: Giuseppe Baccini giuseppe.baccini@suse.com

Describe your changes

Issue ticket number and link

Checklist before requesting a review

  • I have performed a self-review of my code.
  • If it is a core feature, I have added thorough tests.
  • CHANGELOG.md has been updated should there be relevant changes in this PR.

@giubacc giubacc self-assigned this Oct 17, 2022
@giubacc giubacc changed the title build: radosgw binaries have been moved to /radosgw directory in way's Dockerfiles build: radosgw binaries have been moved to /radosgw directory in gateway's Dockerfiles Oct 17, 2022
Copy link
Member

@votdev votdev left a comment

Choose a reason for hiding this comment

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

Could you please add an explanation WHY it is necessary and helpful to relocate the binary into a separate directory.

build/Dockerfile.build-container Outdated Show resolved Hide resolved
@giubacc
Copy link
Author

giubacc commented Oct 18, 2022

Could you please add an explanation WHY it is necessary and helpful to relocate the binary into a separate directory.

sure, here the high level description of what the script updating the binaries inside k8s is supposed to do:

# This script should be used while doing development on s3gw's radosgw binaries.
# By calling this script, we patch the s3gw deployment inside k8s as this:
#
# 1) A base image is used,  ghcr.io/aquarist-labs/s3gw:latest (default).
# 2) We create a PVC and a helper copier pod that mounts that PVC.
# 3) We copy the built binaries on the PVC by calling an equivalent `kubectl cp` command
#    on the copier pod.
#
# 4) We mount the same PVC on the s3gw pod at the location where
#    the binaries are expected (/radosgw).
#
# Patching the deployment forces the s3gw pod to restart with the new binaries in place.

The main trick here is point 4, that is basically replacing binaries with the one you have just built.

@votdev
Copy link
Member

votdev commented Oct 18, 2022

Could you please add an explanation WHY it is necessary and helpful to relocate the binary into a separate directory.

sure, here the high level description of what the script updating the binaries inside k8s is supposed to do:

# This script should be used while doing development on s3gw's radosgw binaries.
# By calling this script, we patch the s3gw deployment inside k8s as this:
#
# 1) A base image is used,  ghcr.io/aquarist-labs/s3gw:latest (default).
# 2) We create a PVC and a helper copier pod that mounts that PVC.
# 3) We copy the built binaries on the PVC by calling an equivalent `kubectl cp` command
#    on the copier pod.
#
# 4) We mount the same PVC on the s3gw pod at the location where
#    the binaries are expected (/radosgw).
#
# Patching the deployment forces the s3gw pod to restart with the new binaries in place.

The main trick here is point 4, that is basically replacing binaries with the one you have just built.

Make sense. Without a dedicated directory, /usr/bin/ will be replaced which will break the container at the end.

@giubacc
Copy link
Author

giubacc commented Oct 18, 2022

@jecluis are you fine with these modifications?
Are you seeing any potential trap?

@giubacc giubacc force-pushed the radosgw-binary-segregation branch from ba200b4 to f30fa62 Compare October 18, 2022 08:39
@jecluis
Copy link
Member

jecluis commented Oct 18, 2022

@jecluis are you fine with these modifications? Are you seeing any potential trap?

Given we are moving the libraries to a path that is not standard, I fear that we might require LD_LIBRARY_PATH to be provided to run the binary. Or find a way to add that path to the ld search path.

@jecluis
Copy link
Member

jecluis commented Oct 18, 2022

@jecluis are you fine with these modifications? Are you seeing any potential trap?

Given we are moving the libraries to a path that is not standard, I fear that we might require LD_LIBRARY_PATH to be provided to run the binary. Or find a way to add that path to the ld search path.

And now I see you already do that in the dockerfile. Let me think a bit on this.

@giubacc
Copy link
Author

giubacc commented Oct 18, 2022

@jecluis are you fine with these modifications? Are you seeing any potential trap?

Given we are moving the libraries to a path that is not standard, I fear that we might require LD_LIBRARY_PATH to be provided to run the binary. Or find a way to add that path to the ld search path.

And now I see you already do that in the dockerfile. Let me think a bit on this.

yes, consider that the whole thing has been already tested and it is working; but maybe there are non obvious drawbacks I'm not seeing.

@jhmarina jhmarina added this to the v0.8.0 milestone Oct 18, 2022
@giubacc giubacc force-pushed the radosgw-binary-segregation branch 3 times, most recently from 1a2d78b to a32c5e2 Compare October 28, 2022 08:56
@m-ildefons
Copy link
Contributor

You could do this simpler by also putting /radosgw in the $PATH and calling radosgw without the absolute path in the entrypoint. Then you can just kubectl cp directly into the s3gw container's /radosgw where the persistent volume is mounted. After that just kubectl delete the pod. The new pod that will be started will execute the radosgw from the persistent volume instead. No need for a "copier pod" and no need to move the "normal" radosgw binaries around.

You might even be able to set the environment variables by supplying them in the deployments manifest or kubectl edit the deployment once it is applied to the cluster.

@giubacc
Copy link
Author

giubacc commented Oct 28, 2022

You could do this simpler by also putting /radosgw in the $PATH and calling radosgw without the absolute path in the entrypoint. Then you can just kubectl cp directly into the s3gw container's /radosgw where the persistent volume is mounted. After that just kubectl delete the pod. The new pod that will be started will execute the radosgw from the persistent volume instead. No need for a "copier pod" and no need to move the "normal" radosgw binaries around.

You might even be able to set the environment variables by supplying them in the deployments manifest or kubectl edit the deployment once it is applied to the cluster.

Ok to implement suggestion nr. 1.
Regarding other suggestions, agree to discuss them, but please comment here instead.
Let's take in account that this doesn't need to be super well designed, it is an helper tool for the developer.
The implementation proposed for sure can be improved, but I'm using it everyday and it does well its job.

…way's Dockerfiles

This patch is needed as prerequisite for: aquarist-labs#164

Signed-off-by: Giuseppe Baccini <giuseppe.baccini@suse.com>
@giubacc giubacc force-pushed the radosgw-binary-segregation branch from a32c5e2 to 4c91226 Compare October 28, 2022 12:57
@0xavi0 0xavi0 merged commit 2a01909 into aquarist-labs:main Oct 31, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

6 participants