Create basic Reconcile methods for WatcherAPI #13
Conversation
cescgina
force-pushed
the
watcherapi_controller
branch
from
d886297 to
a729c7f
Compare
|
thanks for the review @amoralej that was very helpful. I think I addressed all of your comments but the kuttl tests, I'll add those soon |
cescgina
force-pushed
the
watcherapi_controller
branch
from
a729c7f to
d4a7511
Compare
cescgina
force-pushed
the
watcherapi_controller
branch
2 times, most recently
from
0fe6435 to
7436585
Compare
|
According to the CI job logs, I'd say we are missing acl permissions to read secrets. See: |
cescgina
force-pushed
the
watcherapi_controller
branch
4 times, most recently
from
4f85a57 to
634bfe8
Compare
cescgina
force-pushed
the
watcherapi_controller
branch
from
634bfe8 to
0ef8b41
Compare
cescgina
force-pushed
the
watcherapi_controller
branch
2 times, most recently
from
e3e79da to
710986b
Compare
cescgina
force-pushed
the
watcherapi_controller
branch
3 times, most recently
from
5e1c48c to
87371f7
Compare
There was a problem hiding this comment.
would be good to get some overview in the coming team calls to allow more folks to follow along beyond the core group creating the operators
@amoralej so initially I think mkopec was going to help but it seems it is now you and Joan. Is there scope for me to pickup something here or is everything already in progress
|
/lgtm Thank you @cescgina for adding extensive test coverage, It looks good to go!. |
|
lgtm too but I'm not sure if Sean wanted to review it after last changes before merge. |
cescgina
force-pushed
the
watcherapi_controller
branch
from
87371f7 to
d75f613
Compare
| }, timeout, interval).Should(ContainElement("openstack.org/watcherapi")) | ||
| }) | ||
| }) | ||
| When("the secret is created with all the expected fields", func() { |
There was a problem hiding this comment.
if we are adding the input ready condition we should also be testing the negitve case.
currently you are only testing the positive case.
we should be testing creating a cr where the secret does not exist and assert that we get the approate input-ready false condition with a suitable error
that will pass the defaulting and validation web hook and need to be checked in the reconcile loop.
the same is true for the database instance or any other CR that is passed by name.
| // WatcherAPISpec defines the desired state of WatcherAPI | ||
| type WatcherAPISpec struct { | ||
| // INSERT ADDITIONAL SPEC FIELDS - desired state of cluster | ||
| // Important: Run "make" to regenerate code after modifying this file | ||
|
|
||
| // Foo is an example field of WatcherAPI. Edit watcherapi_types.go to remove/update | ||
| Foo string `json:"foo,omitempty"` | ||
| WatcherCommon `json:",inline"` |
There was a problem hiding this comment.
so the openstack version is now used to set the conteinr to use.
since we do not have integration with that until we integrate in the
meta operator we need to have the contianr URL here with 3 levels of precedence
the lowest level is a default in code to the quay.io container
the next level of precedence will be an environment variable set in the bundle and the final level of percendce is the container URL.
when we integrate with the OpenStack version CRD the contienr URL in the CRD will be removed and we will use the OpenStack version as the highest level of precedence.
I'm not sure is we want to defer adding that to when we actually start creating the containers.
we probably can do that.
the other option is to just not add the URL at the CRD level and instead just have the first 2 levels of precedence
the bundle level ENV var and the hard coded quay.io default.
i think I'm leaning in that direction to avoid the CRD churn
what do others think?
There was a problem hiding this comment.
so, first of all, I believe you are commenting on the lack of
ContainerImage string `json:"containerImage"`
in this proposal right? i.e. the fact that we don't carry it here (because this is now) set by OpenStackVersion)
I think your proposal about avoiding CRD churn is reasonable, but not sure if it would be 'unexpected' for anyone that would go looking for the containerImage on the crd (though as you said, eventually we'd want to remove that one too?)
There was a problem hiding this comment.
I'd vote for having the three proposed levels, including the URL in the CRD. I may be missing something but IIUC, in a particular version of operator we may want to deploy different container URLs (for CI testing, or even out of it and having the URL in the watcher CRDs is the easiest way. Also, I think that's what others operators do?
In any case, I'd defer setting the container image parameter until we actually use it. We can discuss it there.
There was a problem hiding this comment.
I purposely left this parameter option out until it could be used (when we implement the deployment) in hopes of having PRs as small as possible. @amoralej has a good point about CI, we might want to use a different container image than the one in the bundle
There was a problem hiding this comment.
so some context
all the lower level crd used to have a required
ContainerImage string json:"containerImage"
filed that was set via the upper layers
but when we created the OpenStack version CRD we moved the URL form the individual CRDs to that common one.
since we don't have that integration for watcher we have 2 choices.
either we only implement the other two levels of precedence
hardcoded fallback to quey.io:latest when an environment variable is not set (vai the bundle)
or we can temporary add ContainerImage string json:"containerImage"
I'm leang toward defering this until we add support for the container (stateful set) creation
we can choose if we want to only support the first 2 levels fo precendce and defer the configibale URL until we have integration with the OpenStack Version CRD
or we can add ContainerImage string json:"containerImage" as an interim solution.
| // Important: Run "make" to regenerate code after modifying this file | ||
|
|
||
| // WatcherCommon defines a spec based reusable for all the CRDs | ||
| type WatcherCommon struct { | ||
| // +kubebuilder:validation:Optional | ||
| // +kubebuilder:default=osp-secret |
There was a problem hiding this comment.
so we do have an issue here.
the secreate field can have a default at the top level
however at the watcherApi level it should be a required filed without a default.
the watcher controller should be creating new secrets for the watcherAPI controller to consume. initially it can just pass the fully secret but we cant share the same definition of the secret field between the top level CRD and the rest.
There was a problem hiding this comment.
I think we should leave this to fix in a follow-up. I was looking at barbican-operator and they seem to use the same approach taken here, the BarbicanAPISpec https://github.com/openstack-k8s-operators/barbican-operator/blob/eaecb1b3e25ad17676b1b10f9c4a3d853effb21a/api/v1beta1/barbicanapi_types.go#L62 contains a BarbicanTemplate which defines the secret shared by all components. What is the problem with this approach?
There was a problem hiding this comment.
its basicaly insecure, the watcher operator shoudl not have access to the barbican secrets :)
openstack-k8s-operators/nova-operator#419
which as part fo https://issues.redhat.com/browse/OSPRH-98 and https://issues.redhat.com/browse/OSPRH-200
There was a problem hiding this comment.
ok, that makes a lot of sense, thanks!
| // If we're not deleting this and the service object doesn't have our finalizer, add it. | ||
| if instance.DeletionTimestamp.IsZero() && controllerutil.AddFinalizer(instance, helper.GetFinalizer()) || isNewInstance { | ||
| return ctrl.Result{}, nil | ||
| } |
There was a problem hiding this comment.
i was going to say this feels like a bug but now
its correct, there is a very explicit reason we have a finaliser on our selves
when the watcherAPI controller reconsoles a CR and create other CRs like stateful sets and configmaps for that API instance we add a finalser to those.
resource or to the keystone resouses as is the case in the nova example.
we place a finalise on the CR were are reconciling to have an opportunity to clean up any resources during delete without the CR deletion completing until we are done.
There was a problem hiding this comment.
thanks, your comment prompted me to go read about these https://kubernetes.io/docs/concepts/overview/working-with-objects/finalizers/
There was a problem hiding this comment.
so the assert for step one assert that a watcher kind is created
as well ad the db and osp secret.
the default watcher kind should deploy a default watcher-api cr too.
although i assume that is not implemented yet which is why you are not asserting that?
There was a problem hiding this comment.
exactly, I'm testing now the current behaviour (need to deploy WatcherAPI explicitely) and will change the tests once the Watcher controller creates the WatcherAPI
There was a problem hiding this comment.
Yes. So far, we are testing independently. Once we integrate WatcherApi (and the rest) into Watcher, we will also integrate the asserts for 2nd level CRDs into Watcher creation
There was a problem hiding this comment.
once we can create the watcherAPI via the watcher cer we should refactor this to do that.
for now this is fine.
| databaseInstance: openstack | ||
| passwordSelectors: | ||
| service: WatcherPassword | ||
| secret: osp-secret |
There was a problem hiding this comment.
this is testing the defaulting behavior.
+1
| metadata: | ||
| finalizers: | ||
| - openstack.org/watcherapi | ||
| name: watcherapi-kuttl |
There was a problem hiding this comment.
this is testign the self finaliser behvior.
if we replace this with creating the watcher API by using the top-level watcher cr we will also need to add the watcher controller finaliser here in the future.
There was a problem hiding this comment.
by the way https://github.com/openstack-k8s-operators/watcher-operator/blob/main/tests/kuttl/test-suites/default/03-assert.yaml is currently in the wrong folder
we should fix that although that is is not related to this pr its an existing bug.
it should be in https://github.com/openstack-k8s-operators/watcher-operator/tree/main/tests/kuttl/test-suites/default/test
There was a problem hiding this comment.
That was my fault in a previous PS. Although, we may fix it in a specific commit in this PR to have proper CI coverage on it.
cescgina
force-pushed
the
watcherapi_controller
branch
from
d75f613 to
ead5fae
Compare
|
@SeanMooney I made a couple of small changes here (fix the kuttl file and getting the db before marking input ready), but will be addressing the rest in a follow-up PR |
cescgina
force-pushed
the
watcherapi_controller
branch
2 times, most recently
from
016f04c to
94feadf
Compare
This patch is basic recond methods to the watcherapi controller, as well as some initial input validation. The change adds the required CR spec and status, and creates the initial structure for initialization and deletion. In addition it some some input validation by accessing the osp secret and the database that should be created by the watcher controller. Finally, it is also adding some initial structure test for functional envtest testing in WatcherAPI. Related: OSPRH-11483
cescgina
force-pushed
the
watcherapi_controller
branch
from
94feadf to
0ae9838
Compare
| db, err := mariadbv1.GetDatabaseByNameAndAccount(ctx, helper, watcher.DatabaseCRName, instance.Spec.DatabaseAccount, instance.Namespace) | ||
| if err != nil { | ||
| instance.Status.Conditions.Set(condition.FalseCondition( | ||
| condition.ServiceConfigReadyCondition, |
There was a problem hiding this comment.
nit: this shoudl really be InputReadyCondition not ServiceConfigReadyCondition
There was a problem hiding this comment.
indeed, I forgot to update this when I moved the code, I'll correct it on my follow-up PR, thanks!
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: amoralej 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 |
openshift-merge-bot
bot
merged commit ae12ade
into
openstack-k8s-operators:main
This patch is basic recond methods to the watcherapi
controller, as well as some initial input validation.
The change adds the required CR spec and status, and creates the initial
structure for initialization and deletion. In addition it some some
input validation by accessing the osp secret and the database that
should be created by the watcher controller.
Finally, it is also adding some initial structure test for functional envtest
testing in WatcherAPI.
Related: OSPRH-11483