-
Notifications
You must be signed in to change notification settings - Fork 89
Target plugin for IBMCloud (VSI) Instance Group #1056
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
base: main
Are you sure you want to change the base?
Target plugin for IBMCloud (VSI) Instance Group #1056
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the contribution! The error handling doesn't need newlines and could use some rewording in places. Hopefully the comments I left guide you in the right direction.
instance_group_id, ok := n.getConfig(configKeyInstanceGroupID, config) | ||
if !ok { | ||
return fmt.Errorf("required config param %s not found", configKeyInstanceGroupID) | ||
} | ||
|
||
api_key, ok := n.getConfig(configKeyAPIKey, config) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
instanceGroupID
and apiKey
would be more idiomatic Go names, but no big deal. Not a blocker!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in ceb5459
// hit that client handle to query information about the victim Instance Group | ||
ig, _, err := n.vpc.GetInstanceGroup(&vpcv1.GetInstanceGroupOptions{ID: &instance_group_id}) | ||
if err != nil { | ||
fmt.Errorf("IG: %s: Exception in GetInstanceGroup: %v\n", instance_group_id, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fmt.Errorf("IG: %s: Exception in GetInstanceGroup: %v\n", instance_group_id, err) | |
return fmt.Errorf("failed to GetInstanceGroup for %s: %w", instance_group_id, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in ceb5459
instanceGroupPatchModel.MembershipCount = core.Int64Ptr(int64(action.Count)) | ||
instanceGroupPatch, err := instanceGroupPatchModel.AsPatch() | ||
if err != nil { | ||
return fmt.Errorf("IG: %s: Exception in instanceGroupPatchModel: %v\n", instance_group_id, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
return fmt.Errorf("IG: %s: Exception in instanceGroupPatchModel: %v\n", instance_group_id, err) | |
return fmt.Errorf("error creating patch for instance group %s: %w", instance_group_id, err) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in ceb5459
Thanks @schmichael ! My really bad go code here is a but humbling -- "how did this ever build?" -- thanks for the suggestions. I changed out the identifiers (I was thinking too much like the terraform code) and tried to convert the exception messages similar to your examples with the previous. Down the road I would like to dedupe some of the code (most of the instanceID, apiKey -> NewVpcV1(...) stuff) but I wanted to share this sooner. |
@schmichael changes are made to address all your suggestions -- mostly wholesale accepting your suggestions (thanks) -- is there anything else we need to merge this? |
@schmichael while this awaits permission to begin workflows, I'm curious whether you prefer that I rebase locally and force-push, or merge main on top of this PR. Can you advise on your preferred workflow? |
Hello @chickenandpork |
thanks, @jeffmccollum .. I think I originally had I'll fix the description so that it's accurate for the next. [edit: updated description] Do you know if I should just keep merging updates to this PR until merged, or whether Hashicorp prefers rebase/squash/force-push ? I'd like to keep it as easy to merge as possible when the PR is approved. |
This comment was marked as resolved.
This comment was marked as resolved.
We'll squash-merge when we're done anyways. GitHub's review tools are a little easier to use if you don't squash while the review is ongoing. |
Looks like
Also could support for trusted profiles be added as an option? |
Strange -- I thought that was checked before, and it was green. taking a look.
Likely, but I'd want to follow this PR with that enhancement. My personal goal is to get this small effort into main so that others can see and leverage, then extend capabilities: minimum-ship before feature-ful. |
@jeffmccollum added, but I think the CI needs approval before checking. |
d03baab
to
9545da2
Compare
@jeffmccollum @schmichael rebased for the 10 commits between my PR and current builds. fixes https://github.com/hashicorp/nomad-autoscaler/actions/runs/14088268351/job/40258769936 The CI makes any outstanding PR have a recurring cost if we keep up (CI that used to pass can now fail if retried post-dependency update, especially because it enforced Would you folks be interested in some effort to accelerate the build process? |
9545da2
to
14ff184
Compare
Easier from the CLI to rebase on main and push. Churn caused due to being blocked on review while #1078 merged |
14ff184
to
ad8b588
Compare
d3cbcc3
to
ce83e63
Compare
This PR brings in a
target
plugin to scale an IBMCloud "instance group". Consider the following:(Terraform)
NOTE this will create an instance group with an ID such as
r006-faceb00c-feed-d00d-beef-123412341234
(nomad-autoscaler config
${NOMAD_TASK_DIR}/config.hcl
)(nomad-autoscaler config
${NOMAD_TASK_DIR}/policies/autoscale-group-policy.hcl
)I've tested this using a label as a metric, and witnessed the autoscaler exercising the target to scale an Instance Group up to 20, limited by the max I've set to 3, and then scaling back to 0.
This is currently in test at my employer but I'm contributing this to Hashicorp with supervisor permission. I'm willing to follow this with a contribution to nomad-autoscaler-demos. I can document the process in an article as well.