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.

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

[upgrade] Bump minikube, k8s, PHP and Laravel versions #325

Merged
merged 22 commits into from
Apr 1, 2023
Merged

[upgrade] Bump minikube, k8s, PHP and Laravel versions #325

merged 22 commits into from
Apr 1, 2023

Conversation

cuppett
Copy link
Contributor

@cuppett cuppett commented Feb 21, 2023

Attempting to resolve issues with Laravel 10 support. Bumping various versions in the package.

This PR updates the following things:

  1. Kubernetes version 1.22 removed, 1.25 added
  2. Kubernetes 1.23->1.25 bumped to current minor versions
  3. Add Laravel 10
  4. Add PHP 8.2 to the testing matrix (Laravel 10 only)
  5. Fixes a couple small issues with some tests (optional kube features, image/registry expansion)
  6. Update PodDisruptionBudget default and tests to v1 (from v1beta1)
  7. Update HorizontalPodAutoscaler default and tests to v2 (from v2beta2)
  8. Update CronJob default and tests to v1 (from v1beta1)

Resolves #324

@cuppett
Copy link
Contributor Author

cuppett commented Feb 21, 2023

Looks like we can't add 1.26.1 without figuring out this with the github action (related to dockershim removal?):

stderr:
W0221 10:26:39.628460    5650 initconfiguration.go:119] Usage of CRI endpoints without URL scheme is deprecated and can cause kubelet errors in the future. Automatically prepending scheme "unix" to the "criSocket" with value "/var/run/cri-dockerd.sock". Please update your configuration!
	[WARNING Swap]: swap is enabled; production deployments should disable swap unless testing the NodeSwap feature gate of the kubelet
	[WARNING FileExisting-socat]: socat not found in system path
	[WARNING Service-Kubelet]: kubelet service is not enabled, please run 'systemctl enable kubelet.service'
error execution phase wait-control-plane: couldn't initialize a Kubernetes cluster
To see the stack trace of this error execute with --v=5 or higher

* Suggestion: Check output of 'journalctl -xeu kubelet', try passing --extra-config=kubelet.cgroup-driver=systemd to minikube start
* Related issue: https://github.com/kubernetes/minikube/issues/4172
Error: Command failed: sudo -E /home/runner/work/_temp/minikube start --vm-driver=none  --kubernetes-version v1.26.1 
    at checkExecSyncError (node:child_process:828:11)
    at Object.execSync (node:child_process:899:15)
    at logExecSync (/home/runner/work/_actions/manusa/actions-setup-minikube/v2.7.2/src/exec.js:8:17)
    at install (/home/runner/work/_actions/manusa/actions-setup-minikube/v2.7.2/src/install.js:25:3)
    at async run (/home/runner/work/_actions/manusa/actions-setup-minikube/v2.7.2/src/index.js:16:3) {
  status: 109,
  signal: null,
  output: [ null, null, null ],
  pid: 2989,
  stdout: null,
  stderr: null
}
Error: Command failed: sudo -E /home/runner/work/_temp/minikube start --vm-driver=none  --kubernetes-version v1.26.1  

@cuppett cuppett marked this pull request as draft February 21, 2023 11:44
@cuppett
Copy link
Contributor Author

cuppett commented Feb 21, 2023

Still working on this. Would like to investigate the following two things a bit more:

  1. Add back PHP 8.0 for Laravel 9 only
  2. Get 1.26.1 working

* Setting Kubernetes versions to current
* Adding Laravel 10
* Update location of sealed secrets CRD
* Add PHP 8.2
* Ensuring we match the supported PHP versions for the versions of Laravel.

See also: https://laravel.com/docs/10.x/releases#support-policy
The actual number of annotations added by the system could vary.
This test actually dependent on whether a feature gate is enabled or not.
Just test for the presence of our target annotations and ignore the rest.

See also: https://kubernetes.io/blog/2022/12/29/scalable-job-tracking-ga/#how-do-i-use-this-feature
Ensuring the image and version are checked, but not necessarily the resolution or substituions due to full qualification.

Saw the following error with KIND:

--- Expected
+++ Actual
@@ @@
-'mysql:5.7'
+'docker.io/library/mysql:5.7'
@cuppett cuppett marked this pull request as ready for review February 22, 2023 12:30
@cuppett
Copy link
Contributor Author

cuppett commented Feb 22, 2023

I don't know I'm familiar enough with how GitHub actions work to get the 1.26 minicube up and running, may also come for free if we just wait a couple weeks. I can carve that into a separate issue.

@Braunson
Copy link

Braunson commented Mar 9, 2023

Bumping this, looks ready to merge?

Attempting public.ecr.aws to avoid rate limit on pulls
Can't use an undefined value as an ARRAY reference at /usr/local/lib/perl5/5.36.0/Math/BigInt/Calc.pm line 1049.
You are using the deprecated option "--no-suggest". It has no effect and will break in Composer 3.
…iate

On KIND, the standard storageclass is there, but the binding
mode is WaitForFirstConsumer.
This lets the test suite progress through this test without issue when
we're working against hosts or clusters configured like that.
Annotations could get added by different admission plugins or API implementations.
No reason to check for empty set here, createOrUpdate() with an empty annotation array
won't actually remove any or guarantee that the array back after save is empty.
* Replace manusa/actions-setup-minikube with medyagh/setup-minikube (from minikube documentation)
* Explicit containerd runtime with dockershim removal
* Updated README
* Bump patch versions of current minor releases
Issue here likely due to speed of test versus timing in VM.
Trying to eliminate this timing issue in this test.
Unclear if 409s should be resolved in client code with a loop or in the library.
It cannot be guaranteed we get the returned values in the same lines.
This test was fragile for where additional empty lines (non-lines) were emitted.
Reducing to a single string of values and ensuring we get all the output.
Also, selectors and queries for tier=backend would pick up MySQL pods from other tests.
Updated annotation as "compute" instead of "backend" to keep this test separate.
…ng job

Previous change to simpler Pi calculation exposed timing issue with test.
Rather than make VM/CPU work harder or create potential for overflow again, sleep.
Then, fixed lookup logic.
@cuppett
Copy link
Contributor Author

cuppett commented Mar 25, 2023

This is ready to merge. I was also able to get the tests fully passing (required some fixes to tests and library) on the current Kubernetes versions (1.24->1.26).

The newest commits:

  1. Drop 1.23, Add 1.26
  2. Switch to using the GH action for minikube from their documentation
  3. Add a timeout to the suite
  4. Change pulls to explicitly pull from AWS public ECR (same images, avoid Docker rate limiting)
  5. Tolerate running the tests on local KIND (storageclass binding is different)
  6. Fixes flakes in some tests to improve the success rates

See test results here: https://github.com/cuppett/php-k8s/actions/runs/4520520534

@cuppett
Copy link
Contributor Author

cuppett commented Mar 25, 2023

@rennokki kindly see above and let me know if this needs anything else to get merged and into a version. Happy to help this project along also if you need beyond this ticket. Thanks!

Copy link
Contributor Author

@cuppett cuppett left a comment

Choose a reason for hiding this comment

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

Comments for a few explanations (that may not be obvious from the code commit messages and comments).

.github/workflows/ci.yml Show resolved Hide resolved
phpunit.xml Outdated
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure anything changed in this file, but either running phpunit or loading in PhpStorm commandeered and restructured this whole thing. 🤷

src/Kinds/K8sCronJob.php Show resolved Hide resolved
tests/ContainerTest.php Show resolved Hide resolved
@@ -95,7 +95,7 @@ public function test_container_build()
$container->removeEnv();

$this->assertFalse($container->isReady());
$this->assertEquals('nginx:1.4', $container->getImage());
$this->assertStringEndsWith('nginx:1.23', $container->getImage());
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The change to assertStringEndsWith is useful to ignore the qualified repository bit (regardless of which one it was).

Even in the unqualified case, I could see in KIND that nginx:1.4 would come back with an assertion fault because it got filled in with docker.io/library/nginx:1.4 when queried back out.

->setImage('perl')
->setCommand(['perl', '-Mbignum=bpi', '-wle', 'print bpi(2000)']);
->setImage('public.ecr.aws/docker/library/perl')
->setCommand(['perl', '-Mbignum=bpi', '-wle', 'print bpi(200)']);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The newer Perls were throwing errors on 2000 overflowing BigNumber or something.

Comment on lines -90 to +93
$pi = K8s::container()
->setName('pi')
->setImage('perl', '5.34.0')
->setCommand(['perl', '-Mbignum=bpi', '-wle', 'print bpi(2000)']);
$busybox = K8s::container()
->setName('busybox-exec')
->setImage('public.ecr.aws/docker/library/busybox')
->setCommand(['/bin/sh', '-c', 'sleep 30']);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

A change similar to this may be more useful (and save a little CPU) where we don't actually care about burning CPU for the test and just need a pod to run for some predictable period of time.

That's what happened and was needed here; shortening the perl computation meant a Job wasn't running long enough for the test to pick it up and validate it in the way it wanted, so I put it back into running with the persistent sleep I saw in another test.

@cuppett
Copy link
Contributor Author

cuppett commented Mar 29, 2023

Happy to have folks try it out and post back how it works:

https://getcomposer.org/doc/05-repositories.md#using-private-repositories

    "require": {
        "renoki-co/php-k8s": "dev-master",
    },
    "repositories": [
        {
            "type": "vcs",
            "url": "https://github.com/cuppett/php-k8s"
        }
    ]

@rennokki
Copy link
Member

rennokki commented Apr 1, 2023

@cuppett This looks awesome. 👍🏼 Let's wait for the tests to see what's going on and I'll be looking in the diff once again.

I appreciate it a lot! 🥺

@codecov
Copy link

codecov bot commented Apr 1, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: -1.05 ⚠️

Comparison is base (0112d92) 95.21% compared to head (343f9be) 94.17%.

Additional details and impacted files
@@             Coverage Diff              @@
##             master     #325      +/-   ##
============================================
- Coverage     95.21%   94.17%   -1.05%     
- Complexity      698      700       +2     
============================================
  Files            69       70       +1     
  Lines          1631     1837     +206     
============================================
+ Hits           1553     1730     +177     
- Misses           78      107      +29     
Impacted Files Coverage Δ
src/Kinds/K8sHorizontalPodAutoscaler.php 100.00% <ø> (ø)
src/Kinds/K8sPodDisruptionBudget.php 100.00% <ø> (ø)
src/Kinds/K8sStatefulSet.php 100.00% <ø> (ø)
src/Kinds/K8sCronJob.php 96.15% <100.00%> (+0.15%) ⬆️
src/Kinds/K8sJob.php 100.00% <100.00%> (ø)

... and 26 files with indirect coverage changes

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rennokki rennokki changed the title Bump minikube, k8s, php and laravel versions [upgrade] Bump minikube, k8s, PHP and Laravel versions Apr 1, 2023
@rennokki rennokki merged commit a86dff7 into renoki-co:master Apr 1, 2023
@rennokki
Copy link
Member

rennokki commented Apr 1, 2023

@cuppett Awesome job! Thank you 🙏🏼

@cuppett
Copy link
Contributor Author

cuppett commented Apr 1, 2023

@cuppett Awesome job! Thank you 🙏🏼

You bet! Great library, thanks for it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Incompatible with Laravel 10 (laravel/framework replaces illuminate/macroable)
3 participants