THREESCALE-9001 / Added Zync Routes Resync Function #846

den-rgb · 2023-07-26T14:10:31Z

WHAT

There are instances where the "bundle exec rake zync:resync:domains" needs to run on the system-sidekiq pod to recreate routes.
This was previously done in RHOAM but now it will be managed by the threescale operator.

TEST

Checkout this branch
Run the following commands:
- make docker-build-only IMG=docker.io/<user>/3scale-operator:<tag>
- make operator-image-push IMG=docker.io/<user>/3scale-operator:<tag>
- make bundle-custom-build IMG=docker.io/<user>/3scale-operator:<tag> BUNDLE_IMG=docker.io/<user>/3scale-operator-bundles:<tag>
- make bundle-image-push BUNDLE_IMG=docker.io/<user>/3scale-operator-bundles:<tag>
- make bundle-run BUNDLE_IMG=docker.io/<user>/3scale-operator-bundles:<tag>
Once that runs and the operator is installed create the apimanager:
- oc new-project 3scale-test
- Create the test secret:

oc apply -f - << EOF
kind: Secret
apiVersion: v1
metadata:
  name: s3-credentials
  namespace: 3scale-test
data:
  AWS_ACCESS_KEY_ID: UkVQTEFDRV9NRQ==
  AWS_BUCKET: UkVQTEFDRV9NRQ==
  AWS_REGION: UkVQTEFDRV9NRQ==
  AWS_SECRET_ACCESS_KEY: UkVQTEFDRV9NRQ==
type: Opaque
EOF

- Create the api manager (Make sure to change the wildcard domain to your personal cluster , you can find it in the url)

oc create -f - << EOF
apiVersion: apps.3scale.net/v1alpha1
kind: APIManager
metadata:
  name: apimanager-sample
  namespace: 3scale-test
spec:
  system:
    fileStorage:
      simpleStorageService:
        configurationSecretRef:
          name: s3-credentials
  wildcardDomain: apps.<cluster>.devshift.org

EOF

- Check the progress of the api manager :
- oc get apimanager apimanager-sample -oyaml | yq '.status'
Navigate to Networking -> Routes on the 3scale-test namespace once all deployments are in the Ready status.
If a route gets deleted it should be restored.

Testing multitenancy

go into Secrets -> system-seed -> get master password
click the master route and put in (username: master) along with the password you got.
Find Accounts -> Create account .
In Networking -> routes there should be new routes created linked to the account you just made.
Delete one of the system-provider or system-developer routes associated with the new tenant they should also recreate.

TODO

Implement unit testing for the additional logic in this pr.

openshift-ci · 2023-07-26T14:10:42Z

Hi @den-rgb. Thanks for your PR.

I'm waiting for a 3scale member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Patryk-Stefanski

Verified that a route got recreated after being deleted, and left one small comment.

pkg/helper/podHelper.go

Patryk-Stefanski · 2023-08-01T10:26:14Z

/lgtm

thomasmaas · 2023-08-02T08:20:23Z

/ok-to-test

codeclimate · 2023-08-10T13:59:04Z

Code Climate has analyzed commit c58eddd and detected 10 issues on this pull request.

Here's the issue category breakdown:

Category	Count
Complexity	2
Style	8

View more on Code Climate.

eguzki

Overall, I have serious doubts whether this is the right approach or not. Not saying it is wrongly implemetned, just saying we are taking some decisions that are controversial.

The operator is acquiring knowledge about the expected routes. This something that belongs to 3scale internals. Although it is true that the status reconciler reads the basic installation's expected routes, this implementation takes one step further.
The implementation is part of the 3scale reconciler, blocking 3scale deployment when doing the task. The operator installation pattern is about creating resources and reading state to take actions. It should never be blocked executing one process in a pod or waiting of some resource to be ready.
The process can be run multiple times in parallel. Note there are multiple event sources that trigger apimanager controller reconciliation. For instance, when any of the deploymentconfigs change status, the reconcile loop is executed.
Have you considered a different approach? For instance, one shot CRD (like ProxyConfigPromote CRD) that will trigger one (and only one) process to recreate the routes. So, when the user misses some routes, or restores a deleted namespace, an instance can be created to run re-sync. I understand that it is much better experience if the operator detects automatically missing routes and self-heals installation. Just rising discussion as the implementation adds complexity

eguzki · 2023-08-17T09:40:17Z

pkg/3scale/amp/operator/zync_reconciler.go

@@ -160,6 +160,32 @@ func (r *ZyncReconciler) Reconcile() (reconcile.Result, error) {
 		return reconcile.Result{}, err
 	}

+	if len(r.apiManager.Status.Deployments.Starting) == 0 && len(r.apiManager.Status.Deployments.Stopped) == 0 && len(r.apiManager.Status.Deployments.Ready) > 0 {


This check could be implemented in apimanager_types.go in a method that can be reused if needed.

eguzki · 2023-08-17T09:43:24Z

pkg/3scale/amp/operator/zync_reconciler.go

+			return reconcile.Result{}, err
+		}
+		if exist {
+			return reconcile.Result{}, nil


why return here? What if we want to reconcile more resources related to zync?

eguzki · 2023-08-17T09:51:21Z

pkg/3scale/amp/operator/zync_reconciler.go

+			// we can force a resync of routes. see below for more details on why this is required:
+			// https://access.redhat.com/documentation/en-us/red_hat_3scale_api_management/2.7/html/operating_3scale/backup-restore#creating_equivalent_zync_routes
+			// This scenario will manifest during a backup and restore and also if the product ns was accidentally deleted.
+			podName, err := r.findSystemSidekiqPod(r.apiManager)


My main concern of this PR is with this implementation. How do you know if the pod has already a process in place running the same command? For many reasons this zync reconciliation can be run multiple times and each time you are running a process in the sidekiq pod.

eguzki · 2023-08-17T09:56:34Z

pkg/3scale/amp/operator/base_apimanager_logic_reconciler.go

+
+	listOps := []client.ListOption{
+		client.InNamespace(namespace),
+		client.MatchingLabels(map[string]string{"deploymentConfig": "system-sidekiq"}),


these labels have been defined elsewhere in this code base. Try to reuse them.

eguzki · 2023-08-17T09:59:17Z

pkg/3scale/amp/operator/base_apimanager_logic_reconciler.go

+    // Wait until system-sidekiq deployments are ready
+    for !helper.ArrayContains(apimanager.Status.Deployments.Ready, "system-sidekiq") {
+        r.Logger().Info("system-sidekiq deployments not ready. Waiting", "APIManager", apimanager.Name)
+        time.Sleep(5 * time.Second)


bad smell. Operator reconcilliation pattern is about returning fast. If you are waiting for something, return with requeue enabled.

eguzki · 2023-08-17T09:59:58Z

pkg/3scale/amp/operator/base_apimanager_logic_reconciler.go

+func (r *BaseAPIManagerLogicReconciler) waitForSystemSidekiq(apimanager *appsv1alpha1.APIManager) error {
+
+    // Wait until system-sidekiq deployments are ready
+    for !helper.ArrayContains(apimanager.Status.Deployments.Ready, "system-sidekiq") {


what if it never gets ready? this is blocking all the reconciliation of 3scale.

eguzki · 2023-08-17T10:10:25Z

pkg/3scale/amp/operator/base_apimanager_logic_reconciler.go

+    return false, fmt.Errorf("base routes not found")
+}
+
+func (r *BaseAPIManagerLogicReconciler) getAccountUrls() ([]string, []string, error){


There is a library where we have this implemented: https://github.com/3scale/3scale-porta-go-client

eguzki · 2023-08-17T10:12:31Z

pkg/helper/podHelper.go

+}
+
+func (p PodExecutor) ExecuteRemoteContainerCommand(ns string, podName string, container string, command []string) (string, string, error) {
+	kubeClient, restConfig, err := getClient()


about this client, is not available from the controller manager?

eguzki · 2023-08-17T10:14:58Z

pkg/helper/podHelper.go

+	buf := &bytes.Buffer{}
+	errBuf := &bytes.Buffer{}
+
+	err = exec.Stream(remotecommand.StreamOptions{


This is blocking, right? Meaning that the controller's runtime will be waiting for the process in the pod to be finished.

austincunningham · 2023-08-28T14:30:52Z

closing this as won't do

den-rgb requested a review from a team as a code owner July 26, 2023 14:10

openshift-ci bot added the needs-ok-to-test label Jul 26, 2023

den-rgb force-pushed the THREESCALE-9001 branch from 6183c1b to e295097 Compare July 31, 2023 14:51

Patryk-Stefanski reviewed Aug 1, 2023

View reviewed changes

pkg/helper/podHelper.go Outdated Show resolved Hide resolved

den-rgb force-pushed the THREESCALE-9001 branch from e295097 to 3ddbed4 Compare August 1, 2023 09:38

openshift-ci bot added ok-to-test and removed needs-ok-to-test labels Aug 2, 2023

den-rgb force-pushed the THREESCALE-9001 branch 6 times, most recently from 00878d5 to 0abb861 Compare August 4, 2023 15:57

THREESCALE-9001 / Zync Routes Resync Function with multitenant

c58eddd

den-rgb force-pushed the THREESCALE-9001 branch from 0abb861 to c58eddd Compare August 10, 2023 13:58

carlkyrillos mentioned this pull request Aug 10, 2023

MGDAPI-5963 Remove resyncRoutes function from 3scale reconciler integr8ly/integreatly-operator#3347

Closed

eguzki requested changes Aug 17, 2023

View reviewed changes

austincunningham closed this Aug 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

THREESCALE-9001 / Added Zync Routes Resync Function #846

THREESCALE-9001 / Added Zync Routes Resync Function #846

den-rgb commented Jul 26, 2023 •

edited

openshift-ci bot commented Jul 26, 2023

Patryk-Stefanski left a comment •

edited

Patryk-Stefanski commented Aug 1, 2023

thomasmaas commented Aug 2, 2023

codeclimate bot commented Aug 10, 2023

eguzki left a comment

eguzki Aug 17, 2023

eguzki Aug 17, 2023

eguzki Aug 17, 2023

eguzki Aug 17, 2023

eguzki Aug 17, 2023

eguzki Aug 17, 2023

eguzki Aug 17, 2023

eguzki Aug 17, 2023

eguzki Aug 17, 2023

austincunningham commented Aug 28, 2023

THREESCALE-9001 / Added Zync Routes Resync Function #846

THREESCALE-9001 / Added Zync Routes Resync Function #846

Conversation

den-rgb commented Jul 26, 2023 • edited

WHAT

TEST

Testing multitenancy

TODO

openshift-ci bot commented Jul 26, 2023

Patryk-Stefanski left a comment • edited

Choose a reason for hiding this comment

Patryk-Stefanski commented Aug 1, 2023

thomasmaas commented Aug 2, 2023

codeclimate bot commented Aug 10, 2023

eguzki left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

austincunningham commented Aug 28, 2023

den-rgb commented Jul 26, 2023 •

edited

Patryk-Stefanski left a comment •

edited