feat(plus-17): add StatefulSet construct #400

abierbaum · 2020-11-19T17:21:34Z

This is a first pass at a statefulset construct. It works well for the cases I need and should serve well as a starting point. Note that I copied the concepts from Deployment so it should look pretty familiar.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

abierbaum · 2020-11-29T15:38:00Z

@iliapolo Any thoughts on this one? I know there could probably be a set of higher level constructs still for some common statefulset cases, but this seems like a pretty good place to start as it basically mirrors Deployment.

iliapolo · 2020-11-29T16:06:02Z

@abierbaum only glanced through it, looks ok, i'll take a closer look soon.

Thanks!

iliapolo · 2020-12-06T21:14:33Z

packages/cdk8s-plus-17/src/statefulset.ts

+    *
+    * @default undefined - allocate a default service based upon servicePort and serviceName.
+    */
+  readonly service?: ServiceProps;


We can just accept a Service here instead, it will prevent some logic duplication and keep things more modular. Its ok to require the caller to instantiate other constructs.

I thought about that but it ends up making the initialization more complex. I was trying to get an API as easy to use as the deployment expose() call.

For example when I am using this in my production chart I end up calling as below. (note: I could have left off the service props in this case since this is default, but I put it there to be explicit for other people that come along later). The key is that people can just say the service name and port and get a good statefulset with standard options, but they can override the type if they want to do something like a node port or something like that.

const redis_set = new kplus.StatefulSet(this, 'redis-set', { serviceName: 'redis-srv', replicas: 2, containers: [redis_container], servicePort: redis_port, service: { type: kplus.ServiceType.CLUSTER_IP, clusterIP: undefined, // force it to allocate }, });

If the user had to allocate the Service it would look like:

const redis_srv = new Service(this, 'RedisService', { metadata: {name: 'redis-srv'}, type: ServiceType.CLUSTER_IP, clusterIP: undefined, ports: [{port: redis_port}], }) const redis_set = new kplus.StatefulSet(this, 'redis-set', { serviceName: 'redis-srv', replicas: 2, containers: [redis_container], servicePort: redis_port, service: redis_srv });

And the callee would have to make sure the service name strings match and the ports match. Seems like this is the type of thing we would want to wrap to make it simpler to handle and then if they need to modify the service they can patch it.

What do you think though? I probably missed something in this one.

@abierbaum

I thought about that but it ends up making the initialization more complex. I was trying to get an API as easy to use as the deployment expose() call.

This is a little different, because a stateful set cannot exist without a service, as opposed to a deployment. I understand the idea was to implicitly create a service, mimicking the expose behavior, but this should be implemented by accepting an optional Service, and defaulting to auto creating it using the information of the stateful set (i.e assigning some name based on the stateful set name, and looking up the correct ports using the containers of the stateful set).

We can definitely do something like that in the future, but I think it's better to think about that in a subsequent PR.

And the callee would have to make sure the service name strings match and the ports match.

Why? what I was thinking the caller would have to do is this:

const redis_set = new kplus.StatefulSet(this, 'redis-set', { replicas: 2, containers: [redis_container], service: redis_srv });

Notice that serviceName and servicePort is not passed, because they can be extracted from the service object using service.name and service.ports. Which means we don't delegate any burden on the caller except for calling new Service.

Will that not work?

iliapolo · 2020-12-06T21:15:42Z

packages/cdk8s-plus-17/src/statefulset.ts

+    *
+    * @default undefined
+    */
+  readonly servicePort?: number;


Won't be needed if we accept a Service.

iliapolo · 2020-12-06T21:15:49Z

packages/cdk8s-plus-17/src/statefulset.ts

+    *
+    * @default undefined
+    */
+  readonly serviceName?: string;


We would still need it if the Service was optional. But if the service was required we could remove it yes.

Yes, my plan was to make it required at the moment.

packages/cdk8s-plus-17/src/statefulset.ts

iliapolo · 2020-12-06T21:19:43Z

packages/cdk8s-plus-17/src/statefulset.ts

+    *
+    * Returns a a copy. Use `selectByLabel()` to add labels.
+    */
+  get labelSelector(): Record<string, string> {


To be consistent:

Suggested change

get labelSelector(): Record<string, string> {

public get labelSelector(): Record<string, string> {

Got it. I am so used to my production projects just adding this type of thing automatically using prettier and tslint that I forgot. Sorry.

github-actions · 2020-12-07T06:14:45Z

This PR has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

iliapolo · 2020-12-07T11:02:08Z

Dont close

abierbaum · 2020-12-21T15:14:07Z

@iliapolo Sorry for the delay. I got this working to where I was able to use it in production and forgot to come back to clean it up for upstream consumption. I have reviewed your comments, made some fixes, and added some feedback. Please let me know your thoughts. I would love to get this through this week.

iliapolo · 2020-12-25T11:39:10Z

packages/cdk8s-plus-17/src/statefulset.ts

+    *
+    * @default undefined - allocate a default service based upon servicePort and serviceName.
+    */
+  readonly service?: ServiceProps;


@abierbaum

I thought about that but it ends up making the initialization more complex. I was trying to get an API as easy to use as the deployment expose() call.

This is a little different, because a stateful set cannot exist without a service, as opposed to a deployment. I understand the idea was to implicitly create a service, mimicking the expose behavior, but this should be implemented by accepting an optional Service, and defaulting to auto creating it using the information of the stateful set (i.e assigning some name based on the stateful set name, and looking up the correct ports using the containers of the stateful set).

We can definitely do something like that in the future, but I think it's better to think about that in a subsequent PR.

And the callee would have to make sure the service name strings match and the ports match.

Why? what I was thinking the caller would have to do is this:

const redis_set = new kplus.StatefulSet(this, 'redis-set', { replicas: 2, containers: [redis_container], service: redis_srv });

Notice that serviceName and servicePort is not passed, because they can be extracted from the service object using service.name and service.ports. Which means we don't delegate any burden on the caller except for calling new Service.

Will that not work?

iliapolo · 2020-12-25T11:41:53Z

packages/cdk8s-plus-17/src/statefulset.ts

+    *
+    * @default undefined
+    */
+  readonly serviceName?: string;


Yes, my plan was to make it required at the moment.

iliapolo · 2020-12-25T11:50:10Z

packages/cdk8s-plus-17/src/statefulset.ts

+      throw new Error('Cannot build a stateful set with a Service without ports.');
+    }
+
+    this._service = new Service(this, 'Service', {


This also makes it impossible to accept an existing service in the future. Since a stateful set only needs a service name, I imagine that eventually it should accept an IService, which we don't currently have.

iliapolo · 2020-12-25T11:51:12Z

packages/cdk8s-plus-17/src/statefulset.ts

+    });
+
+    if (!props.servicePort && !props.service?.ports) {
+      throw new Error('Cannot build a stateful set with a Service without ports.');


This will move to the _toKube method and validate the ports property of the given service object.

github-actions · 2020-12-29T06:19:31Z

This PR has not received a response in a while. If you want to keep this issue open, please leave a comment below and auto-close will be canceled.

iliapolo · 2020-12-29T10:44:04Z

Dont close

abierbaum · 2020-12-31T15:59:45Z

@iliapolo I think this last update should address the core issue pretty well. I changed it to take a Service as input. After seeing it in action, I agree this is a good change for this use case.

note: I rebased off latest master.

This may have been a good idea, but it makes things painful when trying to apply changes for statefulset's where you want to have an assigned ClusterIP automatically. You end up with immutable change warnings because you really don't want to pass '', you want to pass undefined so you just get whatever it assigned.

Co-authored-by: Eli Polonsky <[email protected]>

abierbaum · 2021-01-14T13:42:32Z

@iliapolo Have you had a chance to get back to this one yet?

iliapolo

@abierbaum this is great. Just some minor nit-picks around naming conventions :)

iliapolo · 2021-01-15T11:19:43Z

packages/cdk8s-plus-17/src/statefulset.ts

+import { Volume } from './volume';
+
+
+export enum PodManagementPolicy {


Add some doc strings. Should also be available in the k8s spec.

iliapolo · 2021-01-15T11:21:16Z

packages/cdk8s-plus-17/test/statefulset.test.ts

+
+  const chart = Testing.chart();
+
+  const service = new kplus.Service(chart, 'test_service', {ports: [{port: 80}] });


Suggested change

const service = new kplus.Service(chart, 'test_service', {ports: [{port: 80}] });

const service = new kplus.Service(chart, 'TestService', {ports: [{port: 80}] });

iliapolo · 2021-01-15T11:23:39Z

packages/cdk8s-plus-17/test/statefulset.test.ts

+  expect(srv_spec.type).toEqual(kplus.ServiceType.CLUSTER_IP);
+  expect(srv_spec.clusterIP).toBeUndefined();
+  expect(srv_spec.ports![0].port).toEqual(80);


This doesn't seem related to a statefulset test..if its not covered in the service tests, lets add it there, but if it is, we can drop it.

iliapolo · 2021-01-15T11:24:01Z

packages/cdk8s-plus-17/test/statefulset.test.ts

+
+  const resources = Testing.synth(chart);
+  const srv_spec = resources[0].spec;
+  const set_spec = resources[1].spec;


Suggested change

const set_spec = resources[1].spec;

const setSpec = resources[1].spec;

iliapolo · 2021-01-15T11:24:35Z

packages/cdk8s-plus-17/test/statefulset.test.ts

+  expect(srv_spec.type).toEqual(kplus.ServiceType.NODE_PORT);
+  expect(srv_spec.ports![0].port).toEqual(9000);


again, doesn't seem necessary.

abierbaum · 2021-01-19T16:28:38Z

@iliapolo I have made the changes suggested. I apologize to all for the amount time it took to respond and get everything completed.

mergify · 2021-01-21T18:06:38Z

Your pull request will be updated and merged automatically (do not update manually).

Pull request has been modified.

iliapolo · 2021-01-21T18:07:26Z

@abierbaum No worries, i've been lagging as well :)

Thanks so much for this PR, its awesome.

mergify · 2021-01-21T18:08:31Z

Your pull request will be updated and merged automatically (do not update manually).

abierbaum changed the title ~~feat(plus): add StatefulSet construct~~ feat(plus-17): add StatefulSet construct Nov 19, 2020

abierbaum force-pushed the kplus_statefulset branch from c8086df to 7c49942 Compare November 19, 2020 18:02

SomayaB assigned iliapolo Nov 20, 2020

iliapolo requested changes Dec 6, 2020

View reviewed changes

iliapolo added the response-requested label Dec 6, 2020

github-actions bot added the closing-soon label Dec 7, 2020

github-actions bot removed closing-soon labels Dec 8, 2020

abierbaum force-pushed the kplus_statefulset branch from 235ce24 to 2224781 Compare December 21, 2020 14:51

iliapolo requested changes Dec 25, 2020

View reviewed changes

iliapolo added the response-requested label Dec 25, 2020

github-actions bot added the closing-soon label Dec 29, 2020

github-actions bot removed closing-soon labels Dec 30, 2020

abierbaum force-pushed the kplus_statefulset branch from e72dc00 to cdec555 Compare December 31, 2020 15:58

abierbaum and others added 7 commits December 31, 2020 10:00

Add StatefulSet construct.

d91c468

Lint fixes.

399f81e

Add dependency from statefulset on it's service.

d23ed4a

Update public interface

3c68c34

Update packages/cdk8s-plus-17/src/statefulset.ts

6ce01b7

Co-authored-by: Eli Polonsky <[email protected]>

Refactor statefulset to take Service as input.

17b2947

abierbaum force-pushed the kplus_statefulset branch from cdec555 to 17b2947 Compare December 31, 2020 16:00

iliapolo requested changes Jan 15, 2021

View reviewed changes

iliapolo added the response-requested label Jan 15, 2021

Updated per review.

d1a6dda

github-actions bot removed the response-requested label Jan 20, 2021

iliapolo previously approved these changes Jan 21, 2021

View reviewed changes

Merge branch 'master' into kplus_statefulset

fc9fcc1

iliapolo approved these changes Jan 21, 2021

View reviewed changes

mergify bot merged commit 98aad99 into cdk8s-team:master Jan 21, 2021

	get labelSelector(): Record<string, string> {
	public get labelSelector(): Record<string, string> {

		import { Volume } from './volume';


		export enum PodManagementPolicy {


		const chart = Testing.chart();

		const service = new kplus.Service(chart, 'test_service', {ports: [{port: 80}] });

	const set_spec = resources[1].spec;
	const setSpec = resources[1].spec;

		expect(srv_spec.type).toEqual(kplus.ServiceType.NODE_PORT);
		expect(srv_spec.ports![0].port).toEqual(9000);

feat(plus-17): add StatefulSet construct #400

feat(plus-17): add StatefulSet construct #400

Conversation

abierbaum commented Nov 19, 2020

abierbaum commented Nov 29, 2020

iliapolo commented Nov 29, 2020 • edited Loading

Choose a reason for hiding this comment

abierbaum Dec 21, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

iliapolo Dec 6, 2020 • edited Loading

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 Dec 7, 2020

iliapolo commented Dec 7, 2020

abierbaum commented Dec 21, 2020

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 Dec 29, 2020

iliapolo commented Dec 29, 2020

abierbaum commented Dec 31, 2020 • edited Loading

abierbaum commented Jan 14, 2021

iliapolo left a 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

abierbaum commented Jan 19, 2021

mergify bot commented Jan 21, 2021

iliapolo commented Jan 21, 2021

mergify bot commented Jan 21, 2021

iliapolo commented Nov 29, 2020 •

edited

Loading

abierbaum Dec 21, 2020 •

edited

Loading

iliapolo Dec 6, 2020 •

edited

Loading

abierbaum commented Dec 31, 2020 •

edited

Loading