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

clustermesh-apiserver: fix cmd-line args processing #18277

Merged
merged 1 commit into from
Jan 31, 2022

Conversation

abocim
Copy link
Contributor

@abocim abocim commented Dec 16, 2021

This change fixes commandline arguments processing of clustermesh-apiserver.

Commandline arguments accessed through the option.Config object
(eg. --identity-allocation-mode, --kvstore-opt) were not processed properly.
Function option.Config.Populate() was called too soon (before calling of
rootCmd.Execute(), but os.Args are processed just by rootCmd.Execute()).

Also there was missing debug log-level setting so debug messages did not work at all.

Signed-off-by: Adam Bocim adam.bocim@seznam.cz

@abocim abocim requested a review from a team as a code owner December 16, 2021 13:27
@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 Dec 16, 2021
@gandro gandro added area/clustermesh Relates to multi-cluster routing functionality in Cilium. release-note/bug This PR fixes an issue in a previous release of Cilium. labels Dec 21, 2021
@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 Dec 21, 2021
@jrajahalme
Copy link
Member

/test

@jrajahalme
Copy link
Member

@abocim Thanks for the fix. Could you please rebase this PR to the current Cilium master? There is a go import order related change that makes Go-related checks / generate-api test fail, I think.

@abocim
Copy link
Contributor Author

abocim commented Jan 10, 2022

My pleasure. Ok, I'm going to rebase it.

@abocim abocim force-pushed the master branch 2 times, most recently from 9647f9e to 2965c88 Compare January 11, 2022 07:18
@jrajahalme
Copy link
Member

jrajahalme commented Jan 13, 2022

/test

Job 'Cilium-PR-K8s-1.21-kernel-5.4' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sUpdates Tests upgrade and downgrade from a Cilium stable image to master

Failure Output

FAIL: Timed out after 240.000s.

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.21-kernel-5.4 so I can create a new GitHub issue to track it.

@abocim
Copy link
Contributor Author

abocim commented Jan 13, 2022

I can't see any relation with my PR. If I understand it right, the failed test could not start up cilium-agents (they are in CrashLoopBackOff). There is no clustermesh-apiserver used in the test at all.

After last rebase my fork is 26 commits behind upstream again, so I can try rebase it once more.

@jrajahalme what do you think about that?

@abocim
Copy link
Contributor Author

abocim commented Jan 13, 2022

Maybe there was an k8s related problem. I can see coredns container has been still starting within the test:

NAMESPACE           NAME                              READY   STATUS              RESTARTS      AGE     IP              NODE   NOMINATED NODE   READINESS GATES
cilium-monitoring   grafana-5747bcc8f9-dqjk7          1/1     Running             0             115m    10.0.0.134      k8s1   <none>           <none>
cilium-monitoring   prometheus-655fb888d7-nhknh       1/1     Running             0             115m    10.0.0.218      k8s1   <none>           <none>
kube-system         cilium-mj9hq                      0/1     CrashLoopBackOff    6 (77s ago)   7m46s   192.168.56.12   k8s2   <none>           <none>
kube-system         cilium-operator-99744ffb8-2vm7m   1/1     Running             0             7m46s   192.168.56.11   k8s1   <none>           <none>
kube-system         cilium-operator-99744ffb8-dnhq2   1/1     Running             0             7m46s   192.168.56.12   k8s2   <none>           <none>
kube-system         cilium-znnvx                      0/1     CrashLoopBackOff    6 (96s ago)   7m46s   192.168.56.11   k8s1   <none>           <none>
kube-system         coredns-69b675786c-577hc          0/1     ContainerCreating   0             17m     <none>          k8s2   <none>           <none>
...

@jrajahalme
Copy link
Member

I can't see any relation with my PR. If I understand it right, the failed test could not start up cilium-agents (they are in CrashLoopBackOff). There is no clustermesh-apiserver used in the test at all.

After last rebase my fork is 26 commits behind upstream again, so I can try rebase it once more.

@jrajahalme what do you think about that?

Right, these fails are not related to this change at all.

@abocim
Copy link
Contributor Author

abocim commented Jan 17, 2022

@jrajahalme Is there any possibility to run tests myself?

@jrajahalme
Copy link
Member

@abocim Unfortunately our CI has been flaky lately, and Ginkgo test/unit test fixes merged to master need to be picked up by rebasing to current master. Latest fixes were merged one hour ago (#18506), could you rebase once more? Thanks!

Most checks should trigger automatically once your 1st PR gets merged, I think.

…server

Commandline arguments accessed through the option.Config object
(eg. --identity-allocation-mode, --kvstore-opt) were not processed properly.
Function option.Config.Populate() was called too soon (before calling of
rootCmd.Execute(), but os.Args are processed just by rootCmd.Execute()).

Also there was missing debug log-level setting so debug messages did not work at all.

Signed-off-by: adam.bocim <adam.bocim@firma.seznam.cz>
@abocim
Copy link
Contributor Author

abocim commented Jan 18, 2022

@jrajahalme Rebased :-)

@aanm
Copy link
Member

aanm commented Jan 19, 2022

/test

Job 'Cilium-PR-K8s-1.22-kernel-4.19' failed and has not been observed before, so may be related to your PR:

Click to show.

Test Name

K8sServicesTest Checks graceful termination of service endpoints Checks client terminates gracefully on service endpoint deletion

Failure Output

FAIL: Timed out after 60.000s.

If it is a flake, comment /mlh new-flake Cilium-PR-K8s-1.22-kernel-4.19 so I can create a new GitHub issue to track it.

@glibsm glibsm merged commit d76a06b into cilium:master Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/clustermesh Relates to multi-cluster routing functionality in Cilium. backport-done/1.11 The backport for Cilium 1.11.x for this PR is done. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants