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

[JENKINS-58767] Persist yaml fragment in a single string #573

Merged
merged 5 commits into from
Aug 1, 2019
Merged

[JENKINS-58767] Persist yaml fragment in a single string #573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4a8e70f
[JENKINS-58767] Persist yaml fragment in a single string
Vlatombe Aug 1, 2019
d39c378
Remove deprecated mention
Vlatombe Aug 1, 2019
6cae60b
Damn
Vlatombe Aug 1, 2019
955d70b
Fix consistency
Vlatombe Aug 1, 2019
9d79926
Fix review and add a warning for invalid persisted fragments
Vlatombe Aug 1, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 35 additions & 25 deletions src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
import javax.annotation.CheckForNull;
import javax.annotation.Nonnull;

import com.google.common.annotations.VisibleForTesting;
import org.apache.commons.lang.StringUtils;
import org.csanchez.jenkins.plugins.kubernetes.model.TemplateEnvVar;
import org.csanchez.jenkins.plugins.kubernetes.pod.retention.PodRetention;
Expand Down Expand Up @@ -123,12 +124,14 @@ public class PodTemplate extends AbstractDescribableImpl<PodTemplate> implements
private PodTemplateToolLocation nodeProperties;

/**
* @deprecated Stored as a list of yaml fragments
* Persisted yaml fragment
*/
@Deprecated
private transient String yaml;
private String yaml;

private List<String> yamls = new ArrayList<>();
/**
* List of yaml fragments used for transient pod templates. Never persisted
Copy link
Member

Choose a reason for hiding this comment

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

Could this field just be deleted, and any call to setYamls with a list of size > 1 would just set yaml to something separated by "\n----\n" or whatever? Would be easier to follow the logic and be sure that you are not missing some complication.

Copy link
Member Author

Choose a reason for hiding this comment

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

hum, I guess it would increase complexity of getYaml, not sure I want it

Copy link
Member Author

Choose a reason for hiding this comment

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

Problem I see is that would end up being an invalid yaml fragment to parse into a pod template

Copy link
Member

Choose a reason for hiding this comment

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

As you see fit. I do not really understand what the list of YAMLs is for to begin with.

*/
private transient List<String> yamls;

public YamlMergeStrategy getYamlMergeStrategy() {
return yamlMergeStrategy;
Expand Down Expand Up @@ -170,6 +173,7 @@ public PodTemplate(PodTemplate from) {
this.setActiveDeadlineSeconds(from.getActiveDeadlineSeconds());
this.setVolumes(from.getVolumes());
this.setWorkspaceVolume(from.getWorkspaceVolume());
this.yaml = from.yaml;
this.setYamls(from.getYamls());
this.setShowRawYaml(from.isShowRawYaml());
this.setNodeProperties(from.getNodeProperties());
Expand Down Expand Up @@ -633,31 +637,36 @@ public List<ContainerTemplate> getContainers() {
}

/**
* @return The first yaml fragment for this pod template
* @return The persisted yaml fragment
*/
Vlatombe marked this conversation as resolved.
Show resolved Hide resolved
@Restricted(NoExternalUse.class) // Tests and UI
public String getYaml() {
return yamls == null || yamls.isEmpty() ? null : yamls.get(0);
return yaml;
}

@DataBoundSetter
public void setYaml(String yaml) {
String trimmed = Util.fixEmpty(yaml);
if (trimmed != null) {
this.yamls = Collections.singletonList(yaml);
} else {
this.yamls = Collections.emptyList();
}
this.yaml = Util.fixEmpty(yaml);
}

@Nonnull
public List<String> getYamls() {
if (yamls ==null) {
return Collections.emptyList();
if (yamls == null) {
if (yaml != null) {
return Collections.singletonList(yaml);
} else {
return Collections.emptyList();
}
}
return yamls;
}

@VisibleForTesting
@Restricted(NoExternalUse.class)
List<String> _getYamls() {
return yamls;
}

public void setYamls(List<String> yamls) {
if (yamls != null) {
List<String> ys = new ArrayList<>();
Expand Down Expand Up @@ -716,18 +725,19 @@ protected Object readResolve() {
annotations = new ArrayList<>();
}

if (yamls == null) {
yamls = new ArrayList<>();
}

if (yaml != null) {
yamls.add(yaml);
yaml = null;
}

// JENKINS-57116 remove empty items from yamls
if (!yamls.isEmpty() && StringUtils.isBlank(yamls.get(0))) {
// Sanitize empty values
yaml = Util.fixEmpty(yaml);
if (yamls != null) {
// JENKINS-57116 Sanitize empty values
setYamls(yamls);
// Migration from storage in yamls field
if (!yamls.isEmpty()) {
if (yamls.size() > 1) {
LOGGER.log(Level.WARNING, "Found several persisted YAML fragments in pod template " + name + ". Only the first fragment will be considered, others will be ignored.");
}
yaml = yamls.get(0);
Vlatombe marked this conversation as resolved.
Show resolved Hide resolved
}
yamls = null;
}

if (showRawYaml == null) {
Expand Down
29 changes: 29 additions & 0 deletions src/test/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@
import java.util.Collections;
import java.util.List;
import java.util.Map;
import java.util.logging.Level;

import org.csanchez.jenkins.plugins.kubernetes.model.KeyValueEnvVar;
import org.csanchez.jenkins.plugins.kubernetes.pod.retention.Default;
Expand All @@ -44,6 +45,7 @@
import org.junit.Test;
import org.jvnet.hudson.test.Issue;
import org.jvnet.hudson.test.JenkinsRule;
import org.jvnet.hudson.test.LoggerRule;
import org.jvnet.hudson.test.recipes.LocalData;

import com.cloudbees.plugins.credentials.Credentials;
Expand All @@ -68,6 +70,9 @@ public class KubernetesTest {
@Rule
public JenkinsRule r = new JenkinsRule();

@Rule
public LoggerRule log = new LoggerRule();

private KubernetesCloud cloud;

@Before
Expand All @@ -85,6 +90,30 @@ public void upgradeFrom_1_17_2() throws Exception {
assertThat(cloud.getPodLabelsMap(), hasEntry("biff", "johnson"));
}

@Test
@LocalData
public void upgradeFrom_1_15_9() {
List<PodTemplate> templates = cloud.getTemplates();
assertPodTemplates(templates);
PodTemplate template = templates.get(0);
assertEquals("blah", template.getYaml());
assertEquals(Collections.singletonList("blah"), template.getYamls());
assertNull(template._getYamls());
}

@Test
@LocalData
public void upgradeFrom_1_15_9_invalid() {
Copy link
Member

Choose a reason for hiding this comment

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

Probably overkill, but OK.

log.record(PodTemplate.class, Level.WARNING).capture(1);
List<PodTemplate> templates = cloud.getTemplates();
assertPodTemplates(templates);
PodTemplate template = templates.get(0);
assertEquals("blah", template.getYaml());
assertEquals(Collections.singletonList("blah"), template.getYamls());
assertNull(template._getYamls());
log.getMessages().stream().anyMatch(msg -> msg.contains("Found several persisted YAML fragments in pod template java"));
}

@Test
@LocalData()
@Issue("JENKINS-57116")
Expand Down
144 changes: 144 additions & 0 deletions ...rces/org/csanchez/jenkins/plugins/kubernetes/KubernetesTest/upgradeFrom_1_15_9/config.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,144 @@
<?xml version='1.1' encoding='UTF-8'?>
<hudson>
<disabledAdministrativeMonitors/>
<version>2.138.4</version>
<installStateName>DEVELOPMENT</installStateName>
<numExecutors>2</numExecutors>
<mode>NORMAL</mode>
<useSecurity>true</useSecurity>
<authorizationStrategy class="hudson.security.AuthorizationStrategy$Unsecured"/>
<securityRealm class="hudson.security.SecurityRealm$None"/>
<disableRememberMe>false</disableRememberMe>
<projectNamingStrategy class="jenkins.model.ProjectNamingStrategy$DefaultProjectNamingStrategy"/>
<workspaceDir>${JENKINS_HOME}/workspace/${ITEM_FULLNAME}</workspaceDir>
<buildsDir>${ITEM_ROOTDIR}/builds</buildsDir>
<jdks/>
<viewsTabBar class="hudson.views.DefaultViewsTabBar"/>
<myViewsTabBar class="hudson.views.DefaultMyViewsTabBar"/>
<clouds>
<org.csanchez.jenkins.plugins.kubernetes.KubernetesCloud plugin="kubernetes@1.15.9">
<name>kubernetes</name>
<defaultsProviderTemplate></defaultsProviderTemplate>
<templates>
<org.csanchez.jenkins.plugins.kubernetes.PodTemplate>
<inheritFrom></inheritFrom>
<name>java</name>
<namespace></namespace>
<privileged>false</privileged>
<capOnlyOnAlivePods>false</capOnlyOnAlivePods>
<alwaysPullImage>false</alwaysPullImage>
<instanceCap>2147483647</instanceCap>
<slaveConnectTimeout>100</slaveConnectTimeout>
<idleMinutes>0</idleMinutes>
<activeDeadlineSeconds>0</activeDeadlineSeconds>
<label>java</label>
<nodeSelector></nodeSelector>
<nodeUsageMode>NORMAL</nodeUsageMode>
<customWorkspaceVolumeEnabled>false</customWorkspaceVolumeEnabled>
<workspaceVolume class="org.csanchez.jenkins.plugins.kubernetes.volumes.workspace.EmptyDirWorkspaceVolume">
<memory>false</memory>
</workspaceVolume>
<volumes>
<org.csanchez.jenkins.plugins.kubernetes.volumes.EmptyDirVolume>
<mountPath>/mnt</mountPath>
<memory>false</memory>
</org.csanchez.jenkins.plugins.kubernetes.volumes.EmptyDirVolume>
<org.csanchez.jenkins.plugins.kubernetes.volumes.HostPathVolume>
<mountPath>/host</mountPath>
<hostPath>/mnt/host</hostPath>
</org.csanchez.jenkins.plugins.kubernetes.volumes.HostPathVolume>
</volumes>
<containers>
<org.csanchez.jenkins.plugins.kubernetes.ContainerTemplate>
<name>jnlp</name>
<image>jenkins/jnlp-slave</image>
<privileged>false</privileged>
<alwaysPullImage>false</alwaysPullImage>
<workingDir>/home/jenkins</workingDir>
<command></command>
<args>${computer.jnlpmac} ${computer.name}</args>
<ttyEnabled>false</ttyEnabled>
<resourceRequestCpu>500m</resourceRequestCpu>
<resourceRequestMemory>250Mi</resourceRequestMemory>
<resourceLimitCpu>500m</resourceLimitCpu>
<resourceLimitMemory>250Mi</resourceLimitMemory>
<envVars>
<org.csanchez.jenkins.plugins.kubernetes.PodEnvVar>
<key>a</key>
<value>b</value>
</org.csanchez.jenkins.plugins.kubernetes.PodEnvVar>
<org.csanchez.jenkins.plugins.kubernetes.PodEnvVar>
<key>c</key>
<value>d</value>
</org.csanchez.jenkins.plugins.kubernetes.PodEnvVar>
</envVars>
<ports/>
<livenessProbe>
<execArgs></execArgs>
<timeoutSeconds>0</timeoutSeconds>
<initialDelaySeconds>0</initialDelaySeconds>
<failureThreshold>0</failureThreshold>
<periodSeconds>0</periodSeconds>
<successThreshold>0</successThreshold>
</livenessProbe>
</org.csanchez.jenkins.plugins.kubernetes.ContainerTemplate>
</containers>
<envVars>
<org.csanchez.jenkins.plugins.kubernetes.PodEnvVar>
<key>a</key>
<value>b</value>
</org.csanchez.jenkins.plugins.kubernetes.PodEnvVar>
<org.csanchez.jenkins.plugins.kubernetes.PodEnvVar>
<key>c</key>
<value>d</value>
</org.csanchez.jenkins.plugins.kubernetes.PodEnvVar>
</envVars>
<annotations>
<org.csanchez.jenkins.plugins.kubernetes.PodAnnotation>
<key>aa</key>
<value>bb</value>
</org.csanchez.jenkins.plugins.kubernetes.PodAnnotation>
</annotations>
<imagePullSecrets>
<org.csanchez.jenkins.plugins.kubernetes.PodImagePullSecret>
<name></name>
</org.csanchez.jenkins.plugins.kubernetes.PodImagePullSecret>
</imagePullSecrets>
<yamls>
<string>blah</string>
</yamls>
<podRetention class="org.csanchez.jenkins.plugins.kubernetes.pod.retention.Default"/>
</org.csanchez.jenkins.plugins.kubernetes.PodTemplate>
</templates>
<serverUrl>https://192.168.64.1</serverUrl>
<skipTlsVerify>true</skipTlsVerify>
<addMasterProxyEnvVars>false</addMasterProxyEnvVars>
<capOnlyOnAlivePods>false</capOnlyOnAlivePods>
<namespace>default</namespace>
<containerCap>10</containerCap>
<retentionTimeout>5</retentionTimeout>
<connectTimeout>0</connectTimeout>
<readTimeout>0</readTimeout>
<usageRestricted>false</usageRestricted>
<maxRequestsPerHost>32</maxRequestsPerHost>
<waitForPodSec>600</waitForPodSec>
<podRetention class="org.csanchez.jenkins.plugins.kubernetes.pod.retention.Never"/>
</org.csanchez.jenkins.plugins.kubernetes.KubernetesCloud>
</clouds>
<quietPeriod>5</quietPeriod>
<scmCheckoutRetryCount>0</scmCheckoutRetryCount>
<views>
<hudson.model.AllView>
<owner class="hudson" reference="../../.."/>
<name>all</name>
<filterExecutors>false</filterExecutors>
<filterQueue>false</filterQueue>
<properties class="hudson.model.View$PropertyList"/>
</hudson.model.AllView>
</views>
<primaryView>all</primaryView>
<slaveAgentPort>0</slaveAgentPort>
<label></label>
<nodeProperties/>
<globalNodeProperties/>
</hudson>
Loading