[ResourceCluster Part1] ResourceClustersManager actor + contracts #175

Andyz26 · 2022-04-14T20:58:01Z

Context

This PR adds the new ResourceClustersManagerActor and its dependent classes and contract classes as part of the effort to support non-Mesos cluster type for Mantis.

This new actor will be handling both API requests and requests from other actors related to any resource-cluster-related operations.

To support different cluster types (e.g. Kubernetes/Spinnaker), this new ResourceClustersManager actor and its corresponding contracts will provide abstraction only and not be tied to any framework-specific dependencies.

Checklist

./gradlew build compiles code correctly
Added new tests where applicable
./gradlew test passes all tests
Extended README or added javadocs where applicable
Added copyright headers for new files from CONTRIBUTING.md

github-actions · 2022-04-14T21:05:54Z

Unit Test Results

111 files +36 111 suites +36 5m 26s ⏱️ + 3m 10s
481 tests +95 462 ✔️ +94 19 💤 +2 0 ❌ - 1

Results for commit af10210. ± Comparison against base commit b0f9e60.

♻️ This comment has been updated with latest results.

sundargates · 2022-04-15T03:59:58Z

...c/main/java/io/mantisrx/control/plane/resource/cluster/proto/ListResourceClusterRequest.java

+
+@Builder
+@Value
+public class ListResourceClusterRequest {


move these within the actor class.

All these contracts in a single file are going to make it a huge one though? Also, there are composite referenced classes that are already huge by themselves.

Andyz26 · 2022-04-18T16:33:27Z

@calvin681 @codyrioux @hmit @sundargates: I would love to get some review/feedback, especially regarding the interface of IResourceClusterProvider and the provisioning part in the ResourceClustersManagerActor.

Andyz26 · 2022-04-18T23:09:50Z

@sundargates I've renamed the classes here to avoid naming conflicts. Let me know if you have any other concerns.

build.gradle

...rc/main/java/io/mantisrx/control/plane/resource/cluster/proto/MantisResourceClusterSpec.java

hmitnflx · 2022-04-19T16:49:57Z

...rc/main/java/io/mantisrx/control/plane/resource/cluster/proto/MantisResourceClusterSpec.java

+        int desireSize;
+
+        @JsonCreator
+        public SkuCapacity(


instead of adding an explicit constructor with @Json* annotations here, having @Value is enough since it does -- final @ToString @EqualsAndHashCode @AllArgsConstructor @FieldDefaults(makeFinal = true, level = AccessLevel.PRIVATE) @Getter

This is a workaround (when I tested the original clean version (@value + builder only) in nfmantis the shaded classes and Jackson doesn't play well with each other and other internal SDKs and it can only work when I have both annotation + upgrade shaded Jackson for all combinations).

Let me add some notes here about this.

Can you add some tests to make sure you codify the expected behaviors? In this case, I'm guessing you want the classes to be Jackson serializable (which they already are because of our lombok.config checked in at the root level).

The tricky part is that these work just fine without any of the extra annotations in OSS repo but started to run into trouble imported in the internal wrapper mixing with other non-shaded Jackson. I will have more tests covering in the nfmantis side.

For OSS I will have some extra coverage on thee too in the follow up PR.

...ontrol/plane/resource/cluster/resourceprovider/SimpleFileResourceClusterStorageProvider.java

sundargates · 2022-04-20T00:07:45Z

build.gradle

+        flink   : "1.14.2",
+        hadoop  : "2.7.7",
+        junit4  : "4.11",
+        junit5  : "5.4.+",
+        mockito : "2.0.+",
+        mockito3: "3.+",
+        slf4j   : "1.7.0",
+        vavr    : "0.9.2",


nit: maybe we should try to not align the colons given that this is constantly going to change based on the longest module name.

I think editor config might help streamline these. I will take a look.

sundargates · 2022-04-20T00:08:57Z

...va/io/mantisrx/control/plane/resource/cluster/resourceprovider/IResourceClusterProvider.java

+ * This interface provides the API to connect resource cluster management actor to actual
+ * implementations of different resource cluster clients e.g. k8s.
+ */
+public interface IResourceClusterProvider {


nit: let's rename this to ResourceClusterProvider. We want a consistent naming style for interfaces, and it's already mixed between I.* and regular interface names.

sundargates · 2022-04-20T00:10:57Z

...rc/main/java/io/mantisrx/control/plane/resource/cluster/proto/MantisResourceClusterSpec.java

+    @JsonCreator
+    public MantisResourceClusterSpec(
+            @JsonProperty("name") final String name,
+            @JsonProperty("id") final String id,
+            @JsonProperty("ownerName") final String ownerName,
+            @JsonProperty("ownerEmail") final String ownerEmail,
+            @JsonProperty("envType") final MantisResourceClusterEnvType envType,
+            @JsonProperty("skuSpecs") final Set<SkuTypeSpec> skuSpecs,
+            @JsonProperty("clusterMetadataFields") final Map<String, String> clusterMetadataFields) {
+        this.name = name;
+        this.id = id;
+        this.ownerName = ownerName;
+        this.ownerEmail = ownerEmail;
+        this.envType = envType;
+        this.skuSpecs = skuSpecs;
+        this.clusterMetadataFields = clusterMetadataFields;
+    }


I don't think you should need both @value and @JsonCreator in the same class.

See the other conversation thread regarding the workaround for Jackson.

sundargates · 2022-04-20T00:11:19Z

...rc/main/java/io/mantisrx/control/plane/resource/cluster/proto/MantisResourceClusterSpec.java

+        @JsonCreator
+        public SkuTypeSpec(
+                @JsonProperty("skuId") final String skuId,
+                @JsonProperty("capacity") final SkuCapacity capacity,
+                @JsonProperty("imageId") final String imageId,
+                @JsonProperty("cpuCoreCount") final int cpuCoreCount,
+                @JsonProperty("memorySizeInBytes") final int memorySizeInBytes,
+                @JsonProperty("networkMbps") final int networkMbps,
+                @JsonProperty("diskSizeInBytes") final int diskSizeInBytes,
+                @JsonProperty("skuMetadataFields") final Map<String, String> skuMetadataFields) {
+            this.skuId = skuId;
+            this.capacity = capacity;
+            this.imageId = imageId;
+            this.cpuCoreCount = cpuCoreCount;
+            this.memorySizeInBytes = memorySizeInBytes;
+            this.networkMbps = networkMbps;
+            this.diskSizeInBytes = diskSizeInBytes;
+            this.skuMetadataFields = skuMetadataFields;
+        }
+    }
+


sundargates · 2022-04-20T00:13:24Z

...rc/main/java/io/mantisrx/control/plane/resource/cluster/proto/MantisResourceClusterSpec.java

+        int desireSize;
+
+        @JsonCreator
+        public SkuCapacity(


Can you add some tests to make sure you codify the expected behaviors? In this case, I'm guessing you want the classes to be Jackson serializable (which they already are because of our lombok.config checked in at the root level).

sundargates · 2022-04-20T00:13:55Z

...rc/main/java/io/mantisrx/control/plane/resource/cluster/proto/MantisResourceClusterSpec.java

+        @JsonCreator
+        public SkuCapacity(


nit: Don't think you need this.

sundargates · 2022-04-20T00:16:24Z

...rc/main/java/io/mantisrx/control/plane/resource/cluster/proto/MantisResourceClusterSpec.java

+@Builder
+public class MantisResourceClusterSpec {
+
+    @NonNull


Let's not use the @nonnull annotation. By default, let's assume all fields are non-null by default. If something is nullable, use optional types in case that class is in the API layer (i.e., external-facing) or @nullable if the field is only within a class (i.e., internal-facing).

sundargates · 2022-04-20T00:17:39Z

...rc/main/java/io/mantisrx/control/plane/resource/cluster/proto/MantisResourceClusterSpec.java

+    String ownerEmail;
+
+    @NonNull
+    MantisResourceClusterEnvType envType;


why do we need this? can this not be obtained from the env the mantis-control-plane is being run from?

I would prefer not to have an assumption about env var at this layer but leave it at the entry-point/wrapper to decide how to obtain it.

sundargates · 2022-04-20T00:18:34Z

...rc/main/java/io/mantisrx/control/plane/resource/cluster/proto/MantisResourceClusterSpec.java

+
+    @Builder
+    @Value
+    public static class SkuCapacity {


nit: document.

Andyz26 added 3 commits April 14, 2022 13:03

ResouceClusterActor + contracts

a757a99

Doc Update

fdf4c55

Fix mockito version

179deb4

Andyz26 requested review from codyrioux, nickmahilani, jeffchao, piygoyal, calvin681, sundargates, hmitnflx, markcho and liuml07 as code owners April 14, 2022 20:58

Fix style build err

a019924

sundargates reviewed Apr 15, 2022

View reviewed changes

Andyz26 added 2 commits April 18, 2022 09:45

test fix

327f1af

Rename to ResourceClustersHostManagerActor

7555882

hmitnflx reviewed Apr 19, 2022

View reviewed changes

Address PR comments

af10210

Andyz26 requested a review from fdc-ntflx as a code owner April 19, 2022 22:06

Andyz26 merged commit 7d225d7 into Netflix:master Apr 19, 2022

sundargates reviewed Apr 20, 2022

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ResourceCluster Part1] ResourceClustersManager actor + contracts #175

[ResourceCluster Part1] ResourceClustersManager actor + contracts #175

Andyz26 commented Apr 14, 2022

github-actions bot commented Apr 14, 2022 •

edited

Loading

sundargates Apr 15, 2022

Andyz26 Apr 15, 2022

Andyz26 commented Apr 18, 2022

Andyz26 commented Apr 18, 2022

hmitnflx Apr 19, 2022

Andyz26 Apr 19, 2022

Andyz26 Apr 19, 2022

sundargates Apr 20, 2022

Andyz26 Apr 20, 2022

Andyz26 Apr 20, 2022

sundargates Apr 20, 2022

Andyz26 Apr 20, 2022

sundargates Apr 20, 2022

sundargates Apr 20, 2022

Andyz26 Apr 20, 2022

sundargates Apr 20, 2022

sundargates Apr 20, 2022

sundargates Apr 20, 2022

sundargates Apr 20, 2022

sundargates Apr 20, 2022

Andyz26 Apr 20, 2022

sundargates Apr 20, 2022

[ResourceCluster Part1] ResourceClustersManager actor + contracts #175

[ResourceCluster Part1] ResourceClustersManager actor + contracts #175

Conversation

Andyz26 commented Apr 14, 2022

Context

Checklist

github-actions bot commented Apr 14, 2022 • edited Loading

Unit Test Results

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Andyz26 commented Apr 18, 2022

Andyz26 commented Apr 18, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Apr 14, 2022 •

edited

Loading