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.

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-47793] - Jenkins master and agent configuration pages do not verify negative executor numbers #3141

Merged
merged 11 commits into from Jan 30, 2018

Conversation

4 participants
@ksenia-nenasheva
Copy link
Contributor

commented Nov 14, 2017

See JENKINS-47793.

Field validation for "# of executors" did not work correctly in agent/master configuration pages. It was showing a page with an error and log on "Save". For agents it was also possible to set a negative number in the "# of executors" field.

After this fix:
For the master, "# of executors" should be a non-negative number (include 0).
For agents, "# of executors" should be a positive number (excluding 0).

Proposed changelog entries

  • Entry 1: Fix handling and field validation of "# of executors" in master and agent configurations.
  • ...

Submitter checklist

  • JIRA issue is well described
  • Changelog entry appropriate for the audience affected by the change (users or developer, depending on the change). Examples
    * Use the Internal: prefix if the change has no user-visible impact (API, test frameworks, etc.)
  • Appropriate autotests or explanation to why this change has no tests
  • For dependency updates: links to external changelogs and, if possible, full diffs

Desired reviewers

@mention

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Nov 15, 2017

@@ -1467,6 +1467,11 @@ public void doConfigSubmit( StaplerRequest req, StaplerResponse rsp ) throws IOE
throw new FormException(Messages.ComputerSet_SlaveAlreadyExists(proposedName), "name");
}

String nExecutors = req.getSubmittedForm().getString("numExecutors");
if (!nExecutors.matches("^(\\d*[1-9]\\d*)$")) {

This comment has been minimized.

Copy link
@jglick

jglick Nov 16, 2017

Member

^ and $ are gratuitous when using matches (as opposed to, say, Matcher.find). So are the (), since you are not asking for any group.

More to the point, this will improperly reject 0 AFAICT, which is a valid executor number (for the master node anyway). Or is this code called only for agents?

Maybe simpler to just Integer.parseInt, catch NumberFormatException, and reject a negative result.

@@ -2758,7 +2759,7 @@ public InitMilestone getInitLevel() {
return initLevel;
}

public void setNumExecutors(int n) throws IOException {
public void setNumExecutors(@Nonnegative int n) throws IOException {

This comment has been minimized.

Copy link
@jglick

jglick Nov 16, 2017

Member

No runtime effect AFAIK; should really be matched by an IllegalArgumentException.

@@ -26,6 +26,7 @@
Hudson.BadPortNumber=Bad port number {0}
Hudson.Computer.Caption=Master
Hudson.Computer.DisplayName=master
Hudson.Computer.IncorrectNumberOfExecutors=Incorrect field "# of executors". It should be no negative number.

This comment has been minimized.

Copy link
@jglick
"INPUT.number" : function(e) { registerRegexpValidator(e,/^(\d+|)$/,"Not an integer"); },
"INPUT.number" : function(e) { registerRegexpValidator(e,/^\-?(\d+)$/,"Not an integer"); },
"INPUT.non-negative-number" : function(e) {
registerRegexpValidator(e,/^(\d+)$/,"Not a non-negative number");

This comment has been minimized.

Copy link
@jglick

jglick Nov 16, 2017

Member

gratuitous () BTW

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Nov 27, 2017

Work in progress, I'd guess

@ksenia-nenasheva

This comment has been minimized.

Copy link
Contributor Author

commented Dec 28, 2017

@jglick Done.
Yes, this function is only for agents.
In this PR the master may have a non-negative number of executors (positive and zero), agents may have only a positive number of executors.

Thanks for the review!

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jan 4, 2018

@jenkinsci/code-reviewers back to review

@oleg-nenashev
Copy link
Member

left a comment

It seems that the error processing is not complete. The IllegalArgumentException exceptioneither needs to be thrown or removed from throws (I would vote for the first option). The rest LGTM

@@ -2758,7 +2759,7 @@ public InitMilestone getInitLevel() {
return initLevel;
}

public void setNumExecutors(int n) throws IOException {
public void setNumExecutors(@Nonnegative int n) throws IOException, IllegalArgumentException {

This comment has been minimized.

Copy link
@oleg-nenashev

oleg-nenashev Jan 6, 2018

Member

Hmm, does it really throw IllegalArgumentException?

@@ -51,6 +51,11 @@ public boolean configure(StaplerRequest req, JSONObject json) throws FormExcepti
Jenkins j = Jenkins.getInstance();
try {
// for compatibility reasons, this value is stored in Jenkins

This comment has been minimized.

Copy link
@daniel-beck

daniel-beck Jan 8, 2018

Member

comment displaced, refers to j.setNumExecutors line.

@daniel-beck
Copy link
Member

left a comment

LGTM expect for the displaced comment.

@jglick

jglick approved these changes Jan 8, 2018





This comment has been minimized.

Copy link
@jglick

jglick Jan 8, 2018

Member

BTW it is best to revert unrelated whitespace changes in PRs.

@@ -51,6 +51,11 @@ public boolean configure(StaplerRequest req, JSONObject json) throws FormExcepti
Jenkins j = Jenkins.getInstance();
try {
// for compatibility reasons, this value is stored in Jenkins
String num = json.getString("numExecutors");
if (!num.matches("^(\\d+)")) {

This comment has been minimized.

Copy link
@jglick

jglick Jan 8, 2018

Member

BTW ^ and () both gratuitous here; could be simplified to

if (!num.matches("\\d+")) {
@@ -1467,6 +1467,11 @@ public void doConfigSubmit( StaplerRequest req, StaplerResponse rsp ) throws IOE
throw new FormException(Messages.ComputerSet_SlaveAlreadyExists(proposedName), "name");
}

String nExecutors = req.getSubmittedForm().getString("numExecutors");
if (Integer.parseInt(nExecutors)<=0) {
throw new FormException(Messages.Slave_InvalidConfig_Executors(nodeName), "numExecutors");

This comment has been minimized.

Copy link
@jglick

jglick Jan 8, 2018

Member

As below, this would more naturally live in Slave.setNumExecutors but that would be a source-incompatible change.

@@ -51,6 +51,11 @@ public boolean configure(StaplerRequest req, JSONObject json) throws FormExcepti
Jenkins j = Jenkins.getInstance();
try {
// for compatibility reasons, this value is stored in Jenkins
String num = json.getString("numExecutors");
if (!num.matches("^(\\d+)")) {
throw new FormException(Messages.Hudson_Computer_IncorrectNumberOfExecutors(),"numExecutors");

This comment has been minimized.

Copy link
@jglick

jglick Jan 8, 2018

Member

It would be nicer to just throw the FormException from the setter, so you do not need to duplicate code, but I suppose the problem is that this is a checked exception.

"INPUT.positive-number" : function(e) {
registerRegexpValidator(e,/^(\d*[1-9]\d*|)$/,"Not a positive integer");
registerRegexpValidator(e,/^(\d*[1-9]\d*)$/,"Not a positive integer");

This comment has been minimized.

Copy link
@jglick

jglick Jan 8, 2018

Member

/[1-9]\d*/ might be a more intuitive regexp FWIW.

ksenia-nenasheva added some commits Jan 13, 2018

@oleg-nenashev oleg-nenashev requested a review from jglick Jan 20, 2018

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jan 20, 2018

Needs re-review by @jglick, I'd guess

"INPUT.positive-number" : function(e) {
registerRegexpValidator(e,/^(\d*[1-9]\d*|)$/,"Not a positive integer");
registerRegexpValidator(e,/[1-9]\d*/,"Not a positive integer");

This comment has been minimized.

Copy link
@jglick

jglick Jan 22, 2018

Member

This needs to be anchored with ^…$, as it seems JavaScript String.match(RegExp) searches for the first match, like Java’s find but unlike Java’s matches.

@oleg-nenashev oleg-nenashev requested a review from jglick Jan 28, 2018

@jglick

jglick approved these changes Jan 30, 2018

@daniel-beck

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

@oleg-nenashev Why is this on-hold?

@oleg-nenashev

This comment has been minimized.

Copy link
Member

commented Jan 30, 2018

@daniel-beck daniel-beck removed the on-hold label Jan 30, 2018

@oleg-nenashev oleg-nenashev merged commit 2eda781 into jenkinsci:master Jan 30, 2018

1 check passed

continuous-integration/jenkins/pr-head This commit looks good
Details

@dwnusbaum dwnusbaum referenced this pull request Feb 20, 2018

Closed

[FIXED JENKINS-41182] - Support nodes with 0 executors #2925

0 of 4 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.