-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Support of additional volumes in pod #10099
Conversation
I've pushed a commit that moves the implementation some way along to where I think we want to go based on the proposal. I haven't tested it yet, but hope to in next day or so |
api/src/main/java/io/strimzi/api/kafka/model/common/template/PodTemplate.java
Outdated
Show resolved
Hide resolved
api/src/main/java/io/strimzi/api/kafka/model/common/template/Volume.java
Outdated
Show resolved
Hide resolved
cluster-operator/src/main/java/io/strimzi/operator/cluster/model/TemplateUtils.java
Show resolved
Hide resolved
cluster-operator/src/main/java/io/strimzi/operator/cluster/model/TemplateUtils.java
Outdated
Show resolved
Hide resolved
@MichaelMorrisEst Great work. So besides getting an approval on the proposal, what do you think is missing?
|
Thanks. |
Signed-off-by: Casper Thygesen <cth@trifork.com> Signed-off-by: KastTrifork <kast@trifork.com>
Signed-off-by: KastTrifork <kast@trifork.com>
…nd added handling for zookeeper Also fixed checkstyle issues and implemented workaround in CRD generator to allow compilation Signed-off-by: MichaelMorris <michael.morris@est.tech> Signed-off-by: KastTrifork <kast@trifork.com>
Signed-off-by: MichaelMorris <michael.morris@est.tech> Signed-off-by: KastTrifork <kast@trifork.com>
b4c4502
to
0ca3cc2
Compare
Existing tests now passing as well Signed-off-by: MichaelMorris <michael.morris@est.tech>
Signed-off-by: MichaelMorris <michael.morris@est.tech>
Signed-off-by: Casper Thygesen <cth@trifork.com>
Signed-off-by: Casper Thygesen <cth@trifork.com>
Signed-off-by: Casper Thygesen <cth@trifork.com>
…umes Signed-off-by: Casper Thygesen <cth@trifork.com>
Signed-off-by: Casper Thygesen <cth@trifork.com>
Signed-off-by: Casper Thygesen <cth@trifork.com>
@@ -656,7 +656,7 @@ private void checkClass(Class<?> crdClass) { | |||
|
|||
private void checkJsonInclude(Class<?> crdClass) { | |||
if (!crdClass.isAnnotationPresent(JsonInclude.class)) { | |||
err(crdClass + " is missing @JsonInclude"); | |||
warn(crdClass + " is missing @JsonInclude"); // Changed temporarily to allow compilation of io.fabric8.kubernetes.api.model.Quantity |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MichaelMorrisEst Will you help fix this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I gave it a go, please review 😄
@tombentley @scholzj I think this PR is ready for feedback. We need to adjust a modified testcase change as discussed in the community call, and we need to validate the changes on our own cluster. The PR should be mature enough to get feedback. |
Signed-off-by: KastTrifork <kast@trifork.com>
c66d697
to
4a367f3
Compare
This needs to be solved. |
Signed-off-by: Casper Thygesen <cth@trifork.com>
Signed-off-by: KastTrifork <kast@trifork.com>
Signed-off-by: pegtrifork <peg@trifork.com>
Signed-off-by: pegtrifork <peg@trifork.com>
Signed-off-by: pegtrifork <peg@trifork.com>
/azp run regression |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the code looks good. We just need get a bit more consistency to the actual code and its formatting.
CHANGELOG.md
Outdated
* Additional OAuth configuration options have been added for 'oauth' authentication on the listener and the client. | ||
On the listener `serverBearerTokenLocation` and `userNamePrefix` have been added. | ||
On the client `accessTokenLocation`, `clientAssertion`, `clientAssertionLocation`, `clientAssertionType`, and `saslExtensions` have been added. | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit, but I think this line is not needed.
@@ -195,6 +196,17 @@ public void setTmpDirSizeLimit(String tmpDirSizeLimit) { | |||
this.tmpDirSizeLimit = tmpDirSizeLimit; | |||
} | |||
|
|||
@Description("Additional volumes that can be mounted to the pod.") | |||
@JsonProperty("volumes") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be removed? @JsonProperty("volumes")
cluster-operator/src/main/java/io/strimzi/operator/cluster/model/EntityOperator.java
Outdated
Show resolved
Hide resolved
cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaBridgeCluster.java
Outdated
Show resolved
Hide resolved
cluster-operator/src/main/java/io/strimzi/operator/cluster/model/KafkaCluster.java
Outdated
Show resolved
Hide resolved
cluster-operator/src/main/java/io/strimzi/operator/cluster/model/ZookeeperCluster.java
Outdated
Show resolved
Hide resolved
@@ -545,6 +575,14 @@ public void testTemplate() { | |||
assertThat(sa.getMetadata().getLabels().entrySet().containsAll(saLabels.entrySet()), is(true)); | |||
assertThat(sa.getMetadata().getAnnotations().entrySet().containsAll(saAnots.entrySet()), is(true)); | |||
} | |||
|
|||
private static Volume getVolume(Deployment dep, String volumeName) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm fine with the utility method. But can we keep it up top with the other utility methods and not in the middle of the tests?
private static Volume getVolume(PodSpec podSpec, String volumeName) { | ||
return podSpec.getVolumes().stream().filter(volume -> volumeName.equals(volume.getName())).iterator().next(); | ||
} | ||
|
||
private static VolumeMount getVolumeMount(Container container, String volumeName) { | ||
return container.getVolumeMounts().stream().filter(volumeMount -> volumeName.equals(volumeMount.getName())).iterator().next(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, fine with the utility methods. But can we keep them up tom with the other ones and not in the middle of the tests?
private static Volume getVolume(Pod pod, String volumeName) { | ||
return pod.getSpec().getVolumes().stream().filter(volume -> volumeName.equals(volume.getName())).iterator().next(); | ||
} | ||
|
||
private static VolumeMount getVolumeMount(Container container, String volumeName) { | ||
return container.getVolumeMounts().stream().filter(volumeMount -> volumeName.equals(volumeMount.getName())).iterator().next(); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above.
@@ -1886,7 +1939,7 @@ public void testPodSetWithOAuthWithMissingClientSecret() { | |||
.build(); | |||
KafkaMirrorMaker2 resource = new KafkaMirrorMaker2Builder(this.resource) | |||
.editSpec() | |||
.withClusters(targetClusterWithOAuthWithMissingClientSecret) | |||
.withClusters(targetClusterWithOAuthWithMissingClientSecret) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whit is intentionally indented for better readability.
Co-authored-by: Jakub Scholz <www@scholzj.com> Signed-off-by: Peter Kiib Egede <100785536+pegtrifork@users.noreply.github.com>
Signed-off-by: pegtrifork <peg@trifork.com>
/azp run regression |
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @cthtrifork, @MichaelMorrisEst and @KastTrifork!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Just a nit but no need to change if you want to stay with this. I would make calls to addAdditionalVolumeMounts
and addAdditionalVolumes
explicit with the TemplateUtils
instead of importing them. So, i.e. using TemplateUtils.addAdditionalVolumeMounts
. I think we do this way most of the code in the repository.
Signed-off-by: MichaelMorris <michael.morris@est.tech>
Done |
/azp run build |
Pull request contains merge conflicts. |
It seems that the branch has some conflicts. Can you try to look into it and rebase it if needed? |
Any chance you can rebase this? So that we can get it merged and included in the next release? |
Signed-off-by: Kasper Storgaard <116632810+KastTrifork@users.noreply.github.com>
@@ -321,6 +321,7 @@ public void testTemplate() { | |||
assertThat(dep.getSpec().getTemplate().getSpec().getSchedulerName(), is("my-scheduler")); | |||
assertThat(dep.getSpec().getTemplate().getSpec().getTopologySpreadConstraints(), containsInAnyOrder(tsc1, tsc2)); | |||
assertThat(dep.getSpec().getTemplate().getSpec().getEnableServiceLinks(), is(false)); | |||
assertThat(dep.getSpec().getTemplate().getSpec().getTolerations(), is(singletonList(toleration))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is wrong and is causing the test failure?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for pointing this out
The line mentioned is from the main branch. I noticed that there is another line using the variable assertToleration which was missing, and therefore test failure. I've added the assertToleration variable now, and the test should pass
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think you handled it well - or maybe I misled you. If you compare it with the main branch, this is the correct line:
Line 297 in ff128d8
assertThat(dep.getSpec().getTemplate().getSpec().getTolerations(), is(singletonList(toleration))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my bad. I got confused by your comment and the error message in the pipeline. I will correct it to the right line, the one you are referring to, and remove the old behavior.
Thanks for your patience :-)
Signed-off-by: Kasper Storgaard <116632810+KastTrifork@users.noreply.github.com>
Signed-off-by: Kasper Storgaard <116632810+KastTrifork@users.noreply.github.com>
/azp run regression |
Azure Pipelines successfully started running 1 pipeline(s). |
Thanks for the PR! |
Type of change
Description
This is a solution design for an upcoming proposal (strimzi/proposals#120) supporting #3693
It will need testcoverage and more. Right now it is blocked by an seemingly unrelated compile issue:
CrdGenerator: error: class io.fabric8.kubernetes.api.model.Quantity is missing @JsonInclude
Checklist
Please go through this checklist and make sure all applicable tasks have been done