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

Bump Kubernetes version to 1.24.9 in CI #1106

Merged
merged 4 commits into from
Jan 27, 2023

Conversation

zimnx
Copy link
Collaborator

@zimnx zimnx commented Nov 25, 2022

Currently used minikube with none driver requires dockershim coming
from 3rd party provider when Kubernetes version above 1.24 is requested.
The main reason why we used minikube it's storage provisioner which
comes with it. Even though it's not perfect and we have issues with it (#974).
Adding new dependency doesn't seem right for our usecase.

This change introduces spawning Kube cluster using dedicated tool kubeadm.
It gives us more customizable setup, then the previous one, some of the
existing hacks are now a configuration option.
It also allows us to choose different CRI, in this case I used cri-o.
Missing storage provisioner was replaced with battle tested upstream
kubernetes-sigs/sig-storage-local-static-provisioner.

Fixes #1137
Fixes #975

Requires:

@mykaul
Copy link
Contributor

mykaul commented Nov 25, 2022

Need to ensure we also test on the K8S version EKS has.

@zimnx zimnx force-pushed the mz/bump-kube-version branch 24 times, most recently from 44b0ad2 to 5a39a41 Compare November 25, 2022 17:09
@mykaul
Copy link
Contributor

mykaul commented Nov 27, 2022

Unless this is critical, let's defer this to future milestones.

@zimnx zimnx changed the title Bump Kubernetes version to 1.25.3 in CI Bump Kubernetes version to 1.24.3 in CI Nov 28, 2022
@zimnx
Copy link
Collaborator Author

zimnx commented Nov 28, 2022

Unless this is critical, let's defer this to future milestones.

Our CI uses 1.23.x which will be considered End of Life in 3 months.

@zimnx zimnx force-pushed the mz/bump-kube-version branch 2 times, most recently from 16747a4 to 639c3db Compare December 19, 2022 14:28
@zimnx zimnx force-pushed the mz/bump-kube-version branch 2 times, most recently from d5de8d7 to 8e76325 Compare January 25, 2023 08:31
.github/actions/setup-kubernetes/action.yaml Outdated Show resolved Hide resolved
.github/actions/setup-kubernetes/action.yaml Outdated Show resolved Hide resolved
.github/actions/run-e2e/action.yaml Outdated Show resolved Hide resolved
.github/actions/run-e2e/action.yaml Outdated Show resolved Hide resolved
.github/actions/setup-kubernetes/action.yaml Show resolved Hide resolved
test/e2e/set/scyllacluster/scyllacluster_pv.go Outdated Show resolved Hide resolved
@zimnx
Copy link
Collaborator Author

zimnx commented Jan 25, 2023

@tnozicka Thanks for detailed review, all your comments are addressed. PTAL

@zimnx zimnx force-pushed the mz/bump-kube-version branch 3 times, most recently from bf064e4 to 0707324 Compare January 25, 2023 20:50
.github/actions/run-e2e/action.yaml Outdated Show resolved Hide resolved
.github/actions/run-e2e/action.yaml Outdated Show resolved Hide resolved
.github/actions/setup-kubernetes/action.yaml Outdated Show resolved Hide resolved
.github/actions/setup-kubernetes/action.yaml Show resolved Hide resolved
.github/actions/setup-kubernetes/action.yaml Outdated Show resolved Hide resolved
test/e2e/set/scyllacluster/scyllacluster_pv.go Outdated Show resolved Hide resolved
test/e2e/set/scyllacluster/scyllacluster_pv.go Outdated Show resolved Hide resolved
test/e2e/set/scyllacluster/scyllacluster_pv.go Outdated Show resolved Hide resolved
test/e2e/set/scyllacluster/scyllacluster_pv.go Outdated Show resolved Hide resolved
var (
registry = initRegistry()

imageConfigs = initImageConfigs(registry)
Copy link
Member

Choose a reason for hiding this comment

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

fwiw, kube has it so the downstreams can customize universal images (like for bash, so they play well with their security, like user ids). I don't think we have the need but I don't feel strong one way or the other

@tnozicka tnozicka added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Jan 26, 2023
@zimnx zimnx force-pushed the mz/bump-kube-version branch 5 times, most recently from c0c3847 to 5e83100 Compare January 26, 2023 16:28
Copy link
Member

@tnozicka tnozicka left a comment

Choose a reason for hiding this comment

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

I think thet e2e could avoid waiting for deletion if you'd use UUID and put the name into annotation but it's not worth redoing it.

Got last 3 simple nits, rest lgtm

.github/actions/run-e2e/action.yaml Show resolved Hide resolved
test/e2e/set/scyllacluster/scyllacluster_pv.go Outdated Show resolved Hide resolved
test/e2e/set/scyllacluster/scyllacluster_pv.go Outdated Show resolved Hide resolved
Currently used minikube with `none` driver requires dockershim coming
from 3rd party provider when Kubernetes version above 1.24 is requested.
The main reason why we used minikube it's storage provisioner which
comes with it. Even though it's not perfect and we have issues with it
(scylladb#974).
Adding new dependency doesn't seem right for our usecase.

This change introduces spawning Kube cluster using dedicated tool
`kubeadm`.
It gives us more customizable setup, then the previous one, some of the
existing hacks are now a configuration option.
It also allows us to choose different CRI, in this case I used cri-o.
Missing storage provisioner was replaced with battle tested upstream
kubernetes-sigs/sig-storage-local-static-provisioner.

Fixes scylladb#1137
Fixes scylladb#975
Test logic relied on fact that minikube storage provisioner did not set
any NodeAffinity for provisioned PVs. This allowed logic to trigger
cleanup logic by adding a NodeAffinity pointing to not existing Node.
When proper local volume proviosiner is used, and NodeAffinity is
configured for new PVs, test no longer have option to trigger cleanup
logic because NodeAffinity field is immutable.

To overcome this, the ScyllaCluster is using custom StorageClass not
backed by any provisioner running in the cluster.
ScyllaCluster is going to request PVC from that StorageClass, and the test
is going to request a clone of the original PVC
from the default StorageClass to get any storage. Then the bound PV is
rebounded to the original PVC
but with empty NodeAffinity. This allows the test to trigger the
orphaned PV cleanup logic.
Copy link
Member

@tnozicka tnozicka left a comment

Choose a reason for hiding this comment

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

/approve
/lgtm

Great job @zimnx, bringing us to new Kube is important and this also makes our storage setup much closer to production! Which is also why you hit the test issues and had to make a complex fix there. Thanks!

@tnozicka tnozicka merged commit 04c20d3 into scylladb:master Jan 27, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/machinery Categorizes issue or PR as related to Makefile, scripts or similar changes. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Run e2e on newer Kubernetes version Revert conntrack-cleaner DaemonSet addition
4 participants