Update existing resources when CR changes #119

jianrongzhang89 · 2024-01-05T02:15:57Z

Description

Update the deployed resources when backstage CR is updated.

Which issue(s) does this PR fix or relate to

Fixed Update existing resources when CR changes #72

PR acceptance criteria

[X ] Tests
Documentation

How to test changes / Special notes to the reviewer

Deploy the operator on OpenShift and examples/bs1.yaml.
Update the CR spec to include:

spec:
   application:
      replicas: 2

and verify that the deployed backstage has 2 pods running.
3) Update the CR spec to include:

spec:
   application:
      extraEnvs:
        envs:
        - name: myenv1
          value: my-value-1
        - name: myenv2
          value: my-value-2

and check that the deployed backstage pod has env vars myenv1 and myenv2 set in the main container.
4) Update the CR spec to disable the route:

spec:
   application:
      route:
        enabled: true

and verify that the backstage route bas been cleaned up.
5) Update the CR spec to disable the localDB:

spec:
  database:
    enableLocalDb: false
    authSecretName:  existing-secret

and verify that the local db statefulset, services and secret that were deployed earlier are now deleted.

Note that two status condition types "RouteSynced" and "LocalDbSynced" are introduced to support the functionality required.

github-actions · 2024-01-05T02:18:55Z

PR images are available:

github-actions · 2024-01-05T02:41:12Z

PR images are available:

rm3l

Seems to work fine at first sight, but I also noticed a slight issue:

imagePullSecrets seem to keep adding to the Backstage Deployment Spec if CR contains a spec.application.imagePullSecrets and is updated (I just updated the image and number of replicas), e.g.:

$ kubectl get deployment backstage-my-rhdh -o yaml
...
       volumeMounts:                                                                                                                                                                          
        - mountPath: /opt/app-root/src/dynamic-plugins-root                                                                                                                                    
          name: dynamic-plugins-root                                                                                                                                                           
        - mountPath: /opt/app-root/src/app-config-rhdh.yaml                                                                                                                                    
          name: app-config-rhdh                                                                                                                                                                
          subPath: app-config-rhdh.yaml                                                                                                                                                        
      dnsPolicy: ClusterFirst                                                                                                                                                                  
      imagePullSecrets:                                                                                                                                                                        
      - name: rhdh-pull-secret                                                                                                                                                                 
      - name: rhdh-pull-secret                                                                                                                                                                 
      - name: rhdh-pull-secret
      - name: rhdh-pull-secret
      - name: rhdh-pull-secret
      - name: rhdh-pull-secret
      - name: rhdh-pull-secret
      - name: rhdh-pull-secret
      - name: rhdh-pull-secret
      - name: rhdh-pull-secret
      - name: rhdh-pull-secret
      - name: rhdh-pull-secret
      - name: rhdh-pull-secret
      - name: rhdh-pull-secret
      - name: rhdh-pull-secret
      - name: rhdh-pull-secret
      - name: rhdh-pull-secret
      - name: rhdh-pull-secret
      - name: rhdh-pull-secret
      - name: rhdh-pull-secret
...

Can you take a look?

api/v1alpha1/backstage_types.go

bundle/manifests/backstage-operator.clusterserviceversion.yaml

jianrongzhang89 · 2024-01-05T13:31:12Z

@rm3l fixed imagePullSecrets issue.

github-actions · 2024-01-05T13:31:57Z

PR images are available:

github-actions · 2024-01-05T15:06:47Z

PR images are available:

github-actions · 2024-01-08T02:27:44Z

PR images are available:

api/v1alpha1/backstage_types.go

config/manager/kustomization.yaml

controllers/backstage_controller.go

controllers/backstage_deployment.go

controllers/backstage_route.go

controllers/backstage_controller_test.go

controllers/backstage_controller.go

controllers/backstage_deployment.go

github-actions · 2024-01-08T19:53:03Z

PR images are available:

github-actions · 2024-01-08T20:06:47Z

PR images are available:

github-actions · 2024-01-09T14:08:17Z

PR images are available:

gazarenkov

The main reason we postpone Update to the later version is edge cases which we have due to our configuration specific. I think it would be worth to investigate these cases before and come to some conclusions.
I've created some some issues for that, please take a look, it would be good to resolve it as a prerequisite.

gazarenkov · 2024-01-10T11:27:51Z

api/v1alpha1/backstage_types.go

@@ -21,6 +21,11 @@ import (
 const (
 	RuntimeConditionRunning string = "RuntimeRunning"
 	RuntimeConditionSynced  string = "RuntimeSyncedWithConfig"
+	RouteSynced             string = "RouteSynced"
+	LocalDbSynced           string = "LocalDbSynced"
+	SyncOK                  string = "SyncOK"


How it differs to RuntimeSyncedWithConfig = True condition?

RuntimeSyncedWithConfig status is not currently implemented and we can have a follow up PR to get it done.

I mean RuntimeSyncedWithConfig looks like the same thing as SyncOK, no?

gazarenkov · 2024-01-10T11:28:05Z

api/v1alpha1/backstage_types.go

+	RouteSynced             string = "RouteSynced"
+	LocalDbSynced           string = "LocalDbSynced"
+	SyncOK                  string = "SyncOK"
+	SyncFailed              string = "SyncFailed"


How it differs to RuntimeSyncedWithConfig = False condition?

controllers/backstage_controller.go

controllers/backstage_controller_test.go

controllers/backstage_deployment.go

gazarenkov · 2024-01-10T11:46:25Z

controllers/backstage_route.go

-	if err != nil {
-		if !errors.IsNotFound(err) {
-			return fmt.Errorf("failed to get backstage route, reason: %s", err)
+	if _, err := controllerutil.CreateOrUpdate(ctx, r.Client, route, r.routeObjectMutFun(ctx, route, *backstage, ns)); err != nil {


What's the advantage of using controllerutil.CreateOrUpdate in this (and other similar) cases instead of plan API calls?

controllerutil.CreateOrUpdate() is a nice utility library function call that handles the logic like get the current resource and merge it with the new spec using the mutate(), and then do the update.

gazarenkov · 2024-01-10T11:48:30Z

controllers/backstage_service.go

 		}
-	}
+		service.Spec.ClusterIP = targetService.Spec.ClusterIP
+		for _, ip1 := range targetService.Spec.ClusterIPs {


Are we sure we need to process this for Kubernetes?
I believe all the options offered are quite valid (depending on infrastructure specific), why we need to be that opinionated here

When a service is created, normally k8s may update the spec to include the clusterIP and clusterIPs assigned randomly. These values can not be updated later.

Well, it is often use case but not only, user is able to specify it or to set None for headless service
https://kubernetes.io/docs/concepts/services-networking/service/

Yes the user is able to create headless service (actually the operator deploys a headless service for local db). What I meant here after the service has been created, the cluserIP should not be updated.
https://github.com/kubernetes/api/blob/master/core/v1/types.go#L5040

Not sure I understand it.
Why we have this opinion and check it ourselves?

Because without this checking, reconciliation would fail, as the operator tries to override the IPs that were populated by k8s, leading to an update error.

It is not valid, I believe
According to https://kubernetes.io/docs/concepts/services-networking/service/
If you do not specify ClusterIP (which is a case a believe) it is is assigned by K8s, i.e it is controlled by Kubernetes, Operator's desired state is still "I let K8s manage it" so, I believe operator must not try to override it.

gazarenkov · 2024-01-10T11:49:23Z

controllers/local_db_services.go

+		if len(targetService.Spec.ClusterIP) > 0 && service.Spec.ClusterIP != "" && service.Spec.ClusterIP != "None" && service.Spec.ClusterIP != targetService.Spec.ClusterIP {
+			return fmt.Errorf("db service IP can not be updated: %s, %s, %s", targetService.Name, targetService.Spec.ClusterIP, service.Spec.ClusterIP)
+		}
+		service.Spec.ClusterIP = targetService.Spec.ClusterIP


same as above about ClusterIP

same as above

jianrongzhang89 · 2024-01-10T14:19:16Z

The main reason we postpone Update to the later version is edge cases which we have due to our configuration specific. I think it would be worth to investigate these cases before and come to some conclusions. I've created some some issues for that, please take a look, it would be good to resolve it as a prerequisite.

Yes resource update involves various use cases as you pointed out. Most of the ones in #124 are for default config/configmao/secret changes etc and they can be handled separately later. This PR is for the application of the CR changes only.

github-actions · 2024-01-12T02:07:01Z

PR images are available:

github-actions · 2024-01-12T02:31:09Z

PR images are available:

sonarqubecloud · 2024-01-12T14:49:57Z

Quality Gate passed

Kudos, no new issues were introduced!

0 New issues
0 Security Hotspots
No data about Coverage
2.3% Duplication on New Code

See analysis details on SonarCloud

github-actions · 2024-01-12T14:55:31Z

PR images are available:

Makefile

rm3l

Should it be possible to update the route sub-domain after it is created?
I first created a CR with no spec.application.route field set. Then I added spec.application.route.subdomain to the CR and updated it. But I noticed that the Route was not updated. Is that expected?

rm3l

I've tested this PR against scenarios depicted in https://github.com/janus-idp/operator/blob/main/.rhdh/docs/openshift.adoc, and everything works fine.
We'll need to update https://github.com/janus-idp/operator/blob/main/.rhdh/docs/openshift.adoc (to remove the need to manually delete the Backstage Deployment upon CR changes), but this can done in a subsequent PR/issue.
I think we can go ahead and merge this PR, so that QE can start testing, and also to simplify the docs workflow after CR is modified by the user.
#70 will also need to take that into account.

openshift-ci · 2024-01-13T00:12:39Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rm3l

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:

~~OWNERS~~ [rm3l]

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

jianrongzhang89 · 2024-01-13T00:39:44Z

Should it be possible to update the route sub-domain after it is created? I first created a CR with no spec.application.route field set. Then I added spec.application.route.subdomain to the CR and updated it. But I noticed that the Route was not updated. Is that expected?

Yes this is expected. Since the route host was already created, the subdomain change in the route is ignored by the OpenShift server.

jianrongzhang89 temporarily deployed to internal January 5, 2024 02:16 — with GitHub Actions Inactive

jianrongzhang89 requested review from nickboldt, gazarenkov and rm3l January 5, 2024 02:29

jianrongzhang89 force-pushed the resource-update branch from 159c97f to 288bc09 Compare January 5, 2024 02:38

jianrongzhang89 temporarily deployed to internal January 5, 2024 02:38 — with GitHub Actions Inactive

rm3l reviewed Jan 5, 2024

View reviewed changes

api/v1alpha1/backstage_types.go Outdated Show resolved Hide resolved

api/v1alpha1/backstage_types.go Outdated Show resolved Hide resolved

bundle/manifests/backstage-operator.clusterserviceversion.yaml Outdated Show resolved Hide resolved

jianrongzhang89 temporarily deployed to internal January 5, 2024 13:29 — with GitHub Actions Inactive

jianrongzhang89 force-pushed the resource-update branch from 624d3b5 to a75a2f5 Compare January 5, 2024 15:03

jianrongzhang89 temporarily deployed to internal January 5, 2024 15:03 — with GitHub Actions Inactive

jianrongzhang89 temporarily deployed to internal January 8, 2024 02:23 — with GitHub Actions Inactive

gazarenkov suggested changes Jan 8, 2024

View reviewed changes

rm3l reviewed Jan 8, 2024

View reviewed changes

controllers/backstage_deployment.go Outdated Show resolved Hide resolved

jianrongzhang89 temporarily deployed to internal January 8, 2024 19:50 — with GitHub Actions Inactive

jianrongzhang89 force-pushed the resource-update branch from a187a23 to 946874a Compare January 8, 2024 20:03

jianrongzhang89 temporarily deployed to internal January 8, 2024 20:03 — with GitHub Actions Inactive

jianrongzhang89 temporarily deployed to internal January 9, 2024 14:03 — with GitHub Actions Inactive

jianrongzhang89 mentioned this pull request Jan 9, 2024

Refactor operator logic #70

Closed

gazarenkov suggested changes Jan 10, 2024

View reviewed changes

jianrongzhang89 force-pushed the resource-update branch from ff714ce to 9f163a9 Compare January 10, 2024 21:00

jianrongzhang89 force-pushed the resource-update branch from f4702b0 to 89f5392 Compare January 12, 2024 01:53

jianrongzhang89 temporarily deployed to internal January 12, 2024 01:53 — with GitHub Actions Inactive

jianrongzhang89 temporarily deployed to internal January 12, 2024 02:02 — with GitHub Actions Inactive

jianrongzhang89 temporarily deployed to internal January 12, 2024 02:24 — with GitHub Actions Inactive

jianrongzhang89 added 8 commits January 12, 2024 09:48

Update existing resources when CR changes

08b04ae

fix pullsecret handling issue

224c682

fix RouteSynced and LocalDbSynced status when resource is deleted

44e16be

Address review comments

5058140

revert to use default image tag based on VERSION in Makefile

8cc4036

Rebase the code

4c3fb02

Fix SonarCloud Code Analysis issues

662f42b

Fix localdb secret name issue

302957c

jianrongzhang89 force-pushed the resource-update branch from 157a016 to 302957c Compare January 12, 2024 14:49

jianrongzhang89 temporarily deployed to internal January 12, 2024 14:49 — with GitHub Actions Inactive

rm3l reviewed Jan 12, 2024

View reviewed changes

Makefile Show resolved Hide resolved

rm3l reviewed Jan 12, 2024

View reviewed changes

rm3l approved these changes Jan 13, 2024

View reviewed changes

openshift-ci bot assigned rm3l Jan 13, 2024

openshift-ci bot added the lgtm PR is ready to be merged. Required by Prow. label Jan 13, 2024

openshift-ci bot added the approved PR has been approved by an approver from all required OWNERS files. Required by Prow. label Jan 13, 2024

jianrongzhang89 merged commit 1492c12 into janus-idp:main Jan 13, 2024
6 checks passed

jianrongzhang89 deleted the resource-update branch January 13, 2024 00:40

jianrongzhang89 temporarily deployed to internal January 13, 2024 00:40 — with GitHub Actions Inactive

rm3l mentioned this pull request Jan 15, 2024

Update downstream RHDH docs now that CR updates are supported #145

Merged

2 tasks

Update existing resources when CR changes #119

Update existing resources when CR changes #119

Conversation

jianrongzhang89 commented Jan 5, 2024 • edited by nickboldt Loading

Description

Which issue(s) does this PR fix or relate to

PR acceptance criteria

How to test changes / Special notes to the reviewer

github-actions bot commented Jan 5, 2024

github-actions bot commented Jan 5, 2024

rm3l left a comment

Choose a reason for hiding this comment

jianrongzhang89 commented Jan 5, 2024

github-actions bot commented Jan 5, 2024

github-actions bot commented Jan 5, 2024

github-actions bot commented Jan 8, 2024

github-actions bot commented Jan 8, 2024

github-actions bot commented Jan 8, 2024

github-actions bot commented Jan 9, 2024

gazarenkov 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

Choose a reason for hiding this comment

jianrongzhang89 Jan 10, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gazarenkov Jan 15, 2024 • edited Loading

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

jianrongzhang89 commented Jan 10, 2024

github-actions bot commented Jan 12, 2024

github-actions bot commented Jan 12, 2024

sonarqubecloud bot commented Jan 12, 2024

Quality Gate passed

github-actions bot commented Jan 12, 2024

rm3l left a comment

Choose a reason for hiding this comment

rm3l left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 13, 2024

jianrongzhang89 commented Jan 13, 2024

jianrongzhang89 commented Jan 5, 2024 •

edited by nickboldt

Loading

jianrongzhang89 Jan 10, 2024 •

edited

Loading

gazarenkov Jan 15, 2024 •

edited

Loading