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.

Sign up for GitHub

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

Jump to bottom

MGDAPI-6415 move 3scale from dc to deployments #3488

Merged
merged 5 commits into from
Oct 11, 2024
Merged

MGDAPI-6415 move 3scale from dc to deployments #3488

merged 5 commits into from
Oct 11, 2024

Conversation

MStokluska
Copy link
Member

@MStokluska MStokluska commented Aug 5, 2024 •
edited
Loading

Issue link

https://issues.redhat.com/browse/MGDAPI-6415

What

Move 3scale from deployment configs to deployments
Note: Requires new version of 3scale to pass prow/work

Verification steps

  • Run make cluster/prepare/local
  • Apply CS with index created from this branch (alternatively create it yourself)
cat << EOF | oc create -f -
apiVersion: operators.coreos.com/v1alpha1
kind: CatalogSource
metadata:
  name: rhoam-operators
  namespace: openshift-marketplace
spec:
  sourceType: grpc
  image: quay.io/mstoklus/managed-api-service-index:1.40.0
EOF
  • Create RHMI CR:
IN_PROW=false USE_CLUSTER_STORAGE=true make deploy/integreatly-rhmi-cr.yml
  • Wait for RHOAM to fully install
  • Update catalog source to 1.41
  • Upgrade RHOAM
  • Confirm RHOAM got successfully updated and 3scale is on 0.12.1
  • Confirm e2e on this PR pass
  • Confirm skip preflights checks env is present on the 3scale operator pod and in 3scale operator subscription

@codecov Codecov
Copy link

codecov bot commented Aug 5, 2024 •
edited
Loading

Codecov Report

Attention: Patch coverage is 67.64706% with 33 lines in your changes missing coverage. Please review.

Project coverage is 47.69%. Comparing base (ced8eda) to head (a044aae).
Report is 11 commits behind head on master.

Files with missing lines Patch % Lines
pkg/products/threescale/reconciler.go 77.04% 12 Missing and 2 partials ⚠️
pkg/resources/marketplace/manager.go 0.00% 14 Missing ⚠️
pkg/resources/ratelimit/envoy.go 80.00% 4 Missing and 1 partial ⚠️
Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3488      +/-   ##
==========================================
- Coverage   47.77%   47.69%   -0.09%     
==========================================
  Files          95       95              
  Lines       13515    13534      +19     
==========================================
- Hits         6457     6455       -2     
- Misses       6349     6369      +20     
- Partials      709      710       +1
Files with missing lines Coverage Δ
pkg/products/threescale/envoy_prometheusRules.go 100.00% <100.00%> (ø)
pkg/resources/ratelimit/envoy.go 64.04% <80.00%> (ø)
pkg/products/threescale/reconciler.go 65.00% <77.04%> (-0.23%) ⬇️
pkg/resources/marketplace/manager.go 22.22% <0.00%> (-3.67%) ⬇️

@carlkyrillos
Copy link
Contributor

/retest

carlkyrillos
carlkyrillos reviewed Aug 5, 2024
View reviewed changes
Copy link
Contributor

@carlkyrillos carlkyrillos left a comment

Choose a reason for hiding this comment

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

I left a few minor comments but overall the changes look good to me

pkg/products/threescale/asserts_test.go Outdated Show resolved Hide resolved
pkg/resources/ratelimit/envoy.go Outdated Show resolved Hide resolved
test/common/deployment_types.go Outdated Show resolved Hide resolved
test/common/scale_up_replicas_threescale.go Outdated Show resolved Hide resolved
test/common/verify_3scale_UIBBT_alerts.go Outdated Show resolved Hide resolved
MStokluska
MStokluska commented Aug 6, 2024
View reviewed changes
test/common/deployment_types.go
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
k8sclient "sigs.k8s.io/controller-runtime/pkg/client"
)

var (
commonApiDeploymentsList = []string{
"threeScaleDeployment",
"threeScaleProductDeployment",
"cloudResourceOperatorDeployment",
"observabilityDeployment",
Copy link
Member Author

Choose a reason for hiding this comment

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

@trepel just fyi, the observability deployment does not have it's representation in the getDeploymentConfiguration - meaning we are not testing it.

Copy link
Member

Choose a reason for hiding this comment

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

Since we live with this for some time I don't think this needs to be addressed as part of this PR

MStokluska
MStokluska commented Aug 6, 2024
View reviewed changes
test/common/verify_3scale_UIBBT_alerts.go Outdated

defer func() {
err = scaleDeploymentsConfig(t, "system-app", ThreeScaleProductNamespace, 1, client)
err = scaleDeployments(context.TODO(), t, client, ThreeScaleProductNamespace, "system-app", 1)
Copy link
Member Author

Choose a reason for hiding this comment

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

@trepel fyi, I think this could use also an improvement to actually return to the replica count prior to the scale down.

Copy link
Member

Choose a reason for hiding this comment

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

I think that the most common no. of replicas for system-app is 3, so let's hard-code 3 rather than 1 as a quick improvement. And make a proper fix in a different PR since it is not related to moving away from DeploymentConfig, wdyt?

@trepel
Copy link
Member

trepel commented Aug 6, 2024

I ran the tests that were changed in this PR and all passed!

@MStokluska
Copy link
Member Author

I ran the tests that were changed in this PR and all passed!

Thanks, lets hold them for new 3scale + CRO. Thanks @trepel

carlkyrillos
carlkyrillos reviewed Oct 10, 2024
View reviewed changes
Copy link
Contributor

@carlkyrillos carlkyrillos left a comment

Choose a reason for hiding this comment

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

Reviewed changes and verified the upgrade path.

I noticed two other instance of deploymentconfig that need to be changed to deployment on the Marin3r PrometheusRules here and here. Other than that this is ready to merge.

@MStokluska
Copy link
Member Author

Reviewed changes and verified the upgrade path.

I noticed two other instance of deploymentconfig that need to be changed to deployment on the Marin3r PrometheusRules here and here. Other than that this is ready to merge.

Good catch! Thanks Carl

@MStokluska
Copy link
Member Author

/test rhoam-e2e

@carlkyrillos
Copy link
Contributor

/lgtm
/approve

@openshift-ci openshift-ci bot added the lgtm label Oct 11, 2024
@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Oct 11, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: carlkyrillos

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

@openshift-merge-bot openshift-merge-bot bot merged commit 3b5cdf5 into integr8ly:master Oct 11, 2024
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@carlkyrillos carlkyrillos carlkyrillos left review comments

@trepel trepel trepel left review comments

Assignees

@carlkyrillos carlkyrillos

Labels
approved lgtm
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@MStokluska @carlkyrillos @trepel