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

feat: Move policy/v1beta1 to policy/v1 for K8s 1.25+ compatability #8

Merged

Conversation

jwitko
Copy link
Contributor

@jwitko jwitko commented Feb 21, 2023

Fixes #5

Description

This PR moves policy/v1beta1 to policy/v1 for the PodDisruptionBudget. policy/v1beta1 was deprecated in K8s 1.21 and removed in 1.25. It should be noted that this PR will make K8s <1.21 unsupported.

This PR has:

  • been tested on a real K8S cluster to ensure creation of a brand new Druid cluster works.
  • been tested for backward compatibility on a real K*S cluster by applying the changes introduced here on an existing Druid cluster. If there are any backward incompatible changes then they have been noted in the PR description.
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added documentation for new or modified features or behaviors.

Key changed/added files in this PR
  • PodDisruptionBudget

@AdheipSingh
Copy link
Contributor

@jwitko CI is failing on test. You can run make test locally

@jwitko
Copy link
Contributor Author

jwitko commented Feb 22, 2023

So there was some missed policy/v1beta1 objects which I've now rectified that were causing some major failures in the tests.
It looks like, based on the old repo/PR, that they were added inbetween my original PR and now.

That being said, I'm still getting a failure that I am not sure what is going wrong:

$ make test
test -s /home/jwitkowski/git_repos/druid-operator/bin/controller-gen || GOBIN=/home/jwitkowski/git_repos/druid-operator/bin go install sigs.k8s.io/controller-tools/cmd/controller-gen@v0.9.2
/home/jwitkowski/git_repos/druid-operator/bin/controller-gen crd:generateEmbeddedObjectMeta=true paths="./..." output:crd:artifacts:config=deploy/crds
/home/jwitkowski/git_repos/druid-operator/bin/controller-gen crd:generateEmbeddedObjectMeta=true paths="./..." output:crd:artifacts:config=chart/templates/crds/
/home/jwitkowski/git_repos/druid-operator/bin/controller-gen object:headerFile="hack/boilerplate.go.txt" paths="./..."
go fmt ./...
go vet ./...
KUBEBUILDER_ASSETS="/home/jwitkowski/.local/share/kubebuilder-envtest/k8s/1.25.0-linux-amd64" go test ./... -coverprofile cover.out
?       github.com/druid-io/druid-operator      [no test files]
?       github.com/druid-io/druid-operator/apis/druid/v1alpha1  [no test files]
--- FAIL: TestMakePodDisruptionBudgetForBroker (0.00s)
    handler_test.go:170: Match failed!.
{"level":"info","ts":1677021905.21531,"logger":"controller-runtime.metrics","msg":"Metrics server is starting to listen","addr":":8080"}
{"level":"info","ts":1677021905.2163405,"msg":"Starting server","path":"/metrics","kind":"metrics","addr":"[::]:8080"}
{"level":"info","ts":1677021905.216479,"logger":"controller.druid","msg":"Starting EventSource","reconciler group":"druid.apache.org","reconciler kind":"Druid","source":"kind source: *v1alpha1.Druid"}
{"level":"info","ts":1677021905.2165172,"logger":"controller.druid","msg":"Starting Controller","reconciler group":"druid.apache.org","reconciler kind":"Druid"}
{"level":"info","ts":1677021905.4171314,"logger":"controller.druid","msg":"Starting workers","reconciler group":"druid.apache.org","reconciler kind":"Druid","worker count":1}
{"level":"info","ts":1677021907.354074,"logger":"KubeAPIWarningLogger","msg":"unknown field \"spec.services[0].metadata.creationTimestamp\""}
{"level":"info","ts":1677021907.8984025,"logger":"KubeAPIWarningLogger","msg":"autoscaling/v2beta2 HorizontalPodAutoscaler is deprecated in v1.23+, unavailable in v1.26+; use autoscaling/v2 HorizontalPodAutoscaler"}
FAIL
coverage: 50.8% of statements
FAIL    github.com/druid-io/druid-operator/controllers/druid    28.029s
?       github.com/druid-io/druid-operator/controllers/druid/ext        [no test files]
FAIL
make: *** [Makefile:72: test] Error 1

I'm not sure what is actually going wrong?

Looks like its failing on this test:

func assertEquals(expected, actual interface{}, t *testing.T) {
        if !reflect.DeepEqual(expected, actual) {
                t.Error("Match failed!.")
        }
}

@AdheipSingh
Copy link
Contributor

@jwitko ill take a look. It might be that hash changed, so will need to be updated. Ill get back. Thanks

@jwitko
Copy link
Contributor Author

jwitko commented Feb 22, 2023

ah, any hash of the PDB manifests would have definitely changed. How are the hashes generated and where are they stored?

@AdheipSingh
Copy link
Contributor

@jwitko https://github.com/datainfrahq/druid-operator/blob/master/controllers/druid/testdata/broker-pod-disruption-budget.yaml#L12
Here, you can print the actual and desired object in the test and update the hash. its an annotation in all the objects owned by the druid controller

@jwitko
Copy link
Contributor Author

jwitko commented Feb 22, 2023

update the hash.

How are you generating the hash?
Edit: I see, getObjectHash

@jwitko jwitko force-pushed the feature/move-policy-v1beta1-to-v1 branch from dfb583c to 918ae6d Compare February 22, 2023 15:04
@jwitko
Copy link
Contributor Author

jwitko commented Feb 22, 2023

Thanks for the assist @AdheipSingh. Tests should now pass.

@jwitko
Copy link
Contributor Author

jwitko commented Feb 22, 2023

Now the only question remaining... Do we want to add support for the old policy/v1beta1? Keep it backwards compatible?

@AdheipSingh
Copy link
Contributor

Do we want to add support for the old policy/v1beta1? Keep it backwards compatible?

What version of k8s is compatible with v1beta1 ?

@jwitko
Copy link
Contributor Author

jwitko commented Feb 22, 2023

What version of k8s is compatible with v1beta1 ?

v1.3 - v1.4 : policy/v1alpha1 PodDisruptionBudget introduced
v1.5 - v1.20: policy/v1beta1 PodDisruptionBudget
v1.21 : policy/v1 PodDisruptionBudget introduced (GA) + policy/v1beta1 PodDisruptionBudget deprecated
v1.25 : policy/v1beta1 PodDisruptionBudget removed

@AdheipSingh
Copy link
Contributor

In that case we can remove, since this is going to be a new release.
Also were you able to get the test pass with the hash changes

@AdheipSingh
Copy link
Contributor

thanks @jwitko

@AdheipSingh AdheipSingh merged commit a271a3f into datainfrahq:master Feb 23, 2023
@jwitko jwitko deleted the feature/move-policy-v1beta1-to-v1 branch February 23, 2023 00:27
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.

feat: Move policy/v1beta1 to policy/v1 for K8s 1.25+ compatibility
2 participants