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

ci: add yq shim for v3/v4 support #1819

Conversation

mkulke
Copy link
Collaborator

@mkulke mkulke commented Apr 25, 2024

yq3 (installed in kata builds as a side effect) and yq4 are different in cli and query syntax. the shim should abstract from those differences

@mkulke mkulke added the test_e2e_libvirt Run Libvirt e2e tests label Apr 25, 2024
Copy link
Member

@stevenhorsman stevenhorsman left a comment

Choose a reason for hiding this comment

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

Thanks @mkulke - the code looks good to me and it's nice timing as I've just started to try out ditching the caching and was about to look at this. I will try it in practice building the podvm too now.

@mkulke mkulke force-pushed the mkulke/use-version-independent-yq-shim branch from 7a5ab36 to d7e1ddd Compare April 25, 2024 13:52
@stevenhorsman
Copy link
Member

So the workflow is failing with:

Makefile.defaults:6: *** "/work/cloud-api-adaptor/hack/yq-shim.sh not found, consider doing snap install yq".  Stop.

So maybe there is an issue with the $(ROOT_PATH)/hack/yq-shim.sh not having src/cloud-api-adaptor in the path, or the workflow running in the wrong directory?

Anyway I can confirm that pulling this into my switch to main branch it allows me to install the yq v3 that the kata-agent in main needs, so thanks so much!

@mkulke
Copy link
Collaborator Author

mkulke commented Apr 25, 2024

So maybe there is an issue with the $(ROOT_PATH)/hack/yq-shim.sh not having src/cloud-api-adaptor in the path, or the workflow running in the wrong directory?

Anyway I can confirm that pulling this into my switch to main branch it allows me to install the yq v3 that the kata-agent in main needs, so thanks so much!

The shim wasn't present in the Dockerfile, I pushed a fix. The CI passes the docker build step now, let's see whether the podvm build also works.

I'm running a non-docker build on a fork at the moment that will build kata-agent (and hence intall yq v3) and then later CAA binaries that used to rely on v4, so we'll see whether the fallback works.

@mkulke mkulke force-pushed the mkulke/use-version-independent-yq-shim branch from d7e1ddd to 6c16aa2 Compare April 25, 2024 14:46
Copy link
Member

@beraldoleal beraldoleal left a comment

Choose a reason for hiding this comment

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

Hi @mkulke I got it! :) :You was talking about the caa side, not kata side!

<joke>Apart from introducing another "shim"</joke>, lgtm.

@mkulke
Copy link
Collaborator Author

mkulke commented Apr 25, 2024

Hi @mkulke I got it! :) :You was talking about the caa side, not kata side!

Apart from introducing another "shim", lgtm.

I somehow thought that kata had an abstraction over yq queries, but I must have misremembered. For CAA it's easier to deal with the version ambiguity, as it's only a couple of queries.

Copy link
Member

@surajssd surajssd left a comment

Choose a reason for hiding this comment

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

One nit, otherwise LGTM

src/cloud-api-adaptor/libvirt/config_libvirt.sh Outdated Show resolved Hide resolved
yq3 (installed in kata builds as a side effect) and yq4 are different in
cli and query syntax. the shim should abstract from those differences

Signed-off-by: Magnus Kulke <[email protected]>
@mkulke mkulke force-pushed the mkulke/use-version-independent-yq-shim branch from 6c16aa2 to ced7cbb Compare April 25, 2024 15:45
@mkulke
Copy link
Collaborator Author

mkulke commented Apr 25, 2024

ok the worklfow in which the kata-build installs v3 and things still work passed

the libvirt tests also seem to execute now, so that's good. i'll merge after the libvirt tests hopefully pass

@surajssd
Copy link
Member

BTW, one question, we should use the yq-shim only when dealing with stuff related to kata or everywhere in CAA now we should use yq-shim?

@stevenhorsman
Copy link
Member

The amd-64 podvm image build failures look like a network flake:

#17 139.3 ==> qemu.ubuntu: Retrieving ISO
#17 139.3 ==> qemu.ubuntu: Trying https://cloud-images.ubuntu.com/releases/focal/release-20230107/ubuntu-20.04-server-cloudimg-amd64.img
#17 139.3 ==> qemu.ubuntu: Trying https://cloud-images.ubuntu.com/releases/focal/release-20230107/ubuntu-20.04-server-cloudimg-amd64.img?checksum=sha256%3A3895e38566e5c2c019f5c6f825ab7570ee34dac6b9142fab0c7e5a78084c4280
#17 1409.3 ==> qemu.ubuntu: Download failed read tcp 172.17.0.2:49390->185.125.190.37:443: read: connection reset by peer
#17 1409.3 ==> qemu.ubuntu: error downloading ISO: [read tcp 172.17.0.2:49390->185.125.190.37:443: read: connection reset by peer]
#17 1409.3 Build 'qemu.ubuntu' errored after 21 minutes 10 seconds: error downloading ISO: [read tcp 172.17.0.2:49390->185.125.190.37:443: read: connection reset by peer]

so I've re-run them

@mkulke
Copy link
Collaborator Author

mkulke commented Apr 25, 2024

BTW, one question, we should use the yq-shim only when dealing with stuff related to kata or everywhere in CAA now we should use yq-shim?

kata will mandate v3 for the time being, and it will even install it on you machine, overriding v4. so the yq-shim adds a compatibility layer that will make v4 queries (CAA) work with v3 and v4

this allows you to build kata + caa on the same machine. so you'd use the yq-shim when v3 is required (e.g. the azure build)

@mkulke mkulke merged commit 31d89ea into confidential-containers:main Apr 25, 2024
28 checks passed
@mkulke mkulke deleted the mkulke/use-version-independent-yq-shim branch April 25, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
test_e2e_libvirt Run Libvirt e2e tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants