Skip to content
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

rucio: integrate sidecar container #372

Merged
merged 1 commit into from
Sep 15, 2022
Merged

Conversation

kounelisagis
Copy link
Contributor

Copy link
Member

@tiborsimko tiborsimko left a comment

Choose a reason for hiding this comment

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

Please rebase against latest master, the branch should be based off #370 otherwise VOMS support won't contain changed needed for ESCAPE.

Also, the CI is not passing due to code formatting cosmetics. We are using black, you can run it locally, the following changes are needed to make the PR pass:

diff --git a/reana_job_controller/kubernetes_job_manager.py b/reana_job_controller/kubernetes_job_manager.py
index f7d47e1..95f412b 100644
--- a/reana_job_controller/kubernetes_job_manager.py
+++ b/reana_job_controller/kubernetes_job_manager.py
@@ -446,12 +446,12 @@ class KubernetesJobManager(JobManager):

         rucio_config_file_path = os.path.join(
             current_app.config["RUCIO_CACHE_LOCATION"],
-            'rucio.cfg',
+            "rucio.cfg",
         )

         cern_bundle_path = os.path.join(
             current_app.config["RUCIO_CACHE_LOCATION"],
-            'CERN-bundle.pem',
+            "CERN-bundle.pem",
         )

         rucio_account = os.environ.get("RUCIO_USERNAME")
@@ -471,7 +471,7 @@ class KubernetesJobManager(JobManager):
                     voms_proxy_vo=voms_proxy_vo,
                     cern_bundle_path=cern_bundle_path,
                     rucio_config_file_path=rucio_config_file_path,
-                )
+                ),
             ],
             "name": "reana-auth-rucio",
             "imagePullPolicy": "IfNotPresent",

voms_proxy_vo = os.environ.get("VONAME")

rucio_config_container = {
"image": "reanahub/reana-auth-rucio:latest",
Copy link
Member

Choose a reason for hiding this comment

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

Can you please move this hardcoded string into config.py where we generally assemble all these "magic constants"?

Also, instead of the latest tag, please use the tentative 1.0.0 release tag already. (And for local testing we can simply re-tag it manually on our laptops for development purposes.) In this way the branch will be ready for merge.
We don't use latest officially anywhere, because it is a moving target, not identifying individual versions enough. See also https://vsupalov.com/docker-latest-tag/

If something is static and will never change, we can keep it hard-coded here. If something may change in the future, such as sidecar image version, we should definitely move it to config.py. See the VOMS proxy sidecar "magic constants" for illustration:

VOMSPROXY_CONTAINER_IMAGE = os.getenv(
    "VOMSPROXY_CONTAINER_IMAGE", "reanahub/reana-auth-vomsproxy:1.1.0"
)
"""Default docker image of VOMSPROXY sidecar container."""

VOMSPROXY_CONTAINER_NAME = "voms-proxy"
"""Name of VOMSPROXY sidecar container."""

VOMSPROXY_CERT_CACHE_LOCATION = "/vomsproxy_cache/"
"""Directory of voms-proxy certificate cache.

This directory is shared between job & VOMSPROXY container."""

VOMSPROXY_CERT_CACHE_FILENAME = "x509up_proxy"
"""Name of the voms-proxy certificate cache file."""

@tiborsimko tiborsimko merged commit 25bca3d into reanahub:master Sep 15, 2022
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.

initial implementation
2 participants