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

passt: Build and publish Passt binding CNI image #50

Merged
merged 1 commit into from
Jul 23, 2024

Conversation

oshoval
Copy link
Collaborator

@oshoval oshoval commented Jul 21, 2024

What this PR does / why we need it:
Include DaemonSet manifest to deploy the binary on the cluster nodes.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #

Special notes for your reviewer:

Checklist

This checklist is not enforcing, but it's a reminder of items that could be relevant to every PR.
Approvers are expected to review this list.

Release note:


@kubevirt-bot kubevirt-bot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. labels Jul 21, 2024
@oshoval
Copy link
Collaborator Author

oshoval commented Jul 21, 2024

/uncc @maiqueb @qinqon

@oshoval oshoval force-pushed the passt branch 2 times, most recently from 97aa82d to c320914 Compare July 21, 2024 12:21
@oshoval oshoval changed the title WIP passt: Build and publish Passt binding CNI image Jul 21, 2024
@oshoval oshoval marked this pull request as ready for review July 21, 2024 12:34
@kubevirt-bot kubevirt-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 21, 2024
Copy link
Collaborator

@qinqon qinqon left a comment

Choose a reason for hiding this comment

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

The daemonsets should be living here also a mechanism to deploy it from this repo manually. CNAO should just use those manifests.

.github/workflows/publish-img.yaml Outdated Show resolved Hide resolved
@oshoval oshoval marked this pull request as draft July 22, 2024 14:09
@kubevirt-bot kubevirt-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 22, 2024
@oshoval
Copy link
Collaborator Author

oshoval commented Jul 22, 2024

not ready for review yet, will update once it is
thanks

@oshoval oshoval force-pushed the passt branch 2 times, most recently from 754143d to 1684e99 Compare July 22, 2024 14:21
@oshoval oshoval marked this pull request as ready for review July 22, 2024 14:41
@oshoval
Copy link
Collaborator Author

oshoval commented Jul 23, 2024

Removed the script and adapted the dockerfile to use ADD

@oshoval oshoval force-pushed the passt branch 2 times, most recently from 8e3ede0 to 9251621 Compare July 23, 2024 06:45
passt/passt-binding-cni-ds.yaml Show resolved Hide resolved
passt/passt-binding-cni-ds.yaml Show resolved Hide resolved
.github/workflows/publish-img.yaml Show resolved Hide resolved
.github/workflows/publish-img.yaml Show resolved Hide resolved
passt/Dockerfile Outdated Show resolved Hide resolved
passt/Dockerfile Outdated Show resolved Hide resolved
passt/Dockerfile Outdated Show resolved Hide resolved
@oshoval
Copy link
Collaborator Author

oshoval commented Jul 23, 2024

Please look on kubevirt/cluster-network-addons-operator#1829
no need to review yet because little stuff going to be changed,
the thing that important to understand is as i answered is that a tag on this repo, will publish the 2 images

  1. kubevirt_ipam_controller:tag (as was before)
  2. passt_binding_cni:tag (the new one)

This way once a new tag is released on this repo, CNAO will bump both images together of this component
CNAO also deployed them together under the same flag we already have kubevirtIpamController in the CR

@oshoval
Copy link
Collaborator Author

oshoval commented Jul 23, 2024

answered / addressed comments
updated make targets

Copy link
Collaborator

@qinqon qinqon left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

@kubevirt-bot kubevirt-bot added lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jul 23, 2024
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2024
@oshoval
Copy link
Collaborator Author

oshoval commented Jul 23, 2024

had to change something, hopefully it will fix the e2e in the building / pushing phase (need to use local registry as done for IMG)

@qinqon
Copy link
Collaborator

qinqon commented Jul 23, 2024

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2024
Include DaemonSet manifest to deploy the binary on the cluster nodes.

Signed-off-by: Or Shoval <oshoval@redhat.com>
@kubevirt-bot kubevirt-bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2024
@oshoval
Copy link
Collaborator Author

oshoval commented Jul 23, 2024

had to rebase because otherwise it wont be merged due to github branch protection

@qinqon
Copy link
Collaborator

qinqon commented Jul 23, 2024

/lgtm
/approve

@kubevirt-bot kubevirt-bot added the lgtm Indicates that a PR is ready to be merged. label Jul 23, 2024
@kubevirt-bot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: qinqon

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

@kubevirt-bot kubevirt-bot merged commit d79579c into kubevirt:main Jul 23, 2024
4 checks passed
@oshoval
Copy link
Collaborator Author

oshoval commented Jul 23, 2024

Thanks

We just need to allow pushing to ghcr.io/kubevirt/passt-binding-cni as was done for
ghcr.io/kubevirt/ipam-controller please
https://github.com/kubevirt/ipam-extensions/actions/runs/10057056570/job/27797219641

ERROR: failed to solve: failed to push ghcr.io/kubevirt/passt-binding-cni:latest: failed commit on ref "manifest-sha256:14aa2d7f981bb512c23c017ce1288683b7b983fc5b9d908f8937dbf65e870da7": unexpected status from PUT request to https://ghcr.io/v2/kubevirt/passt-binding-cni/manifests/sha256:14aa2d7f981bb512c23c017ce1288683b7b983fc5b9d908f8937dbf65e870da7: 403 Forbidden

@maiqueb
Copy link
Collaborator

maiqueb commented Jul 23, 2024

Thanks

We just need to allow pushing to ghcr.io/kubevirt/passt-binding-cni as was done for ghcr.io/kubevirt/ipam-controller please https://github.com/kubevirt/ipam-extensions/actions/runs/10057056570/job/27797219641

ERROR: failed to solve: failed to push ghcr.io/kubevirt/passt-binding-cni:latest: failed commit on ref "manifest-sha256:14aa2d7f981bb512c23c017ce1288683b7b983fc5b9d908f8937dbf65e870da7": unexpected status from PUT request to https://ghcr.io/v2/kubevirt/passt-binding-cni/manifests/sha256:14aa2d7f981bb512c23c017ce1288683b7b983fc5b9d908f8937dbf65e870da7: 403 Forbidden

wait so you're adding another image ? didn't we want to have everything in the same image so we don't have to do anything downstream ?

@oshoval
Copy link
Collaborator Author

oshoval commented Jul 23, 2024

wait so you're adding another image ? didn't we want to have everything in the same image so we don't have to do anything downstream ?

We already have a separated downstream image for the CNI binary (Or Mergi did), so it doesnt make sense to enforce everything to be in the same image.
Downstream doesn't need to create a new image, it exists already.
Having both U/S and D/S almost the same is a better and simpler design imho.

The new U/S image is simple, it just downloads the pre compiled binary and put it in a container image.

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. dco-signoff: yes Indicates the PR's author has DCO signed all their commits. lgtm Indicates that a PR is ready to be merged. size/M
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants