Add Heat container image definitions - #76
Conversation
yazug
left a comment
There was a problem hiding this comment.
couple of nits but looks good to me nothing blocking it from my point of view.
rabi
left a comment
There was a problem hiding this comment.
Few questions as well,
Consolidating to one image means the three RELATED_IMAGE_HEAT_* references all have to point at the same pullspec, and needs changes in heat-operator/openstack-operator, right?
Since all three services would then share one image digest, any rebuild of that image would roll all three (api, api-cfn, engine) together, even for a fix that only affects one role (e.g. an httpd CVE bouncing number of heat-engine pods). Has this been taken into consideration?
|
Thanks for consolidating into a single one, that's great. New container name is openstack-heat which didn't exist in tcib. @yazug is that a problem? I think you mentioned to reuse an existing name when possible. |
Yes, the three RELATED_IMAGE_HEAT... should point to the same container image so the three values in OpenStackVersion would point to the same image. I don't know the details of heat-operator but i wouldn't expect this to require any change on it (we are doing the same in watcher-operator). About changes in openstack-operator to get default values, I'd say just setting the default values (https://github.com/search?q=repo%3Aopenstack-k8s-operators%2Fopenstack-operator+RELATED_IMAGE_HEAT_&type=code) . Given that this is different than tcib, we will need some mapping for transition for the mapping of container names to OpenStackVersion fields for CI which is what we just starting adding in https://github.com/openstack-k8s-operators/s2i-openstack-containers/blob/main/containers/image-mappings.yaml I may be missing something but from the impact in operators itself, i think consolidation should be easy to manage. @stuggi , any opinion on this?
Yes. I think it has been considered but it's probably good to wide the discussion and make sure all impact is considered. @yazug may give the opinion of reldel on that and give additional details about how the actual process works. Some considerations to keep in mind.
From the s2i-openstack-operators itself, reducing the number of images will help managing the content from different aspects so it's a fair goal but you are right that there may be other considerations. Do you think it'd be good to have some wider discussion and / or document these considerations somewhere? |
The additional surface area is httpd, mod_ssl, python3-mod_wsgi, since those are now included in the image that's used by heat-engine. If those deps do need to get updated frequently, we're talking about a lot of container restarts anyway across all the APIs. Adding in heat-engine to that list doesn't seem like it would make much difference. heat-engine restarts also shouldn't cause any disruption to any user stacks that might be in progress. That seems worth it for managing 1 less container image. |
Confirmed by @yazug we need the name for the new common container to be an existing one in tcib, heat-api, heat-base, heat-api-cfn or heat-engine. |
I don't see any reason we continue including those in heat-engine.
Well, heat-engine waits for in-progess stacks to complete or timeout before killing the engines. I don't think it's correct to assume that restarting heat-engines unnecessarily won't be an overhead.
I don't think we should name the common image any of the above as it would be misleading/confusing. I would prefer we split them into 2 images heat-api (includes heat-api-cfn) and heat-engine. |
Are we planning to keep the three RELATED_IMAGE_HEAT_* env vars (one per service) now that all three run the same image? If so, we've to guarantee they stay pinned to the same digest and don't diverge over time. Should we collapse to a single reference instead? |
That's TBD at this point. I thin keeping RELATED_IMAGE_HEAT_* on sync is doable, but we may also have a single env variable and consume the same to populate defaults for multiple openstackversion fields. We just need to manage the transition so that it's tied to moving all the CI jobs to new s2i master ones and make sure it's properly managed in the e2e process. |
| # Install wheels temporarily so entry points are available for config generation | ||
| RUN pip3 install --no-cache-dir --prefix=/usr \ | ||
| -c /tmp/build-constraints.txt \ | ||
| --find-links=/wheels/deps \ | ||
| --find-links=/wheels/pkgs \ | ||
| /wheels/pkgs/*.whl |
There was a problem hiding this comment.
You don't run the config generator, so this can also be removed, since the installation is only being done here for config generator.
| # Install wheels temporarily so entry points are available for config generation | |
| RUN pip3 install --no-cache-dir --prefix=/usr \ | |
| -c /tmp/build-constraints.txt \ | |
| --find-links=/wheels/deps \ | |
| --find-links=/wheels/pkgs \ | |
| /wheels/pkgs/*.whl |
| COPY --from=build /configfiles/etc/heat/heat.conf /etc/heat/heat.conf | ||
| COPY --from=build /configfiles/etc/heat/api-paste.ini /etc/heat/api-paste.ini | ||
| COPY --from=build /configfiles/etc/heat/environment.d/default.yaml /etc/heat/environment.d/default.yaml | ||
| COPY --from=build /configfiles/etc/heat/templates/AWS_CloudWatch_Alarm.yaml /etc/heat/templates/AWS_CloudWatch_Alarm.yaml | ||
| COPY --from=build /configfiles/etc/heat/templates/AWS_RDS_DBInstance.yaml /etc/heat/templates/AWS_RDS_DBInstance.yaml |
There was a problem hiding this comment.
The configfiles dir used to be populated by the config generator, which is not being run.
Remove these lines
| COPY --from=build /configfiles/etc/heat/heat.conf /etc/heat/heat.conf | |
| COPY --from=build /configfiles/etc/heat/api-paste.ini /etc/heat/api-paste.ini | |
| COPY --from=build /configfiles/etc/heat/environment.d/default.yaml /etc/heat/environment.d/default.yaml | |
| COPY --from=build /configfiles/etc/heat/templates/AWS_CloudWatch_Alarm.yaml /etc/heat/templates/AWS_CloudWatch_Alarm.yaml | |
| COPY --from=build /configfiles/etc/heat/templates/AWS_RDS_DBInstance.yaml /etc/heat/templates/AWS_RDS_DBInstance.yaml |
There was a problem hiding this comment.
Some of these files (all but heat.conf I think) are not generated via config generator but copied from the upstream repo but it was removed together with the config generator one i think. See:
Those lines need to be restored.
| RUN chmod 640 /etc/heat/heat.conf /etc/heat/api-paste.ini && \ | ||
| chown -R heat:heat /etc/heat /usr/share/heat |
There was a problem hiding this comment.
| RUN chmod 640 /etc/heat/heat.conf /etc/heat/api-paste.ini && \ | |
| chown -R heat:heat /etc/heat /usr/share/heat | |
| RUN chown -R heat:heat /etc/heat /usr/share/heat |
There was a problem hiding this comment.
We may need to keep the chmod 640 for the config files other than heat.conf.
There was a problem hiding this comment.
Agreed. Unfortunately, I cannot edit my suggestion at this time; github is having some technical issues
There was a problem hiding this comment.
diff --git containers/heat/heat/Containerfile containers/heat/heat/Containerfile
index 58ce632..11d90d4 100644
--- containers/heat/heat/Containerfile
+++ containers/heat/heat/Containerfile
@@ -57,12 +57,10 @@ RUN for src_dir in /src/*/ /src/overrides/*/; do \
fi; \
done > /source-built-packages.txt
-# Install wheels temporarily so entry points are available for config generation
-RUN pip3 install --no-cache-dir --prefix=/usr \
- -c /tmp/build-constraints.txt \
- --find-links=/wheels/deps \
- --find-links=/wheels/pkgs \
- /wheels/pkgs/*.whl
+RUN cp -a ${SERVICE_SRC}/etc/heat/api-paste.ini /configfiles/etc/heat/ && \
+ cp -a ${SERVICE_SRC}/etc/heat/environment.d/default.yaml /configfiles/etc/heat/environment.d/ && \
+ cp -a ${SERVICE_SRC}/etc/heat/templates/AWS_CloudWatch_Alarm.yaml /configfiles/etc/heat/templates/ && \
+ cp -a ${SERVICE_SRC}/etc/heat/templates/AWS_RDS_DBInstance.yaml /configfiles/etc/heat/templates/
# --- Runtime stage ---
FROM ${BASE_IMAGE}
@@ -93,7 +91,6 @@ RUN mkdir -p /etc/heat /var/log/heat /var/lib/heat /usr/share/heat && \
chown -R heat:heat /etc/heat /var/log/heat /var/lib/heat /usr/share/heat
# Install config files — each file listed explicitly
-COPY --from=build /configfiles/etc/heat/heat.conf /etc/heat/heat.conf
COPY --from=build /configfiles/etc/heat/api-paste.ini /etc/heat/api-paste.ini
COPY --from=build /configfiles/etc/heat/environment.d/default.yaml /etc/heat/environment.d/default.yaml
COPY --from=build /configfiles/etc/heat/templates/AWS_CloudWatch_Alarm.yaml /etc/heat/templates/AWS_CloudWatch_Alarm.yaml
@@ -101,7 +98,7 @@ COPY --from=build /configfiles/etc/heat/templates/AWS_RDS_DBInstance.yaml /etc/h
COPY common/config/usr/share/heat/heat-dist.conf /usr/share/heat/heat-dist.conf
# Set config file permissions
-RUN chmod 640 /etc/heat/heat.conf /etc/heat/api-paste.ini && \
+RUN chmod 640 /etc/heat/api-paste.ini && \
chown -R heat:heat /etc/heat /usr/share/heat
# Apache httpd setup for WSGI
And you also need to create the /configfiles/etc/heat/environment.d and /configfiles/etc/heat/templates firs in the build layers
You can probably move these COPY lines into the runtime stage instead. I don't think they need to be moved between layers
There was a problem hiding this comment.
These COPY commands are already in the runtime stage. Or do you mean the initial cp commands for these files, just do that in runtime?
There was a problem hiding this comment.
I experienced a dumb moment about these, and confused two sets of config files: the ones from the openstack repo and the ones from the s2i repo.
Please ignore the suggestion to remove the COPY commands from the build stage
f77aa64 to
77899b5
Compare
|
I believe I have addressed all the comments, and got it building again. |
@rabi I'll let you make the call, let me know which you prefer. |
IMO, we should go back, keeping in mind the number of limitations we've with this atm.
We can change back to single image later if other services like nova, cinder etc follow the single image pattern. |
I've separated it back out into heat-api and heat-engine. I think we would just need to drop the env var for heat-api-cfn in openstack-operator and it can use the heat-api env var instead. |
| chown -R heat:heat /etc/heat /var/log/heat /var/lib/heat /usr/share/heat | ||
|
|
||
| # Install config files — each file listed explicitly | ||
| COPY --from=build /configfiles/etc/heat/api-paste.ini /etc/heat/api-paste.ini |
There was a problem hiding this comment.
We don't need this in heat-engine container.
There was a problem hiding this comment.
Removed (and other refs)
| COPY common/config/usr/share/heat/heat-dist.conf /usr/share/heat/heat-dist.conf | ||
|
|
||
| # Set config file permissions | ||
| RUN chmod 640 /etc/heat/api-paste.ini && \ |
|
|
||
| # Install config files — each file listed explicitly | ||
| COPY --from=build /configfiles/etc/heat/api-paste.ini /etc/heat/api-paste.ini | ||
| COPY --from=build /configfiles/etc/heat/environment.d/default.yaml /etc/heat/environment.d/default.yaml |
There was a problem hiding this comment.
We don't need these env files heat-api anyway.
There was a problem hiding this comment.
Removed, and also removed from the build phase
| # Install config files — each file listed explicitly | ||
| COPY --from=build /configfiles/etc/heat/api-paste.ini /etc/heat/api-paste.ini | ||
| COPY --from=build /configfiles/etc/heat/environment.d/default.yaml /etc/heat/environment.d/default.yaml | ||
| COPY --from=build /configfiles/etc/heat/templates/AWS_CloudWatch_Alarm.yaml /etc/heat/templates/AWS_CloudWatch_Alarm.yaml |
There was a problem hiding this comment.
Actually we can get rid these templates as they're history. cloud-watch api has been dropped since long.
There was a problem hiding this comment.
Removed (and other refs)
| @@ -0,0 +1,12 @@ | |||
| [DEFAULT] | |||
There was a problem hiding this comment.
I think we can drop this file completely.
There was a problem hiding this comment.
This comes from the previous rpm packaging. Do we no longer need these settings? Most of them are different from the default values.
There was a problem hiding this comment.
They don't matter. In 17.1 we used to run heat-engine with /usr/bin/heat-engine --config-file /usr/share/heat/heat-dist.conf --config-file /etc/heat/heat.conf but in 18 it's /usr/bin/python3 /usr/bin/heat-engine --config-file /etc/heat/heat.conf.d
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Central YAML (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/lgtm |
amoralej
left a comment
There was a problem hiding this comment.
This looks fine to me.
heat-dist.conf may be adjusted as follow-up if desired.
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: rebtoor The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
elfiesmelfie
left a comment
There was a problem hiding this comment.
Please include an update to containers/image-mappings.yaml
There was a problem hiding this comment.
Have a look at the comment on #15 (comment)
You could do the same here in heat, with a heat/bindeps and heat//bindeps (and for the other {python,}*deps files), to reduce the maintenance effort. There is a reference implementation in cinder.
This can be done in a follow up IMHO
There was a problem hiding this comment.
Ok, I did that for bindeps and builddeps
| # --- Runtime stage --- | ||
| FROM ${BASE_IMAGE} | ||
|
|
||
| LABEL summary="OpenStack Heat engine" \ |
There was a problem hiding this comment.
Update the description if this is used for multiple heat services now
Use a single heat-api image for heat-api and heat-api-cfn and contains httpd, mod_ssl, python3-mod_wsgi. heat-engine is a separate image. The images build Heat from source upstream config files (api-paste.ini, environment.d, templates), and the distgit heat-dist.conf. Note that python3-cryptography is pinned to 43.0.0-4.el10, which matches the major version in upper-constraints.txt. Jira: OSPRH-34691 Jira: OSPRH-34692 Jira: OSPRH-34693 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com> Signed-off-by: James Slagle <jslagle@redhat.com>
fd2bb69
into
openstack-k8s-operators:main
Add Heat container image definitions
Use a single heat-api image for heat-api and heat-api-cfn and contains
httpd, mod_ssl, python3-mod_wsgi. heat-engine is a separate image.
The images build Heat from source upstream config files (api-paste.ini,
environment.d, templates), and the distgit heat-dist.conf.
Note that python3-cryptography is pinned to 43.0.0-4.el10, which matches
the major version in upper-constraints.txt.
Jira: OSPRH-34691
Jira: OSPRH-34692
Jira: OSPRH-34693
Co-Authored-By: Claude Opus 4.6 noreply@anthropic.com
Signed-off-by: James Slagle jslagle@redhat.com