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

Refactor Nomad policy parsing and add tests #114

Merged
merged 3 commits into from
May 4, 2020
Merged

Conversation

lgfa29
Copy link
Contributor

@lgfa29 lgfa29 commented May 1, 2020

This PR makes the logic that parses a policy from Nomad into the internal policy representation the Autoscaler uses.

The new parser makes a few assumptions:

  1. scaling->policy->evaluation_interval defaults to the value of scan_interval default_evaluation_time from the agent config file.
  2. the final Policy.Target.Config map has the values from scaling->policy->target->config (which are defined in the jobspec) merged with the values from api.ScalingPolicy.Target (which are predefined by the Nomad API as the group, job and namespace that has the scaling stanza). Values in the jobspec have higher priority.
  3. the values of 'Job' and 'Group' from api.ScalingPolicy.Target are copied over to Policy.Target.Config as job_id and group if they are present yet (i.e., if the jobspec has job_id the value won't be copied over).

Open questions:

  1. evaluation_interval is used in the jobspec and scan_interval is used in the agent config. Should one of them be renamed so they match? scan_interval renamed to default_evaluation_time.
  2. Renaming Job and Group seems unnecessary, but there are other parts that rely on them being job_id and group, so I didn't want to change them now. Using them as-is would require users to write them capitalize in the jobspec, which is unusual. Maybe have some automatic case conversion?

Closes #99
Closes #98
Closes #71

@lgfa29 lgfa29 requested review from cgbaker and jrasell May 1, 2020 23:22
@lgfa29 lgfa29 self-assigned this May 1, 2020
Target: parseTarget(p.Policy[keyTarget]),
Strategy: parseStrategy(p.Policy[keyStrategy]),
// Policy is enabled by default.
if p.Enabled == nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

might put the default above in the initializer.

configMapString = make(map[string]string)

for k, v := range targetAttr {
Copy link
Contributor

Choose a reason for hiding this comment

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

on my phone, can't trace this back, but wanted to make sure that this is only expected to be set if "config" existed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, this moving values from targetMap["config"] (which is a map[string]interface{}) into configMapString (which is map[string]string).

So if "config" is not set we don't need to do anything.

Copy link
Member

@jrasell jrasell left a comment

Choose a reason for hiding this comment

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

tested locally and this resolves #99 #98 #71

my only comment would be that the parsing error log formatting is a little weird:

2020-05-04T09:53:10.923+0200 [ERROR] policy_handler.policy_handler: invalid policy: 1 error occurred:
	* scaling->policy->source must be string, found []interface {}

: policy_id=f99ca263-7a1e-bc3a-b331-7db33053173f

not sure if there is a way to improve this, or its a by-product of multierror.

@lgfa29
Copy link
Contributor Author

lgfa29 commented May 4, 2020

tested locally and this resolves #99 #98 #71

my only comment would be that the parsing error log formatting is a little weird:

2020-05-04T09:53:10.923+0200 [ERROR] policy_handler.policy_handler: invalid policy: 1 error occurred:
	* scaling->policy->source must be string, found []interface {}

: policy_id=f99ca263-7a1e-bc3a-b331-7db33053173f

not sure if there is a way to improve this, or its a by-product of multierror.

Yeah, that's not great. These are validation error messages, so I will improve them in a new PR.

@lgfa29 lgfa29 merged commit a3f3f7c into master May 4, 2020
@lgfa29 lgfa29 deleted the add-policy-parse-tests branch May 4, 2020 17:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants