Quadlet - Support template dependency #27169
Conversation
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: ygalblum 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-ci
bot
added
the
approved
ygalblum
force-pushed
the
quadlet-template-dependency
branch
from
de61858 to
e9fb660
Compare
test/system/252-quadlet.bats
Outdated
| } | ||
|
|
||
| # A quadlet container template depends on a quadlet volume template | ||
| @test "quadlet - template volume dependency" { |
There was a problem hiding this comment.
should we also add a test for the network file as well, I think we have another bug for that open as well and if I read the code right network is fixed too
There was a problem hiding this comment.
You're right. I added the network dependency to the same test to reduce the test time added by this change
ygalblum
force-pushed
the
quadlet-template-dependency
branch
from
e9fb660 to
70dda9c
Compare
test/system/252-quadlet.bats
Outdated
| # For template units, the volume name should have -%i appended | ||
| volume_name=${volume_name%@}-%i | ||
|
|
||
| # Save the unit name to use as the volume template for the container template |
test/system/252-quadlet.bats
Outdated
| for UNIT_FILE in ${UNIT_FILES[@]}; do | ||
| rm $UNIT_FILE | ||
| done | ||
| UNIT_FILES=() |
There was a problem hiding this comment.
doesn't the main teardown function already handle that? is there a reason why it must be here done again? IF so a comment might help.
There was a problem hiding this comment.
Added some more cleanup and a comment explaining why it's needed at the test level
There was a problem hiding this comment.
Thanks, note that there is an issue with that though because you cannot rely on that point here being reached. If there is a test or podman bug and let's say
run_podman network exists ${network_name_instance} fails then the test is aborted and further lines won't get executed. It then jump directly into the teardown function.
As such the cleanup in teardown must be able to handle the templated services as well otherwise we risk leaking resources on failed tests.
There was a problem hiding this comment.
As such the cleanup in teardown must be able to handle the templated services as well otherwise we risk leaking resources on failed tests.
You are right. But, this statement is true throughout in many other places. You can see other tests ending with manually deleting resources created by the test. Similarly, these cleanup are skipped if the test fails somewhere between their creation and cleanup.
When running as part of the CI, I don't see this as a problem because the environment is temporary and the test fails anyhow, so it will need to be executed again. When running locally, this is an issue. But, I have to say that running these tests locally makes such a mess (basically deleting everything else) that I'm not sure this leftover is the biggest issue.
I think that the main reason for cleanup is actually for the successful path making sure leftovers don't cause the next test to fail.
Having said that, I've added another commit (so we can squash or remove) that handles the volume and network services. Note that it does not handle the main container service since doing so will not allow deleting the volume.
Add support for Volumes and Networks Add e2e and system tests Resolves: containers#25136 Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
ygalblum
force-pushed
the
quadlet-template-dependency
branch
from
70dda9c to
83e65f9
Compare
|
[NON-BLOCKING] Packit jobs failed. @containers/packit-build please check. Everyone else, feel free to ignore. |
| return serviceName | ||
| } | ||
| return removeExtension(quadletUnitFile.Filename, "", defaultExtraSuffix) | ||
| baseServiceName := removeExtension(quadletUnitFile.Filename, "", "") |
There was a problem hiding this comment.
no chance this could be empty is there?
There was a problem hiding this comment.
Good thinking. But. from what I can tell (and test), it won't.
What I'm thinking is that this theoretically could happen if the file had the form of .network (or any other quadlet unit extension). But, if you look at the implementation of removeExtenstion:
podman/pkg/systemd/quadlet/quadlet.go
Line 507 in 3747e3d
You can see that it trims the extension only the last dot is not the first character. If it is (which is my case), it is not trimmed and since I'm passing two empty strings, I get the same value back.
Can you think of a different input that could have returned an empty string?
Signed-off-by: Ygal Blum <ygal.blum@gmail.com>
Does this PR introduce a user-facing change?
Yes
Resolves: #25136