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.

Sign up for GitHub

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

Jump to bottom

Integrate set_openstack_containers in validated architecture playbook #2321

Merged
merged 1 commit into from
Oct 22, 2024
Merged

Integrate set_openstack_containers in validated architecture playbook #2321

merged 1 commit into from
Oct 22, 2024

Conversation

ciecierski
Copy link
Contributor

set_openstack_containers role is used to update openstack container images using openstack version cr in update jobs. Let's use the same to update the containers in va.

It will be used in downstream VA jobs to use RHEL openstack services container.

@github-actions github-actions bot marked this pull request as draft September 5, 2024 11:22
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 5, 2024

Thanks for the PR! ❤️
I'm marking it as a draft, once your happy with it merging and the PR is passing CI, click the "Ready for review" button below.

@ciecierski ciecierski force-pushed the soc_va_integration branch 2 times, most recently from ff609fe to 31dda41 Compare September 12, 2024 20:09
@ciecierski ciecierski added the minor update Changes for minor update automation label Sep 13, 2024
@softwarefactory-project-zuul softwarefactory-project-zuul
Copy link

Build failed (check pipeline). Post recheck (without leading slash)
to rerun all jobs. Make sure the failure cause has been resolved before
you rerun jobs.

https://softwarefactory-project.io/zuul/t/rdoproject.org/buildset/22d4d4ce35104af69ea43d1dc6731d88

✔️ openstack-k8s-operators-content-provider SUCCESS in 2h 09m 36s
✔️ podified-multinode-edpm-deployment-crc SUCCESS in 1h 14m 59s
✔️ cifmw-crc-podified-edpm-baremetal SUCCESS in 1h 25m 24s
✔️ noop SUCCESS in 0s
✔️ cifmw-pod-ansible-test SUCCESS in 7m 45s
✔️ cifmw-pod-pre-commit SUCCESS in 7m 14s
❌ build-push-container-cifmw-client TIMED_OUT in 1h 30m 42s
✔️ cifmw-molecule-reproducer SUCCESS in 15m 18s

@ciecierski ciecierski marked this pull request as ready for review October 9, 2024 08:04
@ciecierski ciecierski requested review from sdatko and sathlan and removed request for sdatko October 11, 2024 09:39
sathlan
sathlan approved these changes Oct 14, 2024
View reviewed changes
Copy link
Contributor

@sathlan sathlan left a comment

Choose a reason for hiding this comment

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

Nice.

playbooks/06-deploy-architecture.yml Show resolved Hide resolved
@ciecierski ciecierski requested review from sdatko and arxcruz and removed request for sdatko October 16, 2024 09:36
pablintino
pablintino requested changes Oct 16, 2024
View reviewed changes
playbooks/06-deploy-architecture.yml Outdated
@@ -197,6 +197,15 @@
- update_containers
- edpm_bootstrap

- name: Update containers in deployed OSP operators using set_openstack_containers role
vars:
cifmw_set_openstack_containers_extra_vars: "{{ cifmw_edpm_prepare_extra_vars }}"
Copy link
Collaborator

Choose a reason for hiding this comment

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

edpm_prepare vars are not used in VA/DT deployments

playbooks/06-deploy-architecture.yml
ansible.builtin.include_role:
name: set_openstack_containers
tags:
- set_openstack_containers
Copy link
Collaborator

Choose a reason for hiding this comment

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

A few comments:

  • You assume we call deploy-edpm only using the reproducer, that with this change, will inject the tag in the skip list
  • You assume that we want the role to be executed by default, that's the meaning of adding a tag here without a when condition.
  • This skip strategy is not aligned with the rest of "skip flags" we have all over the framework. Please, use a flag like cifmw_update_containers with a sane false default.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ciecierski sorry mate, I may miss-expressed what I wanted. cifmw_update_containers was just an example of the kind of flag we usually do, for this new task create a new one called: cifmw_set_openstack_containers that defaults to false. Yes, false, as the regular behavior we had (and we still want) is to preserve what's deployed. If someone wants to patch the images all he/she needs is to make the flag true.

Copy link
Collaborator

Choose a reason for hiding this comment

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

PS: We doc those vars here, so add the new one in that file too.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pablintino I added cifmw_update_containers to not create more vars, but then I realized that there are deployments jobs which have cifmw_update_containers set to false. Those deployment jobs are using containers from compose. I'm going to add cifmw_set_openstack_containers for set_openstack_containers role include.
I'm not sure if I should remove cifmw_update_containers from update_containers include. update_containers role default value for cifmw_update_containers is false, but it used only for applying generated template: https://github.com/openstack-k8s-operators/ci-framework/blob/main/roles/update_containers/tasks/main.yml#L30
Do you think we want to generate template always? Should I remove when: cifmw_update_containers or it can stay?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@ciecierski I thought about telling you to do so, but I'd prefer to keep your change as scoped to your issue as posible, so no worries, we will do it during the cleanup of the framework.
Care to add the new var to this doc? https://github.com/openstack-k8s-operators/ci-framework/blob/main/docs/source/usage/01_usage.md

pablintino
pablintino requested changes Oct 16, 2024
View reviewed changes
playbooks/06-deploy-architecture.yml
ansible.builtin.include_role:
name: set_openstack_containers
tags:
- set_openstack_containers
Copy link
Collaborator

Choose a reason for hiding this comment

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

@ciecierski sorry mate, I may miss-expressed what I wanted. cifmw_update_containers was just an example of the kind of flag we usually do, for this new task create a new one called: cifmw_set_openstack_containers that defaults to false. Yes, false, as the regular behavior we had (and we still want) is to preserve what's deployed. If someone wants to patch the images all he/she needs is to make the flag true.

@lewisdenny lewisdenny self-requested a review October 17, 2024 01:55
@pablintino
Copy link
Collaborator

/approve

@openshift-ci OpenShift CI
Copy link
Contributor

openshift-ci bot commented Oct 17, 2024

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: pablintino

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

lewisdenny
lewisdenny requested changes Oct 21, 2024
View reviewed changes
Copy link
Collaborator

@lewisdenny lewisdenny left a comment

Choose a reason for hiding this comment

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

lgtm other than a small nit, thanks!

playbooks/06-deploy-architecture.yml Outdated Show resolved Hide resolved
@ciecierski
Integrate set_openstack_containers into deploy_architecture playbok
b0d6227 
set_openstack_containers role is used to update openstack container images
using openstack version cr in crc update jobs. Let's use the same to update
the containers in architecture jobs. It will be used in downstream VA architecture
jobs to use RHEL openstack services container.

set_openstack_containers role in update job is activate using cifmw_set_openstack_containers
var set to true. By default the role is not activate.
lewisdenny
lewisdenny approved these changes Oct 21, 2024
View reviewed changes
Copy link
Collaborator

@lewisdenny lewisdenny left a comment

Choose a reason for hiding this comment

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

/lgtm

@openshift-ci openshift-ci bot added the lgtm label Oct 21, 2024
@openshift-merge-bot openshift-merge-bot bot merged commit 599aab0 into openstack-k8s-operators:main Oct 22, 2024
5 checks passed
@ciecierski
Copy link
Contributor Author

/cherry-pick 18.0-fr1

@openshift-cherrypick-robot
Copy link

@ciecierski: new pull request created: #2480

In response to this:

/cherry-pick 18.0-fr1

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-sigs/prow repository.

@openshift-cherrypick-robot openshift-cherrypick-robot mentioned this pull request Oct 22, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdatko sdatko sdatko left review comments

@pablintino pablintino pablintino approved these changes

@lewisdenny lewisdenny lewisdenny approved these changes

@sathlan sathlan sathlan approved these changes

@arxcruz arxcruz Awaiting requested review from arxcruz

Assignees

@lewisdenny lewisdenny

Labels
approved lgtm minor update Changes for minor update automation Ready For Review
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@ciecierski @pablintino @openshift-cherrypick-robot @sathlan @lewisdenny @sdatko