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

Expose MCM logs to the end-user #382

Merged

Conversation

vlvasilev
Copy link
Contributor

How to categorize this PR?

/area logging
/kind enhancement
/platform aws

What this PR does / why we need it:
With this PR we allow end-users to see the MCM logs.
The logs will be visible after the merge of g/g#4387

Which issue(s) this PR fixes:
Рartially fixes g/g#4340

Special notes for your reviewer:

Release note:

machine-controller-manager logs are exposed to the end-users

@vlvasilev vlvasilev requested review from a team as code owners July 20, 2021 12:48
@gardener-robot gardener-robot added area/logging Logging related kind/enhancement Enhancement, improvement, extension platform/aws Amazon web services platform/infrastructure needs/review Needs review size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py) labels Jul 20, 2021
@gardener-robot-ci-2 gardener-robot-ci-2 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 20, 2021
@dkistner
Copy link
Member

Does this PR depend on gardener/gardener#4387?
Or is it just without this merged and release in g/g the logs from mcm can't be displayed?

So far it looks good to me, but I'm not an expert in this area so would appreciate an opinion from @Kristian-ZH @istvanballok
/invite @Kristian-ZH @istvanballok

@vlvasilev
Copy link
Contributor Author

Does this PR depend on gardener/gardener#4387?
Or is it just without this merged and release in g/g the logs from mcm can't be displayed?

This PR does not depend on gardener/gardener#4387.
The log will be stored in the Loki under user tenant but will be not visible until the above PR is merged.

@ialidzhikov ialidzhikov added the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 22, 2021
@gardener-robot-ci-1 gardener-robot-ci-1 removed the reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) label Jul 22, 2021
dkistner
dkistner previously approved these changes Jul 22, 2021
Copy link
Member

@dkistner dkistner left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold due to ongoing release validation
cc @ialidzhikov

@gardener-robot gardener-robot added reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Jul 22, 2021
@ialidzhikov
Copy link
Member

/invite @Kristian-ZH @istvanballok
for review

@gardener-robot gardener-robot added needs/review Needs review and removed reviewed/lgtm Has approval for merging labels Jul 22, 2021
@gardener-robot
Copy link

@Kristian-ZH You have pull request review open invite, please check

@Kristian-ZH
Copy link

I see that you have created a dedicated rewrite_tag filter for the MCM, which is different approach than the previous (ccm, csi-driver), where all components were in one filter.

If we want to follow the new approach, maybe the CCM and the CSI-driver also should be splitted in two different filters.

@Kristian-ZH
Copy link

Also this PR: gardener/gardener#4387 is still not released.
If we want to keep the backwards-compatibility (older version of Gardener combined with newer version of this extension) we can also keep the old logic of exposing components for a few more releases.

@vlvasilev
Copy link
Contributor Author

Also this PR: gardener/gardener#4387 is still not released.
If we want to keep the backwards-compatibility (older version of Gardener combined with newer version of this extension) we can also keep the old logic of exposing components for a few more releases.

Who said that I had removed the old logic?

I see that you have created a dedicated rewrite_tag filter for the MCM, which is different approach than the previous (ccm, csi-driver), where all components were in one filter.

The filter will become a lot of complex and hard to read so I decided to separate it.

@Kristian-ZH
Copy link

Who said that I had removed the old logic?

Opps, my bad...

The filter will become a lot of complex and hard to read so I decided to separate it.

Then what about creating a different filters for CCM and CSI-driver also. E.g. Each component to have its own filter ?

@vlvasilev
Copy link
Contributor Author

Then what about creating a different filters for CCM and CSI-driver also. E.g. Each component to have its own filter ?

You are welcome to do it if you wish. I have no time for that.

@vlvasilev vlvasilev force-pushed the expose-MCM-logs-to-the-end-user branch from d4615ad to 7093f74 Compare July 23, 2021 13:53
@gardener-robot-ci-1 gardener-robot-ci-1 added reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) and removed reviewed/ok-to-test Has approval for testing (check PR in detail before setting this label because PR is run on CI/CD) labels Jul 23, 2021
@vlvasilev vlvasilev requested a review from dkistner July 23, 2021 14:05
@gardener-robot
Copy link

@Kristian-ZH You have pull request review open invite, please check

Copy link
Member

@vpnachev vpnachev left a comment

Choose a reason for hiding this comment

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

/lgtm

@gardener-robot gardener-robot added reviewed/lgtm Has approval for merging and removed needs/review Needs review labels Aug 3, 2021
@vpnachev vpnachev merged commit 4642948 into gardener:master Aug 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/logging Logging related kind/enhancement Enhancement, improvement, extension needs/ok-to-test Needs approval for testing (check PR in detail before setting this label because PR is run on CI/CD) platform/aws Amazon web services platform/infrastructure reviewed/do-not-merge Has no approval for merging as it may break things, be of poor quality or have (ext.) dependencies reviewed/lgtm Has approval for merging size/xs Size of pull request is tiny (see gardener-robot robot/bots/size.py)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Expose logs of machine-controller-manager to end users
8 participants