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

Initial support for PV and PVC creation #684

Merged
merged 56 commits into from
Mar 4, 2024

Conversation

tcassaert
Copy link
Contributor

To FUSE mount an S3 bucket in the batch jobs, a PV and PVC should be precreated. These resources use the CSI-S3 driver, which requires us to do static provisioning if we want to mount a subdirectory of a bucket.

@tcassaert
Copy link
Contributor Author

tcassaert commented Feb 15, 2024

Things to add:

  • Delete the PV and PVC on job cancel
  • Delete the PV and PVC on job finish?
  • Mount the PVC in the pod by default (not possible to do it via custom config as the name is generated)
  • Actually write job results to disk instead of via S3

@tcassaert
Copy link
Contributor Author

@soxofaan as issues like #283 are introduced by me... Could you guide me in how to feature flag correctly?

Copy link
Member

@soxofaan soxofaan left a comment

Choose a reason for hiding this comment

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

some notes

openeogeotrellis/integrations/kubernetes.py Show resolved Hide resolved
openeogeotrellis/backend.py Outdated Show resolved Hide resolved
openeogeotrellis/backend.py Outdated Show resolved Hide resolved
openeogeotrellis/backend.py Outdated Show resolved Hide resolved

rendered = jinja_template.render(**kwargs)

return yaml.safe_load(rendered)
Copy link
Member

Choose a reason for hiding this comment

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

this looks quite convoluted: jinja-rendering a YAML file and directly YAML-parsing it to produce a python dictionary.
Isn't it easier to just produce a dictionary directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I was repeating what we're already doing with the sparkapplication.yaml file. It's true that we don't really need the jinja2 templating here at this point. But if we want to make it more dynamically configurable (for example the csi part in the PersistentVolume) as we do with the SparkApplication, I think this jinja templating is easier?

Copy link
Member

Choose a reason for hiding this comment

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

I'm fine with leaving it as-is for now in this PR.
But I think we should consider making the configuration aspect more explicit and robust. Because if I understand correctly at the moment, the configuration is done by overwriting certain (brittle) deep paths in the container at container run time

openeogeotrellis/integrations/kubernetes.py Show resolved Hide resolved
@soxofaan
Copy link
Member

@soxofaan as issues like #283 are introduced by me... Could you guide me in how to feature flag correctly?

I don't think this should be covered in this PR, that will lead us too far

@tcassaert
Copy link
Contributor Author

@soxofaan as issues like #283 are introduced by me... Could you guide me in how to feature flag correctly?

I don't think this should be covered in this PR, that will lead us too far

For sure. I was not thinking about fixing those in #283, I was just thinking about maybe feature-flagging this and I didn't want to introduce more technical debt.

@soxofaan
Copy link
Member

The new recommended way to define config options or feature flags is GpsBackendConfig, where you can define a new field, it's type and assign it a default value. In particular deployments this field can then be overriden from python based config files. (e.g. backendconfig_mep.py in openeo-deploy or backendconfig_prod.py in os_creodias_openeo_k8s)

@tcassaert
Copy link
Contributor Author

@soxofaan should the cleanup of the PV and PVCs be handled by openEO and if so, where exactly would this have to happen?

@soxofaan
Copy link
Member

should the cleanup of the PV and PVCs be handled by openEO and if so,

I don't think that clean up (how and triggered from where) is in scope of this ticket, right @jdries ?

@tcassaert tcassaert marked this pull request as ready for review February 23, 2024 12:52
@tcassaert
Copy link
Contributor Author

@soxofaan I've added a feature flag. Is this how it should be done?

Copy link
Contributor

@jdries jdries left a comment

Choose a reason for hiding this comment

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

only one remark/question. I would then merge so we can start testing on staging.

openeogeotrellis/backend.py Outdated Show resolved Hide resolved
@soxofaan
Copy link
Member

@soxofaan I've added a feature flag. Is this how it should be done?

yes that fuse_mount_batchjob_s3_bucket config looks good

@tcassaert tcassaert merged commit c116027 into master Mar 4, 2024
@tcassaert tcassaert deleted the openeo_cdse_infra_46_fuse_mount_results_bucket branch March 4, 2024 09:06
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.

5 participants