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

operator: Ship slimmer binaries #10972

Merged
merged 2 commits into from May 29, 2020
Merged

operator: Ship slimmer binaries #10972

merged 2 commits into from May 29, 2020

Conversation

errordeveloper
Copy link
Contributor

@errordeveloper errordeveloper commented Apr 14, 2020 •
edited

This is a follow-up from #10758. The aim is to close #9920.

Sizes of new stripped binaries:

$ du -hs cilium-operator*
 63M    cilium-operator
 61M    cilium-operator-aws
 50M    cilium-operator-azure
 48M    cilium-operator-generic
$

@errordeveloper errordeveloper added wip release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/operator Impacts the cilium-operator component integration/cloud Related to integration with cloud environments such as AKS, EKS, GKE, etc. labels Apr 14, 2020
@errordeveloper errordeveloper requested a review from a team as a code owner April 14, 2020 15:26
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Apr 14, 2020
@errordeveloper
Copy link
Contributor Author

errordeveloper commented Apr 14, 2020 •
edited

TODOs

  • adjust the build logic to build 4 operator binaries instead of one
  • ensure IPAM flag handling is appropriate in each different binary

Follow-up TODOs

  • make changes in the Helm chat to select optimal binary based on size
  • add documentation (upgrade guide, release notes)

@coveralls
Copy link

coveralls commented Apr 14, 2020 •
edited

Coverage Status

Coverage decreased (-0.03%) to 36.85% when pulling 3528fa9 on pr/errordeveloper/operator-tags into 33cb7a5 on master.

errordeveloper
errordeveloper commented Apr 17, 2020
View reviewed changes
operator/Makefile Outdated Show resolved Hide resolved
@errordeveloper errordeveloper force-pushed the pr/errordeveloper/operator-tags branch from 2fd8cbb to 610cb76 Compare April 17, 2020 09:50
@errordeveloper
Copy link
Contributor Author

test-me-please

@errordeveloper
Copy link
Contributor Author

Tests fail on GKE with this:

[Fail] K8sDatapathConfig MonitorAggregation [It] Checks that monitor aggregation flags send notifications

Seems unrelated/flake.

@errordeveloper
Copy link
Contributor Author

test-me-please

@aanm aanm marked this pull request as draft April 23, 2020 10:12
@aanm aanm removed the wip label Apr 23, 2020
@aanm
Copy link
Member

aanm commented Apr 23, 2020

this PR has been marked as a draft PR since it had a WIP label. Please click in "Ready for review" [below vvv ] once the PR is ready to be reviewed. CI will still run for draft PRs.

@errordeveloper errordeveloper mentioned this pull request Apr 28, 2020
Merged
@errordeveloper errordeveloper force-pushed the pr/errordeveloper/operator-tags branch 2 times, most recently from d677fde to 3e8edd2 Compare May 19, 2020 10:48
errordeveloper
errordeveloper commented May 19, 2020
View reviewed changes
operator/.gitignore Outdated Show resolved Hide resolved
@errordeveloper errordeveloper force-pushed the pr/errordeveloper/operator-tags branch from 3e8edd2 to b3bf764 Compare May 19, 2020 18:24
errordeveloper
errordeveloper commented May 19, 2020
View reviewed changes
operator/flags.go
option.BindEnv(option.IPAM)

rootCmd.PreRunE = func(cmd *cobra.Command, args []string) error {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's not particularly pretty, but seems okay to me. A better options would be to have a wrapper binary that calls out to the correct implementation, but I think this will do to start with.

@errordeveloper errordeveloper requested review from aanm and tgraf May 19, 2020 18:42
@errordeveloper errordeveloper marked this pull request as ready for review May 19, 2020 18:42
@errordeveloper errordeveloper requested a review from a team as a code owner May 19, 2020 18:42
@errordeveloper errordeveloper requested a review from a team May 19, 2020 18:42
@errordeveloper errordeveloper requested a review from a team as a code owner May 19, 2020 18:42
@errordeveloper
Copy link
Contributor Author

There are still a few TODOs, but I believe it has all Go code changes need to be considered for 1.8 queue.

errordeveloper
errordeveloper commented May 19, 2020
View reviewed changes
install/kubernetes/cilium/charts/operator/templates/deployment.yaml Outdated
{{- else if .Values.global.azure.enabled }}
- cilium-operator-azure
{{- else }}
- cilium-operator-generic
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will imply --ipam=cluster-pool will be the default. Is that reasonable for new installations, or we should stick to one of the older modes for some time?

@errordeveloper
Copy link
Contributor Author

test-gke

@errordeveloper errordeveloper force-pushed the pr/errordeveloper/operator-tags branch 2 times, most recently from ba36a32 to 328c8bb Compare May 28, 2020 19:24
@errordeveloper

This comment has been minimized.

Sign in to view
@errordeveloper errordeveloper force-pushed the pr/errordeveloper/operator-tags branch from 328c8bb to bd26b4f Compare May 28, 2020 20:00
@errordeveloper
Copy link
Contributor Author

errordeveloper commented May 28, 2020 •
edited

I've dropped Helm chart changes for now as those were incomplete, will aim to address those in a separate PR.

@errordeveloper

This comment has been minimized.

Sign in to view
@errordeveloper errordeveloper force-pushed the pr/errordeveloper/operator-tags branch from bd26b4f to a82a505 Compare May 28, 2020 21:40
@errordeveloper
Copy link
Contributor Author

test-4.19

@errordeveloper
Copy link
Contributor Author

K8s-1.17-Kernel-4.19 passed, will fix 'go vet' now.

@errordeveloper
operator: Build 3 new slimmer binaries
0759d95 
- add per-provider binaries
  - handle --ipam flag
  - avoid adding deprecated flags in new binaries
- continue building fat binary and maintain the same name
  for compatibility

Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>
@errordeveloper
build: Place operator binaries in separate images
3528fa9 
Signed-off-by: Ilya Dmitrichenko <errordeveloper@gmail.com>
@errordeveloper
Copy link
Contributor Author

@aanm I've addressed the 'go vet' issue, it should pass on Travis now, since it passed in Jenkins earlier, do you think we can merge without running all the tests?

@errordeveloper errordeveloper force-pushed the pr/errordeveloper/operator-tags branch from a82a505 to 3528fa9 Compare May 29, 2020 08:18
@aanm
Copy link
Member

aanm commented May 29, 2020

https://travis-ci.com/github/cilium/cilium/jobs/341289019

@aanm aanm merged commit 05daeef into master May 29, 2020
1.8.0 automation moved this from In progress to Merged May 29, 2020
@aanm aanm deleted the pr/errordeveloper/operator-tags branch May 29, 2020 09:05
@errordeveloper errordeveloper mentioned this pull request Jun 1, 2020
Closed
tklauser added a commit that referenced this pull request Jun 3, 2020
@tklauser
operator: re-add install target to Makefile
544e025 
This was removed in #10972, re-add so binaries can easily be installed
e.g. to a temporary location during development.

Fixes: 053fc86 ("operator: Build 3 new slimmer binaries")
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
@tklauser tklauser mentioned this pull request Jun 3, 2020
Closed
@jrajahalme jrajahalme mentioned this pull request Jun 16, 2020
Merged
jrajahalme added a commit that referenced this pull request Jun 16, 2020
@jrajahalme
build: Fix operator image builds when DOCKER_BUILDKIT=1
1c775de 
Fixes: #10972
Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
nebril pushed a commit that referenced this pull request Jun 20, 2020
@jrajahalme @nebril
build: Fix operator image builds when DOCKER_BUILDKIT=1
4b5455e 
Fixes: #10972
Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
aanm pushed a commit that referenced this pull request Jun 21, 2020
@jrajahalme @aanm
build: Fix operator image builds when DOCKER_BUILDKIT=1
00a4673 
[ upstream commit 4b5455e ]

Fixes: #10972
Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: André Martins <andre@cilium.io>
aanm pushed a commit that referenced this pull request Jun 22, 2020
@jrajahalme @aanm
build: Fix operator image builds when DOCKER_BUILDKIT=1
678c104 
[ upstream commit 4b5455e ]

Fixes: #10972
Signed-off-by: Jarno Rajahalme <jarno@covalent.io>
Signed-off-by: André Martins <andre@cilium.io>
@tklauser tklauser mentioned this pull request Aug 13, 2020
Closed
@lsowen lsowen mentioned this pull request Apr 22, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tgraf tgraf tgraf approved these changes

@aanm aanm aanm approved these changes

Assignees
No one assigned
Labels
area/operator Impacts the cilium-operator component integration/cloud Related to integration with cloud environments such as AKS, EKS, GKE, etc. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Split operator to separate out cloud provider specific code
4 participants
@errordeveloper @coveralls @aanm @tgraf