Skip to content
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.

Sign up for GitHub

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

Jump to bottom

K8s Task launcher to use task launcher account properties #4546

Closed
wants to merge 3 commits into from
Closed

K8s Task launcher to use task launcher account properties #4546

wants to merge 3 commits into from

Conversation

ilayaperumalg
Copy link
Contributor

  • Introduce Platform task launcher properties to include K8s task launcher properties
  • Construct the task launcher using this explicit property
  • Update test

Resolves #4186

@ilayaperumalg ilayaperumalg added this to the 2.8.0-RC1 milestone May 27, 2021
@ilayaperumalg ilayaperumalg mentioned this pull request May 27, 2021
Closed
@ilayaperumalg
K8s Task launcher to use task launcher account properties
24ba63e 
 - Introduce Platform task launcher properties to include K8s task launcher properties
 - Construct the task launcher using this explicit property
 - Update test

Resolves spring-cloud#4186
@ilayaperumalg ilayaperumalg force-pushed the GH-4186-task-launcer-k8s-properties branch from 4486a91 to 24ba63e Compare May 27, 2021 20:16
cppwfs
cppwfs reviewed May 27, 2021
View reviewed changes
...mework/cloud/dataflow/server/config/kubernetes/KubernetesPlatformTaskLauncherProperties.java
@@ -0,0 +1,28 @@
/*
* Copyright 2017-2019 the original author or authors.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

2021

@cppwfs
Copy link
Contributor

cppwfs commented May 27, 2021

LGTM
Created Dev platform with 2 limits 1 for memory the other for cpu a

  1. Launched task using dev platform and both limits showed up properly in the pod.
  2. Launched task using dev platform again overriding memory limit and it showed up properly in the pod.
  3. Launched task using default and its values showed up properly in the pod
  4. Launched task using default platform and overrode its values and they showed up properly in the pod.

@ilayaperumalg
Copy link
Contributor Author

Thanks @cppwfs . While the above covers the regression part, we need to be testing with properties such as backoffLimit, restartPolicy that are part of platform account properties.

cppwfs
cppwfs reviewed May 28, 2021
View reviewed changes
...ingframework/cloud/dataflow/server/config/kubernetes/KubernetesTaskPlatformFactoryTests.java
KubernetesDeployerProperties deployerProperties = new KubernetesDeployerProperties();
deployerProperties.getLimits().setMemory("5555Mi");
KubernetesTaskLauncherProperties taskLauncherProperties = new KubernetesTaskLauncherProperties();
taskLauncherProperties.setBackoffLimit(5);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where do we test for the backofflimit?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. We aren't testing backoffLimit in this test case rather the next one does. I kept this test as it used to be (except adding the new properties arg)

@cppwfs
Copy link
Contributor

cppwfs commented May 28, 2021

LGTM

@cppwfs
Copy link
Contributor

cppwfs commented May 28, 2021

Rebased, Squashed, Merged

@cppwfs cppwfs closed this May 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cppwfs cppwfs cppwfs left review comments

Assignees

@cppwfs cppwfs

Labels
None yet
Projects
None yet
Milestone
2.8.0-RC1
Development

Successfully merging this pull request may close these issues.

TaskLauncherProperties not being mapped properly from platform account
2 participants
@ilayaperumalg @cppwfs