[JENKINS-53790] PodTemplate Container Docker image check #497

pyieh · 2019-06-10T22:52:03Z

Check on ContainerWaitingState if the Docker image can't be found, prints message to build console and cancels and removes job from build queue.

JENKINS-53790
Cleaned up and reforked, previous PR #440

…ass it down to AllContainersPodWatcher, which does a check for invalid docker images used, will print an error msg to the build console and cancel the job to prevent infinte looping

Vlatombe · 2019-06-11T08:11:52Z

CHANGELOG.md

@@ -11,6 +11,10 @@ Unreleased
 ----------
 * Explicit inheritance should override implicit inheritance. Declarative k8s template do not inherit from parent pod template by default. [#480](https://github.com/jenkinsci/kubernetes-plugin/pull/480) [JENKINS-57548](https://issues.jenkins-ci.org/browse/JENKINS-57548)

+1.15.7
+------
+* Check on ContainerWaitingState if the Docker image can't be found, prints message to build console and cancels and removes job from build queue. [#497](https://github.com/jenkinsci/kubernetes-plugin/pull/497) [JENKINS-53790](https://issues.jenkins-ci.org/browse/JENKINS-53790)


Need to be moved to Unreleased

src/main/java/org/csanchez/jenkins/plugins/kubernetes/AllContainersRunningPodWatcher.java

Vlatombe · 2019-06-11T08:19:05Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/AllContainersRunningPodWatcher.java

+                            Queue q = jenkins.getQueue();
+                            for (Queue.Item item : q.getItems()) {
+                                Label itemLabel = item.getAssignedLabel();
+                                if (itemLabel != null && isCorrespondingLabels(itemLabel.getDisplayName(), pod.getMetadata().getName())) {


I understand the intent, but this is out of line here. Eventually could be added to exception handling in the launcher

kubernetes-plugin/src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesLauncher.java

Lines 200 to 208 in 3225773

setProblem(ex);

LOGGER.log(Level.WARNING, String.format("Error in provisioning; agent=%s, template=%s", slave, template), ex);

LOGGER.log(Level.FINER, "Removing Jenkins node: {0}", slave.getNodeName());

try {

slave.terminate();

} catch (IOException | InterruptedException e) {

LOGGER.log(Level.WARNING, "Unable to remove Jenkins node", e);

}

throw Throwables.propagate(ex);

Are you saying that the KubernetesLauncher should be in charge of canceling the job? We've seen that unless a job is canceled, it'll get put back in the queue and infinitely fail due to that bad pod template.

It would be a better place than in AllContainersRunningPodWatcher, certainly.

src/main/java/org/csanchez/jenkins/plugins/kubernetes/AllContainersRunningPodWatcher.java

…eady

…ntion

…ld item to use for canceling bad docker image pod template jobs, added tests for this

pyieh · 2019-08-08T23:15:42Z

@jglick @Vlatombe please take a look at the new changes.
We introduced a cleaner way of matching the created faulty pod to the build item in the queue that needs to be canceled.
Also added new tests for this.

CHANGELOG.md

markjacksonfishing · 2019-08-16T14:13:55Z

@jglick @Vlatombe I had a chat with the person who opened this PR at JWDW yesterday. I explained what they needed to do in order to get this PR correct

pyieh · 2019-12-02T20:13:04Z

@jglick Looks like you made some changes and have fixed the issue I was seeing.

jglick · 2019-12-02T20:31:21Z

Looks like you made some changes

#655

pyieh · 2020-01-13T21:23:19Z

@jglick @Vlatombe can I get a review for this? It was originally showing as all checks passing, but now checks are failing.

jglick · 2020-01-13T21:28:53Z

Test failures seem to be infrastructural. Suggest git pull origin master:master && git push to trigger a new build.

Vlatombe

Looks like I had several comments pending for a while, sorry for that

Vlatombe · 2019-12-13T08:21:46Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/AllContainersRunningPodWatcher.java

+                    String waitingStateMsg = waitingState.getMessage();
+                    if (waitingStateMsg != null && waitingStateMsg.contains("Back-off pulling image")) {
+                        runListener.error("Unable to pull Docker image \""+containerStatus.getImage()+"\". Check if image name is spelled correctly");
+                        throw new IllegalStateException("BAD_DOCKER_IMAGE");


Create a specific exception type.

The exception is needed in the stack of AllContainersRunningPodWatcher.areAllContainersRunning()<- periodicAwait() <- await().

Creating a new exception type causes a clash in AllContainersRunningPodWatcher.eventReceived() which is overriding the parent method of Watcher.eventReceived()
To allow the aforementioned flow, it also causes the flow of areAllContainersRunning() <- updateState() <- eventReceived()
Would you suggest:

catching the exception at updateState() or eventReceived() such that eventReceived() has to also pass it along.

Remain with the branching usage of the IllegalStateException, which eventReceived() already throws

Some alternative ?

Make your exception extend RuntimeException.

Vlatombe · 2019-12-13T08:22:33Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesLauncher.java

            try (Watch _w = client.pods().inNamespace(namespace1).withName(podName).watch(watcher)) {
                watcher.await(template.getSlaveConnectTimeout(), TimeUnit.SECONDS);
+            } catch (IllegalStateException e) {
+                if (e.getMessage().equals("BAD_DOCKER_IMAGE")) {


src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesLauncher.java

Vlatombe · 2019-12-13T08:25:13Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesLauncher.java

+    /* itemTaskName is format of "part of <ORGANIZATION> <JOB NAME> >> <BRANCH> #<BUILD NUMBER> */
+    /* <ORGANIZATION> and <BRANCH> are only there if Pipeline created through BlueOcean, else just <JOB NAME>  */
+    private String getJobName(String itemTaskName) {
+        final String partOfStr = "part of ";


very fragile

Removed this method as the only use was logging in the case there was an unexpected job name and not actually used in the feature functionality.

Vlatombe · 2019-12-13T08:26:26Z

src/main/java/org/csanchez/jenkins/plugins/kubernetes/PodTemplate.java

@@ -14,6 +14,7 @@
 import javax.annotation.Nonnull;

 import com.google.common.annotations.VisibleForTesting;
+import hudson.model.*;


Set your IDE to avoid wildcard imports.

…sed getJobName method

Vlatombe · 2020-01-23T07:48:42Z

We have a flake (ContainerExecDecoratorTest.testCommandExecution) which can be ignored, but there are two other tests failing that need to be addressed.

kind / org.csanchez.jenkins.plugins.kubernetes.pipeline.KubernetesDeclarativeAgentTest.declarativeWithNonexistentDockerImage | 3 min 0 sec | 4
kind / org.csanchez.jenkins.plugins.kubernetes.pipeline.KubernetesDeclarativeAgentTest.declarativeWithNonexistentDockerImageLongLabel | 3 min 0 sec | 4
kind / org.csanchez.jenkins.plugins.kubernetes.pipeline.ContainerExecDecoratorTest.testCommandExecution

pyieh · 2020-02-07T23:14:21Z

@Vlatombe I've fixed those tests. But the KubernetesPipelineTest.runInPodFromYaml is failing in Jenkins, but it's passing for me locally.

However, KubernetesPipelineTest.cascadeDelete is failing for me locally. Even a clean clone from the jenkinsci/kubernetes-plugin master is failing.
Any suggestions?

Vlatombe

Looks okay overall.

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesLauncher.java

pyieh · 2020-02-10T19:59:06Z

@jglick please review. Thank you.

pyieh added 2 commits June 10, 2019 15:48

PodTemplate will get TaskListener from PodTemplateStepExectuion and p…

ebd3552

…ass it down to AllContainersPodWatcher, which does a check for invalid docker images used, will print an error msg to the build console and cancel the job to prevent infinte looping

updated CHANGELOG to have PR number

a38615d

pyieh mentioned this pull request Jun 10, 2019

[JENKINS-53790] PodTemplate container Docker image check #440

Closed

Vlatombe changed the title ~~PodTemplate Container Docker image check (revised)~~ [JENKINS-53790] PodTemplate Container Docker image check Jun 11, 2019

Vlatombe requested changes Jun 11, 2019

View reviewed changes

jglick reviewed Jun 11, 2019

View reviewed changes

src/main/java/org/csanchez/jenkins/plugins/kubernetes/AllContainersRunningPodWatcher.java Outdated Show resolved Hide resolved

src/main/java/org/csanchez/jenkins/plugins/kubernetes/AllContainersRunningPodWatcher.java Outdated Show resolved Hide resolved

pyieh changed the title ~~[JENKINS-53790] PodTemplate Container Docker image check~~ [JENKINS-53790] PodTemplate Container Docker image check (revised) Jun 11, 2019

pyieh changed the title ~~[JENKINS-53790] PodTemplate Container Docker image check (revised)~~ [JENKINS-53790] PodTemplate Container Docker image check Jun 11, 2019

pyieh added 5 commits June 11, 2019 11:21

moved CHANGELOG msg to unreleased

ad45bc2

Merge remote-tracking branch 'mainRepo/master'

193d12e

moved CHANGELOG msg to unreleased

fa5960e

changed getInstanceOrNull() call to get() as it handles null case alr…

15a0c39

…eady

Moved canceling of job to KubernetesLauncher

5738775

Vlatombe added the enhancement Improvements label Jul 6, 2019

pyieh added 7 commits August 2, 2019 14:10

fixed merge conflicts

314ae4b

BadDockerImage test

f866997

removed BadDockerImageTemplate stuff

2e45487

fixed isCorrespondingLabels() to take into account podId suffix conve…

8f4b2d0

…ntion

Removed debugging logs

fe45af1

Removed debugging logs

621ab12

Added pod annotation when creating pod to be used to link back to bui…

6564e29

…ld item to use for canceling bad docker image pod template jobs, added tests for this

jglick self-requested a review August 9, 2019 18:44

jglick reviewed Aug 16, 2019

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

jglick added the work in progress label Aug 16, 2019

pyieh added 4 commits August 20, 2019 13:25

Merge branch 'master' of https://github.com/jenkinsci/kubernetes-plugin

665a45c

Merge branch 'master' of https://github.com/jenkinsci/kubernetes-plugin

9ff1367

removed my changes to do clean test

ae2a201

removed new tests

883ebf0

pyieh requested a review from jglick December 13, 2019 23:14

Vlatombe reviewed Jan 14, 2020

View reviewed changes

Vlatombe and others added 8 commits January 14, 2020 09:29

Merge branch 'master' into master

24ce8c2

Removed wildcard import

c5777c1

removed unnecessary job name parsing

166724f

created specific exception type for invalid docker image, removed unu…

47c5965

…sed getJobName method

Merge branch 'master' of https://github.com/pyieh/kubernetes-plugin

79dd83c

resolve merge conflicts

23a970f

Merge branch 'master' into master

75bcac8

dummy commit to retrigger build

33fee99

changed check to contains in InvalidDockerImageException

973b6ac

Merge branch 'master' into master

01ffb7c

Vlatombe approved these changes Feb 10, 2020

View reviewed changes

src/main/java/org/csanchez/jenkins/plugins/kubernetes/KubernetesLauncher.java Outdated Show resolved Hide resolved

pyieh added 2 commits February 10, 2020 11:51

removed msg contains check because specific excpetion type created

f97b87c

Merge branch 'master' of https://github.com/pyieh/kubernetes-plugin

5f84340

Vlatombe removed the work in progress label Feb 12, 2020

Vlatombe added 3 commits February 17, 2020 09:54

Merge branch 'master' into master

d736fbf

Merge branch 'master' into master

d3586a2

Merge branch 'master' into master

ecc1f9a

Vlatombe merged commit a2abfcc into jenkinsci:master Feb 19, 2020

Vlatombe mentioned this pull request Mar 9, 2020

Fix copy constructor of PodTemplate to keep TaskListener #730

Merged

Vlatombe mentioned this pull request Mar 17, 2020

Log events in build log during pod provisioning #742

Merged

jglick mentioned this pull request Dec 8, 2021

Removing unused AllContainersRunningPodWatcher.runListener #1088

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[JENKINS-53790] PodTemplate Container Docker image check #497

[JENKINS-53790] PodTemplate Container Docker image check #497

pyieh commented Jun 10, 2019 •

edited

Vlatombe Jun 11, 2019

Vlatombe Jun 11, 2019

pyieh Jun 11, 2019

Vlatombe Jun 12, 2019

pyieh commented Aug 8, 2019

markjacksonfishing commented Aug 16, 2019

pyieh commented Dec 2, 2019

jglick commented Dec 2, 2019

pyieh commented Jan 13, 2020

jglick commented Jan 13, 2020

Vlatombe left a comment

Vlatombe Dec 13, 2019

pyieh Jan 14, 2020

Vlatombe Jan 15, 2020

Vlatombe Dec 13, 2019

Vlatombe Dec 13, 2019

pyieh Jan 14, 2020

Vlatombe Dec 13, 2019

Vlatombe commented Jan 23, 2020

pyieh commented Feb 7, 2020 •

edited

Vlatombe left a comment

pyieh commented Feb 10, 2020

	setProblem(ex);
	LOGGER.log(Level.WARNING, String.format("Error in provisioning; agent=%s, template=%s", slave, template), ex);
	LOGGER.log(Level.FINER, "Removing Jenkins node: {0}", slave.getNodeName());
	try {
	slave.terminate();
	} catch (IOException \| InterruptedException e) {
	LOGGER.log(Level.WARNING, "Unable to remove Jenkins node", e);
	}
	throw Throwables.propagate(ex);

[JENKINS-53790] PodTemplate Container Docker image check #497

[JENKINS-53790] PodTemplate Container Docker image check #497

Conversation

pyieh commented Jun 10, 2019 • edited

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

pyieh commented Aug 8, 2019

markjacksonfishing commented Aug 16, 2019

pyieh commented Dec 2, 2019

jglick commented Dec 2, 2019

pyieh commented Jan 13, 2020

jglick commented Jan 13, 2020

Vlatombe 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

Vlatombe commented Jan 23, 2020

pyieh commented Feb 7, 2020 • edited

Vlatombe left a comment

Choose a reason for hiding this comment

pyieh commented Feb 10, 2020

pyieh commented Jun 10, 2019 •

edited

pyieh commented Feb 7, 2020 •

edited