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

THREESCALE-9315 set backend-listener to async mode also #862

Conversation

austincunningham
Copy link
Contributor

@austincunningham austincunningham commented Aug 29, 2023

Jira

THREESCALE-9315

What

set async by default on backend worker and listener for non logical database see https://github.com/3scale/apisonator/blob/master/docs/openshift_horizontal_scaling.md#enable-async for reference

Verification

3scale configured with Logical databases ( default installation)

  • Create a project oc new-project 3scale-test
  • make install
  • make run
  • Create a s3 secret
kubectl apply -f - <<EOF
---
kind: Secret
apiVersion: v1
metadata:
  name: s3-credentials
data:
  AWS_ACCESS_KEY_ID: UkVQTEFDRV9NRQ==
  AWS_BUCKET: UkVQTEFDRV9NRQ==
  AWS_REGION: UkVQTEFDRV9NRQ==
  AWS_SECRET_ACCESS_KEY: UkVQTEFDRV9NRQ==
type: Opaque
EOF
  • get the default application router
DOMAIN=$(oc get routes console -n openshift-console -o json | jq -r '.status.ingress[0].routerCanonicalHostname' | sed 's/router-default.//')
  • Create an apimanager CR using that secret e.g.
kubectl apply -f - <<EOF
---
apiVersion: apps.3scale.net/v1alpha1
kind: APIManager
metadata:
  name: apimanager-sample
spec:
  system:
    fileStorage:
      simpleStorageService:
        configurationSecretRef:
          name: s3-credentials
  wildcardDomain: $DOMAIN
EOF
  • Check the backend worker pod for the CONFIG_REDIS_ASYNC envar
$ oc get po | grep backend-worker
backend-worker-1-deploy       0/1     Completed   0             12m
backend-worker-1-jn29j        1/1     Running     0             12m
$ oc exec backend-worker-1-jn29j -- env | grep CONFIG_REDIS_ASYNC
# should return nothing
  • Check the backend listener pod for the CONFIG_REDIS_ASYNC envar
$ oc get po | grep backend-listener
backend-listener-1-deploy       0/1     Completed   0             12m
backend-listener-1-jn29j        1/1     Running     0             12m
$ oc exec backend-listener-1-jn29j -- env | grep CONFIG_REDIS_ASYNC
Defaulted container "backend-listener" out of: backend-listener, backend-redis-svc (init)
CONFIG_REDIS_ASYNC=0
$ oc exec backend-listener-1-jn29j -- env | grep LISTENER_WORKERS
# should return nothing
  • delete the apimanger cr
  • stop the 3scale operator from running

3scale configured with non logical databases

  • you can Run CRO locally to provision 2 on cluster redis
make cluster/prepare
make run
REDIS_NAME=redis-storage make cluster/seed/redis
REDIS_NAME=redis-que make cluster/seed/redis

The redis will be installed in the cloud-resources-operator ns there should be 2 secrets redis-que-sec and redis-storage-sec with the port and the uri for the redis

  • once they are created you can stop the CRO operator running

  • create a key/value secret in the 3scale-test ns called backend-redis and add 2 name values with the connection uri and port for the env vars REDIS_QUEUES_URL & REDIS_STORAGE_URL
    image

  • start the 3scale-operator running locally

  • Create an apimanager CR with external components set for the backend

kubectl apply -f - <<EOF
---
apiVersion: apps.3scale.net/v1alpha1
kind: APIManager
metadata:
  name: apimanager-sample
spec:
  externalComponents:
    backend:
      redis: true
  system:
    fileStorage:
      simpleStorageService:
        configurationSecretRef:
          name: s3-credentials
  wildcardDomain: $DOMAIN
EOF
  • Check the backend worker pod for the CONFIG_REDIS_ASYNC envar
$ oc get po | grep backend-worker
backend-worker-1-deploy       0/1     Completed   0             12m
backend-worker-1-jn29j        1/1     Running     0             12m
$ oc exec backend-worker-1-jn29j -- env | grep CONFIG_REDIS_ASYNC
Defaulted container "backend-worker" out of: backend-worker, backend-redis-svc (init)
CONFIG_REDIS_ASYNC=1
  • Check the backend listener pod for the CONFIG_REDIS_ASYNC envar and LISTENER_WORKERS
$ oc get po | grep backend-listener
backend-listener-1-deploy       0/1     Completed   0             12m
backend-listener-1-jn29j        1/1     Running     0             12m
$ oc exec backend-listener-1-jn29j -- env | grep CONFIG_REDIS_ASYNC
Defaulted container "backend-listener" out of: backend-listener, backend-redis-svc (init)
CONFIG_REDIS_ASYNC=1
$ oc exec backend-listener-1-jn29j -- env | grep LISTENER_WORKERS
Defaulted container "backend-listener" out of: backend-listener, backend-redis-svc (init)
LISTENER_WORKERS=1
  • Check the backend listener for the falcon Arg
$ oc get dc backend-listener -oyaml | yq '.spec.template.spec.containers[0].args'
- bin/3scale_backend
- -s
- falcon
- start
- -e
- production
- -p
- "3000"
- -x
- /dev/stdout
  • remove the envar from the backend-worker and backend-listener deployment config spec.containers.env and see if its reconciled back e.g. remove this
- name: CONFIG_REDIS_ASYNC
  value: '1'
  • set the value in env var CONFIG_REDIS_ASYNC to '0' and it should revert to '1'

@austincunningham austincunningham requested a review from a team as a code owner August 29, 2023 07:32
@austincunningham austincunningham changed the title THREESCALE-9315 set backend-listener to async mode also WIP THREESCALE-9315 set backend-listener to async mode also Aug 30, 2023
@austincunningham austincunningham changed the title WIP THREESCALE-9315 set backend-listener to async mode also THREESCALE-9315 set backend-listener to async mode also Aug 30, 2023
@MStokluska
Copy link
Contributor

👀

@MStokluska
Copy link
Contributor

/lgtm

@carlkyrillos
Copy link
Contributor

/approve

eguzki
eguzki previously requested changes Sep 5, 2023
Copy link
Member

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

Async mode on does not support logical databases 3scale/apisonator#135

So, default deployment (with backend redis having two logical databases for stats and queue) will be broken

We are currently using aysnc-redis v0.7.0. I have checked and latest is v0.8.0 and it does not seem to be addressed this issue.

* f11a366 - (3 months ago) Bump minor version. - Samuel Williams (HEAD -> main, tag: v0.8.0, origin/main, origin/HEAD)
* 8920797 - (3 months ago) Modernize gem. - Samuel Williams
* 100b4e0 - (3 months ago) More test fixes. - Samuel Williams
* 5a03fb9 - (3 months ago) Update tests. (#47) - Samuel Williams
* f67fd18 - (3 months ago) Relax dependency on protocol-redis. - Samuel Williams
* 36f3786 - (3 months ago) Add pubsub example. - Samuel Williams
* 71155be - (3 months ago) Modernize gem. (#46) - Samuel Williams
| * 03ecd4d - (3 months ago) Modernize gem. - Samuel Williams (origin/modernize-gem)
|/  
* 491f098 - (3 months ago) Disable performance spec. - Samuel Williams
* d9cdb85 - (3 months ago) Add bake-test to `gems.rb`. - Samuel Williams
* ab4f8ef - (9 months ago) A bunch of random examples. - Samuel Williams
* c3321ad - (1 year, 1 month ago) Bump minor version. - Samuel Williams (tag: v0.7.0)

Their issue asking for this was closed socketry/async-redis#15
I would not expect this to be implemented soon unless we go for it ourselves

@austincunningham
Copy link
Contributor Author

/hold

@austincunningham
Copy link
Contributor Author

/unhold

@austincunningham austincunningham force-pushed the THREESCALE-9315-backend-listener branch from 01a6dab to c04d2de Compare September 29, 2023 15:00
@eguzki
Copy link
Member

eguzki commented Oct 9, 2023

Looks like the backend listener needs more deployment changes. Check out the doc about HPA in backend. https://github.com/3scale/apisonator/blob/master/docs/openshift_horizontal_scaling.md

Basically, instead of "pumas" (backend HTTP framework), async modes requires "falcons" (backend async HTTP framework).

@austincunningham
Copy link
Contributor Author

/retest-required

@austincunningham austincunningham force-pushed the THREESCALE-9315-backend-listener branch from fd01d33 to 71a48cd Compare October 11, 2023 14:42
@austincunningham
Copy link
Contributor Author

/unhold

@valerymo
Copy link
Contributor

valerymo commented Nov 12, 2023

@austincunningham , maybe need rebase (?)

@valerymo
Copy link
Contributor

valerymo commented Nov 12, 2023

@austincunningham , I checked both cases. These are few notes.

  1. First case (default installation ) - not working as expected. Env vars CONFIG_REDIS_ASYNC and LISTENER_WORKERS are missing in backend-listener / worker pods.
    It happens as noted in code comments because initial urls REDIS_QUEUES_URL & REDIS_STORAGE_URL are differ just with suffix 1/0, and suffix is deleted here https://github.com/austincunningham/3scale-operator/blob/THREESCALE-9315-backend-listener/pkg/3scale/amp/operator/backend_reconciler.go#L49-L50, so Mutation method where initial values should be set - is not entered. It's only in 1st test case.
  2. Second test case is working as expected
  3. Seems to me that initial values for CONFIG_REDIS_ASYNC and LISTENER_WORKERS should be set NOT in Mutation method (that seems intended for changes), Maybe initial values could be set similar to BACKEND_REDIS_SENTINEL_HOSTS (?), from secret (or simple hardcoded initial values = 0 ? : )
    Thanks

pkg/3scale/amp/operator/backend_reconciler.go Show resolved Hide resolved
pkg/3scale/amp/operator/backend_reconciler.go Outdated Show resolved Hide resolved
pkg/reconcilers/deploymentconfig.go Outdated Show resolved Hide resolved
@austincunningham
Copy link
Contributor Author

@austincunningham , I checked both cases. These are few notes.

  1. First case (default installation ) - not working as expected. Env vars CONFIG_REDIS_ASYNC and LISTENER_WORKERS are missing in backend-listener / worker pods.
    It happens as noted in code comments because initial urls REDIS_QUEUES_URL & REDIS_STORAGE_URL are differ just with suffix 1/0, and suffix is deleted here https://github.com/austincunningham/3scale-operator/blob/THREESCALE-9315-backend-listener/pkg/3scale/amp/operator/backend_reconciler.go#L49-L50, so Mutation method where initial values should be set - is not entered. It's only in 1st test case.
  2. Second test case is working as expected
  3. Seems to me that initial values for CONFIG_REDIS_ASYNC and LISTENER_WORKERS should be set NOT in Mutation method (that seems intended for changes), Maybe initial values could be set similar to BACKEND_REDIS_SENTINEL_HOSTS (?), from secret (or simple hardcoded initial values = 0 ? : )
    Thanks

See comment #862 (comment) for 1 the default case.

@valerymo
Copy link
Contributor

valerymo commented Nov 16, 2023

/lgtm
@austincunningham , It looks good, but seems need rebase (? , although seems no conflicts)

@austincunningham
Copy link
Contributor Author

austincunningham commented Nov 16, 2023

@valerymo it doesn't need rebase, they are old needs-rebase labels its in sync now no need to do it again.

THREESCALE-9315 only add CONFIG_REDIS_ASYNC if it is missing, dont reconcile other envvars

THREESCALE-9315 always set CONFIG_REDIS_ASYNC to 1
@austincunningham austincunningham force-pushed the THREESCALE-9315-backend-listener branch from 71a48cd to f62eb77 Compare November 16, 2023 14:49
@MStokluska
Copy link
Contributor

Thanks @austincunningham, to me, it could use some improvements around the env and args reconciliation functions.

Copy link

codeclimate bot commented Nov 20, 2023

Code Climate has analyzed commit 69f802d and detected 3 issues on this pull request.

Here's the issue category breakdown:

Category Count
Style 3

View more on Code Climate.

@MStokluska
Copy link
Contributor

/lgtm

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

Successfully merging this pull request may close these issues.

6 participants