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

Fix PowerVS builds by including wasm-shims file #1467

Merged
merged 1 commit into from
May 21, 2024

Conversation

Amulyam24
Copy link
Contributor

Change description

Removes unused variable containerd_wasm_shims_runtime_versions while copying containerd config file.

Related issues

Additional context

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label May 13, 2024
@k8s-ci-robot k8s-ci-robot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label May 13, 2024
@Amulyam24
Copy link
Contributor Author

/retest

@Amulyam24
Copy link
Contributor Author

/cc @mboersma

@mboersma
Copy link
Contributor

mboersma commented May 16, 2024

This should be a no-op overall: since containerd_wasm_shims_runtimes defaults to empty, it won't actually install anything. But it seems that /images/capi/packer/config/wasm-shims.json isn't included in the powervs/centos build.

What if instead of removing these lines in main.yml, we add packer/config/wasm-shims.json to COMMON_POWERVS_VAR_FILES? I think that will fix the build.

COMMON_POWERVS_VAR_FILES := packer/config/kubernetes.json \

@Amulyam24
Copy link
Contributor Author

This should be a no-op overall: since containerd_wasm_shims_runtimes defaults to empty, it won't actually install anything. But it seems that /images/capi/packer/config/wasm-shims.json isn't included in the powervs/centos build.

What if instead of removing these lines in main.yml, we add packer/config/wasm-shims.json to COMMON_POWERVS_VAR_FILES? I think that will fix the build.

COMMON_POWERVS_VAR_FILES := packer/config/kubernetes.json \

Since containerd_wasm_shims_runtime is empty, I feel it is better to remove the var as a fix. Any particular reason that it was added?

@mboersma
Copy link
Contributor

mboersma commented May 20, 2024

Any particular reason that it was added?

In #1037, the idea was that any distro could install the containerd-wasm-shims by populating the containerd_wasm_shims variable with one or more Wasm backends, but that by default that variable is empty. I think we just forgot to take into account PowerVS builds.

The simple and IMHO consistent fix would be to include that file. You could certainly argue it's cleaner not to include it all, since I don't think the containerd-wasm-shims are built for that architecture yet, but removing it from images/capi/ansible/roles/containerd/tasks/main.yml obviously breaks most other builds. Maybe you could just define containerd_wasm_shims_runtime_versions as an empty variable in the PowerVS JSON config file?

@Amulyam24
Copy link
Contributor Author

Any particular reason that it was added?

In #1037, the idea was that any distro could install the containerd-wasm-shims by populating the containerd_wasm_shims variable with one or more Wasm backends, but that by default that variable is empty. I think we just forgot to take into account PowerVS builds.

The simple and IMHO consistent fix would be to include that file. You could certainly argue it's cleaner not to include it all, since I don't think the containerd-wasm-shims are built for that architecture yet, but removing it from images/capi/ansible/roles/containerd/tasks/main.yml obviously breaks most other builds. Maybe you could just define containerd_wasm_shims_runtime_versions as an empty variable in the PowerVS JSON config file?

Thanks @mboersma for clarifying, updated the changes. PTAL

Copy link
Contributor

@mboersma mboersma left a comment

Choose a reason for hiding this comment

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

/lgtm
/assign @AverageMarcus @drew-viles

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label May 21, 2024
@mboersma
Copy link
Contributor

/retitle Fix PowerVS builds by including wasm-shims file

@k8s-ci-robot k8s-ci-robot changed the title Remove unused var while copying containerd config file Fix PowerVS builds by including wasm-shims file May 21, 2024
@drew-viles
Copy link
Contributor

/approve

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: drew-viles

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 21, 2024
@k8s-ci-robot k8s-ci-robot merged commit 8ea279e into kubernetes-sigs:main May 21, 2024
12 checks passed
@mboersma
Copy link
Contributor

Thanks again @Amulyam24! We don't have coverage of all targets in CI unfortunately, so we have to rely on intrepid community members like you to let us know when something is broken.

@Amulyam24 Amulyam24 deleted the fix branch May 21, 2024 14:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Building CAPI powervs and qemu images fail
5 participants