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

fix: change k8s job container creation logic to retain existing value… #9532

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cmanzi-bestow
Copy link

…s from job manifest

Fixes #9409

Fixes: #9409
Related: Relevant tracking issues, for context
Merge before/after: Dependent or prerequisite PRs

Description

This changes the logic that applies the provided container properties for executing a custom action (when using a custom job manifest) so that only the provided values (if not empty) are overwritten, and the container is not replaced entirely. I have done an initial test and this does now allow the preservation of volumeMounts and other properties that are currently erased.

Using the overrides config does not work here, as the overrides are applied prior to the logic that replaces the container.

This is a proposed solution for the referenced issue. Having looked under the hood while trying to debug this, I'm not exactly sure what the intended behavior was when providing a custom manifest, so I am open to other possible solutions - perhaps if a custom manifest is provided, the containers block should be unnecessary, since theoretically this should already be defined in the manifest?

This is currently requiring us to do a pretty extensive workaround to get this working with Cloud Deploy, as we need to run pre-deploy migrations for most of our services, and we have existing manifests that we need to use (the cut-down Container interface isn't sufficient for our purposes).

User facing changes (remove if N/A)

This would change the current behavior (which appears to be a bug, at least according to what one would expect) when providing a custom job manifest for a Kubernetes custom action. For example:

customActions:
- name: db-migration
  containers:
  - name: db-migrate
    image: db-migration-image:latest
    command:
    - /bin/sh
    - -c
    - run-db-migration
  executionMode:
    kubernetesCluster:
      jobManifestPath: migration-job.yaml

Follow-up Work (remove if N/A)

Copy link

google-cla bot commented Sep 26, 2024

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

@cmanzi-bestow
Copy link
Author

I am working on some tests right now, I just wanted some initial feedback on the solution I'm proposing.

It would help if I could better understand what the intended behavior is when using a custom K8s job manifest, as how it's set up right now really doesn't make sense to me.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Custom actions ignores container volumes
1 participant