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.

Sign up for GitHub

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

Jump to bottom

fix(*) only mount logs directory and use a different prefix #474

Merged
merged 3 commits into from
Dec 14, 2023
Merged

fix(*) only mount logs directory and use a different prefix #474

merged 3 commits into from
Dec 14, 2023

Conversation

fffonion
Copy link
Contributor

@fffonion fffonion commented Nov 24, 2023 •
edited by Tieske
Loading

Pongo mounts /kong-plugin to current folder and use /kong-plugin/servroot as default prefix
so that developer can easily access logs. On macOS, such directory is implemented as VM file shared
and doesn't support binding a unix domain socket; on windows, if current directory from the 9p fs shared
from Windows volumes, binding unix domain socket is also not supported.

This fix makes only $KONG_PREFIX/logs a mount from host on such platform, thus makes Kong able to start.
This is considered a short term fix until Kong implemented to move UDS sockets to a subdirectory. By then,
the whole $KONG_PREFIX can be mounted again, while the sockets subdirectory will become a symlink.

KAG-5588
KAG-3284

@Tieske
Copy link
Member

Tieske commented Nov 24, 2023

where are the unix sockets currently located? At the top of the prefix?

Tieske
Tieske reviewed Nov 24, 2023
View reviewed changes
Copy link
Member

@Tieske Tieske left a comment

Choose a reason for hiding this comment

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

Am I correct we loose access to everything but the logs? eg. no way to inspect the generated nginx config files from the host?

assets/docker-compose.yml Outdated Show resolved Hide resolved
@fffonion
Copy link
Contributor Author

where are the unix sockets currently located? At the top of the prefix?

yes

Am I correct we loose access to everything but the logs? eg. no way to inspect the generated nginx config files from the host?

yes, others are not being re-mounted or symlinked in this PR. As from the comments that logs are the reason we use prefix from mount at first place, so that is being implemented here as first iterration.

@fffonion fffonion mentioned this pull request Dec 1, 2023
Merged
@fffonion fffonion marked this pull request as ready for review December 5, 2023 05:59
@fffonion fffonion force-pushed the only-mount-logs branch 3 times, most recently from 08cd14d to 29b91ef Compare December 5, 2023 08:17
Tieske
Tieske requested changes Dec 5, 2023
View reviewed changes
Copy link
Member

@Tieske Tieske left a comment

Choose a reason for hiding this comment

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

somehow I still see the files appear on the mount.

pongo.sh Outdated Show resolved Hide resolved
@Tieske
Copy link
Member

Tieske commented Dec 5, 2023

somehow I still see the files appear on the mount.

That's because I used the old containers. pongo nuke to remove all of them solved it.

@fffonion fffonion force-pushed the only-mount-logs branch 2 times, most recently from 1e522d4 to 2b7e889 Compare December 6, 2023 07:07
@fffonion fffonion requested a review from Tieske December 13, 2023 07:36
outsinre
outsinre approved these changes Dec 13, 2023
View reviewed changes
assets/docker-compose-linux-extend.yml
services:
kong:
volumes:
- ${PONGO_WD}/servroot:/kong-prefix
Copy link
Contributor

Choose a reason for hiding this comment

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

Conflicts with https://github.com/Kong/kong-pongo/blob/master/assets/docker-compose.yml#L171-L172?

Copy link
Member

Choose a reason for hiding this comment

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

@fffonion mind having a look? I think we can remove the one from the main docker compose file no?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

actually no conflict here, the mount from main file is for getting in the plugin code for test.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

pwd -> /kong-plugin
pwd/servroot -> /kong-prefix
we can't do
pwd/servroot -> /kong-plugin/servroot
anymore because that will be a bind mount on host and not supported on macos

@Tieske
Copy link
Member

Tieske commented Dec 14, 2023

@fffonion can you squash the commits first, and then merge (squashing via ui is disabled)

@Tieske
Copy link
Member

Tieske commented Dec 14, 2023

nvm I'll do it

@fffonion
Copy link
Contributor Author

Thank you @Tieske

@Tieske Tieske merged commit 8660faf into master Dec 14, 2023
5 checks passed
@Tieske Tieske deleted the only-mount-logs branch December 14, 2023 08:47
fffonion added a commit that referenced this pull request Dec 15, 2023
@fffonion
Revert "fix(*) only mount logs directory and use a different prefix (#…
a3adc0c 
…474)"

This reverts commit 8660faf.
@fffonion fffonion mentioned this pull request Dec 15, 2023
Merged
Tieske pushed a commit that referenced this pull request Dec 15, 2023
@fffonion @Tieske
Revert "fix(*) only mount logs directory and use a different prefix (#…
20755b4 
…474)"

This reverts commit 8660faf.
Tieske pushed a commit that referenced this pull request Dec 15, 2023
@fffonion @Tieske
fix(*) only mount logs directory and use a different prefix (#474)
e518c9d
Tieske pushed a commit that referenced this pull request Dec 18, 2023
@fffonion @Tieske
fix(*) only mount logs directory and use a different prefix (#474)
9406f34
Tieske pushed a commit that referenced this pull request Jan 30, 2024
@fffonion @Tieske
fix(*) only mount logs directory and use a different prefix (#474)
b8d16b8
Tieske pushed a commit that referenced this pull request Feb 7, 2024
@fffonion @Tieske
fix(*) only mount logs directory and use a different prefix (#474)
cfe2871
Tieske pushed a commit that referenced this pull request Feb 12, 2024
@fffonion @Tieske
fix(*) only mount logs directory and use a different prefix (#474)
baab7a9
Tieske pushed a commit that referenced this pull request Feb 12, 2024
@fffonion @Tieske
fix(*) only mount logs directory and use a different prefix (#474)
9b9650d
Tieske pushed a commit that referenced this pull request Feb 12, 2024
@fffonion @Tieske
fix(*) only mount logs directory and use a different prefix (#474)
d14f2ea
Tieske pushed a commit that referenced this pull request Feb 12, 2024
@fffonion @Tieske
fix(*) only mount logs directory and use a different prefix (#474)
ae17151
Tieske pushed a commit that referenced this pull request Jul 9, 2024
@fffonion @Tieske
fix(*) only mount logs directory and use a different prefix (#474)
7aa4168
Tieske pushed a commit that referenced this pull request Jul 17, 2024
@fffonion @Tieske
fix(*) only mount logs directory and use a different prefix (#474)
49d8f92
Tieske pushed a commit that referenced this pull request Aug 9, 2024
@fffonion @Tieske
fix(*) only mount logs directory and use a different prefix (#474)
beb51dc
SomnolentKalash pushed a commit to razorpay/kong-pongo that referenced this pull request Sep 25, 2024
@fffonion @SomnolentKalash
fix(*) only mount logs directory and use a different prefix (Kong#474)
a3b3312 
(cherry picked from commit 8660faf)
SomnolentKalash pushed a commit to razorpay/kong-pongo that referenced this pull request Sep 25, 2024
@fffonion @SomnolentKalash
Revert "fix(*) only mount logs directory and use a different prefix (K…
77eb8cd 
…ong#474)"

This reverts commit 8660faf.

(cherry picked from commit 20755b4)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@outsinre outsinre outsinre approved these changes

@Tieske Tieske Tieske approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@fffonion @Tieske @outsinre