Skip to content

Update the rootFses to be an array instead of a map#113

Closed
ivo1116 wants to merge 1 commit intocloudfoundry:mainfrom
ivo1116:Fix-incompatible-changes-introduced-with-diego-release-2.109.0
Closed

Update the rootFses to be an array instead of a map#113
ivo1116 wants to merge 1 commit intocloudfoundry:mainfrom
ivo1116:Fix-incompatible-changes-introduced-with-diego-release-2.109.0

Conversation

@ivo1116
Copy link
Copy Markdown
Contributor

@ivo1116 ivo1116 commented Jan 31, 2025

Summary

The PR makes sure the FSes are always used in a consistent way. The map was removed in favour of an array. The FSes should be stored in ascending order, ie. [ cflinuxfs3, cflinuxfs4 ]

The github issue: cloudfoundry/diego-release#983

Backward Compatibility

Breaking Change? No
No breaking changes, just making sure the fses are used in a consistent way

@ivo1116 ivo1116 requested a review from a team as a code owner January 31, 2025 09:42
Copy link
Copy Markdown
Member

@ameowlia ameowlia left a comment

Choose a reason for hiding this comment

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

❓ are there any tests to show that it is now returning the rootFSes in order?

gardenHealthcheckRootFS = rootFSPath
break
if len(rootFSes) > 0 {
gardenHealthcheckRootFS = rootFSes[len(rootFSes)-1]
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This code changes the behavior (I think) from before.

Before it looks like gardenHealthcheckRootFS was set to the first rootFSPath. Now it looks like it is being set to the last last rootFSPath.

❓ What is the intention behind this change? Is there a test change to go with it?

@ameowlia
Copy link
Copy Markdown
Member

Hi @ivo1116 , I made two notes, please ping me on CFF slack (I am @ameowlia there as well) when you reply. Github notifications often get lost in the noise.

@ivo1116
Copy link
Copy Markdown
Contributor Author

ivo1116 commented Feb 3, 2025

Hello @ameowlia,

The change does not modify the behaviour, since it was inconsistent anyway.
ie. assuming we have multiple stacks: fs3, fs4, and they are in this exact order
the previous behaviour would take the first one, but since it was taking from a map, the behaviour was inconsistent, returning on random one of the stacks

gardenHealthcheckRootFS = rootFSPath

The new implementation uses an array and thus we can rely on a consistent behaviour in terms of the positioning of the FSes. We take the last one, since we want to preserve the current ordering and to exclude the FS3 from the list of candidates for sidecar processes like healthcheck and envoy, since it does not work with the latest envoy binary.

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

Labels

None yet

Projects

Development

Successfully merging this pull request may close these issues.

2 participants