Refactor operator logic #70
Conversation
# Conflicts: # .gitignore # controllers/backstage_controller.go # controllers/backstage_controller_test.go # controllers/backstage_deployment.go # controllers/backstage_service.go # controllers/local_db_deployment.go # controllers/local_db_storage.go
# Conflicts: # api/v1alpha1/backstage_types.go # config/crd/bases/janus-idp.io_backstages.yaml # controllers/backstage_backend_auth.go # controllers/backstage_controller.go # controllers/backstage_controller_test.go # controllers/local_db_statefulset.go
|
@gazarenkov Now I got an error when deploying examples/bs1.yaml. Can you take a look? |
|
@gazarenkov I found that only one service is deployed for the local db. The db statefulset requires a headless service (with serviceName in the statefulset spec) but the headless service is missing. Can you double check? |
controllers/backstage_controller.go
Outdated
| if err != nil { | ||
| return fmt.Errorf("failed to read YAML: %w", err) | ||
|
|
||
| if err := r.Update(ctx, obj.Object()); err != nil { |
There was a problem hiding this comment.
Simply calling update() will cause unexpected result because the existing k8s object's meta data information (such as labels, annotations) may get lost. Administrators often may want to add additional labels or annotations in order to manage the cluster resources.
You should consider to use the CreateOrUpdate() library call used currently, or you may have to implement the logic to merge the configuration before calling update.
There was a problem hiding this comment.
As discussed, if we want Backstage owning the objects (which is the case) the only way to reshape runtime objects is to change default/raw/spec configuration, that is the desired state
There was a problem hiding this comment.
Yes, we do want to have runtime objects to be based on our configuration, however we should not prevent admins to add the labels/annotations to them, and this is often done as automation scripts. k8s may inject additional information to the metadata. This will be a serious limitation which will not get accepted by the field.
If you consider it important please make sure it is added in test suite, it is not that obvious w/o ability to test it. |
Do you mean this failed for you? |
Got the same issue after applying |
| if err := r.reconcileBackstageService(ctx, &backstage, req.Namespace); err != nil { | ||
| return ctrl.Result{}, err | ||
| err = r.applyObjects(ctx, bsModel.Objects) | ||
| if err != nil { |
There was a problem hiding this comment.
Errors of conflicting objects are temporary. They will be cleared in next reconciliations and should not cause the CR status to get updated.
There was a problem hiding this comment.
Not sure I understand...We can hope it to be recovered, but status is about current state, which is not desired one I believe (we want it to be deleted but it is not).
There was a problem hiding this comment.
This kind of conflicting errors are vey common in operators and all they need to do is to retry the reconciliation. They are typically self-recoverable so they should not cause un-necessary status updates.
Example on this is handled currently in main branch:
https://github.com/janus-idp/operator/blob/main/controllers/backstage_deployment.go#L155
|
Without such service, it will be confusing for users - the serviceName is set in the statefulset, but the actual service is missing. This is already in the test cases in main, see https://github.com/janus-idp/operator/blob/main/controllers/backstage_controller_test.go#L396. Please make sure you do not delete existing test cases during refactoring. Otherwise makes us difficult to validate the changes. IMO, refactoring should not even modify existing test cases so that we know none of existing test cases get broken. |
|
PR needs rebase. 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. |
rm3l
added
the
do-not-merge/hold
gazarenkov
removed
needs-rebase
do-not-merge/hold
As stated in #69 with more comments here
the goal of this PR is to improve design, supportability and testablity of Operator.
For this purpose all the configuration complexity , forming Desired State of Custom Resource (see 3 layers of configuration) was moved out of Controller to the Model.
Description
Configuration logic moved out from controller package to the model package, tests added accordingly.
Which issue(s) does this PR fix or relate to
#69
PR acceptance criteria
Integration and (new) unit tests passed.