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

feat: added K8s CronJob implementation #45

Merged
merged 7 commits into from
Jun 21, 2023
Merged

feat: added K8s CronJob implementation #45

merged 7 commits into from
Jun 21, 2023

Conversation

d7oc
Copy link
Contributor

@d7oc d7oc commented Jun 19, 2023

This PR implements #12.

@d7oc d7oc requested a review from xoxys June 19, 2023 17:06
@d7oc
Copy link
Contributor Author

d7oc commented Jun 19, 2023

@xoxys How should we deal with cronLog. This setting is no more needed with this PR as the logs of the CronJob execution will be found in K8s.

@xoxys
Copy link
Contributor

xoxys commented Jun 19, 2023

Drop it I would say.

@xoxys
Copy link
Contributor

xoxys commented Jun 20, 2023

I'm not that convinced by the huge include block for the env vars. Wouldn't be using a ConfigMap cleaner if we need to share all env vars with other containers?

@d7oc
Copy link
Contributor Author

d7oc commented Jun 20, 2023

I'm not that convinced by the huge include block for the env vars. Wouldn't be using a ConfigMap cleaner if we need to share all env vars with other containers?

We could switch to the ConfigMap (or Secret in here), but the pure amount will still be the same. I'm also not quite sure if there won't be any conflict between the still existing overwrite.config.php and the new file which we will place there build from the config map. I see parameters like those https://github.com/owncloud-docker/base/blob/62d4e7ff12f79fd3a02b17ab6fdca432605ca870/latest/overlay/etc/templates/config.php#L25-L31 which would be set empty as there is no check upfront. So there might be a race condition if I'm right. What we might try is to place the config map "on top" the overwrite.config.php with an overlay mount. I'm just not sure if this works. Needs a test.

So in a nutshell the template approach sounds simpler to me.

d7oc added 2 commits June 20, 2023 15:37
- added annotation for non-root containers
- changed deleted comment line
@xoxys
Copy link
Contributor

xoxys commented Jun 20, 2023

That's not what I was talking about. The idea was to just use a ConfigMap to properly share the env vars by using envFrom.configMapRef, see https://kubernetes.io/docs/tasks/configure-pod-container/configure-pod-configmap/#configure-all-key-value-pairs-in-a-configmap-as-container-environment-variables

@d7oc
Copy link
Contributor Author

d7oc commented Jun 20, 2023

Ah this. Ok this might work. Will give it a try.

- added configmap with all environment variables
- added -cronjob to cronjob names
- added container restart on config changes via checksum
@d7oc
Copy link
Contributor Author

d7oc commented Jun 20, 2023

Done, also tested working on a local OpenShift cluster, please recheck.

@xoxys
Copy link
Contributor

xoxys commented Jun 21, 2023

How is crondEnabled intended to be used? Right now, cronjobs can't be disabled, right?

@d7oc
Copy link
Contributor Author

d7oc commented Jun 21, 2023

How is crondEnabled intended to be used? Right now, cronjobs can't be disabled, right?

You're right, this was missing, good catch. Added in c85c92d.

@xoxys xoxys linked an issue Jun 21, 2023 that may be closed by this pull request
@d7oc d7oc merged commit 499d695 into main Jun 21, 2023
@delete-merged-branch delete-merged-branch bot deleted the d7oc/featurerequest12 branch June 21, 2023 08:35
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.

[FR] Implement container internal cronjob via k8s CronJob
2 participants