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.

Sign up for GitHub

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

Jump to bottom

Enable operator operation without kubernetes. #21344

Merged
merged 1 commit into from
Oct 20, 2022
Merged

Enable operator operation without kubernetes. #21344

merged 1 commit into from
Oct 20, 2022

Conversation

pruiz
Copy link
Contributor

@pruiz pruiz commented Sep 17, 2022 •
edited

Hi, latelly I've been working on setting up cilium with nomad, instead of kubernetes. By following some advices from @joestringer at #18334, and inspecting some documentation and guidance on howto setup cilium with docker, I've been able to integrate cilium agent, CNI & docker plugins to nomad.

However, the only component not working out of the box is the operator (actually the generic operator). This is due to some code paths assuming kubernetes availability, and failing (obviously) when no k8s apiserver that can be contacted.

This PR addresses this issues by adding a new command argument (--disable-k8s-services, w/ false as default), which enables basic usage of the operator (lease cleanup, ipam address gc, etc.), but disables any k8s-specific code/features.

There are some more missing pieces/features which I would like to add to operator in future PRs, like being able to provide/distribute policy statements to agents from kvstore, instead of using kubernetes CRDs, etc. But this is the bare initial support for operator w/o kubernetes.

PD: For nomad users interested on this, this github issue is the actual place where using cilium w/ nomad is being handled (hashicorp/nomad#12120)

@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 17, 2022
@joestringer joestringer added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Sep 20, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Sep 20, 2022
joestringer
joestringer reviewed Sep 20, 2022
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

I'm actually not sure whether to mark this as release-note/minor or release-note/major, as we could consider this to be a major announcement of initial Nomad support 🎉

I ran the sanity checker CI jobs and I can kick off the full CI suite on demand, let me know if/when you're ready to run those.

Typically I would wait for the contributor to mark the PR as "ready for review" before providing comments, but since we've had some discussions before and you pinged me in the description, I took an early look. Couple of minor comments below.

operator/option/config.go Outdated Show resolved Hide resolved
operator/cmd/root.go Outdated Show resolved Hide resolved
operator/cmd/root.go Show resolved Hide resolved
@maintainer-s-little-helper
Copy link

Commit 235bc7eb03e5da43beea92fa2514b5288851b71c 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 maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Sep 21, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Sep 21, 2022
@maintainer-s-little-helper
Copy link

Commit 0be6c2c17793cd5ed615796941310b5d4ba16e5d 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 maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Sep 21, 2022
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Sep 22, 2022
@maintainer-s-little-helper
Copy link

Commit fe28bad20ae527253073efa1252c2a3a4102fd80 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 maintainer-s-little-helper bot added the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Sep 22, 2022
@pruiz pruiz marked this pull request as ready for review September 22, 2022 07:09
@pruiz pruiz requested a review from a team as a code owner September 22, 2022 07:09
@pruiz pruiz requested a review from nebril September 22, 2022 07:09
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Sep 22, 2022
@pruiz
Copy link
Contributor Author

pruiz commented Sep 22, 2022

Updated branch w/ changes and rebased against current master.

joestringer
joestringer requested changes Sep 22, 2022
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

Minor nit. Could you also squash all the patches together?

You will also need to run make && make -C Documentation update-cmdref after changing the below in order to properly update the docs. Then the CI targets should pass.

operator/cmd/flags.go Outdated Show resolved Hide resolved
@pruiz
Copy link
Contributor Author

pruiz commented Sep 25, 2022

Fixed cli help reference, rebased and squashed changes.

@joestringer
Copy link
Member

joestringer commented Sep 26, 2022 •
edited

The tests failed because the autogenerated docs need to be updated. Please be sure to build the binaries locally and then run 'make -C Documentation update-cmdref', amend the commit to include the latest changes, and re-push the PR.

https://github.com/cilium/cilium/pull/21344/checks?check_run_id=8538833626

@pruiz
Copy link
Contributor Author

pruiz commented Sep 26, 2022 via email

joestringer
joestringer approved these changes Sep 28, 2022
View reviewed changes
Copy link
Member

@joestringer joestringer left a comment •
edited

Choose a reason for hiding this comment

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

LGTM. I'll run the full CI. It would be also good to get a review from @cilium/operator.

@joestringer
Copy link
Member

joestringer commented Sep 28, 2022 •
edited by maintainer-s-little-helper bot

/test

Job 'Cilium-PR-K8s-1.25-kernel-net-next' failed:

Click to show.

Test Name

K8sDatapathConfig Check BPF masquerading with ip-masq-agent VXLAN

Failure Output

FAIL: Failed to add ip route

If it is a flake and a GitHub issue doesn't already exist to track it, comment /mlh new-flake Cilium-PR-K8s-1.25-kernel-net-next so I can create one.

@joestringer
Copy link
Member

joestringer commented Sep 28, 2022 •
edited by maintainer-s-little-helper bot

/mlh new-flake Cilium-PR-K8s-1.25-kernel-net-next

👍 created #21488

@joestringer
Copy link
Member

Testing is passing, just looking for one more reviewer on the operator side. I think that the main concern this will draw over time is whether we can better structure the operator to support enabling these sorts of functionalities in a more modular manner, rather than scattering conditional checks around the code. At the same time, I'm not sure if we've got to a point where the necessary abstractions are clear so I don't have strong opinions about whether we should try to address that now as we begin these steps or whether we can just address that over time. Hopefully someone with a better familiarity with that part of the codebase could provide some thoughts in that regard.

@joestringer joestringer added the dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. label Sep 29, 2022
@joestringer
Copy link
Member

Sorry, looks like something else went into the tree that caused a conflict. Can you rebase against the latest master branch?

nebril
nebril approved these changes Sep 30, 2022
View reviewed changes
Copy link
Member

@nebril nebril left a comment

Choose a reason for hiding this comment

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

LGTM!

@joestringer
Copy link
Member

joestringer commented Oct 19, 2022 •
edited

This seems like it's got the necessary approvals, can you rebase against master @pruiz ? After that we can trigger CI and hopefully merge.

@pruiz
Copy link
Contributor Author

pruiz commented Oct 19, 2022

@joestringer I'll try, but this is getting a bit too much overhead to get merged. I understand each project has it's own mechanics.. but this is likely the fifth time I have to rebase this, everytime with additional overhead in order to fix conflicts. :(

@pruiz
Enable operator operation without kubernetes.
1291049 
This is intended for use-cases of cilium with other orquestrators compatible
with CNI or docker's network plugins, so we can have an operator running
against kvstore, but w/o requiring mandatory access to k8s.

This is addresses by adding a new command argument (--enable-k8s=false,
w/ true as default), which enables basic usage of the operator (lease cleanup, ipam address gc, etc.).

Signed-off-by: Pablo Ruiz <pablo.ruiz@gmail.com>
@pruiz
Copy link
Contributor Author

pruiz commented Oct 19, 2022

Rebased, and tested locally (w/o k8s), let's hope CI tests finish ok. :?

@joestringer
Copy link
Member

Github infrastructure appears to be having some trouble at the moment, causing image CI builds to fail. I'll try closing/reopening the PR to re-kick it.

@joestringer joestringer reopened this Oct 19, 2022
@joestringer
Copy link
Member

@pruiz I understand, thanks for bearing with us in this process. I'll keep an eye out for CI but unless something broke during the recent rebase I expect this should be OK to merge.

@joestringer
Copy link
Member

/test

@pruiz
Copy link
Contributor Author

pruiz commented Oct 19, 2022

@pruiz I understand, thanks for bearing with us in this process. I'll keep an eye out for CI but unless something broke during the recent rebase I expect this should be OK to merge.

understood.. I'll try to keep an eye. Thnks

@joestringer
Copy link
Member

k8s-1.23-kernel-5.4 workflow hit known flaky issue #21735, no need to block this PR on the failure.

@joestringer
Copy link
Member

That was the only failure, reviews are in, this looks good to merge. Thanks for your contribution! 🎉

@joestringer joestringer merged commit 3c9a5c1 into cilium:master Oct 20, 2022
@pruiz
Copy link
Contributor Author

pruiz commented Oct 21, 2022 via email

@lgfa29 lgfa29 mentioned this pull request Feb 15, 2023
Closed
@pippolo84 pippolo84 mentioned this pull request Mar 21, 2023
Merged
@EtienneM EtienneM mentioned this pull request Apr 5, 2023
Closed
2 tasks
pippolo84 added a commit to pippolo84/cilium that referenced this pull request Apr 12, 2023
@pippolo84
k8s, operator: Move enable-k8s option to clientset config
6930f26 
The enable-k8s option has been introduced to avoid some operator code
paths that assumed to run in a k8s environment.  Instead of passing
around the flag, we use it to configure the clientset, so that the check
`clientset.IsEnabled()` will keep that too into account and avoid the
execution of kubernetes-specific code.

The operator api server cell and tests have been updated accordingly.

Related: cilium#21344

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
dylandreimerink pushed a commit that referenced this pull request Apr 13, 2023
@pippolo84 @dylandreimerink
k8s, operator: Move enable-k8s option to clientset config
d5005bf 
The enable-k8s option has been introduced to avoid some operator code
paths that assumed to run in a k8s environment.  Instead of passing
around the flag, we use it to configure the clientset, so that the check
`clientset.IsEnabled()` will keep that too into account and avoid the
execution of kubernetes-specific code.

The operator api server cell and tests have been updated accordingly.

Related: #21344

Signed-off-by: Fabio Falzoi <fabio.falzoi@isovalent.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lazedo lazedo lazedo left review comments

@joestringer joestringer joestringer approved these changes

@nebril nebril nebril approved these changes

@christarazi christarazi christarazi approved these changes

Assignees

@joestringer joestringer

Labels
dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@pruiz @joestringer @lazedo @nebril @christarazi