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 K3S start command #7677

Merged
merged 5 commits into from
Oct 24, 2023
Merged

Fix K3S start command #7677

merged 5 commits into from
Oct 24, 2023

Conversation

tgeens
Copy link
Contributor

@tgeens tgeens commented Oct 17, 2023

This PR fixes #6770 by swapping the deprecated/removed --no-deploy flag with the replacement flag --disable

This change is backwards compatible and tested with a wide range of versions:

  • oldest version succesfully tested: rancher/k3s:v1.17.17-k3s1
  • current version (updated in tests): rancher/k3s:v1.25.14-k3s1

See:

@tgeens tgeens requested a review from a team as a code owner October 17, 2023 08:33
@eddumelendez
Copy link
Member

Thanks for your contribution, @tgeens ! Can we add a test with rancher/k3s:v1.17.17-k3s1 as a proof of being backward compatible, please?

@eddumelendez eddumelendez added this to the next milestone Oct 17, 2023
@eddumelendez eddumelendez changed the title Fixes #6770: k3s >= 1.24 will not start Fix K3S start command Oct 17, 2023
eddumelendez
eddumelendez reviewed Oct 18, 2023
View reviewed changes
Copy link
Member

@eddumelendez eddumelendez left a comment

Choose a reason for hiding this comment

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

I've left a few comments/suggestions. After that I'll be merging the PR. Thanks again!.

modules/k3s/src/test/java/org/testcontainers/k3s/Fabric8K3sContainerTest.java Outdated Show resolved Hide resolved
modules/k3s/src/test/java/org/testcontainers/k3s/OfficialClientK3sContainerTest.java Outdated Show resolved Hide resolved
eddumelendez
eddumelendez previously approved these changes Oct 20, 2023
View reviewed changes
@eddumelendez
Copy link
Member

eddumelendez commented Oct 20, 2023 •
edited

Hi @tgeens, looks like the wait strategy fails using rancher/k3s:v1.17.17-k3s1. Can you take a look, please?

@github-actions github-actions bot added the github_actions Pull requests that update Github_actions code label Oct 23, 2023
eddumelendez
eddumelendez requested changes Oct 23, 2023
View reviewed changes
.github/workflows/ci.yml Outdated
@@ -17,7 +17,7 @@ on:
- 'RELEASING.md'
- '.sdkmanrc'
push:
branches: [ main ]
branches: [ main, k3s-disable-flag ]
Copy link
Member

Choose a reason for hiding this comment

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

this is not needed

modules/k3s/src/main/java/org/testcontainers/k3s/K3sContainer.java Outdated
setWaitStrategy(
new LogMessageWaitStrategy()
.withRegEx(".*Node controller sync successful.*")
.withStartupTimeout(Duration.ofMinutes(5))
Copy link
Member

Choose a reason for hiding this comment

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

so, 5 min is required? The max should be 1 min for good experience.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, this were my own tests, I should have done these experiments on a separate branch, not on this PR.

I'm trying to figure out why local ./gradlew :k3s:test works just fine locally, but not on GHA

@github-actions github-actions bot removed the github_actions Pull requests that update Github_actions code label Oct 23, 2023
@tgeens
Copy link
Contributor Author

tgeens commented Oct 24, 2023 •
edited

Before this PR, GHA builds/tests already fail for the following versions:

  • v1.17.17-k3s1
  • v1.18.20-k3s1
  • v1.19.16-k3s1

Note that it works locally with ./gradlew test just fine - and I could not figure out why.

The oldest (minor) version that succeeds with GHA builds is v1.20.15-k3s1

@eddumelendez
Copy link
Member

Thanks for the information, @tgeens! For me even locally fails with the versions you mentioned on M1. It would have been great to test against the minimal version but given our examples were using rancher/k3s:v1.21.3-k3s1, I'm ok with merging this.

@eddumelendez eddumelendez merged commit 0c52c41 into testcontainers:main Oct 24, 2023
87 checks passed
@eddumelendez
Copy link
Member

Thanks for your contribution, @tgeens ! This is now merged in main branch and it will be part of the next release.

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

@eddumelendez eddumelendez eddumelendez approved these changes

Assignees
No one assigned
Labels
modules/k3s type/bug
Projects
None yet
Milestone
1.19.2
Development

Successfully merging this pull request may close these issues.

[Bug]: k3s > 1.24.x will not start
2 participants
@tgeens @eddumelendez