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

Add ability to use cilium config map in cilium-operator automatically #10347

Merged
merged 5 commits into from Feb 27, 2020

Conversation

aanm
Copy link
Member

@aanm aanm commented Feb 26, 2020

Please read commit by commit. I tried to make gradual changes but it was a little difficult with the flags.

Cilium Operator can now use the flags specified `cilium-config` k8s configuration map

This change is Reviewable

@aanm aanm added pending-review kind/cleanup This includes no functional changes. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Feb 26, 2020
@aanm aanm requested a review from a team February 26, 2020 15:37
@aanm aanm requested review from a team as code owners February 26, 2020 15:37
@aanm aanm requested a review from a team February 26, 2020 15:37
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Feb 26, 2020
@aanm
Copy link
Member Author

aanm commented Feb 26, 2020

test-me-please

@aanm aanm requested a review from a team February 26, 2020 15:38
@coveralls
Copy link

coveralls commented Feb 26, 2020

Coverage Status

Coverage increased (+0.09%) to 45.549% when pulling aa129d6 on pr/clean-operator-flags into c9de62b on master.

Copy link
Member

@ungureanuvladvictor ungureanuvladvictor left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r1, 11 of 11 files at r2, 6 of 6 files at r3, 3 of 3 files at r4, 6 of 6 files at r5.
Reviewable status: :shipit: complete! all files reviewed, 7 unresolved discussions (waiting on @aanm)


install/kubernetes/quick-install.yaml, line 121 at r5 (raw file):

  masquerade: "true"
  enable-xt-socket-fallback: "true"

Was this missing from somewhere?


install/kubernetes/quick-install.yaml, line 411 at r5 (raw file):

              - /cni-uninstall.sh
        resources:
          {}

Do you think it's valuable to leave this here but with empty resources block?


install/kubernetes/cilium/charts/config/templates/configmap.yaml, line 66 at r5 (raw file):

  # which the Cilium Operator will have their metrics exposed.
  # NOTE that this will open the port on the nodes where Cilium operator pods
  # are scheduled.

where Cilium operator pod is scheduled since we cannot run the operator in HA mode?


operator/flags.go, line 82 at r2 (raw file):

	option.BindEnv(option.CNPNodeStatusGCInterval)

	flags.Duration(option.CNPStatusUpdateInterval, 1*time.Second, "interval between CNP status updates sent to the k8s-apiserver per-CNP")

s/interval/Interval just to keep it consistent related to capitalization of other description of options.


operator/flags.go, line 154 at r2 (raw file):

	option.BindEnv(option.Version)

	// TODO: Urgent fix

Fix related to ?


operator/main.go, line 136 at r2 (raw file):

		option.Config.K8sAPIServer,
		option.Config.K8sKubeConfigPath,
		float32(option.Config.K8sClientQPSLimit),

option.Config.K8sClientQPSLimit is of type float32 already, don't think you need the cast


operator/metrics.go, line 43 at r3 (raw file):

func getPrometheusServerAddr() string {
	if option.Config.OperatorPrometheusServeAddr == "" {
		return metricsAddress

given we'll use metricsAddress just here is it worth keeping in operator/main.go?

Signed-off-by: André Martins <andre@cilium.io>
@aanm
Copy link
Member Author

aanm commented Feb 26, 2020

Reviewed 2 of 2 files at r1, 11 of 11 files at r2, 6 of 6 files at r3, 3 of 3 files at r4, 6 of 6 files at r5.
Reviewable status: :shipit: complete! all files reviewed, 7 unresolved discussions (waiting on @aanm)

install/kubernetes/quick-install.yaml, line 121 at r5 (raw file):

  masquerade: "true"
  enable-xt-socket-fallback: "true"

Was this missing from somewhere?

This file is auto generated it was probably some older PR that merged changes without generating the file

install/kubernetes/quick-install.yaml, line 411 at r5 (raw file):

              - /cni-uninstall.sh
        resources:
          {}

Do you think it's valuable to leave this here but with empty resources block?

This file is auto generated it was probably some older PR that merged changes without generating the file. But to answer your question, this does seem suspicious. I've open #10350 to track this.

install/kubernetes/cilium/charts/config/templates/configmap.yaml, line 66 at r5 (raw file):

  # which the Cilium Operator will have their metrics exposed.
  # NOTE that this will open the port on the nodes where Cilium operator pods
  # are scheduled.

where Cilium operator pod is scheduled since we cannot run the operator in HA mode?

👍

operator/flags.go, line 82 at r2 (raw file):

	option.BindEnv(option.CNPNodeStatusGCInterval)

	flags.Duration(option.CNPStatusUpdateInterval, 1*time.Second, "interval between CNP status updates sent to the k8s-apiserver per-CNP")

s/interval/Interval just to keep it consistent related to capitalization of other description of options.

👍

operator/flags.go, line 154 at r2 (raw file):

	option.BindEnv(option.Version)

	// TODO: Urgent fix

Fix related to ?

This was intended for me to not forget about it. I'll fix it once the PR is merged since it requires backport to 1.7. It's basically because the operator is opening that port on host network level to serve health requests to kubelet.

operator/main.go, line 136 at r2 (raw file):

		option.Config.K8sAPIServer,
		option.Config.K8sKubeConfigPath,
		float32(option.Config.K8sClientQPSLimit),

option.Config.K8sClientQPSLimit is of type float32 already, don't think you need the cast

it's of type float64. Unfortunately, viper does not have viper.GetFloat32

operator/metrics.go, line 43 at r3 (raw file):

func getPrometheusServerAddr() string {
	if option.Config.OperatorPrometheusServeAddr == "" {
		return metricsAddress

given we'll use metricsAddress just here is it worth keeping in operator/main.go?

good point, will do

This commit helps to have consistency among the flags being used by both
cilium-agent and cilium-operator as well as it will help with the
removal of all environment variables being currently used in the
cilium-operator.

Signed-off-by: André Martins <andre@cilium.io>
Some flags are redundant so we can deprecated those and give the user
ability to enable / disable such features with existing or new flags.

Signed-off-by: André Martins <andre@cilium.io>
Similar to what we have in cilium-agent we can now have the same in
cilium-operator where the options can be specified in a directory for
better UX when deploying cilium-operator in a cluster.

Signed-off-by: André Martins <andre@cilium.io>
As cilium-operator has the ability to read from the configuration
directory there is no point in using environment variables to read from
the ConfigMap, instead mount the full ConfigMap into a directory similar
to what is being done in the cilium-agent.

Signed-off-by: André Martins <andre@cilium.io>
@aanm
Copy link
Member Author

aanm commented Feb 26, 2020

test-me-please

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.

Docs changes LGTM. I didn't look at the other changes in detail.

@joestringer
Copy link
Member

test-docs-please

Copy link
Member

@ungureanuvladvictor ungureanuvladvictor left a comment

Choose a reason for hiding this comment

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

Reviewed 20 of 20 files at r7.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@aanm aanm merged commit 0d7533f into master Feb 27, 2020
1.8.0 automation moved this from In progress to Merged Feb 27, 2020
@aanm aanm deleted the pr/clean-operator-flags branch February 27, 2020 07:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/cleanup This includes no functional changes. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
No open projects
1.8.0
  
Merged
Development

Successfully merging this pull request may close these issues.

None yet

4 participants