Skip to content

Move Cilium CRDs to special Helm directory for auto installation upon helm install#13310

Closed
christarazi wants to merge 3 commits intocilium:masterfrom
christarazi:pr/christarazi/move-crds-helm
Closed

Move Cilium CRDs to special Helm directory for auto installation upon helm install#13310
christarazi wants to merge 3 commits intocilium:masterfrom
christarazi:pr/christarazi/move-crds-helm

Conversation

@christarazi
Copy link
Copy Markdown
Member

@christarazi christarazi commented Sep 28, 2020 •
edited
Loading

This PR doesn't add the CRDs to the quick-install.yaml and experimental-install.yaml because it would make them too big to be practical. The reason is because the CRDs contain validation which can be quite verbose.

Manually tested by deploying the following and ensuring that kubectl get crds shows Cilium CRDs as soon as the Helm install begins:

diff --git a/install/kubernetes/cilium/charts/agent/templates/daemonset.yaml b/install/kubernetes/cilium/charts/agent/templates/daemonset.yaml
index 8bd0374a2..f757b779a 100644
--- a/install/kubernetes/cilium/charts/agent/templates/daemonset.yaml
+++ b/install/kubernetes/cilium/charts/agent/templates/daemonset.yaml
@@ -53,6 +53,7 @@ spec:
 {{- else }}
       - args:
         - --config-dir=/tmp/cilium/config-map
+        - --skip-crd-creation
         command:
         - cilium-agent
         livenessProbe:

See commit msgs.

Updates: #12737

Allow Helm to install Cilium CRDs instead of Operator, with Operator acting as a fallback in case Helm was unable to install CRDs

@christarazi
install: Move CRDs into special Helm directory
d0ed038 
This directory is designated for Helm to install CRDs via `helm
install`. [1] Note, that a `helm uninstall` will NOT remove the CRDs, as
one may assume.

[1]:
https://helm.sh/docs/chart_best_practices/custom_resource_definitions/

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi requested review from a team as code owners September 28, 2020 20:33
@christarazi christarazi requested a review from a team September 28, 2020 20:33
@christarazi christarazi added area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. release-note/misc This PR makes changes that have no direct user impact. labels Sep 28, 2020
@maintainer-s-little-helper maintainer-s-little-helper Bot added dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. and removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Sep 28, 2020
@christarazi christarazi changed the title This PR doesn't add the CRDs to the quick-install.yaml and experimental-install.yaml because it would make them too big to be practical. The reason is because the CRDs contain validation which can be quite verbose. Move Cilium CRDs to special Helm directory for auto installation upon helm install Sep 28, 2020
@christarazi
Makefile, k8s: Change CRD generation directory
6331fcb 
Following the commit to move the CRD YAMLs to a specific Helm directory,
update the Makefile target accordingly, as well as code referencing the
old embedded CRD bytes.

Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi
CODEOWNERS: Update to reflect new CRD directory
6a234a0 
Signed-off-by: Chris Tarazi <chris@isovalent.com>
@christarazi christarazi force-pushed the pr/christarazi/move-crds-helm branch from 7d45a00 to 6a234a0 Compare September 28, 2020 20:41
@christarazi
Copy link
Copy Markdown
Member Author

christarazi commented Sep 28, 2020

test-me-please

@christarazi christarazi mentioned this pull request Sep 28, 2020
Closed
12 tasks
@christarazi
Copy link
Copy Markdown
Member Author

christarazi commented Sep 29, 2020

retest-net-next

sayboras
sayboras approved these changes Sep 29, 2020
View reviewed changes
Copy link
Copy Markdown
Member

@sayboras sayboras left a comment

Choose a reason for hiding this comment

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

Just one note on below flag in helm 3, I feel like we need to document somewhere saying that this flag must not be set. Keen to hear your input.

The rest is LGTM 👍

      --skip-crds                    if set, no CRDs will be installed. By default, CRDs are installed if not already present

@aanm
Copy link
Copy Markdown
Member

aanm commented Sep 29, 2020

@sayboras that's a good point, how come when we are generating the template the CRDs are not created in the quick-install.yaml?

@sayboras
Copy link
Copy Markdown
Member

sayboras commented Sep 29, 2020

@sayboras that's a good point, how come when we are generating the template the CRDs are not created in the quick-install.yaml?

yes, I normally used the below flag to include crds in template. As far as I understand from PR description, we don't want to bundle crds in either quick-install or experimential-install.

      --include-crds                 include CRDs in the templated output

@aanm
Copy link
Copy Markdown
Member

aanm commented Sep 29, 2020

@sayboras oh ok, I think it's fine to leave it as is because we just don't want to have a huge quick-install.yaml but if the user runs helm install we want the CRDs to be installed (if they aren't already)

@christarazi did you test installing with helm on k8s 1.12?

@christarazi
Copy link
Copy Markdown
Member Author

christarazi commented Sep 29, 2020 •
edited
Loading

Update: This PR is failing on K8s versions below 1.15, because Helm does not know how to install v1beta1 CRDs on versions of K8s that do not understand v1. See this comment for more info. Pinning and converting to draft mode to be picked up in the future.

@christarazi christarazi added dont-merge/waiting-for-upstream Only merge once upstream library or kernel features have landed pinned These issues are not marked stale by our issue bot. labels Sep 29, 2020
@christarazi christarazi marked this pull request as draft September 29, 2020 21:47
@rolinh rolinh added the area/helm Impacts helm charts and user deployment experience label Sep 30, 2020
@sayboras sayboras mentioned this pull request Oct 12, 2020
Merged
@aanm aanm added the needs/triage This issue requires triaging to establish severity and next steps. label Oct 13, 2020
@christarazi
Copy link
Copy Markdown
Member Author

christarazi commented Jun 8, 2021

Obsoleted by #12737 (comment)

@christarazi christarazi closed this Jun 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aanm aanm aanm approved these changes

@sayboras sayboras sayboras approved these changes

Assignees

No one assigned

Labels

area/helm Impacts helm charts and user deployment experience area/k8s Impacts the kubernetes API, or kubernetes -> cilium internals translation layers. dont-merge/waiting-for-upstream Only merge once upstream library or kernel features have landed needs/triage This issue requires triaging to establish severity and next steps. pinned These issues are not marked stale by our issue bot. release-note/misc This PR makes changes that have no direct user impact.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@christarazi @aanm @sayboras @rolinh