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

Replace pid with flock for runtime config loading #5435

Merged
merged 2 commits into from
Jan 22, 2025
Merged

Replace pid with flock for runtime config loading #5435

merged 2 commits into from
Jan 22, 2025

Conversation

ncopa
Copy link
Collaborator

@ncopa ncopa commented Jan 14, 2025

Use lock file and flock(2) to ensure there is only a single instance of k0s running. This is more reliable than storing the pid in the runtime config.

This also solves false positives with k0s runtime config leftovers.

Fixes: #5399

Description

Fixes #5399

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Documentation update

How Has This Been Tested?

  • Manual test
  • Auto test added

Checklist:

  • My code follows the style guidelines of this project
  • My commit messages are signed-off
  • I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules
  • I have checked my code and corrected any misspellings

@ncopa ncopa requested review from a team as code owners January 14, 2025 12:40
@ncopa ncopa requested review from kke and twz123 January 14, 2025 12:40
@ncopa ncopa mentioned this pull request Jan 14, 2025
Closed
16 tasks
@twz123
Copy link
Member

twz123 commented Jan 14, 2025

Shouldn't we leave it as is and eventually remove the whole runtime config file instead?

@ncopa
Copy link
Collaborator Author

ncopa commented Jan 14, 2025

Shouldn't we leave it as is and eventually remove the whole runtime config file instead?

how do you mean?

@github-actions GitHub Actions
Copy link
Contributor

This pull request has merge conflicts that need to be resolved.

@ncopa ncopa force-pushed the lock-config branch 2 times, most recently from b58069c to 92be067 Compare January 16, 2025 13:56
@ncopa
Copy link
Collaborator Author

ncopa commented Jan 16, 2025

Shouldn't we leave it as is and eventually remove the whole runtime config file instead?

That appears to be a fairly intrusive change. This is a cheap and non-intrusive way to fix the specific problem at hand that easily can be backported.

twz123
twz123 reviewed Jan 16, 2025
View reviewed changes
internal/pkg/flock/flock_windows.go Outdated Show resolved Hide resolved
internal/pkg/flock/flock_windows.go Outdated Show resolved Hide resolved
pkg/config/runtime.go Show resolved Hide resolved
pkg/config/runtime.go Outdated Show resolved Hide resolved
pkg/config/runtime.go Outdated Show resolved Hide resolved
pkg/config/runtime_test.go Outdated Show resolved Hide resolved
pkg/config/runtime_test.go Outdated Show resolved Hide resolved
pkg/config/runtime_test.go Outdated Show resolved Hide resolved
internal/pkg/flock/flock_unix.go Outdated Show resolved Hide resolved
@github-actions GitHub Actions
Copy link
Contributor

This pull request has merge conflicts that need to be resolved.

@ncopa ncopa force-pushed the lock-config branch 4 times, most recently from 58b09ae to 4b0f2f1 Compare January 17, 2025 17:09
twz123
twz123 reviewed Jan 19, 2025
View reviewed changes
pkg/config/runtime.go Show resolved Hide resolved
pkg/config/runtime.go Show resolved Hide resolved
pkg/config/flock_unix.go Outdated Show resolved Hide resolved
pkg/config/runtime_test.go Outdated Show resolved Hide resolved
pkg/config/runtime_test.go Outdated Show resolved Hide resolved
pkg/config/runtime_test.go Outdated Show resolved Hide resolved
@ncopa ncopa force-pushed the lock-config branch 4 times, most recently from 9a20f31 to d5e2254 Compare January 20, 2025 16:52
@ncopa ncopa enabled auto-merge January 20, 2025 16:57
@twz123 twz123 disabled auto-merge January 21, 2025 11:12
@github-actions GitHub Actions
Copy link
Contributor

This pull request has merge conflicts that need to be resolved.

ncopa added 2 commits January 22, 2025 14:28
@ncopa
Replace pid with flock for runtime config loading
d5d06b0 
Use lock file and flock(2) to ensure there is only a single instance of
k0s running. This is more reliable than storing the pid in the runtime
config.

This solves false positives with k0s runtime config leftovers.

Fixes: k0sproject#5399
Signed-off-by: Natanael Copa <ncopa@mirantis.com>
@ncopa
Fix kubeconfig admin test
c45698d 
Since we removed the pid from runtime config we can no longer skip the
is-k0s-running-test by letting the pid be zero. So we need to create a
proper runtime lock file by using NewRuntimeConfig.

Signed-off-by: Natanael Copa <ncopa@mirantis.com>
@ncopa ncopa requested a review from twz123 January 22, 2025 13:37
@ncopa ncopa enabled auto-merge January 22, 2025 13:37
@ncopa ncopa merged commit 05df36d into k0sproject:main Jan 22, 2025
90 checks passed
@ncopa ncopa deleted the lock-config branch January 22, 2025 15:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@twz123 twz123 twz123 approved these changes

@kke kke Awaiting requested review from kke kke is a code owner automatically assigned from k0sproject/k0s-maintainers

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

Successfully merging this pull request may close these issues.

k0s error "an instance of k0s is already running" but it is not running
2 participants
@ncopa @twz123