initial implementation for tiered storage support

[Staniel]

Type of change

Select the type of your PR

• new feature

Description

Please describe your pull request
Support tiered storage: https://github.com/strimzi/proposals/blob/main/065-support-tiered-storage.md

Checklist

Please go through this checklist and make sure all applicable tasks have been done

• Write tests
• Make sure all tests pass
• Update documentation
• Check RBAC rights for Kubernetes / OpenShift roles
• Try your changes from Pod inside your Kubernetes and OpenShift cluster, not just locally
• Reference relevant issue(s) and close them after merging
• Update CHANGELOG.md
• Supply screenshots for visual changes, such as Grafana dashboards

[scholzj]

@Staniel I think this looks pretty good ... nice work. I left few minor comments. Also, if you could run make crd_install, it will copy the CRD files to the places where they need to be updated and hopefully help the CI build to pass.

[scholzj]

@im-konge @see-quick Can you please think a bit about the system tests for this? We do not have any plugin to test it with (Kafka has the [LocalTieredStorage)[https://github.com/apache/kafka/blob/trunk/storage/src/test/java/org/apache/kafka/server/log/remote/storage/LocalTieredStorage.java], but even that is not part of the Kafka distribution. That makes this a bit hard to test as we would need to first add it to the container image and build a new container and then use it to test something. Any ideas how to do it? Is it worth doing it? Or do you think unit testing that the configuration works is sufficient?

[ppatierno]

I left some nits but looks overall good to me.

[im-konge]

@scholzj IIRC we discussed this a while ago and there were two ideas:

• having something like that on the long running environment and there do some tests
• building the image (as we have f.e. for the Connect with File sink plugin) and then use it in our STs.

But I think having this tested on the ST level makes sense and it can possibly find some issues, which unit tests will not catch.
The only issue (which currently comes to my mind) is some upgrade testing, where we would need more than just one image with the plugin built before running those (I'm not sure if it's the requirement or not, I'm just thinking about it).

I can take a look at it :)

[scholzj]

@scholzj IIRC we discussed this a while ago and there were two ideas:

• having something like that on the long running environment and there do some tests
• building the image (as we have f.e. for the Connect with File sink plugin) and then use it in our STs.

But I think having this tested on the ST level makes sense and it can possibly find some issues, which unit tests will not catch. The only issue (which currently comes to my mind) is some upgrade testing, where we would need more than just one image with the plugin built before running those (I'm not sure if it's the requirement or not, I'm just thinking about it).

I can take a look at it :)

I think using it in one of your long-running clusters would make sense. But I'm not completely sure how much would the system test really add. Would we just check that the cluster starts with given config? In any case, can then please help @Staniel with how to do it and point him in the right direction as this is not really a question of simply copying some existing test with the custom image.

[scholzj]

I left some more nits around formatting of the code. But it looks good to me. I tried it as well and it seems to work for me with the LocalTieredStorage plugin as well as with the Aiven S3 plugin. So I think we just need to figure out the system tests to be able to merge it.

[Staniel]

I left some more nits around formatting of the code. But it looks good to me. I tried it as well and it seems to work for me with the LocalTieredStorage plugin as well as with the Aiven S3 plugin. So I think we just need to figure out the system tests to be able to merge it.

Thank you! I have also verified the change using a local build Strimzi image on my K8s cluster and the config is working as expected.

[im-konge]

In any case, can then please help @Staniel with how to do it and point him in the right direction as this is not really a question of simply copying some existing test with the custom image.

Yeah I will help @Staniel for sure. I'm just thinking that in case that it will be on the long running cluster, the STs will be skipped from this repository.
Regarding STs in this repository - I guess that we can always try to deploy the cluster with tiered storage, do some changes there, do a message transmission and check that everything is okay.

I will take a look at it tomorrow and provide info regarding STs. Maybe - would it be possible to add STs in separate PR, or it would be a blocker?

[scholzj]

@im-konge I think we can do that in a separate PR, yes.

[scholzj]

/azp run regression

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[scholzj]

Looks goo to me. Thanks for the PR.

[ppatierno]

I left some nits/suggestions but LGTM overall. Thanks for the PR!

[see-quick]

LGTM, Thanks 👍

[im-konge]

@Staniel @scholzj to not block this PR, let's have the STs added in the different PR. Thanks!

[fvaleri]

Thanks @Staniel.

Look mostly good for me, I just left a couple of comments.

Additionally, I tried with LocalTieredStorage and it works fine.

[PaulRMellor]

Great! I've left a few suggestions related to the doc.

[scholzj]

@Staniel You will need to updated the derived files:

ERROR: Uncommitted changes in derived resources:
M	documentation/modules/appendix_crds.adoc
M	packaging/helm-charts/helm3/strimzi-kafka-operator/crds/040-Crd-kafka.yaml
M	packaging/install/cluster-operator/040-Crd-kafka.yaml
Run the following to add up-to-date resources:
  mvn clean verify -DskipTests -DskipITs \
    && make crd_install \
    && make dashboard_install \
    && make helm_install \
    && git add packaging/install/ packaging/helm-charts/ documentation/modules/appendix_crds.adoc cluster-operator/src/main/resources/cluster-roles \
    && git commit -s -m 'Update derived resources'

[scholzj]

@PaulRMellor @fvaleri Does it look good now?

[fvaleri]

LGTM.

[PaulRMellor]

Thanks for the updates

[scholzj]

Thanks for the PR @Staniel