Skip to content

Commit

Permalink
Fix missing ephemeral storeage and hugepages properties.
Browse files Browse the repository at this point in the history 
See spring-cloud#500

Fix missing ephemeral storeage and hugepages properties.

Fix formatting.

Fix missing ephemeral storeage and hugepages properties.

Revert LimitsResources constructor to deprecated version.

Fix missing ephemeral storeage and hugepages properties.

Revert RequestsResources constructor to previous version and mark as deprecated.

Added extra all args constructors.
Documented and updated gpuVendor usage. According to documentation there is no common format for the gpu vendor string and it has to be provided as needed.

Remove resolve from JFrog-CLI to have it resolve depedencies from settings and pom files

Signed-off-by: Glenn Renfro <[email protected]>

Update gpu based tests in RunAbstractKubernetesDeployerTests

It was determined that our addition of /gpu in limit determination was incorrect.  meaning it is not standard.
The tests were not updated based on this code change.  This commit resolves this

Signed-off-by: Glenn Renfro <[email protected]>

Remove deprecated constructors from KubernetesDeployerProperties.

One deprecation was in error and removed from this PR.
Signed-off-by: Glenn Renfro <[email protected]>
  • Loading branch information
@cppwfs
Corneil du Plessis authored and cppwfs committed Dec 17, 2024
1 parent 83aebd8 commit f6def17
Show file tree
Hide file tree
Showing 4 changed files with 308 additions and 59 deletions.
82 changes: 39 additions & 43 deletions .../java/org/springframework/cloud/deployer/spi/kubernetes/DeploymentPropertiesResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -164,46 +164,38 @@ List<Volume> getVolumes(Map<String, String> kubernetesDeployerProperties) {
* @return the resource limits to use
*/
Map<String, Quantity> deduceResourceLimits(Map<String, String> kubernetesDeployerProperties) {
String memory = PropertyParserUtils.getDeploymentPropertyValue(kubernetesDeployerProperties,
this.propertyPrefix + ".limits.memory");
String memory = PropertyParserUtils.getDeploymentPropertyValue(kubernetesDeployerProperties, this.propertyPrefix + ".limits.memory", properties.getLimits().getMemory());
String cpu = PropertyParserUtils.getDeploymentPropertyValue(kubernetesDeployerProperties, this.propertyPrefix + ".limits.cpu", properties.getLimits().getCpu());
String ephemeralStorage = PropertyParserUtils.getDeploymentPropertyValue(kubernetesDeployerProperties, ".limits.ephemeral-storage", properties.getLimits().getEphemeralStorage());
String hugePages2Mi = PropertyParserUtils.getDeploymentPropertyValue(kubernetesDeployerProperties, ".limits.hugepages-2Mi", properties.getLimits().getHugepages2Mi());
String hugePages1Gi = PropertyParserUtils.getDeploymentPropertyValue(kubernetesDeployerProperties, ".limits.hugepages-1Gi", properties.getLimits().getHugepages1Gi());
String gpuVendor = PropertyParserUtils.getDeploymentPropertyValue(kubernetesDeployerProperties, this.propertyPrefix + ".limits.gpuVendor", properties.getLimits().getGpuVendor());
String gpuCount = PropertyParserUtils.getDeploymentPropertyValue(kubernetesDeployerProperties, this.propertyPrefix + ".limits.gpuCount", properties.getLimits().getGpuCount());

if (!StringUtils.hasText(memory)) {
memory = properties.getLimits().getMemory();
}

String cpu = PropertyParserUtils.getDeploymentPropertyValue(kubernetesDeployerProperties,
this.propertyPrefix + ".limits.cpu");
Map<String,Quantity> limits = new HashMap<String,Quantity>();

if (!StringUtils.hasText(cpu)) {
cpu = properties.getLimits().getCpu();
if (StringUtils.hasText(memory)) {
limits.put("memory", new Quantity(memory));
}

String gpuVendor = PropertyParserUtils.getDeploymentPropertyValue(kubernetesDeployerProperties,
this.propertyPrefix + ".limits.gpuVendor");

if (!StringUtils.hasText(gpuVendor)) {
gpuVendor = properties.getLimits().getGpuVendor();
if (StringUtils.hasText(cpu)) {
limits.put("cpu", new Quantity(cpu));
}

String gpuCount = PropertyParserUtils.getDeploymentPropertyValue(kubernetesDeployerProperties,
this.propertyPrefix + ".limits.gpuCount");

if (!StringUtils.hasText(gpuCount)) {
gpuCount = properties.getLimits().getGpuCount();
if (StringUtils.hasText(ephemeralStorage)) {
limits.put("ephemeral-storage", new Quantity(ephemeralStorage));
}

Map<String,Quantity> limits = new HashMap<String,Quantity>();

if (StringUtils.hasText(memory)) {
limits.put("memory", new Quantity(memory));
if (StringUtils.hasText(hugePages2Mi)) {
limits.put("hugepages-2Mi", new Quantity(hugePages2Mi));
}

if (StringUtils.hasText(cpu)) {
limits.put("cpu", new Quantity(cpu));
if (StringUtils.hasText(hugePages1Gi)) {
limits.put("hugepages-1Gi", new Quantity(hugePages1Gi));
}

if (StringUtils.hasText(gpuVendor) && StringUtils.hasText(gpuCount)) {
limits.put(gpuVendor + "/gpu", new Quantity(gpuCount));
limits.put(gpuVendor, new Quantity(gpuCount));
}

return limits;
Expand Down Expand Up @@ -245,32 +237,36 @@ ImagePullPolicy deduceImagePullPolicy(Map<String, String> kubernetesDeployerProp
* @return the resource requests to use
*/
Map<String, Quantity> deduceResourceRequests(Map<String, String> kubernetesDeployerProperties) {
String memOverride = PropertyParserUtils.getDeploymentPropertyValue(kubernetesDeployerProperties,
this.propertyPrefix + ".requests.memory");

if (memOverride == null) {
memOverride = properties.getRequests().getMemory();
}


String cpuOverride = PropertyParserUtils.getDeploymentPropertyValue(kubernetesDeployerProperties,
this.propertyPrefix + ".requests.cpu");

if (cpuOverride == null) {
cpuOverride = properties.getRequests().getCpu();
}
String memOverride = PropertyParserUtils.getDeploymentPropertyValue(kubernetesDeployerProperties, this.propertyPrefix + ".requests.memory", properties.getRequests().getMemory());
String cpuOverride = PropertyParserUtils.getDeploymentPropertyValue(kubernetesDeployerProperties, this.propertyPrefix + ".requests.cpu", properties.getRequests().getCpu());

logger.debug("Using requests - cpu: " + cpuOverride + " mem: " + memOverride);

String ephemeralStorage = PropertyParserUtils.getDeploymentPropertyValue(kubernetesDeployerProperties, ".requests.ephemeral-storage", properties.getRequests().getEphemeralStorage());
String hugePages2Mi = PropertyParserUtils.getDeploymentPropertyValue(kubernetesDeployerProperties, ".requests.hugepages-2Mi", properties.getRequests().getHugepages2Mi());
String hugePages1Gi = PropertyParserUtils.getDeploymentPropertyValue(kubernetesDeployerProperties, ".requests.hugepages-1Gi", properties.getRequests().getHugepages1Gi());

Map<String,Quantity> requests = new HashMap<String, Quantity>();

if (memOverride != null) {
if (StringUtils.hasText(memOverride)) {
requests.put("memory", new Quantity(memOverride));
}

if (cpuOverride != null) {
if (StringUtils.hasText(cpuOverride)) {
requests.put("cpu", new Quantity(cpuOverride));
}
if(StringUtils.hasText(ephemeralStorage)) {
requests.put("ephemeral-storage", new Quantity(ephemeralStorage));
}

if(StringUtils.hasText(hugePages2Mi)) {
requests.put("hugepages-2Mi", new Quantity(hugePages2Mi));
}

if(StringUtils.hasText(hugePages1Gi)) {
requests.put("hugepages-1Gi", new Quantity(hugePages1Gi));
}


return requests;
}
Expand Down
125 changes: 115 additions & 10 deletions .../java/org/springframework/cloud/deployer/spi/kubernetes/KubernetesDeployerProperties.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -89,9 +89,28 @@ public static class LimitsResources {
* Container resource memory limit.
*/
private String memory;
/**
* Container resource ephemeral-storage limit.
*/
private String ephemeralStorage;

/**
* Container resource hugepages-2Mi limit.
*/
private String hugepages2Mi;

/**
* Container resource hugepages-1Gi limit.
*/
private String hugepages1Gi;

/**
* Container GPU vendor name for limit
* If gpuVendor=nvidia.com/gpu and gpuCount=2 then the following will be used
* {@code
* limits:
* nvidia.com/gpu: 2
* }
*/
private String gpuVendor;

Expand All @@ -102,20 +121,24 @@ public static class LimitsResources {

public LimitsResources() {
}

/**
* 'All' args constructor
*
* @param cpu Container resource cpu limit
* @param memory Container resource memory limit
* @deprecated This method should no longer be used to set all fields at construct time.
* <p>
* Use the default constructor and set() methods instead.
* New all args constructor
* @param cpu Container limit for cpu resource
* @param memory Container limit for memory resource
* @param ephemeralStorage Container limit for ephemetal storage
* @param hugepages2Mi Container limit for 2M huge pages
* @param hugepages1Gi Container limit for 1G huge pages
* @param gpuVendor The complete limit entry name for gpu vendor.
* @param gpuCount
*/
@Deprecated
public LimitsResources(String cpu, String memory) {
public LimitsResources(String cpu, String memory, String ephemeralStorage, String hugepages2Mi, String hugepages1Gi, String gpuVendor, String gpuCount) {
this.cpu = cpu;
this.memory = memory;
this.ephemeralStorage = ephemeralStorage;
this.hugepages2Mi = hugepages2Mi;
this.hugepages1Gi = hugepages1Gi;
this.gpuVendor = gpuVendor;
this.gpuCount = gpuCount;
}

public String getCpu() {
Expand All @@ -134,6 +157,30 @@ public void setMemory(String memory) {
this.memory = memory;
}

public String getEphemeralStorage() {
return ephemeralStorage;
}

public void setEphemeralStorage(String ephemeralStorage) {
this.ephemeralStorage = ephemeralStorage;
}

public String getHugepages2Mi() {
return hugepages2Mi;
}

public void setHugepages2Mi(String hugepages2Mi) {
this.hugepages2Mi = hugepages2Mi;
}

public String getHugepages1Gi() {
return hugepages1Gi;
}

public void setHugepages1Gi(String hugepages1Gi) {
this.hugepages1Gi = hugepages1Gi;
}

public String getGpuVendor() {
return gpuVendor;
}
Expand Down Expand Up @@ -166,13 +213,47 @@ public static class RequestsResources {
*/
private String memory;

/**
* Container resource ephemeral-storage request.
*/
private String ephemeralStorage;

/**
* Container resource hugepages-2Mi request.
*/
private String hugepages2Mi;

/**
* Container resource hugepages-1Gi request.
*/
private String hugepages1Gi;


public RequestsResources() {
}

/**
* 'All' args constructor
*
* @param cpu Container resource requested cpu
* @param memory Container resource requested memory
* @deprecated This method should no longer be used to set all fields at construct time.
* <p>
* Use the default constructor and set() methods instead.
*/
@Deprecated
public RequestsResources(String cpu, String memory) {
this.cpu = cpu;
this.memory = memory;
}
@Deprecated
public RequestsResources(String cpu, String memory, String ephemeralStorage, String hugepages2Mi, String hugepages1Gi) {
this.cpu = cpu;
this.memory = memory;
this.ephemeralStorage = ephemeralStorage;
this.hugepages2Mi = hugepages2Mi;
this.hugepages1Gi = hugepages1Gi;
}

public String getCpu() {
return cpu;
Expand All @@ -189,6 +270,30 @@ public String getMemory() {
public void setMemory(String memory) {
this.memory = memory;
}

public String getEphemeralStorage() {
return ephemeralStorage;
}

public void setEphemeralStorage(String ephemeralStorage) {
this.ephemeralStorage = ephemeralStorage;
}

public String getHugepages2Mi() {
return hugepages2Mi;
}

public void setHugepages2Mi(String hugepages2Mi) {
this.hugepages2Mi = hugepages2Mi;
}

public String getHugepages1Gi() {
return hugepages1Gi;
}

public void setHugepages1Gi(String hugepages1Gi) {
this.hugepages1Gi = hugepages1Gi;
}
}

public static class StatefulSet {
Expand Down
59 changes: 53 additions & 6 deletions ...org/springframework/cloud/deployer/spi/kubernetes/RunAbstractKubernetesDeployerTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,24 +77,24 @@ public void limitGpu_noDeploymentProperty_incompleteServerProperty2_noGpu() {

@Test
public void limitGpu_noDeploymentProperty_serverProperty_usesServerProperty() {
kubernetesDeployerProperties.getLimits().setGpuVendor("nvidia.com");
kubernetesDeployerProperties.getLimits().setGpuVendor("nvidia.com/gpu");
kubernetesDeployerProperties.getLimits().setGpuCount("2");
Map<String, Quantity> limits = this.deploymentPropertiesResolver.deduceResourceLimits(deploymentRequest.getDeploymentProperties());
assertThat(limits.get("nvidia.com/gpu")).isEqualTo(new Quantity("2"));
}

@Test
public void limitGpu_deploymentPropertyVendor_usesDeploymentProperty() {
kubernetesDeployerProperties.getLimits().setGpuVendor("nvidia.com");
kubernetesDeployerProperties.getLimits().setGpuVendor("nvidia.com/gpu");
kubernetesDeployerProperties.getLimits().setGpuCount("2");
deploymentProperties.put("spring.cloud.deployer.kubernetes.limits.gpu_vendor", "ati.com");
deploymentProperties.put("spring.cloud.deployer.kubernetes.limits.gpu_vendor", "ati.com/gpu");
Map<String, Quantity> limits = this.deploymentPropertiesResolver.deduceResourceLimits(deploymentRequest.getDeploymentProperties());
assertThat(limits.get("ati.com/gpu")).isEqualTo(new Quantity("2"));
}

@Test
public void limitGpu_deploymentPropertyCount_usesDeploymentProperty() {
kubernetesDeployerProperties.getLimits().setGpuVendor("nvidia.com");
kubernetesDeployerProperties.getLimits().setGpuVendor("nvidia.com/gpu");
kubernetesDeployerProperties.getLimits().setGpuCount("2");
deploymentProperties.put("spring.cloud.deployer.kubernetes.limits.gpu_count", "1");
Map<String, Quantity> limits = this.deploymentPropertiesResolver.deduceResourceLimits(deploymentRequest.getDeploymentProperties());
Expand All @@ -103,9 +103,9 @@ public void limitGpu_deploymentPropertyCount_usesDeploymentProperty() {

@Test
public void limitGpu_deploymentPropertyBoth_usesDeploymentProperty() {
kubernetesDeployerProperties.getLimits().setGpuVendor("nvidia.com");
kubernetesDeployerProperties.getLimits().setGpuVendor("nvidia.com/gpu");
kubernetesDeployerProperties.getLimits().setGpuCount("2");
deploymentProperties.put("spring.cloud.deployer.kubernetes.limits.gpu_vendor", "ati.com");
deploymentProperties.put("spring.cloud.deployer.kubernetes.limits.gpu_vendor", "ati.com/gpu");
deploymentProperties.put("spring.cloud.deployer.kubernetes.limits.gpu_count", "1");
Map<String, Quantity> limits = this.deploymentPropertiesResolver.deduceResourceLimits(deploymentRequest.getDeploymentProperties());
assertThat(limits.get("ati.com/gpu")).isEqualTo(new Quantity("1"));
Expand Down Expand Up @@ -170,4 +170,51 @@ public void requestMemory_deploymentProperty_usesDeploymentProperty() {
Map<String, Quantity> requests = this.deploymentPropertiesResolver.deduceResourceRequests(deploymentRequest.getDeploymentProperties());
assertThat(requests.get("memory")).isEqualTo(new Quantity("256Mi"));
}
@Test
public void requestEphemeralStorage_deploymentProperty_usesDeploymentProperty() {
kubernetesDeployerProperties.getRequests().setEphemeralStorage("2Gi");
deploymentProperties.put("spring.cloud.deployer.kubernetes.requests.ephemeral-storage", "2Gi");
Map<String, Quantity> requests = this.deploymentPropertiesResolver.deduceResourceRequests(deploymentRequest.getDeploymentProperties());
assertThat(requests.get("ephemeral-storage")).isEqualTo(new Quantity("2Gi"));
}

@Test
public void limitEphemeralStorage_deploymentProperty_usesDeploymentProperty() {
kubernetesDeployerProperties.getLimits().setEphemeralStorage("2Gi");
deploymentProperties.put("spring.cloud.deployer.kubernetes.limits.ephemeral-storage", "2Gi");
Map<String, Quantity> limits = this.deploymentPropertiesResolver.deduceResourceLimits(deploymentRequest.getDeploymentProperties());
assertThat(limits.get("ephemeral-storage")).isEqualTo(new Quantity("2Gi"));
}

@Test
public void requestHugepages1Gi_deploymentProperty_usesDeploymentProperty() {
kubernetesDeployerProperties.getRequests().setHugepages1Gi("4");
deploymentProperties.put("spring.cloud.deployer.kubernetes.requests.hugepages-1Gi", "4");
Map<String, Quantity> requests = this.deploymentPropertiesResolver.deduceResourceRequests(deploymentRequest.getDeploymentProperties());
assertThat(requests.get("hugepages-1Gi")).isEqualTo(new Quantity("4"));
}

@Test
public void limitHugepages1Gi_deploymentProperty_usesDeploymentProperty() {
kubernetesDeployerProperties.getLimits().setHugepages1Gi("4");
deploymentProperties.put("spring.cloud.deployer.kubernetes.limits.hugepages-1Gi", "4");
Map<String, Quantity> limits = this.deploymentPropertiesResolver.deduceResourceLimits(deploymentRequest.getDeploymentProperties());
assertThat(limits.get("hugepages-1Gi")).isEqualTo(new Quantity("4"));
}

@Test
public void requestHugepages2Mi_deploymentProperty_usesDeploymentProperty() {
kubernetesDeployerProperties.getRequests().setHugepages2Mi("40");
deploymentProperties.put("spring.cloud.deployer.kubernetes.requests.hugepages-2Mi", "40");
Map<String, Quantity> requests = this.deploymentPropertiesResolver.deduceResourceRequests(deploymentRequest.getDeploymentProperties());
assertThat(requests.get("hugepages-2Mi")).isEqualTo(new Quantity("40"));
}

@Test
public void limitHugepages2Mi_deploymentProperty_usesDeploymentProperty() {
kubernetesDeployerProperties.getLimits().setHugepages2Mi("40");
deploymentProperties.put("spring.cloud.deployer.kubernetes.limits.hugepages-2Mi", "40");
Map<String, Quantity> limits = this.deploymentPropertiesResolver.deduceResourceLimits(deploymentRequest.getDeploymentProperties());
assertThat(limits.get("hugepages-2Mi")).isEqualTo(new Quantity("40"));
}
}
Loading

0 comments on commit f6def17

Please sign in to comment.