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 action for building other docker images, add a test for open-iscsi #555

Merged
merged 3 commits into from
Sep 13, 2024

Conversation

krnowak
Copy link
Member

@krnowak krnowak commented Sep 5, 2024

This PR adds a github action that can be used for building docker images based on dockerfiles stored in the other-dockerfiles directory. The action takes one parameter, which is a name of a subdirectory in other-dockerfiles. Currently there's just one, targetcli-fb. The action has no other triggers than workflow-dispatch. For now, the targetcli-fb docker image does not need to be rebuilt all that often - supposedly only when the dockerfile gets updated.

Second change is an addition of the packages subtest for checking the open-iscsi functionality. The test is based on steps outlined by @chewi in flatcar/scripts#2261.

CI run for amd64: http://jenkins.infra.kinvolk.io:8080/job/container/job/test/26905/console

The CI was running with some extra modifications in the branch, which are now gone:

  • Some debugging stuff like set -x to the shell scripts.
  • Used ghcr.io/krnowak/targetcli-fb instead of ghcr.io/flatcar/targetcli-fb, and the action was also changed to upload to my account, instead of flatcar.

These dockerfiles will be consumed by the github action, that will
build docker images and upload to our ghcr.io registry. The new image
will be used later by the open-iscsi test.
@krnowak krnowak force-pushed the krnowak/open-iscsi-test branch 4 times, most recently from 6680733 to 5ca0cca Compare September 11, 2024 14:24
@krnowak krnowak marked this pull request as ready for review September 11, 2024 14:38
@krnowak krnowak requested a review from a team September 11, 2024 14:38
Copy link
Contributor

@chewi chewi left a comment

Choose a reason for hiding this comment

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

Nice!!

Not being familiar with GHA, do you not need to define the targetcli-fb Docker image job somewhere? Or will that be done through the web UI?

What is rtslib-fb-target for?

@krnowak
Copy link
Member Author

krnowak commented Sep 11, 2024

Nice!!

Thanks. :)

Not being familiar with GHA, do you not need to define the targetcli-fb Docker image job somewhere? Or will that be done through the web UI?

That yaml file defines it. So, I imagine that what we could merge this PR, then kick off the action, so we will have ghcr.io/flatcar/targetcli-fb docker image available and then update the mantle manifest in scripts. That way, the kola test won't fail because of missing docker image.

So yeah, kicking off is something to do through the web UI. I don't think it makes sense to rebuild the image periodically - debian is rather stable, isnt' it? :)

What is rtslib-fb-target for?

At first, I thought this is where info about all this stuff that we create through targetcli is stored. But I'm not sure, it's stored somewhere in the kernel side, doesn't it? So, I suppose it's for some config, so maybe we don't need to mount any persistent directory there.

@chewi
Copy link
Contributor

chewi commented Sep 11, 2024

That yaml file defines it. So, I imagine that what we could merge this PR, then kick off the action, so we will have ghcr.io/flatcar/targetcli-fb docker image available and then update the mantle manifest in scripts. That way, the kola test won't fail because of missing docker image.

Right, but that yaml is only a template, it doesn't actually mention targetcli-fb anywhere. So, you have to enter it as a parameter in the UI? It's not a job we can add to a list?

At first, I thought this is where info about all this stuff that we create through targetcli is stored. But I'm not sure, it's stored somewhere in the kernel side, doesn't it? So, I suppose it's for some config, so maybe we don't need to mount any persistent directory there.

When I tried this, the server side on Gentoo stored some JSON in /etc/target. Maybe it's called something else in Debian. This is only used to load the config back into the kernel following a reboot though. The live config just lives in the kernel.

@krnowak
Copy link
Member Author

krnowak commented Sep 12, 2024

That yaml file defines it. So, I imagine that what we could merge this PR, then kick off the action, so we will have ghcr.io/flatcar/targetcli-fb docker image available and then update the mantle manifest in scripts. That way, the kola test won't fail because of missing docker image.

Right, but that yaml is only a template, it doesn't actually mention targetcli-fb anywhere. So, you have to enter it as a parameter in the UI?

Right, it's a parameterized job. Something like the Dispatch kola tests job in scripts. I think I could make it non-parameterized by hardcoding targetcli-fb, as, for now, this is the only "other Dockerfile" we have. But I'd still leave it as triggerable only through the "workflow_dispatch" event, because I don't think there is a point in rebuilding the image e.g. every week.

It's not a job we can add to a list?

Honestly, you lost me here a bit. What list do you have in mind? Once this PR is merged it will show up in mantle's github actions page. Actually, as an example, I made this branch a flatcar-master branch on my fork, so you can see that the "Build other docker image" workflow is available on the list - https://github.com/krnowak/mantle/actions/workflows/build-other-docker-image.yaml.

At first, I thought this is where info about all this stuff that we create through targetcli is stored. But I'm not sure, it's stored somewhere in the kernel side, doesn't it? So, I suppose it's for some config, so maybe we don't need to mount any persistent directory there.

When I tried this, the server side on Gentoo stored some JSON in /etc/target. Maybe it's called something else in Debian.

Quite possibly that Debian has something else. Or maybe it's just a version difference (you know, Debian being a museum of free software jokes and stuff). By the name of the directory, I assumed that it isn't targetcli-fb that stores the configuration there directly, but rather one of its dependencies.

This is only used to load the config back into the kernel following a reboot though. The live config just lives in the kernel.

I'll drop the --mount flag for the config directory and test to check if it matters - we are not rebooting the server, so it should not.

@chewi
Copy link
Contributor

chewi commented Sep 12, 2024

Thanks, seeing Dispatch kola tests helped me understand a little better. I just thought there might be way to run the job with predefined parameters so you wouldn't need to remember that targetcli-fb is one of the options.

@krnowak
Copy link
Member Author

krnowak commented Sep 12, 2024

Thanks, seeing Dispatch kola tests helped me understand a little better. I just thought there might be way to run the job with predefined parameters so you wouldn't need to remember that targetcli-fb is one of the options.

There seem to be a input type named choice, so maybe it will render as a drop-down box of possible options. I'll try that out.

There isn't a clear way to tell when the image should be rebuilt
automatically. In theory it should be when the relevant debian package
gets an update or when a new Debian version is released. In practice,
these are annoying to track, so the action is to be invoked on demand.
@krnowak krnowak force-pushed the krnowak/open-iscsi-test branch 2 times, most recently from 10fd375 to e20bf97 Compare September 12, 2024 16:42
@krnowak
Copy link
Member Author

krnowak commented Sep 12, 2024

I updated the github action, so the input parameter is a drop-down box, so I don't need to type targetcli-fb there. Also added the missing set -euo pipefail in the final client check script. Stuff seems to be still working (run on a branch that fetches docker image from my fork and adds some debug spew):

amd64: http://jenkins.infra.kinvolk.io:8080/job/container/job/test/26959/console
arm64: http://jenkins.infra.kinvolk.io:8080/job/container/job/test/26961/console

@krnowak krnowak requested a review from chewi September 12, 2024 16:45
@krnowak krnowak merged commit 183aeea into flatcar-master Sep 13, 2024
2 checks passed
@krnowak krnowak deleted the krnowak/open-iscsi-test branch September 13, 2024 09:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants