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

Upstream rebase to 2.12.1 #22

Merged
merged 103 commits into from
Mar 8, 2024
Merged

Conversation

joelsmith
Copy link

@joelsmith joelsmith commented Mar 1, 2024

  1. Create a rebase tree using rebasebot:
rebasebot --source https://github.com/kedacore/keda:release/v2.12 --dest openshift/keda:main \
          --rebase joelsmith/keda:rebase --tag-policy=strict --update-go-modules --dry-run \
          --github-user-token <(yaml2json ~/.config/hub | jq -r '."github.com"[0].oauth_token')
  1. cd .rebase
  2. Update .ci-operator.yaml and squash into existing carry commit
  3. Clean up carry commits, update OWNERS, squash where appropriate
  4. Add verify history drop commit to use upstream's release/v2.12 branch as the merge base
  5. Push to own fork, open PR

The new upstream version brings in optional support for opentelemetry metrics, which we don't use. Two additional PRs on the main branch were cherry-picked here (UPSTREAM: 5436: Add ENABLE_OPENTELEMETRY flag and UPSTREAM: 5578: e2e: Fix e2e test in non-opentelemetry case) to make that new feature default-disabled so that the opentelemetry e2e test can pass.

zroubalik and others added 30 commits June 22, 2023 18:56
Signed-off-by: Zbynek Roubalik <zroubalik@gmail.com>
Signed-off-by: Yoon Park <yoongon.park@gmail.com>
Signed-off-by: Indresh-Prakash <indreshprakash24@gmail.com>
… `msgBacklog` (kedacore#4736)

Co-authored-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
Signed-off-by: dylan <dylan@channel.io>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
Signed-off-by: spiritzhou <iammrzhouzhenghan@gmail.com>
Signed-off-by: SpiritZhou <iammrzhouzhenghan@gmail.com>
Signed-off-by: Tom Kerkhove <kerkhove.tom@gmail.com>
Co-authored-by: Tom Kerkhove <kerkhove.tom@gmail.com>
…edacore#4768)

Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
Signed-off-by: Jorge Turrado <jorge_turrado@hotmail.es>
Signed-off-by: Jorge Turrado Ferrero <Jorge_turrado@hotmail.es>
…4707)

Signed-off-by: Yoon Park <yoongon.park@gmail.com>
)

Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>
Signed-off-by: Jorge Turrado <jorge.turrado@scrm.lidl>
…acore#4839)

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
@joelsmith joelsmith force-pushed the rebase branch 2 times, most recently from 3a6340a to 9444f3e Compare March 6, 2024 18:01
@joelsmith
Copy link
Author

/retest

1 similar comment
@joelsmith
Copy link
Author

/retest

@joelsmith
Copy link
Author

Strange... without the fix for TestCpuScaler, all the other tests pass, but with the fix, TestEvents failed twice in a row and TestScaledObjectGeneral failed once. Are they flaky or fully broken?

/retest

@joelsmith
Copy link
Author

/hold
Waiting for kedacore#5578 to merge

@openshift-ci openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 7, 2024
.ci-operator.yaml Outdated Show resolved Hide resolved
joelsmith and others added 12 commits March 7, 2024 14:48
This is used by CPaaS where Cachito will make a cached copy of anything
needed during the build so that an offline brew build can succeed.
* hack/cma-verify-history.sh: checks history to make sure to use
  rebasebot-friendly commit messages
* Allow privileged pods in OpenShift test namespaces

The way the e2e test suite is set up, there are several pods that are
to running with more privilege than our "restricted" SCC provides.
Long-term I don't think there is anything in here that *requires* the
privilege, but we'll need to do some testing and find out.

In the mean time, this injects an adjusted pod admission policy into
each test namespace via the centralized namespace creation helper
function so that those privileged pods can run in the test namespaces.

* Specify securityContext for privileged e2e pods

The e2e test suite here is used to running in a "vanilla kube"
environment that does not have OpenShift/OpenShift CI restrictions. This
becomes a problem when one of the test containers attempts to do
something privileged (like bind to a privileged port) and is denied.

This just adds securityContexts to the pods that require privilege so
that they can get assigned a proper SCC and successfully run. The
securityContext addition is limited to only the tests that OpenShift
runs (internal, sequential, cpu/memory/kafka scalers) because we haven't
tested the others.

* Allow e2e test image overrides for OpenShift CI

The e2e test suite references multiple images spread across multiple
public registries (ghcr.io,docker.io,k8s.io) and some of those
registries have pull limits, which will cause our tests to fail.

We also cache some of these upstream images in our CI system, and so it
is beneficial to be able to reference our cached copy rather than have
to pull it from "the internet" every time.

Anyway, the way that the e2e tests are set up, all those images are
hard-coded in each of the manifests, which are just vars that exist in
each test's .go file. They are not templated. There is, however a
central helper function that applies all these resources (using
kubectl).

So, in order for us to be able to override the image list for CI, this
temporarily:
- adds an image rewrite map that specifies replacement images for
  images we might have difficulty pulling
- adds a helper function that will let those replacement images be
  specified by environment variables for use in CI
until we can figure out a more elegant refactor.

* Account for OpenShift CI in Prometheus build test

There is a test in the prometheus sequential suite that checks the git
commit hash of the current code and compares it to the containers
running in the test to make sure that the test version matches the code
version.

This version is injected as GIT_COMMIT during the docker builds
in the Makefile, but it does not get injected when the containers are
build in OpenShift CI. I would like to find a way to inject it via CI,
but until then we are supplying a dummy string "dummy-ci-commit-value"
that is at least "yes you are running against a CI payload we built, and
not one that you pulled from upstream".

Eventually when we figure out how to make all the variables available in
CI and inject them, this can go away because then the commits will
match.
* Pull test container dockerfile out of CI and into keda repo

Previously we were building the test container in CI from a
dockerfile_literal, which was kind of hacky and more difficult to manage
than it being here in the keda repo.

This pulls that dockerfile out of CI and into a Dockerfile.tests which
we now just reference from CI.

* Add Makefile targets to makefile for OpenShift tests

We kind of stuffed those tests into CI quick so we had something, and
when we did we didn't heavily consider ergonomics. Now that we find
ourselves having to enable additional tests for fixes and new features,
it will be much easier in the long run if we can manage the test targets
here in the repo so we don't have to put in a separate PR to the release
repo to see if our changes work.

This adds some e2e-test-openshift* makefile targets that we can point
and whatever we need to, and once CI is updated, it can just call those
targets, whatever they happen to entail.

* Reenable CPU scaler test

Now that we figured out how the CPU test was broken, we can add it back
in to the testing since it's supported.

This adds the cpu test into the e2e-test-openshift Makefile target, so
when CI calls it, it will run with the rest of the scaler tests
This excludes deps and tests from snyk scans to cut down on noise.

This also excludes the tests/ directory as it contains some launcher .go
files that don't end in _test.go, but are part of the testing suite and
are not shipped with the final product.
…e/v2.12 branch instead of main

Signed-off-by: Joel Smith <joelsmith@redhat.com>
Signed-off-by: Joel Smith <joelsmith@redhat.com>
The CPU scaler test assumes a default metrics window of 30s, so those
testing on platforms where it is set to a larger value will potentially
fail the CPU scaler test because the metrics won't be ready by the time
the test starts.

This:
- Adds a helper that waits for either the metrics to show up in the
HPA, or for some amount of time to pass, whichever happens first
- Uses said helper to ensure that the metrics are ready before the CPU
test starts testing scaling

Signed-off-by: John Kyros <jkyros@redhat.com>
Signed-off-by: Joel Smith <joelsmith@redhat.com>
@jkyros
Copy link

jkyros commented Mar 7, 2024

looks good!
/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Mar 7, 2024
Copy link

openshift-ci bot commented Mar 7, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jkyros, joelsmith

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Copy link

openshift-ci bot commented Mar 8, 2024

@joelsmith: all tests passed!

Full PR test history. Your PR dashboard.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@joelsmith
Copy link
Author

Upstream e2e test fix merged.
/hold cancel

@openshift-ci openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Mar 8, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 60aea7b into openshift:main Mar 8, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.