Skip to content
This repository has been archived by the owner on Oct 22, 2021. It is now read-only.

fix: CCDB secret rotation: set explicit resource limits #1420

Merged
merged 6 commits into from
Oct 2, 2020

Conversation

mook-as
Copy link
Contributor

@mook-as mook-as commented Oct 1, 2020

Description

Set a memory limit on the CCDB key rotation job, so that it has enough resources to run.
From experimenting locally on a minikube:

  • a limit of 128Mi is not enough to run
  • a limit of 192Mi is enough to run.

So I'm picking a request of 192Mi, and a limit of 512Mi just in case (it should never get that high anyway).

I'm also disabling the log sidecar for that job, since it's a job and having the sidecar just means it'll never complete.

Motivation and Context

CI was failing. Not sure @viovanov ever filed a bug on it.

How Has This Been Tested?

Ran the job locally (manually triggering the QJob) until it passed.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist:

  • My code has security implications.
  • My code follows the code style of this project.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.

We were failing in CI when attempting to rotate the CCDB keys; this
turns out to be because were were getting OOMKilled.  It appears that
the job needs more than 128Mi to complete (as in, 192Mi appears to work;
intermediate values were not tested).  Set the limits for the process
explicitly to hopefully fix CI.
The CCDB database key rotation job is a single-purpose job that doesn't
require a log sidecar, as only one container has meaningful output, and
it's not long-running.
@mook-as mook-as added the Type: Bug Something isn't working label Oct 1, 2020
@mook-as mook-as requested a review from jandubois October 1, 2020 18:34
jandubois
jandubois previously approved these changes Oct 1, 2020
@cfcontainerizationbot cfcontainerizationbot added the pr-test-queue PRs with this label get tested label Oct 1, 2020
@github-actions github-actions bot removed the pr-test-queue PRs with this label get tested label Oct 1, 2020
viovanov
viovanov previously approved these changes Oct 1, 2020
@cfcontainerizationbot cfcontainerizationbot added the pr-test-queue PRs with this label get tested label Oct 1, 2020
@github-actions github-actions bot removed the pr-test-queue PRs with this label get tested label Oct 1, 2020
jandubois
jandubois previously approved these changes Oct 1, 2020
@cfcontainerizationbot cfcontainerizationbot added the pr-test-queue PRs with this label get tested label Oct 1, 2020
@github-actions github-actions bot removed the pr-test-queue PRs with this label get tested label Oct 1, 2020
viovanov
viovanov previously approved these changes Oct 1, 2020
@cfcontainerizationbot cfcontainerizationbot added the pr-test-queue PRs with this label get tested label Oct 1, 2020
@github-actions github-actions bot removed the pr-test-queue PRs with this label get tested label Oct 1, 2020
@cfcontainerizationbot cfcontainerizationbot added the pr-test-queue PRs with this label get tested label Oct 2, 2020
@github-actions github-actions bot removed the pr-test-queue PRs with this label get tested label Oct 2, 2020
@viovanov viovanov merged commit de1f1f6 into master Oct 2, 2020
@viovanov viovanov deleted the marky/ccdb-rotate-memory-limits branch October 2, 2020 07:26
@fargozhu fargozhu added changelog Issue must be present in the release notes. Status: Done Implemented and PR merged labels Oct 2, 2020
@fargozhu fargozhu added this to the 2.5.1 milestone Oct 2, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
changelog Issue must be present in the release notes. Status: Done Implemented and PR merged Type: Bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants