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

Add watcher target to deploy operator via olm #8

Merged

Conversation

raukadah
Copy link
Contributor

@raukadah raukadah commented Nov 15, 2024

This pr adds two targets:
* make watcher: It generates olm.yaml file and installs the
watcher-operator using oc.
* make watcher_cleanup: It generates the olm.yaml file and undeploy
the watcher-operator using oc.

It also provides $CATALOG_IMAGE var to pass custom watcher-operator index image.

Jira: https://issues.redhat.com/browse/OSPRH-11419

@raukadah raukadah requested review from abays and SeanMooney and removed request for frenzyfriday November 15, 2024 13:00
@raukadah raukadah marked this pull request as draft November 18, 2024 05:57
@raukadah raukadah force-pushed the watcher_target branch 3 times, most recently from 18ce6d9 to db51d23 Compare November 18, 2024 08:44
@raukadah raukadah marked this pull request as ready for review November 18, 2024 08:45
@openshift-ci openshift-ci bot requested review from lewisdenny and stuggi November 18, 2024 08:45
Copy link
Contributor

@amoralej amoralej left a comment

Choose a reason for hiding this comment

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

Why not to stick tot the starndard way of doing things of other operators via install_yamls? I think, it'd be good at least for consistency with the rest.

Makefile Outdated
bash ci/olm.sh
oc apply -f ci/olm.yaml
$(eval csvname=$(shell oc get csv -n openstack-operators -o jsonpath='{range .items[*]}{@.metadata.name}{"\n"}{end}' | grep -E "^watcher-operator\.v"))
timeout 30 bash -c 'until [ "$(shell oc get -n openstack-operators csv/$(csvname) -o jsonpath='{.status.phase}')" == "Succeeded" ]; do sleep 5; done'
Copy link
Contributor

Choose a reason for hiding this comment

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

This is failing in my test:

$ make watcher error: arguments in resource/name form must have a single resource and name bash ci/olm.sh oc apply -f ci/olm.yaml namespace/openstack-operators created catalogsource.operators.coreos.com/watcher-operator-index created operatorgroup.operators.coreos.com/openstack created subscription.operators.coreos.com/watcher-operator created timeout 30 bash -c 'until [ "" == "Succeeded" ]; do sleep 5; done' make: *** [Makefile:383: watcher] Error 124

I'd say it's not related to timeout, i tested with a much longer one and get the same issue.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you @amoralej for testing it! I realized I also hit the same issue in multiple runs. I have fixed it now.

Copy link
Contributor

Choose a reason for hiding this comment

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

@raukadah I'm hitting the same problem, I think the problem is that by the time the first oc get csv runs, the csv does not yet exist, so it can't capture any name to use in the second command

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@cescgina Thank you for the review. Can you try now?

Copy link
Contributor

Choose a reason for hiding this comment

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

@raukadah thanks, the last version works perfectly!

@raukadah
Copy link
Contributor Author

raukadah commented Nov 18, 2024

Why not to stick tot the starndard way of doing things of other operators via install_yamls? I think, it'd be good at least for consistency with the rest.

@amoralej Thank you for review it. While implementing I went with both approach Install_yamls one targets and keeping target in watcher-operator repo.

The service operator target in install_yamls are not much used for development. For installing watcher-operator in install_yamls and VA based CI, we will be needing manifest files to deploy watcher operator. So, we wanted to keep the manifest file in watcher-operator and reuse the same at all places. That's why we have not went via install_yamls route.
If we want to go via install_yamls route to follow the same standard, I will update the pr.

In future, watcher-operator will be installed via openstack-operator. It might not needed.

@raukadah
Copy link
Contributor Author

Why not to stick tot the starndard way of doing things of other operators via install_yamls? I think, it'd be good at least for consistency with the rest.

@amoralej Thank you for review it. While implementing I went with both approach Install_yamls one targets and keeping target in watcher-operator repo.

The service operator target in install_yamls are not much used for development. For installing watcher-operator in install_yamls and VA based CI, we will be needing manifest files to deploy watcher operator. So, we wanted to keep the manifest file in watcher-operator and reuse the same at all places. That's why we have not went via install_yamls route. If we want to go via install_yamls route to follow the same standard, I will update the pr.

In future, watcher-operator will be installed via openstack-operator. It might not needed.

Based on the discussion on team call, we will be defining the make targets in install_yamls and then clone the install_yamls repo and then reuse it in watcher-operator repo. Thank you @amoralej :-)

@marios
Copy link

marios commented Nov 19, 2024

Why not to stick tot the starndard way of doing things of other operators via install_yamls? I think, it'd be good at least for consistency with the rest.

@amoralej Thank you for review it. While implementing I went with both approach Install_yamls one targets and keeping target in watcher-operator repo.
The service operator target in install_yamls are not much used for development. For installing watcher-operator in install_yamls and VA based CI, we will be needing manifest files to deploy watcher operator. So, we wanted to keep the manifest file in watcher-operator and reuse the same at all places. That's why we have not went via install_yamls route. If we want to go via install_yamls route to follow the same standard, I will update the pr.
In future, watcher-operator will be installed via openstack-operator. It might not needed.

Based on the discussion on team call, we will be defining the make targets in install_yamls and then clone the install_yamls repo and then reuse it in watcher-operator repo. Thank you @amoralej :-)

My understanding was we would have the make targets defined in this repo (as you have done here) and then we can also add targets in install_yamls which would call these ones (if we need/wanted to have them in install_yamls too for consistency or if anyone else wanted to install this).

Is that right? ^ I am a bit confused by Chkumar last comment that we clone install_yamls and re-use it here for the installation (which implies the targets are defined in install_yamls)

@amoralej
Copy link
Contributor

Why not to stick tot the starndard way of doing things of other operators via install_yamls? I think, it'd be good at least for consistency with the rest.

@amoralej Thank you for review it. While implementing I went with both approach Install_yamls one targets and keeping target in watcher-operator repo.
The service operator target in install_yamls are not much used for development. For installing watcher-operator in install_yamls and VA based CI, we will be needing manifest files to deploy watcher operator. So, we wanted to keep the manifest file in watcher-operator and reuse the same at all places. That's why we have not went via install_yamls route. If we want to go via install_yamls route to follow the same standard, I will update the pr.
In future, watcher-operator will be installed via openstack-operator. It might not needed.

Based on the discussion on team call, we will be defining the make targets in install_yamls and then clone the install_yamls repo and then reuse it in watcher-operator repo. Thank you @amoralej :-)

My understanding was we would have the make targets defined in this repo (as you have done here) and then we can also add targets in install_yamls which would call these ones (if we need/wanted to have them in install_yamls too for consistency or if anyone else wanted to install this).

Is that right? ^ I am a bit confused by Chkumar last comment that we clone install_yamls and re-use it here for the installation (which implies the targets are defined in install_yamls)

That was also my understanding of the conversation.

From my side, we can move on with this PR.

This pr adds two targets:
* `make watcher`: It generates olm.yaml file and installs the
  watcher-operator using oc.
* `make watcher_cleanup`: It generates the olm.yaml file and undeploy
  the watcher-operator using oc.
* `make watcher_deploy`: To deploy the watcher service
* `make watcher_deploy_cleanup`: To undeploy the watcher service

It also provides $CATALOG_IMAGE var to pass custom watcher-operator
index image.

Jira: https://issues.redhat.com/browse/OSPRH-11419

Signed-off-by: Chandan Kumar <[email protected]>
@raukadah
Copy link
Contributor Author

raukadah commented Nov 19, 2024

Why not to stick tot the starndard way of doing things of other operators via install_yamls? I think, it'd be good at least for consistency with the rest.

@amoralej Thank you for review it. While implementing I went with both approach Install_yamls one targets and keeping target in watcher-operator repo.
The service operator target in install_yamls are not much used for development. For installing watcher-operator in install_yamls and VA based CI, we will be needing manifest files to deploy watcher operator. So, we wanted to keep the manifest file in watcher-operator and reuse the same at all places. That's why we have not went via install_yamls route. If we want to go via install_yamls route to follow the same standard, I will update the pr.
In future, watcher-operator will be installed via openstack-operator. It might not needed.

Based on the discussion on team call, we will be defining the make targets in install_yamls and then clone the install_yamls repo and then reuse it in watcher-operator repo. Thank you @amoralej :-)

My understanding was we would have the make targets defined in this repo (as you have done here) and then we can also add targets in install_yamls which would call these ones (if we need/wanted to have them in install_yamls too for consistency or if anyone else wanted to install this).
Is that right? ^ I am a bit confused by Chkumar last comment that we clone install_yamls and re-use it here for the installation (which implies the targets are defined in install_yamls)

That was also my understanding of the conversation.

From my side, we can move on with this PR.

Thank you @amoralej @marios for correcting it. We will define the target here in watcher-operator and reuse it in install_yamls. But I still think adding target in install_yamls (calling watcher-operator target) will confuse the user. I think we can keep the watcher-operator target only.

@cescgina
Copy link
Contributor

/lgtm

raukadah added a commit to raukadah/watcher-operator that referenced this pull request Nov 19, 2024
raukadah added a commit to raukadah/watcher-operator that referenced this pull request Nov 19, 2024
Copy link

openshift-ci bot commented Nov 19, 2024

[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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-merge-bot openshift-merge-bot bot merged commit 29c4598 into openstack-k8s-operators:main Nov 19, 2024
5 checks passed
@raukadah raukadah deleted the watcher_target branch November 19, 2024 15:16
raukadah added a commit to raukadah/watcher-operator that referenced this pull request Nov 19, 2024
raukadah added a commit to raukadah/watcher-operator that referenced this pull request Nov 20, 2024
raukadah added a commit to raukadah/watcher-operator that referenced this pull request Nov 25, 2024
This pr:
- Adds watcher-operator-base job from
  podified-multinode-edpm-deployment-crc-2comp parent. This job will
  deploy 2 node EDPM deployment and then deploy watcher operator using
  make targets from watcher-operator repo.
- It adds hook to deploy watcher service via ci-framework hook.

Depends-On: openstack-k8s-operators#8

Signed-off-by: Chandan Kumar <[email protected]>
raukadah added a commit to raukadah/watcher-operator that referenced this pull request Nov 26, 2024
This pr:
- Adds watcher-operator-base job from
  podified-multinode-edpm-deployment-crc-2comp parent. This job will
  deploy 2 node EDPM deployment and then deploy watcher operator using
  make targets from watcher-operator repo.
- It adds hook to deploy watcher service via ci-framework hook.

Depends-On: openstack-k8s-operators#8

Signed-off-by: Chandan Kumar <[email protected]>
openshift-merge-bot bot pushed a commit that referenced this pull request Nov 26, 2024
This pr:
- Adds watcher-operator-base job from
  podified-multinode-edpm-deployment-crc-2comp parent. This job will
  deploy 2 node EDPM deployment and then deploy watcher operator using
  make targets from watcher-operator repo.
- It adds hook to deploy watcher service via ci-framework hook.

Depends-On: #8

Signed-off-by: Chandan Kumar <[email protected]>
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.

4 participants