Conversation
c528161 to
d6d439d
Compare
operator/src/main/java/org/projectnessie/operator/resource/NessieSpec.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/org/projectnessie/operator/resource/NessieImage.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/org/projectnessie/operator/resource/NessieImage.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/org/projectnessie/operator/resource/NessieImage.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/org/projectnessie/operator/resource/NessieSpec.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/org/projectnessie/operator/resource/Nessie.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/org/projectnessie/operator/resource/NessieAwsSecret.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/org/projectnessie/operator/resource/NessieAuthentication.java
Outdated
Show resolved
Hide resolved
bd00481 to
02e994a
Compare
.../main/java/org/projectnessie/operator/dependent/HorizontalPodAutoscalerV2Beta1Dependent.java
Show resolved
Hide resolved
operator/src/main/java/org/projectnessie/operator/helper/EventsHelper.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/org/projectnessie/operator/resource/NessieVersionStore.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/org/projectnessie/operator/resource/NessieAwsCredentials.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/org/projectnessie/operator/resource/NessieImage.java
Show resolved
Hide resolved
snazy
left a comment
There was a problem hiding this comment.
Not really reviewing yet - just adding comments "randomly".
de7990f to
68051e7
Compare
4a2d6f7 to
544c574
Compare
c39a06f to
3073462
Compare
7fea756 to
30bfae6
Compare
422f674 to
2a7f761
Compare
2a7f761 to
27df175
Compare
348e44c to
91283d0
Compare
snazy
left a comment
There was a problem hiding this comment.
Just another batch of review comments.
| filter(ReplaceTokens::class, mapOf("tokens" to mapOf("projectVersion" to project.version))) | ||
| } | ||
|
|
||
| tasks.named("quarkusAppPartsBuild").configure { |
There was a problem hiding this comment.
Do you know where these messages during the build come from?
Invalid AnsiLogger Stream -> Swapping to default sdt out logger.
[WARN] Could not detect project version. Using 'latest'.
There was a problem hiding this comment.
I don't know where the first line comes from.
The second one is from dekorate:
operator/src/main/java/org/projectnessie/operator/reconciler/nessie/resource/Nessie.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/org/projectnessie/operator/reconciler/nessie/resource/Nessie.java
Outdated
Show resolved
Hide resolved
operator/src/main/java/org/projectnessie/operator/exception/NessieOperatorException.java
Show resolved
Hide resolved
| import org.projectnessie.operator.reconciler.nessie.resource.options.AutoscalingOptions; | ||
|
|
||
| @KubernetesDependent(labelSelector = NessieReconciler.DEPENDENT_RESOURCES_SELECTOR) | ||
| public class HorizontalPodAutoscalerV2Beta1Dependent |
There was a problem hiding this comment.
All three HorizontalPodAutoScaler* classes look quite similar, rather equal. Any chance to unify the common/similar code?
There was a problem hiding this comment.
Difficult because the classes look similar but share no common code in fabric8's code. However we can ask ourselves if it's still useful to support the beta variants, they have been all deprecated for a while now.
| @@ -0,0 +1,149 @@ | |||
| /* | |||
There was a problem hiding this comment.
./gradlew :nessie-operator:test prints a lot to the console.
Some are warnings:
ismatched Quarkus version found: "3.10.0", expected: "3.8.3" by at least a minor version and things might not work as expected.
Mismatched Quarkus-provided Fabric8 Kubernetes Client version found: "6.11.0", expected: "6.10.0" by at least a minor version and things might not work as expected.
Build time property cannot be changed at runtime:
- quarkus.tls.trust-all is set to 'true' but it is build time fixed to 'false'. Did you change the property quarkus.tls.trust-all after building the application?
Any chance to not let the exception be printed to the console?
There was a problem hiding this comment.
Got this during the bigtable IT:
Unable to mount a file from test host into a running container. This may be a misconfiguration or limitation of your Docker environment. Some features might not work.
and then this output
There was a problem hiding this comment.
Anything that needs to be configured for podman?
There was a problem hiding this comment.
Any chance to not let the exception be printed to the console?
The first line comes from here:
The "build time property cannot be changed" message comes probably from some dev service, but I couldn't find which one. See also quarkusio/quarkus#23680.
I don't know how to suppress these messages since they are logged during build, not during tests.
Got this during the bigtable IT:
Is this happening all the time, or randomly? I've never seen this. I will have to install podman to investigate. It's not related to BigTableIT, but rather to the start of the K3s container before the tests.
There was a problem hiding this comment.
I went through the CRDs, reviewed some of the code and this is a good start for an operator. The operator framework is a decent fit and cuts down on the amount of logic to write and lifecycle to manage.
Note, I have not used it in anger, so I don't have any comments on the usability aspect beyond being able to say the CRDs are well formed and logica, in otherwords, I know what I'm configuring just by reading them.
Also, I think you may find that you will ultimately get a request for an operator UI as what you see here https://github.com/zalando/postgres-operator/blob/master/docs/operator-ui.md
Final comment, generally speaking, it does seem helm charts still remain very popular, so you will have to maintain both this and a helm chart, if there are not resources and you have to pick one I would pick the helm chart, but you will have users that prefer the operator, or even demand it.
Note about the helm chart, I feel we (devops) want to use operators if we can but want to do it using regular helm charts. There are a lot of operators support installation + configuration using helm chart. |
|
@dorsegal our plan is to distribute the operator using Helm charts initially. Operator Hub would be the ultimate goal, but that requires a lot more work. |
| intTestImplementation(project(":nessie-keycloak-testcontainer")) | ||
| intTestImplementation(project(":nessie-container-spec-helper")) | ||
|
|
||
| intTestCompileOnly(libs.microprofile.openapi) |
There was a problem hiding this comment.
| intTestCompileOnly(libs.microprofile.openapi) | |
| intTestCompileOnly(libs.microprofile.openapi) | |
| intTestCompileOnly(libs.immutables.value.annotations) |
| # Version is managed by Renovate - do not edit. | ||
| # See https://cloud.google.com/sdk/docs/downloads-docker#docker_image_options | ||
| # Use debian_component_based because it supports linux/arm | ||
| FROM gcr.io/google.com/cloudsdktool/google-cloud-cli:483.0.0-debian_component_based |
There was a problem hiding this comment.
| FROM gcr.io/google.com/cloudsdktool/google-cloud-cli:483.0.0-debian_component_based | |
| FROM gcr.io/google.com/cloudsdktool/google-cloud-cli:484.0.0-debian_component_based |
| @@ -0,0 +1,3 @@ | |||
| # Dockerfile to provide the image name and tag to a test. | |||
| # Version is managed by Renovate - do not edit. | |||
| FROM rancher/k3s:v1.30.2-k3s2 | |||
There was a problem hiding this comment.
| FROM rancher/k3s:v1.30.2-k3s2 | |
| FROM docker.io/rancher/k3s:v1.30.2-k3s2 |
| @@ -0,0 +1,3 @@ | |||
| # Dockerfile to provide the image name and tag to a test. | |||
| # Version is managed by Renovate - do not edit. | |||
| FROM mongo:7.0.12 | |||
There was a problem hiding this comment.
| FROM mongo:7.0.12 | |
| FROM docker.io/mongo:7.0.12 |
| @@ -0,0 +1,3 @@ | |||
| # Dockerfile to provide the image name and tag to a test. | |||
| # Version is managed by Renovate - do not edit. | |||
| FROM postgres:16.3 | |||
There was a problem hiding this comment.
| FROM postgres:16.3 | |
| FROM docker.io/postgres:16.3 |
.github/workflows/ci.yml
Outdated
| job-name: 'int-test-quarkus' | ||
| java-version: ${{ matrix.java-version }} | ||
|
|
||
| int-test-operator: |
There was a problem hiding this comment.
Just a heads-up - when it's merged.
| import org.projectnessie.operator.reconciler.nessie.resource.options.AutoscalingOptions; | ||
|
|
||
| @KubernetesDependent(labelSelector = NessieReconciler.DEPENDENT_RESOURCES_SELECTOR) | ||
| public class HorizontalPodAutoscalerV2Beta1Dependent |
...ator/src/intTest/java/org/projectnessie/operator/testinfra/K3sContainerLifecycleManager.java
Show resolved
Hide resolved
| container = createContainer(); | ||
| container | ||
| .withNetwork(Network.SHARED) | ||
| .withLogConsumer(new Slf4jLogConsumer(logger)) |
There was a problem hiding this comment.
| .withLogConsumer(new Slf4jLogConsumer(logger)) | |
| .withLogConsumer(new Slf4jLogConsumer(logger).withPrefix(container.getDockerImageName())) |
| implements QuarkusTestResourceLifecycleManager { | ||
|
|
||
| protected C container; | ||
| protected String inDockerIpAddress; |
There was a problem hiding this comment.
| protected String inDockerIpAddress; | |
| private String inDockerIpAddress; | |
| protected String inDockerIpAddress() { | |
| if (inDockerIpAddress == null) { | |
| inDockerIpAddress = getInDockerIpAddress(); | |
| } | |
| return inDockerIpAddress; | |
| } | |
| inDockerIpAddress = | ||
| Objects.requireNonNull( | ||
| getInDockerIpAddress(), "could not determine container's in-docker IP address"); |
There was a problem hiding this comment.
| inDockerIpAddress = | |
| Objects.requireNonNull( | |
| getInDockerIpAddress(), "could not determine container's in-docker IP address"); |
|
|
||
| @Override | ||
| public Map<String, String> start() { | ||
| Logger logger = LoggerFactory.getLogger(getClass()); |
There was a problem hiding this comment.
| Logger logger = LoggerFactory.getLogger(getClass()); | |
| inDockerIpAddress = null; | |
| Logger logger = LoggerFactory.getLogger(getClass()); |
|
Still can't get the integration tests to work. K3s doesn't work properly in rootless. There seem to be some ways to get that working - but currently hitting this error from k3s. |
Yeah I'm not sure running a Kubernetes cluster inside a rootless container is even possible. |
| // Mitigate eviction issues in CI by setting eviction thresholds for nodefs very low | ||
| commandParts.add("--kubelet-arg=eviction-hard=nodefs.available<1%,nodefs.inodesFree<1%"); | ||
| // Enable rootless containers | ||
| commandParts.add("--kubelet-arg=feature-gates=KubeletInUserNamespace=true"); |
There was a problem hiding this comment.
| commandParts.add("--kubelet-arg=feature-gates=KubeletInUserNamespace=true"); | |
| commandParts.add("--kubelet-arg=feature-gates=KubeletInUserNamespace=true"); | |
| commandParts.add("--rootless"); | |
| commandParts.add("--snapshotter=fuse-overlayfs"); |
There was a problem hiding this comment.
K3s refuses to start for me:
2024-07-22 10:21:14 time="2024-07-22T08:21:14Z" level=warning msg="Running RootlessKit as the root user is unsupported."
2024-07-22 10:21:14 time="2024-07-22T08:21:14Z" level=warning msg="The host root filesystem is mounted as \"master:34\". Setting child propagation to \"\" is not supported."
2024-07-22 10:21:14 time="2024-07-22T08:21:14Z" level=fatal msg="failed to setup UID/GID map: failed to compute uid/gid map: open /etc/subuid: no such file or directory"
|
Do you plan to have GC implemented as part of the operator? I think it can be useful to have CRD for it |
See here: #9415 So it's planned yes, but no ETA at the moment due to lack of interest from the community. |
No description provided.