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

feat(install): remove host dependency on sh or mount #635

Closed
wants to merge 1 commit into from

Conversation

ernado
Copy link

@ernado ernado commented Nov 27, 2021

This allows running cilium install on Talos.

Signed-off-by: Aleksandr Razumov ernado@ya.ru

Ref: cilium/cilium#17883
Ref: cilium/cilium#16815

Checked on Talos with one control node and one worker:

$ cilium connectivity test

✅ All 11 tests (80 actions) successful, 0 tests skipped, 0 scenarios skipped.

@ernado ernado requested a review from a team as a code owner November 27, 2021 15:53
@ernado ernado requested a review from ldelossa November 27, 2021 15:53
@ernado ernado temporarily deployed to ci November 27, 2021 15:53 Inactive
@ernado
Copy link
Author

ernado commented Nov 27, 2021

Some connectivity tests failed, but not sure if related to PR:

Run # Background wait for job to complete or timeout
job.batch/cilium-cli condition met
Error from server: Get "https://10.168.0.47:10250/containerLogs/kube-system/cilium-cli-mpq68/test?timestamps=true": dial timeout, backstop
Error: Process completed with exit code 1.

Looks like cilium install was not called at all, so assuming that it is unrelated.

Please restart pipeline.

Copy link
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

LGTM - thanks.

// are available.
//
// See https://github.com/cilium/cilium/issues/17883
"cp /usr/bin/cilium-mount /hostbin/cilium-mount && " +
Copy link
Member

Choose a reason for hiding this comment

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

The mountCmd that you are replacing at L678 is to mount the BPF filesystem. However, L692-696 are for mounting cgroup fs.

Copy link
Author

Choose a reason for hiding this comment

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

Looks like we've already mounting BPF here:

VolumeMounts: []corev1.VolumeMount{
{
Name: "bpf-maps",
MountPath: "/sys/fs/bpf",
MountPropagation: &bidirectional,

I've changed MountPropagation to bidirectional, reflecting cilium/cilium#16656.

So I've updated #L678-L745 also with some context from #627

PTAL, should be ok now.

@aditighag
Copy link
Member

Thanks for the PR.
However, the changes are incorrect. You'll need to make changes equivalent to the file install/kubernetes/cilium/templates/cilium-agent/daemonset.yaml changes from PR - cilium/cilium#16656.

@aditighag aditighag added the dont-merge/bad-bot To prevent MLH from marking ready-to-merge. label Nov 29, 2021
@ernado
Copy link
Author

ernado commented Nov 29, 2021

Thank you! Will update my implementation according to referred PR.

@ernado ernado marked this pull request as draft November 29, 2021 18:37
@ernado ernado temporarily deployed to ci November 29, 2021 18:58 Inactive
Copy link
Contributor

@ldelossa ldelossa left a comment

Choose a reason for hiding this comment

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

revoking approval as @aditighag found issues.

@ernado ernado temporarily deployed to ci November 29, 2021 19:29 Inactive
@ernado ernado temporarily deployed to ci November 29, 2021 20:09 Inactive
@ernado ernado temporarily deployed to ci November 29, 2021 21:12 Inactive
@ernado ernado marked this pull request as ready for review November 29, 2021 21:22
@ernado ernado requested a review from ldelossa November 30, 2021 07:30
Comment on lines 14 to 20
"github.com/cilium/cilium-cli/defaults"
"github.com/cilium/cilium-cli/internal/certs"
"github.com/cilium/cilium-cli/internal/k8s"
"github.com/cilium/cilium-cli/internal/utils"
"github.com/cilium/cilium-cli/status"
"github.com/cilium/cilium/pkg/versioncheck"

Copy link
Member

Choose a reason for hiding this comment

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

Nit: please don't move this import group. We generally try to use the order 1. standard library, 2. packages from own repo, i.e. github.com/cilium/cilium-cli/... in that case) and 3. other 3rd party packages.

Copy link
Author

Choose a reason for hiding this comment

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

Done, thank you

@ernado ernado temporarily deployed to ci November 30, 2021 23:42 Inactive
@ernado ernado marked this pull request as draft November 30, 2021 23:48
@maintainer-s-little-helper
Copy link

Commit 2b1679f does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper
Copy link

Commit 2b1679f does not contain "Signed-off-by".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

This allows running `cilium install` on Talos.

Signed-off-by: Aleksandr Razumov <ernado@ya.ru>
@ernado ernado temporarily deployed to ci December 1, 2021 06:54 Inactive
@ernado ernado marked this pull request as ready for review December 1, 2021 06:57
@ernado
Copy link
Author

ernado commented Dec 1, 2021

Kind is not working without BPFFS init container, however BPFFS should be auto-mounted here with help of bidirectional volume.

Not sure how to fix it, so keeping init container for kind.

PTAL.

@ernado
Copy link
Author

ernado commented Dec 7, 2021

Gentle ping @aditighag

@aditighag
Copy link
Member

Kind is not working without BPFFS init container, however BPFFS should be auto-mounted here with help of bidirectional volume.

Not sure how to fix it, so keeping init container for kind.

PTAL.

What error are you hitting?

@aditighag
Copy link
Member

Also, this comment is not addresses - #635 (comment). This PR is adding cgroup mount init container - #627.

@ernado
Copy link
Author

ernado commented Dec 7, 2021

Looks like I'm lacking knowledge of internals, so closing this for now can't fully understand how eBPF fs is mounted.

For anybody interested: this PR works with Talos, rebase and build to use if needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dont-merge/bad-bot To prevent MLH from marking ready-to-merge.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants