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

Fixing Hubble ServiceMonitor k8s-app label #14473

Merged
merged 1 commit into from
Dec 28, 2020

Conversation

guilhermef
Copy link
Contributor

@guilhermef guilhermef commented Dec 22, 2020

The current ServiceMonitor is trying to match a k8s-app=cilium, but the current service has k8s-app=hubble.
https://github.com/cilium/cilium/blob/master/install/kubernetes/cilium/templates/cilium-agent-service.yaml#L56

Please ensure your pull request adheres to the following guidelines:

  • For first time contributors, read Submitting a pull request
  • All code is covered by unit and/or runtime tests where feasible.
  • All commits contain a well written commit description including a title,
    description and a Fixes: #XXX line if the commit addresses a particular
    GitHub issue.
  • All commits are signed off. See the section Developer’s Certificate of Origin
  • Provide a title or release-note blurb suitable for the release notes.
  • Thanks for contributing!

Fixes: #11886

@guilhermef guilhermef requested review from a team as code owners December 22, 2020 13:30
@maintainer-s-little-helper

This comment has been minimized.

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels Dec 22, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot added this to In progress in 1.10.0 Dec 22, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label Dec 22, 2020
@guilhermef guilhermef force-pushed the fix-hubble-service-monitor branch 2 times, most recently from e855877 to 0705d3e Compare December 22, 2020 13:44
@gandro gandro added the release-note/bug This PR fixes an issue in a previous release of Cilium. label Dec 22, 2020
@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 22, 2020
@gandro gandro added the area/helm Impacts helm charts and user deployment experience label Dec 22, 2020
Copy link
Member

@fristonio fristonio left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for pointing this out 💯

Copy link
Member

@gandro gandro left a comment

Choose a reason for hiding this comment

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

Thanks! Marking for backport to 1.9, as this seems to have regressed in the Great Helm refactor for Cilium 1.9.

@maintainer-s-little-helper maintainer-s-little-helper bot added this to Needs backport from master in 1.9.2 Dec 22, 2020
@kaworu
Copy link
Member

kaworu commented Dec 22, 2020

Fixes: #11886

@guilhermef this is referencing a closed PR where I would have expected an open issue instead. Whas that intended?

@guilhermef
Copy link
Contributor Author

Fixes: #11886

@guilhermef this is referencing a closed PR where I would have expected an open issue instead. Whas that intended?

Yes, that's a regression, it was fixed on the referenced PR before

Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

Change looks good to me; a comment on the commit itself.

The current ServiceMonitor is trying to match a `k8s-app=cilium`, but the current service has `k8s-app=hubble`.
https://github.com/cilium/cilium/blob/master/install/kubernetes/cilium/templates/cilium-agent-service.yaml#L56

Signed-off-by: Guilherme Souza <guilhermef@users.noreply.github.com>

Fixes: e9cb43c ("Helm: full refactor of helm charts, default values implemented, tests updated, kind cni integration")
@christarazi
Copy link
Member

test-me-please

@PhilipSchmid
Copy link
Contributor

@guilhermef Thanks for your PR 🎉. I just run into the same issue and also found the wrong label matching - but you now took the work from me to create a PR - thanks!

Would be awesome to see this PR merged in the next release 1.9.2.

Regards,
Philip

@christarazi
Copy link
Member

net-next hit #13071. All other CI has passed and we have code owner approval. Merging.

@christarazi christarazi merged commit 2a3de6e into cilium:master Dec 28, 2020
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.2 Jan 6, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Needs backport from master to Backport pending to v1.9 in 1.9.2 Jan 6, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.2 Jan 14, 2021
@maintainer-s-little-helper maintainer-s-little-helper bot moved this from Backport pending to v1.9 to Backport done to v1.9 in 1.9.2 Jan 14, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Impacts helm charts and user deployment experience release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
1.9.2
Backport done to v1.9
Development

Successfully merging this pull request may close these issues.

None yet

8 participants