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: fix bugs on reading configuration from config-map #10520

Merged
merged 5 commits into from Mar 12, 2020
Merged

operator: fix bugs on reading configuration from config-map #10520

merged 5 commits into from Mar 12, 2020

Conversation

aanm
Copy link
Member

@aanm aanm commented Mar 9, 2020 •
edited

cobra.OnInitialize should be called directly in the init function.

Due refactoring, cilium-agent --version and cilium-operator --version no longer work, this PR also fixes this bug by rewriting the code.

Also, stop executing flag validation if flags are not set

Fixes: 67b147c ("operator: add ability to ready options from a config directory")
Signed-off-by: André Martins andre@cilium.io

Fixes #10384

This change is Reviewable

@aanm aanm added pending-review area/operator Impacts the cilium-operator component release-note/misc This PR makes changes that have no direct user impact. labels Mar 9, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.8.0 Mar 9, 2020
@aanm aanm added wip and removed pending-review labels Mar 9, 2020
@aanm aanm requested a review from a team March 9, 2020 22:57
@aanm aanm changed the title operator: read options from config-map operator: fix bugs on reading configuration from config-map Mar 9, 2020
@aanm aanm added pending-review and removed wip labels Mar 9, 2020
@coveralls
Copy link

coveralls commented Mar 9, 2020 •
edited

Coverage Status

Coverage increased (+0.0006%) to 45.647% when pulling 72a6c02 on pr/fix-operator-read-config into f1f29c0 on master.

@aanm aanm added kind/bug This is a bug in the Cilium logic. pending-review and removed wip labels Mar 9, 2020
@aanm
Copy link
Member Author

aanm commented Mar 9, 2020

test-me-please

@aanm aanm force-pushed the pr/fix-operator-read-config branch from f80e7eb to 5eff360 Compare March 9, 2020 23:30
@aanm
Copy link
Member Author

aanm commented Mar 9, 2020

test-me-please

@aanm aanm force-pushed the pr/fix-operator-read-config branch from 5eff360 to 36c62ae Compare March 10, 2020 09:30
@aanm
Copy link
Member Author

aanm commented Mar 10, 2020

test-me-please

@aanm
Copy link
Member Author

aanm commented Mar 10, 2020 •
edited

test-me-please previous failure https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/17825/execution/node/179

@aanm
Copy link
Member Author

aanm commented Mar 10, 2020

test-me-please

@aanm aanm force-pushed the pr/fix-operator-read-config branch from 36c62ae to ebf522d Compare March 10, 2020 22:54
@aanm
Copy link
Member Author

aanm commented Mar 10, 2020

test-me-please

1 similar comment
@aanm
Copy link
Member Author

aanm commented Mar 11, 2020

test-me-please

@aanm aanm force-pushed the pr/fix-operator-read-config branch from ebf522d to 5f4e28f Compare March 11, 2020 10:13
@aanm
Copy link
Member Author

aanm commented Mar 11, 2020

test-me-please

tklauser
tklauser reviewed Mar 11, 2020
View reviewed changes
Copy link
Member

@tklauser tklauser left a comment

Choose a reason for hiding this comment

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

LGTM in general.

Noticed that operator now has two init functions one of which (in operator/flags.go) calls cobra.OnInitialize(option.InitConfig...)hooking up the handling of--versionand the other (inoperator/main.go) handles --versionby itself. Would it be possible to somehow merge the two so we can a centralized handling of--versioninInitConfig`?

@aanm aanm force-pushed the pr/fix-operator-read-config branch from 5f4e28f to 219b81f Compare March 12, 2020 02:13
@aanm
Copy link
Member Author

aanm commented Mar 12, 2020 •
edited by tklauser

test-me-please

previous failure: https://jenkins.cilium.io/job/Cilium-PR-Ginkgo-Tests-Validated/17871/

@aanm aanm requested a review from tklauser March 12, 2020 02:13
@tklauser
Copy link
Member

test-me-please

@aanm
operator: read options from config-map
d049557 
cobra.OnInitialize should be called directly in the init function.

Fixes: 67b147c ("operator: add ability to ready options from a config directory")
Signed-off-by: André Martins <andre@cilium.io>
@aanm
operator/cilium: only print version with --version
e0a5ecf 
The programs should be executed when running with `--version` and should
only print the version and terminate immediately.

Fixes: 67b147c ("operator: add ability to ready options from a config directory")
Signed-off-by: André Martins <andre@cilium.io>
@aanm
pkg/option: ignore warnings if flags are not set
2d7a8fc 
Signed-off-by: André Martins <andre@cilium.io>
@aanm
test: delete cilium operator deployment
4f32b64 
As cilium operator now relies in the config map changes, we need to
restart it the same way as cilium agent so that it can pick up new
changes from the cilium-config `config-map`.

Signed-off-by: André Martins <andre@cilium.io>
@aanm
test: fix message with number of nodes running cilium
72a6c02 
Fixes: 622dae3 ("Test: Added Pre flight checks on ExpectCiliumReady")
Signed-off-by: André Martins <andre@cilium.io>
@aanm aanm force-pushed the pr/fix-operator-read-config branch from 219b81f to 72a6c02 Compare March 12, 2020 11:35
@aanm aanm requested a review from a team as a code owner March 12, 2020 11:35
houndci-bot
houndci-bot reviewed Mar 12, 2020
View reviewed changes
test/helpers/kubectl.go
@@ -1493,6 +1493,43 @@ func (kub *Kubectl) GetPublicIface() (string, error) {
return kub.getIfaceByIPAddr(K8s1, ipAddr)
}

func (kub *Kubectl) DeleteCiliumOperator() error {
Copy link

Choose a reason for hiding this comment

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

exported method Kubectl.DeleteCiliumOperator should have comment or be unexported

@aanm
Copy link
Member Author

aanm commented Mar 12, 2020

test-me-please

nebril
nebril requested changes Mar 12, 2020
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.

One small comment, lgtm otherwise

test/helpers/kubectl.go

select {
case <-ctx.Done():
return fmt.Errorf("timed out waiting to delete Cilium Operator: pods still remaining: %s", pods)
Copy link
Member

Choose a reason for hiding this comment

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

pods will be uninitialized here on first run, probably won't ever happen, but can we move whole select to the end of the loop body? I assume we want to run through the loop body at least once.

Copy link
Member Author

Choose a reason for hiding this comment

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

@nebril I'm going to fix this in another PR as this PR is blocking some of my work.

@aanm aanm merged commit a734d81 into master Mar 12, 2020
1.8.0 automation moved this from In progress to Merged Mar 12, 2020
@aanm aanm deleted the pr/fix-operator-read-config branch March 12, 2020 15:23
@aanm aanm mentioned this pull request Mar 12, 2020
Merged
@brb brb mentioned this pull request Mar 13, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@houndci-bot houndci-bot houndci-bot left review comments

@nebril nebril nebril requested changes

@tklauser tklauser tklauser approved these changes

Assignees
No one assigned
Labels
area/operator Impacts the cilium-operator component kind/bug This is a bug in the Cilium logic. release-note/misc This PR makes changes that have no direct user impact.
Projects
No open projects
1.8.0
  
Merged
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cilium-operator prints warnings for non existing flags in cilium-operator >= 1.8
5 participants
@aanm @coveralls @tklauser @nebril @houndci-bot