sktm should make multiple attempts to query Jenkins #116

idorax · 2018-07-19T15:33:20Z

Since self.jk.is_build_complete() throws an exception when jenkins cannot be reached, we try to use 'try ... except ...' in is_build_complete() to avoid exceptions.

coveralls · 2018-07-19T15:37:13Z

Pull Request Test Coverage Report for Build 405

4 of 38 (10.53%) changed or added relevant lines in 2 files are covered.
2 unchanged lines in 2 files lost coverage.
Overall coverage decreased (-0.7%) to 37.234%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
sktm/executable.py	1	5	20.0%
sktm/jenkins.py	3	33	9.09%

Files with Coverage Reduction	New Missed Lines	%
sktm/executable.py	1	16.13%
sktm/jenkins.py	1	12.06%

Totals
Change from base Build 399:	-0.7%
Covered Lines:	465
Relevant Lines:	1122

💛 - Coveralls

veruu · 2018-07-20T09:56:37Z

Hi Vector! If you get to the final version of the pull, can you please squash the commits into a single one and let us know so we can check it?

idorax · 2018-07-20T14:58:15Z

Hi Veronika, the commits are squashed, please check it, thanks!

veruu

Okay, finally got to check this out :) I like the idea of resiliency eg. in case of temporary network failures, but it needs more thought. Would it be possible to make the retry count a command line option with a default value, and use similar logic every time sktm tries to communicate with jenkins, not only in this method? And in case we still can't reach jenkins after all those retries, abort with exception or something similar?

idorax · 2018-07-23T05:52:25Z

Veronika, I have updated the related .py files to support a command line option, please check again, thanks!

veruu

Thanks Vector, this is looking much better! :) I have some smaller comments for improvement, but the main idea is good. Can you also exchange the rest of get_job and get_build calls in the repo for the new methods so we have the resiliency everywhere?

veruu · 2018-07-23T11:33:24Z

sktm/executable.py

@@ -38,6 +38,7 @@ def setup_parser():
    parser.add_argument("--jurl", help="Jenkins URL")
    parser.add_argument("--jlogin", help="Jenkins login")
    parser.add_argument("--jpass", help="Jenkins password")
+    parser.add_argument("--jretry", help="Counter to retry Jenkins")


You can add type=int keyword argument here and then don't need to cast the option to int later. Granted, the value read from the config file won't be int, so you can combine this comment with the one below (about the default value assignment in load_config and do the retyping there.

Can you also mention the default value in the help string please? A simple (defaults to X) at the end is enough.

The default value is added to the help string.

veruu · 2018-07-23T11:41:10Z

sktm/jenkins.py

@@ -23,7 +23,8 @@

 class JenkinsProject(object):
    """Jenkins project interface"""
-    def __init__(self, name, url, username=None, password=None):
+    def __init__(self, name, url, username=None, password=None,
+                 retry_cnt="30"):


I don't think this will work (that said, the username and password params don't need the defaults either) -- if the option is not present, cfg.get('jretry') will return None, which is a perfectly valid value, and since the option will be passed the default is never used.

Instead, you should either assign a default value at the end of the load_config method (if the option is not None) or in this method (the first one is preferred)

Does this change work fine? (I didn't update load_config() yet)
- parser.add_argument("--jretry", help="Counter to retry Jenkins")
+ parser.add_argument("--jretry", help="Counter to retry Jenkins",
+ type=int, default=30)

This would work for command line options but not the config file. You'll still need to add the logic for retyping it because of that

veruu · 2018-07-23T11:46:14Z

sktm/jenkins.py

@@ -320,6 +326,56 @@ def build(self, baserepo=None, ref=None, baseconfig=None,
        logging.info("submitted build: %s", build)
        return build.get_number()

+    def __get_job(self):
+        """
+        Get Jenkens job by job name. Retry Jenkins N times in case of temporary


Can you mention the retry_count and the default value here instead of N so it's clear where the value comes from?

veruu · 2018-07-23T11:47:44Z

sktm/jenkins.py

+        network failures.
+
+        Return:
+            (job, None) if succeed, else return (None, e) where e is the last


Since the exception is going to be raised anyways, wouldn't this be too complicated? Can we just return the job if we succeed, and raise the exception if we can't reach Jenkins at all?

Sure. Updated.

veruu · 2018-07-23T11:50:10Z

sktm/jenkins.py

+            (job, None) if succeed, else return (None, e) where e is the last
+            exception.
+        """
+        retry_cnt = self.retry_cnt


Is this really necessary? Why not use self.retry_count directly?

Use self.retry_cnt instead.

veruu · 2018-07-23T11:51:50Z

sktm/jenkins.py

+        logging.error("fail to get job after retry %d times" % retry_cnt)
+        return (None, e)
+
+    def __get_build(self, job, buildid):


Same comments as with __get_jobid here :)

idorax · 2018-07-24T08:09:25Z

Thanks Veronika for your review, I have updated the related files according to your comments, please take a look again.

idorax · 2018-07-25T04:45:32Z

Reopen the PR after updating load_config().

veruu

I'm sorry, but the load_config changes in their current state won't work. Also, can you exchange the rest of job and build retrievals for the new methods?

veruu · 2018-07-25T13:47:10Z

sktm/executable.py

@@ -47,6 +49,9 @@ def setup_parser():
    parser.add_argument("--jurl", help="Jenkins URL")
    parser.add_argument("--jlogin", help="Jenkins login")
    parser.add_argument("--jpass", help="Jenkins password")
+    parser.add_argument("--jretry",
+                        help="Counter to retry Jenkins, default to 30",
+                        type=int, default=30)


Why use a numeric default here instead of the added constant?

Sorry I forgot to replace the number with the added constant. Now default = 30 is removed and help string is updated.

veruu · 2018-07-25T13:48:21Z

sktm/executable.py

@@ -205,6 +210,9 @@ def load_config(args):
    else:
        cfg['report_footer'] = os.path.abspath(cfg['report_footer'])

+    if not cfg.get('jretry'):


This condition will never follow through because of the default added to the parser. Because of that, the value will also never be updated by the one from configuration file. I think it would be easier to remove the default from the parser and keep this condition, with an else branch that would retype the value to int (the value read form configuration file is a string, so leaving that part out will break sktm), what do you say?

I misunderstood the logic to load a configuration variable from file. It's updated, pls take a look again.

veruu · 2018-07-25T13:56:17Z

sktm/jenkins.py

@@ -320,6 +326,51 @@ def build(self, baserepo=None, ref=None, baseconfig=None,
        logging.info("submitted build: %s", build)
        return build.get_number()

+    def __get_job(self):
+        """
+        Get Jenkens job by job name. Retry Jenkins $retry_cnt times in case


This is a really minor nitpick, but can we do self.retry_cnt instead of the dollar notation? First, it's missing the self notion and self.retry_cnt and retry_cnt are two very different variables so someone new reading the code would get pretty confused; and secondly, well, it's python and not bash :)

Well, now use self.retry_cnt instead of $retry_cnt, please check. Again, thank you very much for your review.

idorax · 2018-07-26T02:45:13Z

Veronika, I updated the related files according to your comments, please take a look again. Your comments are much appreciated!

veruu

Thanks Vector, functionality-wise it's looking good! However, I'm still missing the exchange of the rest of the server.get_job and job.get_build calls. Can you add those?

veruu · 2018-07-26T11:09:07Z

sktm/executable.py

@@ -205,6 +210,12 @@ def load_config(args):
    else:
        cfg['report_footer'] = os.path.abspath(cfg['report_footer'])

+    jretry = cfg.get('jretry')


There's no need for this - if not cfg.get('jretry') and cfg['jretry'] = int(cfg.get('jretry')) should work just fine :)

Agree, updated.

idorax · 2018-07-27T02:48:43Z

Thanks Veronika for review again, I have updated the rest of server.get_job and job.get_build calls to use __get_job()/__get_build(). And I'm not sure job.get_last_build() / job.get_next_build_number() should be enhanced to avoid random networking failures, so just leave them as they are. If you have more concerns/comments, please let me know, thank you very much.

veruu

Thanks Vector, LGTM now! I'd say it would be cool to have the rest of methods that query Jenkins modified in a similar way, if you want to implement them feel free to do so and submit another pull

idorax · 2018-07-31T02:48:08Z

Thanks Veronika again. Okay, I'll implement them this week and file another PR for review.

veruu suggested changes Jul 20, 2018

View reviewed changes

veruu suggested changes Jul 23, 2018

View reviewed changes

idorax closed this Jul 25, 2018

idorax reopened this Jul 25, 2018

veruu suggested changes Jul 25, 2018

View reviewed changes

veruu suggested changes Jul 26, 2018

View reviewed changes

sktm should make multiple attempts to query Jenkins

4174d26

veruu approved these changes Jul 30, 2018

View reviewed changes

veruu merged commit 1eb5651 into CKI-project:master Jul 30, 2018

sktm should make multiple attempts to query Jenkins #116

sktm should make multiple attempts to query Jenkins #116

Conversation

idorax commented Jul 19, 2018

coveralls commented Jul 19, 2018 • edited Loading

Pull Request Test Coverage Report for Build 405

💛 - Coveralls

veruu commented Jul 20, 2018

idorax commented Jul 20, 2018 • edited Loading

veruu left a comment

Choose a reason for hiding this comment

idorax commented Jul 23, 2018

veruu 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

idorax Jul 24, 2018 • 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

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

idorax commented Jul 24, 2018

idorax commented Jul 25, 2018

veruu 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

idorax commented Jul 26, 2018

veruu left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

idorax commented Jul 27, 2018

veruu left a comment

Choose a reason for hiding this comment

idorax commented Jul 31, 2018

coveralls commented Jul 19, 2018 •

edited

Loading

idorax commented Jul 20, 2018 •

edited

Loading

idorax Jul 24, 2018 •

edited

Loading

veruu left a comment •

edited

Loading