Skip to content

Fix CloudFoundry tagger collector#8644

Merged
zippolyte merged 1 commit intomainfrom
hippo/fixcf
Jul 20, 2021
Merged

Fix CloudFoundry tagger collector#8644
zippolyte merged 1 commit intomainfrom
hippo/fixcf

Conversation

@zippolyte
Copy link
Copy Markdown
Contributor

What does this PR do?

Fixes container collection in cloud foundry

Motivation

@zippolyte zippolyte added this to the 7.30.0 milestone Jul 19, 2021
@zippolyte zippolyte requested a review from a team as a code owner July 19, 2021 14:44
Comment on lines -34 to -36
if !config.IsFeaturePresent(config.CloudFoundry) {
return NoCollection, nil
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Any reason why this didn't work as expected? Should we rather fix detectCloudFoundry() ? (just curious about the bug, we can ship this fix as is in 7.30 and fix detectCloudFoundry in 7.31 ofc)

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This tests the connection to the socket of the garden process. But if it's not started yet at the time the agent starts, then we skip collection and there's no retry ever. The correct logic is in the cloudfoundry.GetGardenUtil() function just below this, that returns a retriable error.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation - in any case let's backlog something about this to fix detectCloudFoundry in 7.31 🙇

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we expect the socket to exist even if Garden process is not started yet?
It could be easily modified in detectCloudFoundry, otherwise it's useless and should be removed

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

No the socket won't exist if the garden process is not started yet.
What's the use case for the detectCloudFoundry given we already have the Detect method ?

Copy link
Copy Markdown
Contributor Author

@zippolyte zippolyte Jul 20, 2021

Choose a reason for hiding this comment

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

Ah ! Then we should be able to check the config option cloud_foundry to check if we're even in a cloud foundry environment

config.BindEnvAndSetDefault("cloud_foundry", false)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

cool thanks! a proper fix would be simple then 💯

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Should I make it for 7.30 instead of this quick and dirty one ?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Ideally speaking we would want to auto set cloud_foundry based on some system/environment/files/whatever that the Agent could auto-detect (that's the purpose of detectCloudFoundry).
If not possible we can reverse it and use cloud_foundry as an indicator to activate the Cloud Foundry features.

I think it'd be better to fix it in 7.30.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Well that flag in the config is the detector then. It's automatically set in the configuration when the agent is installed in a cloud foundry environment.
I'll make a PR

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants